Patchwork D6612: tests: show the files fields of changelogs for many merges

login
register
mail settings
Submitter phabricator
Date July 7, 2019, 4:18 p.m.
Message ID <differential-rev-PHID-DREV-eff5mtusmrdk5actppez-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40806/
State Superseded
Headers show

Comments

phabricator - July 7, 2019, 4:18 p.m.
valentin.gatienbaron created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Question: this test takes 1min to run, so I'm not sure if it's worth
  keeping around? It was certainly useful when writing the next commit
  at least. And it may be possible to speed it up.
  
  I don't think there's coverage for many of the subtle cases, and I
  found it hard to understand what the code is doing by reading it.
  
  I have yet to find a description of what the files field is supposed
  to be for merges. I thought it could be one of:
  
  1. the files added/modified/removed relative to p1 (wouldn't seem useful, but `hg diff -c -r mergerev` has this behavior)
  2. the files with filelog nodes not in either parent (i.e., what is
  
  needed to create a bundle out of a commit)
  
  3. the files added/removed/modified files by merge itself [1]
  
  It's clearly not 1, because file contents merges are symmetric. It's
  clearly not 2 because removed files and exec bit changes are
  listed. It's also not 3 but I think it's intended to be 3 and the
  differences are bugs.
  
  Assuming 3, the test shows that, for merges, the list of files both
  overapproximates and underapproximates. All the cases involve file
  changes not in the filelog but in the manifest (existence of file
  at revision, exec bit and file vs symlink).
  
  I didn't look at all underapproximations, but they looked minor. The
  two overapproximations are problematic though because they both cause
  potentially long lists of files when merging cleanly.
  
  [1] even what it means for the merge commit itself to change a file is
  not completely trivial. A file in the merge being the same as in one
  of the parent is too lax as it would consider that merges change
  nothing when they revert all the changes done on one side. The
  criteria used in the test and in the next commit for "merge didn't
  touch a file" is:
  
  - the parents and the merge all have the same file
  - or, one parent didn't touch the file and the other parent contains the same file as the merge

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6612

AFFECTED FILES
  tests/test-merge-combination.t

CHANGE DETAILS




To: valentin.gatienbaron, #hg-reviewers
Cc: mercurial-devel
phabricator - July 17, 2019, 10:23 p.m.
durin42 added a comment.
durin42 added subscribers: martinvonz, durin42.


  I know @martinvonz has been looking at files in changelogs recently. Do you have an opinion on this? It looks fine to me.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6612/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6612

To: valentin.gatienbaron, #hg-reviewers
Cc: durin42, martinvonz, mercurial-devel
phabricator - July 17, 2019, 10:26 p.m.
durin42 added a comment.
durin42 accepted this revision as: durin42.


  (Martin, this looks fine to me, if you've got no objections let's include it in 5.1)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6612/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6612

To: valentin.gatienbaron, #hg-reviewers, durin42
Cc: durin42, martinvonz, mercurial-devel
phabricator - July 17, 2019, 10:36 p.m.
martinvonz added a comment.


  In D6612#97313 <https://phab.mercurial-scm.org/D6612#97313>, @durin42 wrote:
  
  > (Martin, this looks fine to me, if you've got no objections let's include it in 5.1)
  
  Yes, I agree. I wish there had been a clear and simple definition of what `{files}` should contain (as there is for `{file_adds}` and `{file_dels}` since https://www.mercurial-scm.org/repo/hg/rev/0c72eddb4be5). I spent some time trying to think of something, but I don't have a better proposal than what Valentin did in the next patch, so I think this is good. Great testing too.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6612/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6612

To: valentin.gatienbaron, #hg-reviewers, durin42
Cc: durin42, martinvonz, mercurial-devel
phabricator - July 18, 2019, 12:16 p.m.
valentin.gatienbaron added a comment.


  Thanks! I didn't expect this would make it for 5.1.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6612/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6612

To: valentin.gatienbaron, #hg-reviewers, durin42
Cc: durin42, martinvonz, mercurial-devel

Patch

diff --git a/tests/test-merge-combination.t b/tests/test-merge-combination.t
new file mode 100644
--- /dev/null
+++ b/tests/test-merge-combination.t
@@ -0,0 +1,190 @@ 
+This file shows what hg says are "modified" files for a merge commit
+(hg log -T {files}), somewhat exhaustively.
+It shows merges that involves files contents changing, and merges that
+involve executable bit changing, but not merges with multiple or zero
+merge ancestors, nor copies/renames, and nor identical file contents
+with different filelog revisions.
+
+genmerges is the workhorse. Given:
+- a range function describing the possible values for file a
+- a isgood function to filter out uninteresting combination
+- a createfile function to actually write the values for file a on the
+filesystem
+it print a series of lines that look like: abcd C: output of -T {files}
+describing the file a at respectively the base, p2, p1, merge
+revision. "C" indicates that hg merge had conflicts.
+  $ genmerges () {
+  >   for base in `range` -; do
+  >     for r1 in `range $base` -; do
+  >       for r2 in `range $base $r1` -; do
+  >         for m in `range $base $r1 $r2` -; do
+  >           line="$base$r1$r2$m"
+  >           isgood $line || continue
+  >           hg init repo
+  >           cd repo
+  >           make_commit () {
+  >             v=$1; msg=$2; file=$3;
+  >             if [ $v != - ]; then
+  >               createfile $v
+  >             else
+  >               if [ -f a ]
+  >               then hg rm -f a -q
+  >               else touch $file; hg add -q $file
+  >               fi
+  >             fi
+  >             hg commit -q -m $msg || exit 123
+  >           }
+  >           echo foo > foo; hg add -q foo
+  >           make_commit $base base b
+  >           make_commit $r1 r1 c
+  >           hg up -r 0 -q
+  >           make_commit $r2 r2 d
+  >           hg merge -q -r 1 > output 2>&1
+  >           if [ -s output ]; then conflicts=" C"; else conflicts="  "; fi
+  >           hg resolve -m --all -q
+  >           make_commit $m m e
+  >           if [ $m = $r1 ] && [ $m = $r2 ]
+  >           then expected=
+  >           elif [ $m = $r1 ]
+  >           then if [ $base = $r2 ]
+  >                then expected=
+  >                else expected=a
+  >                fi
+  >           elif [ $m = $r2 ]
+  >           then if [ $base = $r1 ]
+  >                then expected=
+  >                else expected=a
+  >                fi
+  >           else expected=a
+  >           fi
+  >           got=`hg log -r 3 --template '{files}\n' | tr --delete 'e '`
+  >           if [ "$got" = "$expected" ]
+  >           then echo "$line$conflicts: agree on \"$got\""
+  >           else echo "$line$conflicts: hg said \"$got\", expected \"$expected\""
+  >           fi
+  >           cd ../
+  >           rm -rf repo
+  >         done
+  >       done
+  >     done
+  >   done
+  > }
+
+All the merges of various file contents.
+
+  $ range () {
+  >   max=0
+  >   for i in $@; do
+  >     if [ $i = - ]; then continue; fi
+  >     if [ $i -gt $max ]; then max=$i; fi
+  >   done
+  >   $TESTDIR/seq.py `expr $max + 1`
+  > }
+  $ isgood () { true; }
+  $ createfile () {
+  >   if [ -f a ] && [ "`cat a`" = $1 ]
+  >   then touch $file; hg add -q $file
+  >   else echo $v > a; hg addr -q a
+  >   fi
+  > }
+
+  $ genmerges
+  1111  : agree on ""
+  1112  : agree on "a"
+  111-  : agree on "a"
+  1121  : agree on "a"
+  1122  : agree on ""
+  1123  : agree on "a"
+  112-  : agree on "a"
+  11-1  : hg said "", expected "a"
+  11-2  : agree on "a"
+  11--  : agree on ""
+  1211  : agree on "a"
+  1212  : agree on ""
+  1213  : agree on "a"
+  121-  : agree on "a"
+  1221  : agree on "a"
+  1222  : agree on ""
+  1223  : agree on "a"
+  122-  : agree on "a"
+  1231 C: agree on "a"
+  1232 C: agree on "a"
+  1233 C: agree on "a"
+  1234 C: agree on "a"
+  123- C: agree on "a"
+  12-1 C: agree on "a"
+  12-2 C: hg said "", expected "a"
+  12-3 C: agree on "a"
+  12-- C: agree on "a"
+  1-11  : hg said "", expected "a"
+  1-12  : agree on "a"
+  1-1-  : hg said "a", expected ""
+  1-21 C: agree on "a"
+  1-22 C: hg said "", expected "a"
+  1-23 C: agree on "a"
+  1-2- C: agree on "a"
+  1--1  : agree on "a"
+  1--2  : agree on "a"
+  1---  : agree on ""
+  -111  : agree on ""
+  -112  : agree on "a"
+  -11-  : agree on "a"
+  -121 C: agree on "a"
+  -122 C: agree on "a"
+  -123 C: agree on "a"
+  -12- C: agree on "a"
+  -1-1  : agree on ""
+  -1-2  : agree on "a"
+  -1--  : agree on "a"
+  --11  : agree on ""
+  --12  : agree on "a"
+  --1-  : agree on "a"
+  ---1  : agree on "a"
+  ----  : agree on ""
+
+All the merges of executable bit.
+
+  $ range () {
+  >   max=a
+  >   for i in $@; do
+  >     if [ $i = - ]; then continue; fi
+  >     if [ $i > $max ]; then max=$i; fi
+  >   done
+  >   if [ $max = a ]; then echo f; else echo f x; fi
+  > }
+  $ isgood () { case $line in *f*x*) true;; *) false;; esac; }
+  $ createfile () {
+  >   if [ -f a ] && (([ -x a ] && [ $v = x ]) || (! [ -x a ] && [ $v != x ]))
+  >   then touch $file; hg add -q $file
+  >   else touch a; if [ $v = x ]; then chmod +x a; else chmod -x a; fi; hg addr -q a
+  >   fi
+  > }
+
+#if execbit
+  $ genmerges
+  fffx  : agree on "a"
+  ffxf  : agree on "a"
+  ffxx  : agree on ""
+  ffx-  : agree on "a"
+  ff-x  : hg said "", expected "a"
+  fxff  : hg said "", expected "a"
+  fxfx  : hg said "a", expected ""
+  fxf-  : agree on "a"
+  fxxf  : agree on "a"
+  fxxx  : agree on ""
+  fxx-  : agree on "a"
+  fx-f  : hg said "", expected "a"
+  fx-x  : hg said "", expected "a"
+  fx--  : hg said "", expected "a"
+  f-fx  : agree on "a"
+  f-xf  : agree on "a"
+  f-xx  : hg said "", expected "a"
+  f-x-  : agree on "a"
+  f--x  : agree on "a"
+  -ffx  : agree on "a"
+  -fxf C: agree on "a"
+  -fxx C: hg said "", expected "a"
+  -fx- C: agree on "a"
+  -f-x  : hg said "", expected "a"
+  --fx  : agree on "a"
+#endif