[PATCH 0 of 1] [thg] visdiff: do not reuse snapshots for different changeset pairs, fix #131

[patrice....@gmail.com]

This patch fixes the issue #131.

The origin of the issue is that when comparing two changesets, thg creates a snapshot for each changeset (named after their hash) and compares them. If a changeset is then compared with another one, its snapshot is reused, and extra differing files are ADDED to it, resulting in more files shown in the visual diff than expected.

My fix names snapshots after the changeset's hash, but also the other changeset(s) it is compared to. Therefore, when comparing A with B, then with C, A has two snapshots, A-B and A-C.

Best regards

Thanks go to André Sintzoff for having reviewed this code.

Patrice LACOUTURE

[patrice....@gmail.com]

# HG changeset patch
# User patrice....@gmail.com
# Date 1297872566 -3600
# Branch stable
# Node ID 243a3fb7d8998626efce02a046fbccf86a199680
# Parent 10cdfc8ad20cfe414b87c0c7d761552e11491e97
visdiff: do not reuse snapshots for different diff pairs, fix #131

diff --git a/tortoisehg/hgqt/visdiff.py b/tortoisehg/hgqt/visdiff.py
--- a/tortoisehg/hgqt/visdiff.py
+++ b/tortoisehg/hgqt/visdiff.py
@@ -46,13 +46,13 @@

# Always make a copy of ctx1a
files1a = sources | mod_a | rem_a | ((mod_b | add_b) - add_a)
- dir1a, fns_mtime1a = snapshot(repo, files1a, ctx1a)
+ dir1a, fns_mtime1a = snapshot(repo, files1a, ctx1a, [ctx1b, ctx2])
label1a = '@%d' % ctx1a.rev()

# Make a copy of ctx1b if relevant
if ctx1b:
files1b = sources | mod_b | rem_b | ((mod_a | add_a) - add_b)
- dir1b, fns_mtime1b = snapshot(repo, files1b, ctx1b)
+ dir1b, fns_mtime1b = snapshot(repo, files1b, ctx1b, [ctx1a, ctx2])
label1b = '@%d' % ctx1b.rev()
else:
dir1b = None
@@ -63,14 +63,14 @@
files2 = mod_a | add_a | mod_b | add_b
if ctx2.rev() is None:
if copyworkingdir:
- dir2, fns_mtime2 = snapshot(repo, files2, ctx2)
+ dir2, fns_mtime2 = snapshot(repo, files2, ctx2, [ctx1a, ctx1b])
else:
dir2 = repo.root
fns_mtime2 = []
# If ctx2 is working copy, use empty label.
label2 = ''
else:
- dir2, fns_mtime2 = snapshot(repo, files2, ctx2)
+ dir2, fns_mtime2 = snapshot(repo, files2, ctx2, [ctx1a, ctx1b])
label2 = '@%d' % ctx2.rev()

dirs = [dir1a, dir1b, dir2]
@@ -78,11 +78,14 @@
fns_and_mtimes = [fns_mtime1a, fns_mtime1b, fns_mtime2]
return dirs, labels, fns_and_mtimes

-def snapshot(repo, files, ctx):
+def snapshot(repo, files, ctx, otherctxs=None):
'''snapshot files as of some revision'''
dirname = os.path.basename(repo.root) or 'root'
if ctx.rev() is not None:
dirname = '%s.%s' % (dirname, str(ctx))
+ if otherctxs:
+ dirname = '%s-%s' % (dirname,
+ '-'.join(map(str, filter(None, otherctxs))))
base = os.path.join(qtlib.gettempdir(), dirname)
fns_and_mtime = []
if not os.path.exists(base):

[Patrice LACOUTURE]

I guess that if this fix goes to the repo, it will have to be
backported to hgtk as well, as it's the same code.

Patrice LACOUTURE

[Steve Borho]

On Wed, Feb 16, 2011 at 2:55 PM, <patrice....@gmail.com> wrote:
> # HG changeset patch
> # User patrice....@gmail.com
> # Date 1297872566 -3600
> # Branch stable
> # Node ID 243a3fb7d8998626efce02a046fbccf86a199680
> # Parent  10cdfc8ad20cfe414b87c0c7d761552e11491e97
> visdiff: do not reuse snapshots for different diff pairs, fix #131

Ah, Nice. I see you've found the root of the problem already. My
only problem with this solution is that the temporary file paths end
up being very log, and on Windows the path space is very tight.

So instead of:

%TEMP%/qtlib.gettempdir()/#hash##hash#

we could use an incrementing global counter and revision number:

%TEMP%/qtlib.gettempdir()/#count#_#revnum#/

Hgtk deleted the temp folder when the diff tool exited, and each
visual diff invocation built a new temp folder so there were no
collisions like this. Just numerous bug reports about diff tools
being broken because the files were being deleted prematurely.

--
Steve Borho

[Patrice LACOUTURE]

On Feb 16, 10:13 pm, Steve Borho <st...@borho.org> wrote:
>
> Ah, Nice.  I see you've found the root of the problem already.  My
> only problem with this solution is that the temporary file paths end
> up being very log, and on Windows the path space is very tight.
>
> So instead of:
>
> %TEMP%/qtlib.gettempdir()/#hash##hash#
>
> we could use an incrementing global counter and revision number:
>
> %TEMP%/qtlib.gettempdir()/#count#_#revnum#/
>

Nice idea. Anyway, these references are local to the repo, so we don't
need full hashes... My initial understanding was that we could cache
the snapshot contents, but apparently that's not done, and if we did
and things changed in between (I guess after strip/import or in patch
queues), this would break easily.

If we don't intend to cache these results, your suggestion is fine, I
can code it. I guess if comparing many changeset pairs far apart in
large repos, this could become a quick disk hog...

We could imagine using the subprocess package's features to monitor
the spawned visual diff processes, and delete the snapshots after they
terminate, but what if the spawned process actually spaws the actual
diff tool in turn, then terminates immediately? We could also place a
restriction not to use diff tools this way (I don't know if anyone
does so anyway).

Patrice

[Steve Borho]

That was indeed the problem that led to the persistent temp folder in
the first place. Many tools do spawn subprocesses and not all of them
even offer workarounds for it. In hgtk, I forced users of those tools
to use the visual diff window, which held the temp files until the
window was closed.

I'm not terribly concerned about the size of temp folder, but we could
easily add the folder to the purge tool if it became an issue.

--
Steve Borho

[Patrice LACOUTURE]

On Feb 16, 10:51 pm, Steve Borho <st...@borho.org> wrote:
>
> I'm not terribly concerned about the size of temp folder, but we could
> easily add the folder to the purge tool if it became an issue.

Here at work we DO have huge repos with frequent changes... We also
have the disks to fit (mostly ;-) ), and I can always close/reopen thg
as an acceptable workaround.

Patrice