Patchwork extdiff: copy back execbit-only changes to the working directory

login
register
mail settings
Submitter Matt Harbison
Date May 12, 2017, 3:18 a.m.
Message ID <862a9d7605a23d7108e5.1494559130@Envy>
Download mbox | patch
Permalink /patch/20584/
State Accepted
Headers show

Comments

Matt Harbison - May 12, 2017, 3:18 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1494556425 14400
#      Thu May 11 22:33:45 2017 -0400
# Node ID 862a9d7605a23d7108e54b1a533dd3b091a6b12d
# Parent  b83aecac21cae0ea9e0256a00c744ca120799bc3
extdiff: copy back execbit-only changes to the working directory

Some tools like BeyondCompare allow the file mode to be changed.  The change
was previously applied if the content of the file changed (either according to
size or mtime), but was not being copied back for a mode-only change.  That
would seem to indicate handling this in an 'elif' branch, but I opted not to in
order to avoid copying back the mode without the content changes when mtime and
size are unchanged.  (Yes, that's a rare corner case, but all the more reason
not to have a subtle difference in behavior.)

The only way I can think to handle this undetected change is to set each file in
the non-wdir() snapshot to readonly, and check for that attribute (as well as
mtime) when deciding to copy back.  That would avoid the overhead of copying the
whole file when only the mode changed.  But a chmod in a diff tool is likely
rare.  See also affd753ddaf1.
Yuya Nishihara - May 14, 2017, 4:51 a.m.
On Thu, 11 May 2017 23:18:50 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1494556425 14400
> #      Thu May 11 22:33:45 2017 -0400
> # Node ID 862a9d7605a23d7108e54b1a533dd3b091a6b12d
> # Parent  b83aecac21cae0ea9e0256a00c744ca120799bc3
> extdiff: copy back execbit-only changes to the working directory

Looks good. Queued, thanks.

Patch

diff --git a/hgext/extdiff.py b/hgext/extdiff.py
--- a/hgext/extdiff.py
+++ b/hgext/extdiff.py
@@ -280,7 +280,11 @@ 
             # all changes.  A size check will detect more cases, but not all.
             # The only certain way to detect every case is to diff all files,
             # which could be expensive.
-            if cpstat.st_mtime != st.st_mtime or cpstat.st_size != st.st_size:
+            # copyfile() carries over the permission, so the mode check could
+            # be in an 'elif' branch, but for the case where the file has
+            # changed without affecting mtime or size.
+            if (cpstat.st_mtime != st.st_mtime or cpstat.st_size != st.st_size
+                or (cpstat.st_mode & 0o100) != (st.st_mode & 0o100)):
                 ui.debug('file changed while diffing. '
                          'Overwriting: %s (src: %s)\n' % (working_fn, copy_fn))
                 util.copyfile(copy_fn, working_fn)
diff --git a/tests/test-extdiff.t b/tests/test-extdiff.t
--- a/tests/test-extdiff.t
+++ b/tests/test-extdiff.t
@@ -329,6 +329,7 @@ 
   > # Mimic a tool that syncs all attrs, including mtime
   > cp $1/a $2/a
   > touch -r $1/a $2/a
+  > chmod +x $2/a
   > echo "** custom diff **"
   > EOF
 #if execbit
@@ -366,6 +367,32 @@ 
   $ cat a
   a
 
+#if execbit
+  $ [ -x a ]
+
+  $ cat > 'dir/tool.sh' << 'EOF'
+  > #!/bin/sh
+  > chmod -x $2/a
+  > echo "** custom diff **"
+  > EOF
+
+  $ hg --debug tl --config extdiff.tl= --config merge-tools.tl.executable=$tool
+  making snapshot of 2 files from rev * (glob)
+    a
+    b
+  making snapshot of 2 files from working directory
+    a
+    b
+  running '$TESTTMP/a/dir/tool.sh a.* a' in */extdiff.* (glob)
+  ** custom diff **
+  file changed while diffing. Overwriting: $TESTTMP/a/a (src: */extdiff.*/a/a) (glob)
+  cleaning up temp directory
+  [1]
+
+  $ [ -x a ]
+  [1]
+#endif
+
   $ cd ..
 
 #if symlink