| Message ID | a8d4825a323d5c1e7b2dc1edc8621c51c030ae1e.1603468885.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] t/: new helper for tests that pass with ort but fail with recursive | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > There are a number of tests that the "recursive" backend does not handle > correctly but which the redesign in "ort" will. Add a new helper in > lib-merge.sh for selecting a different test expectation based on the > setting of GIT_TEST_MERGE_ALGORITHM, and use it in various testcases to > document which ones we expect to fail under recursive but pass under > ort. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > t/lib-merge.sh | 15 +++++++++++++++ > t/t6416-recursive-corner-cases.sh | 11 ++++++----- > t/t6422-merge-rename-corner-cases.sh | 7 ++++--- > t/t6423-merge-rename-directories.sh | 13 +++++++------ > t/t6426-merge-skip-unneeded-updates.sh | 3 ++- > t/t6430-merge-recursive.sh | 3 ++- > 6 files changed, 36 insertions(+), 16 deletions(-) > create mode 100644 t/lib-merge.sh > > diff --git a/t/lib-merge.sh b/t/lib-merge.sh > new file mode 100644 > index 0000000000..fac2bc5919 > --- /dev/null > +++ b/t/lib-merge.sh > @@ -0,0 +1,15 @@ > +# Helper functions used by merge tests. > + > +test_expect_merge_algorithm () { > + status_for_recursive=$1 > + shift > + status_for_ort=$1 > + shift Smaller than minor, but I'd find status_for_recursive=$1 status_for_ort=$2 shift 2 easier to see that which one is for which by matching the order in which the calling sites, e.g. test_expect_merge_algorithm success failure \ here comes the commands being tested lists them. > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + test_expect_${status_for_ort} "$@" > + else > + test_expect_${status_for_recursive} "$@" > + fi I expect this to be purely transitory, so it is fine. If not, something along the lines of ... eval test_expect='$'status_for_"$GIT_TEST_MERGE_ALGORITHM" $test_expect "$@" ... might be what I would suggest, though ;-). And the users are just too pleasant to see, with full of "failure sucess", which is the second best outcome we want to see ;-) > +test_expect_merge_algorithm failure success 'check symlink mo... > +test_expect_merge_algorithm failure success 'check symlink ad... > +test_expect_merge_algorithm failure success 'check submodule ... > +test_expect_merge_algorithm failure success 'check submodule ... > +test_expect_merge_algorithm failure success 'check conflictin... > +test_expect_merge_algorithm failure success 'rad-check: renam... > +test_expect_merge_algorithm failure success 'rrdd-check: rena... > +test_expect_merge_algorithm failure success 'mod6-check: chai... > +test_expect_merge_algorithm failure success '6b1: Same rename... > +test_expect_merge_algorithm failure success '6b2: Same rename... > +test_expect_merge_algorithm failure success '10e: Does git co... > +test_expect_merge_algorithm failure success '12b1: Moving two... > +test_expect_merge_algorithm failure success '12c1: Moving one... > +test_expect_merge_algorithm failure success '12f: Trivial dir... > +test_expect_merge_algorithm failure success '4a: Change on A,... > +test_expect_merge_algorithm failure success 'merge-recursive ...
On Fri, Oct 23, 2020 at 9:48 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > There are a number of tests that the "recursive" backend does not handle > > correctly but which the redesign in "ort" will. Add a new helper in > > lib-merge.sh for selecting a different test expectation based on the > > setting of GIT_TEST_MERGE_ALGORITHM, and use it in various testcases to > > document which ones we expect to fail under recursive but pass under > > ort. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > t/lib-merge.sh | 15 +++++++++++++++ > > t/t6416-recursive-corner-cases.sh | 11 ++++++----- > > t/t6422-merge-rename-corner-cases.sh | 7 ++++--- > > t/t6423-merge-rename-directories.sh | 13 +++++++------ > > t/t6426-merge-skip-unneeded-updates.sh | 3 ++- > > t/t6430-merge-recursive.sh | 3 ++- > > 6 files changed, 36 insertions(+), 16 deletions(-) > > create mode 100644 t/lib-merge.sh > > > > diff --git a/t/lib-merge.sh b/t/lib-merge.sh > > new file mode 100644 > > index 0000000000..fac2bc5919 > > --- /dev/null > > +++ b/t/lib-merge.sh > > @@ -0,0 +1,15 @@ > > +# Helper functions used by merge tests. > > + > > +test_expect_merge_algorithm () { > > + status_for_recursive=$1 > > + shift > > + status_for_ort=$1 > > + shift > > Smaller than minor, but I'd find > > status_for_recursive=$1 status_for_ort=$2 > shift 2 > > easier to see that which one is for which by matching the order in > which the calling sites, e.g. > > test_expect_merge_algorithm success failure \ > here comes the commands being tested > > lists them. I can fix that up. > > > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > > + then > > + test_expect_${status_for_ort} "$@" > > + else > > + test_expect_${status_for_recursive} "$@" > > + fi > > I expect this to be purely transitory, so it is fine. If not, > something along the lines of ... > > eval test_expect='$'status_for_"$GIT_TEST_MERGE_ALGORITHM" > $test_expect "$@" > > ... might be what I would suggest, though ;-). I also expect it to be transitory, but how long that transition is depends more on how long it takes for others to become comfortable with removing merge-recursive.c once merge-ort.c is in place. I suspect at a minimum it's a cycle or two after ort becomes the default (which at a minimum would come a cycle or two after ort is finished and available). If for some reason folks don't eventually become comfortable with removing merge-recursive, then we might need to revisit. > And the users are just too pleasant to see, with full of "failure > sucess", which is the second best outcome we want to see ;-) > > > +test_expect_merge_algorithm failure success 'check symlink mo... > > +test_expect_merge_algorithm failure success 'check symlink ad... > > +test_expect_merge_algorithm failure success 'check submodule ... > > +test_expect_merge_algorithm failure success 'check submodule ... > > +test_expect_merge_algorithm failure success 'check conflictin... > > +test_expect_merge_algorithm failure success 'rad-check: renam... > > +test_expect_merge_algorithm failure success 'rrdd-check: rena... > > +test_expect_merge_algorithm failure success 'mod6-check: chai... > > +test_expect_merge_algorithm failure success '6b1: Same rename... > > +test_expect_merge_algorithm failure success '6b2: Same rename... > > +test_expect_merge_algorithm failure success '10e: Does git co... > > +test_expect_merge_algorithm failure success '12b1: Moving two... > > +test_expect_merge_algorithm failure success '12c1: Moving one... > > +test_expect_merge_algorithm failure success '12f: Trivial dir... > > +test_expect_merge_algorithm failure success '4a: Change on A,... > > +test_expect_merge_algorithm failure success 'merge-recursive ... :-)
One more comment... On Fri, Oct 23, 2020 at 10:25 AM Elijah Newren <newren@gmail.com> wrote: > > On Fri, Oct 23, 2020 at 9:48 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > > And the users are just too pleasant to see, with full of "failure > > sucess", which is the second best outcome we want to see ;-) > > > > > +test_expect_merge_algorithm failure success 'check symlink mo... > > > +test_expect_merge_algorithm failure success 'check symlink ad... > > > +test_expect_merge_algorithm failure success 'check submodule ... > > > +test_expect_merge_algorithm failure success 'check submodule ... > > > +test_expect_merge_algorithm failure success 'check conflictin... > > > +test_expect_merge_algorithm failure success 'rad-check: renam... > > > +test_expect_merge_algorithm failure success 'rrdd-check: rena... > > > +test_expect_merge_algorithm failure success 'mod6-check: chai... > > > +test_expect_merge_algorithm failure success '6b1: Same rename... > > > +test_expect_merge_algorithm failure success '6b2: Same rename... > > > +test_expect_merge_algorithm failure success '10e: Does git co... > > > +test_expect_merge_algorithm failure success '12b1: Moving two... > > > +test_expect_merge_algorithm failure success '12c1: Moving one... > > > +test_expect_merge_algorithm failure success '12f: Trivial dir... > > > +test_expect_merge_algorithm failure success '4a: Change on A,... > > > +test_expect_merge_algorithm failure success 'merge-recursive ... > > :-) Actually, there are another 12 submodule-related tests that pass under ort but not under recursive, spread across t3512, t3513, t5572, t6437, and t6438. I didn't (yet) apply the same change there, so they all show up as "TODO passed" if you check out the 'ort' branch of my repo and run the tests with GIT_TEST_MERGE_ALGORITHM=ort. I delayed marking them as expecting success under ort because I suspect that nearby tests should also pass but are just coded too stringently. (For example, perhaps they expected a directory/submodule conflict to result in all files within the conflicting directory to be renamed out of the way instead of expecting the submodule to be moved aside -- moving the submodule aside results in massively less rename handling pressure and is an easier way to make sure that the files under the conflicting directory aren't written into and over entries within the submodule.) I was hoping to get a submodule expert to look over those tests and provide some opinions...
On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > +test_expect_merge_algorithm () { > + status_for_recursive=$1 > + shift > + status_for_ort=$1 > + shift > + > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + test_expect_${status_for_ort} "$@" > + else > + test_expect_${status_for_recursive} "$@" > -test_expect_failure 'check symlink modify/modify' ' > +test_expect_merge_algorithm failure success 'check symlink modify/modify' ' I find this series of "failure success" hard to decode without understanding what it would be, then I need to keep rememberring which status is corresponding with with algorithm. Perhaps this patch is a bit easier to read. This is largely based on your patch. (I haven't read other patches, yet). What do you think? -------------8<------------ From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= <congdanhqx@gmail.com> Date: Sat, 24 Oct 2020 17:41:02 +0700 Subject: [PATCH] t/: new helper for testing merge that allow failure for some algorithm There are a number of tests that the "recursive" backend does not handle correctly but which the redesign in "ort" will. Add a new helper in lib-merge.sh for selecting a different test expectation based on the setting of GIT_TEST_MERGE_ALGORITHM, and use it in various testcases to document that we expect them to be success by default but failure with certain algorithm. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- t/lib-merge.sh | 19 +++++++++++++++++++ t/t6416-recursive-corner-cases.sh | 11 ++++++----- t/t6422-merge-rename-corner-cases.sh | 7 ++++--- t/t6423-merge-rename-directories.sh | 13 +++++++------ t/t6426-merge-skip-unneeded-updates.sh | 3 ++- t/t6430-merge-recursive.sh | 3 ++- 6 files changed, 40 insertions(+), 16 deletions(-) create mode 100644 t/lib-merge.sh diff --git a/t/lib-merge.sh b/t/lib-merge.sh new file mode 100644 index 0000000000..efd8b9615c --- /dev/null +++ b/t/lib-merge.sh @@ -0,0 +1,19 @@ +# Helper functions used by merge tests. + +test_expect_merge_success() { + exception="$1" + : "${GIT_TEST_MERGE_ALGORITHM:=recursive}" + case ",$exception," in + *,$GIT_TEST_MERGE_ALGORITHM=failure,*) + shift + test_expect_failure "$@" ;; + *,$GIT_TEST_MERGE_ALGORITHM=*) + BUG "exception must be failure only" ;; + *=failure,) + shift + test_expect_success "$@" ;; + *) + # No exception + test_expect_success "$@" ;; + esac +} diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index fd98989b14..d13c1afb4a 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -3,6 +3,7 @@ test_description='recursive merge corner cases involving criss-cross merges' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh # # L1 L2 @@ -1069,7 +1070,7 @@ test_expect_success 'setup symlink modify/modify' ' ) ' -test_expect_failure 'check symlink modify/modify' ' +test_expect_merge_success recursive=failure 'check symlink modify/modify' ' ( cd symlink-modify-modify && @@ -1135,7 +1136,7 @@ test_expect_success 'setup symlink add/add' ' ) ' -test_expect_failure 'check symlink add/add' ' +test_expect_merge_success recursive=failure 'check symlink add/add' ' ( cd symlink-add-add && @@ -1223,7 +1224,7 @@ test_expect_success 'setup submodule modify/modify' ' ) ' -test_expect_failure 'check submodule modify/modify' ' +test_expect_merge_success recursive=failure 'check submodule modify/modify' ' ( cd submodule-modify-modify && @@ -1311,7 +1312,7 @@ test_expect_success 'setup submodule add/add' ' ) ' -test_expect_failure 'check submodule add/add' ' +test_expect_merge_success recursive=failure 'check submodule add/add' ' ( cd submodule-add-add && @@ -1386,7 +1387,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' ' ) ' -test_expect_failure 'check conflicting entry types (submodule vs symlink)' ' +test_expect_merge_success recursive=failure 'check conflicting entry types (submodule vs symlink)' ' ( cd submodule-symlink-add-add && diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh index 3375eaf4e7..0c8eb4df8a 100755 --- a/t/t6422-merge-rename-corner-cases.sh +++ b/t/t6422-merge-rename-corner-cases.sh @@ -4,6 +4,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses" # t6036 has corner cases that involve both criss-cross merges and renames . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh test_setup_rename_delete_untracked () { test_create_repo rename-delete-untracked && @@ -878,7 +879,7 @@ test_setup_rad () { ) } -test_expect_failure 'rad-check: rename/add/delete conflict' ' +test_expect_merge_success recursive=failure 'rad-check: rename/add/delete conflict' ' test_setup_rad && ( cd rad && @@ -951,7 +952,7 @@ test_setup_rrdd () { ) } -test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' +test_expect_merge_success recursive=failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' test_setup_rrdd && ( cd rrdd && @@ -1040,7 +1041,7 @@ test_setup_mod6 () { ) } -test_expect_failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' +test_expect_merge_success recursive=failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' test_setup_mod6 && ( cd mod6 && diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 06b46af765..249fbb6853 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -26,6 +26,7 @@ test_description="recursive merge with directory renames" # files that might be renamed into each other's paths.) . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh ########################################################################### @@ -1339,7 +1340,7 @@ test_setup_6b1 () { ) } -test_expect_failure '6b1: Same renames done on both sides, plus another rename' ' +test_expect_merge_success recursive=failure '6b1: Same renames done on both sides, plus another rename' ' test_setup_6b1 && ( cd 6b1 && @@ -1412,7 +1413,7 @@ test_setup_6b2 () { ) } -test_expect_failure '6b2: Same rename done on both sides' ' +test_expect_merge_success recursive=failure '6b2: Same rename done on both sides' ' test_setup_6b2 && ( cd 6b2 && @@ -3471,7 +3472,7 @@ test_setup_10e () { ) } -test_expect_failure '10e: Does git complain about untracked file that is not really in the way?' ' +test_expect_merge_success recursive=failure '10e: Does git complain about untracked file that is not really in the way?' ' test_setup_10e && ( cd 10e && @@ -4104,7 +4105,7 @@ test_setup_12b1 () { ) } -test_expect_failure '12b1: Moving two directory hierarchies into each other' ' +test_expect_merge_success recursive=failure '12b1: Moving two directory hierarchies into each other' ' test_setup_12b1 && ( cd 12b1 && @@ -4272,7 +4273,7 @@ test_setup_12c1 () { ) } -test_expect_failure '12c1: Moving one directory hierarchy into another w/ content merge' ' +test_expect_merge_success recursive=failure '12c1: Moving one directory hierarchy into another w/ content merge' ' test_setup_12c1 && ( cd 12c1 && @@ -4632,7 +4633,7 @@ test_setup_12f () { ) } -test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' ' +test_expect_merge_success recursive=failure '12f: Trivial directory resolve, caching, all kinds of fun' ' test_setup_12f && ( cd 12f && diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh index 699813671c..8510d4da8b 100755 --- a/t/t6426-merge-skip-unneeded-updates.sh +++ b/t/t6426-merge-skip-unneeded-updates.sh @@ -23,6 +23,7 @@ test_description="merge cases" # files that might be renamed into each other's paths.) . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh ########################################################################### @@ -666,7 +667,7 @@ test_setup_4a () { # correct requires doing the merge in-memory first, then realizing that no # updates to the file are necessary, and thus that we can just leave the path # alone. -test_expect_failure '4a: Change on A, change on B subset of A, dirty mods present' ' +test_expect_merge_success recursive=failure '4a: Change on A, change on B subset of A, dirty mods present' ' test_setup_4a && ( cd 4a && diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh index a328260d42..4795a7abd0 100755 --- a/t/t6430-merge-recursive.sh +++ b/t/t6430-merge-recursive.sh @@ -3,6 +3,7 @@ test_description='merge-recursive backend test' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh test_expect_success 'setup 1' ' @@ -641,7 +642,7 @@ test_expect_success 'merge-recursive copy vs. rename' ' test_cmp expected actual ' -test_expect_failure 'merge-recursive rename vs. rename/symlink' ' +test_expect_merge_success recursive=failure 'merge-recursive rename vs. rename/symlink' ' git checkout -f rename && git merge rename-ln &&
On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > > On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > +test_expect_merge_algorithm () { > > + status_for_recursive=$1 > > + shift > > + status_for_ort=$1 > > + shift > > + > > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > > + then > > + test_expect_${status_for_ort} "$@" > > + else > > + test_expect_${status_for_recursive} "$@" > > -test_expect_failure 'check symlink modify/modify' ' > > +test_expect_merge_algorithm failure success 'check symlink modify/modify' ' > > I find this series of "failure success" hard to decode without > understanding what it would be, then I need to keep rememberring which > status is corresponding with with algorithm. > > Perhaps this patch is a bit easier to read. This is largely based on > your patch. (I haven't read other patches, yet). > > What do you think? It is easier to read and I think something along these lines would make a lot of sense if this weren't a transient change (the idea is to eventually drop the recursive backend in favor of ort, and then these can all switch to just using test_expect_success). Maybe it still makes sense to make further changes here anyway, but if we do go this route, there are 1-2 things we can/should change: First, while a lot of my contributions aren't that important, and the new test_expect_* function certainly falls in that category, one of the driving goals behind a new merge algorithm was fixing up edge and corner cases that were just too problematic in the recursive backend. Thus, the patch where I get to flip the test expectation is one that I care about more than most out of the (I'm guessing on this number) 100+ patches that will be part of this new merge algorithm. Having you take over ownership of that patch thus isn't right; we should instead keep my original patch and apply your suggested changes on top (or have a patch from you introducing a new function first, and then have a patch from me using it to flip test expectations on top). Second, I think that lines like test_expect_merge_success recursive=failure ... read like a contradiction and are also confusing. I think it'd be better if it read something like test_expect_merge recursive=failure ort=success ... or something along those lines. > -------------8<------------ > From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= > <congdanhqx@gmail.com> > Date: Sat, 24 Oct 2020 17:41:02 +0700 > Subject: [PATCH] t/: new helper for testing merge that allow failure for some > algorithm > > There are a number of tests that the "recursive" backend does not handle > correctly but which the redesign in "ort" will. > > Add a new helper in lib-merge.sh for selecting a different test expectation > based on the setting of GIT_TEST_MERGE_ALGORITHM, and use it in various > testcases to document that we expect them to be success by default > but failure with certain algorithm. > > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > --- > t/lib-merge.sh | 19 +++++++++++++++++++ > t/t6416-recursive-corner-cases.sh | 11 ++++++----- > t/t6422-merge-rename-corner-cases.sh | 7 ++++--- > t/t6423-merge-rename-directories.sh | 13 +++++++------ > t/t6426-merge-skip-unneeded-updates.sh | 3 ++- > t/t6430-merge-recursive.sh | 3 ++- > 6 files changed, 40 insertions(+), 16 deletions(-) > create mode 100644 t/lib-merge.sh > > diff --git a/t/lib-merge.sh b/t/lib-merge.sh > new file mode 100644 > index 0000000000..efd8b9615c > --- /dev/null > +++ b/t/lib-merge.sh > @@ -0,0 +1,19 @@ > +# Helper functions used by merge tests. > + > +test_expect_merge_success() { > + exception="$1" > + : "${GIT_TEST_MERGE_ALGORITHM:=recursive}" > + case ",$exception," in > + *,$GIT_TEST_MERGE_ALGORITHM=failure,*) > + shift > + test_expect_failure "$@" ;; > + *,$GIT_TEST_MERGE_ALGORITHM=*) > + BUG "exception must be failure only" ;; > + *=failure,) > + shift > + test_expect_success "$@" ;; > + *) > + # No exception > + test_expect_success "$@" ;; > + esac > +} > diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh > index fd98989b14..d13c1afb4a 100755 > --- a/t/t6416-recursive-corner-cases.sh > +++ b/t/t6416-recursive-corner-cases.sh > @@ -3,6 +3,7 @@ > test_description='recursive merge corner cases involving criss-cross merges' > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-merge.sh > > # > # L1 L2 > @@ -1069,7 +1070,7 @@ test_expect_success 'setup symlink modify/modify' ' > ) > ' > > -test_expect_failure 'check symlink modify/modify' ' > +test_expect_merge_success recursive=failure 'check symlink modify/modify' ' > ( > cd symlink-modify-modify && > > @@ -1135,7 +1136,7 @@ test_expect_success 'setup symlink add/add' ' > ) > ' > > -test_expect_failure 'check symlink add/add' ' > +test_expect_merge_success recursive=failure 'check symlink add/add' ' > ( > cd symlink-add-add && > > @@ -1223,7 +1224,7 @@ test_expect_success 'setup submodule modify/modify' ' > ) > ' > > -test_expect_failure 'check submodule modify/modify' ' > +test_expect_merge_success recursive=failure 'check submodule modify/modify' ' > ( > cd submodule-modify-modify && > > @@ -1311,7 +1312,7 @@ test_expect_success 'setup submodule add/add' ' > ) > ' > > -test_expect_failure 'check submodule add/add' ' > +test_expect_merge_success recursive=failure 'check submodule add/add' ' > ( > cd submodule-add-add && > > @@ -1386,7 +1387,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' ' > ) > ' > > -test_expect_failure 'check conflicting entry types (submodule vs symlink)' ' > +test_expect_merge_success recursive=failure 'check conflicting entry types (submodule vs symlink)' ' > ( > cd submodule-symlink-add-add && > > diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh > index 3375eaf4e7..0c8eb4df8a 100755 > --- a/t/t6422-merge-rename-corner-cases.sh > +++ b/t/t6422-merge-rename-corner-cases.sh > @@ -4,6 +4,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses" > # t6036 has corner cases that involve both criss-cross merges and renames > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-merge.sh > > test_setup_rename_delete_untracked () { > test_create_repo rename-delete-untracked && > @@ -878,7 +879,7 @@ test_setup_rad () { > ) > } > > -test_expect_failure 'rad-check: rename/add/delete conflict' ' > +test_expect_merge_success recursive=failure 'rad-check: rename/add/delete conflict' ' > test_setup_rad && > ( > cd rad && > @@ -951,7 +952,7 @@ test_setup_rrdd () { > ) > } > > -test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' > +test_expect_merge_success recursive=failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' > test_setup_rrdd && > ( > cd rrdd && > @@ -1040,7 +1041,7 @@ test_setup_mod6 () { > ) > } > > -test_expect_failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' > +test_expect_merge_success recursive=failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' > test_setup_mod6 && > ( > cd mod6 && > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > index 06b46af765..249fbb6853 100755 > --- a/t/t6423-merge-rename-directories.sh > +++ b/t/t6423-merge-rename-directories.sh > @@ -26,6 +26,7 @@ test_description="recursive merge with directory renames" > # files that might be renamed into each other's paths.) > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-merge.sh > > > ########################################################################### > @@ -1339,7 +1340,7 @@ test_setup_6b1 () { > ) > } > > -test_expect_failure '6b1: Same renames done on both sides, plus another rename' ' > +test_expect_merge_success recursive=failure '6b1: Same renames done on both sides, plus another rename' ' > test_setup_6b1 && > ( > cd 6b1 && > @@ -1412,7 +1413,7 @@ test_setup_6b2 () { > ) > } > > -test_expect_failure '6b2: Same rename done on both sides' ' > +test_expect_merge_success recursive=failure '6b2: Same rename done on both sides' ' > test_setup_6b2 && > ( > cd 6b2 && > @@ -3471,7 +3472,7 @@ test_setup_10e () { > ) > } > > -test_expect_failure '10e: Does git complain about untracked file that is not really in the way?' ' > +test_expect_merge_success recursive=failure '10e: Does git complain about untracked file that is not really in the way?' ' > test_setup_10e && > ( > cd 10e && > @@ -4104,7 +4105,7 @@ test_setup_12b1 () { > ) > } > > -test_expect_failure '12b1: Moving two directory hierarchies into each other' ' > +test_expect_merge_success recursive=failure '12b1: Moving two directory hierarchies into each other' ' > test_setup_12b1 && > ( > cd 12b1 && > @@ -4272,7 +4273,7 @@ test_setup_12c1 () { > ) > } > > -test_expect_failure '12c1: Moving one directory hierarchy into another w/ content merge' ' > +test_expect_merge_success recursive=failure '12c1: Moving one directory hierarchy into another w/ content merge' ' > test_setup_12c1 && > ( > cd 12c1 && > @@ -4632,7 +4633,7 @@ test_setup_12f () { > ) > } > > -test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' ' > +test_expect_merge_success recursive=failure '12f: Trivial directory resolve, caching, all kinds of fun' ' > test_setup_12f && > ( > cd 12f && > diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh > index 699813671c..8510d4da8b 100755 > --- a/t/t6426-merge-skip-unneeded-updates.sh > +++ b/t/t6426-merge-skip-unneeded-updates.sh > @@ -23,6 +23,7 @@ test_description="merge cases" > # files that might be renamed into each other's paths.) > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-merge.sh > > > ########################################################################### > @@ -666,7 +667,7 @@ test_setup_4a () { > # correct requires doing the merge in-memory first, then realizing that no > # updates to the file are necessary, and thus that we can just leave the path > # alone. > -test_expect_failure '4a: Change on A, change on B subset of A, dirty mods present' ' > +test_expect_merge_success recursive=failure '4a: Change on A, change on B subset of A, dirty mods present' ' > test_setup_4a && > ( > cd 4a && > diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh > index a328260d42..4795a7abd0 100755 > --- a/t/t6430-merge-recursive.sh > +++ b/t/t6430-merge-recursive.sh > @@ -3,6 +3,7 @@ > test_description='merge-recursive backend test' > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-merge.sh > > test_expect_success 'setup 1' ' > > @@ -641,7 +642,7 @@ test_expect_success 'merge-recursive copy vs. rename' ' > test_cmp expected actual > ' > > -test_expect_failure 'merge-recursive rename vs. rename/symlink' ' > +test_expect_merge_success recursive=failure 'merge-recursive rename vs. rename/symlink' ' > > git checkout -f rename && > git merge rename-ln && > -- > 2.29.0.rc1
On 2020-10-24 09:53:18-0700, Elijah Newren <newren@gmail.com> wrote: > On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh > <congdanhqx@gmail.com> wrote: > > > > On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > > +test_expect_merge_algorithm () { > > > + status_for_recursive=$1 > > > + shift > > > + status_for_ort=$1 > > > + shift > > > + > > > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > > > + then > > > + test_expect_${status_for_ort} "$@" > > > + else > > > + test_expect_${status_for_recursive} "$@" > > > -test_expect_failure 'check symlink modify/modify' ' > > > +test_expect_merge_algorithm failure success 'check symlink modify/modify' ' > > > > I find this series of "failure success" hard to decode without > > understanding what it would be, then I need to keep rememberring which > > status is corresponding with with algorithm. > > > > Perhaps this patch is a bit easier to read. This is largely based on > > your patch. (I haven't read other patches, yet). > > > > What do you think? > > It is easier to read and I think something along these lines would > make a lot of sense if this weren't a transient change (the idea is to > eventually drop the recursive backend in favor of ort, and then these > can all switch to just using test_expect_success). Maybe it still > makes sense to make further changes here anyway, but if we do go this > route, there are 1-2 things we can/should change: > > First, while a lot of my contributions aren't that important, and the Mine aren't that important, either > new test_expect_* function certainly falls in that category, one of > the driving goals behind a new merge algorithm was fixing up edge and > corner cases that were just too problematic in the recursive backend. > Thus, the patch where I get to flip the test expectation is one that I > care about more than most out of the (I'm guessing on this number) Make sense. > 100+ patches that will be part of this new merge algorithm. Having > you take over ownership of that patch thus isn't right; we should > instead keep my original patch and apply your suggested changes on top > (or have a patch from you introducing a new function first, and then > have a patch from me using it to flip test expectations on top). You can take back the ownership, the patch was based on yours, anyway. I wrote like that since I need to rewrite part of the message to match with my changes ;) No need to generate extra noise of additional patch. > Second, I think that lines like > test_expect_merge_success recursive=failure ... > read like a contradiction and are also confusing. I think it'd be > better if it read something like > test_expect_merge recursive=failure ort=success ... > or something along those lines. When I wrote the patch, I was expecting something like test_expect_merge_success recursive=failure,other=failure ... in order to merge all algorithm into single parameters. How about something like: test_expect_merge_success exception=recursive,other ... Not that we have "other" algorithm to begin with. Thanks, -- Danh
On Sun, Oct 25, 2020 at 6:49 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > > On 2020-10-24 09:53:18-0700, Elijah Newren <newren@gmail.com> wrote: > > On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh > > <congdanhqx@gmail.com> wrote: > > > > > > On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > > > > +test_expect_merge_algorithm () { > > > > + status_for_recursive=$1 > > > > + shift > > > > + status_for_ort=$1 > > > > + shift > > > > + > > > > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > > > > + then > > > > + test_expect_${status_for_ort} "$@" > > > > + else > > > > + test_expect_${status_for_recursive} "$@" > > > > -test_expect_failure 'check symlink modify/modify' ' > > > > +test_expect_merge_algorithm failure success 'check symlink modify/modify' ' > > > > > > I find this series of "failure success" hard to decode without > > > understanding what it would be, then I need to keep rememberring which > > > status is corresponding with with algorithm. > > > > > > Perhaps this patch is a bit easier to read. This is largely based on > > > your patch. (I haven't read other patches, yet). > > > > > > What do you think? > > > > It is easier to read and I think something along these lines would > > make a lot of sense if this weren't a transient change (the idea is to > > eventually drop the recursive backend in favor of ort, and then these > > can all switch to just using test_expect_success). Maybe it still > > makes sense to make further changes here anyway, but if we do go this > > route, there are 1-2 things we can/should change: > > > > First, while a lot of my contributions aren't that important, and the > > Mine aren't that important, either > > > new test_expect_* function certainly falls in that category, one of > > the driving goals behind a new merge algorithm was fixing up edge and > > corner cases that were just too problematic in the recursive backend. > > Thus, the patch where I get to flip the test expectation is one that I > > care about more than most out of the (I'm guessing on this number) > > Make sense. > > > 100+ patches that will be part of this new merge algorithm. Having > > you take over ownership of that patch thus isn't right; we should > > instead keep my original patch and apply your suggested changes on top > > (or have a patch from you introducing a new function first, and then > > have a patch from me using it to flip test expectations on top). > > You can take back the ownership, the patch was based on yours, anyway. > > I wrote like that since I need to rewrite part of the message to match > with my changes ;) > > No need to generate extra noise of additional patch. Just to be clear, others have made suggestions like yours in the past where they've taken over ownership of a patch in a series and I've been totally fine with it. Your suggestion to do the same would have been fine here, but I'm just kinda attached to being able to flip the test expectation for these tests; I've been working towards it for a _long_ time. I think an extra patch, attributed to you, actually makes the most sense here. > > Second, I think that lines like > > test_expect_merge_success recursive=failure ... > > read like a contradiction and are also confusing. I think it'd be > > better if it read something like > > test_expect_merge recursive=failure ort=success ... > > or something along those lines. > > When I wrote the patch, I was expecting something like > > test_expect_merge_success recursive=failure,other=failure ... > > in order to merge all algorithm into single parameters. > > How about something like: > > test_expect_merge_success exception=recursive,other ... > > Not that we have "other" algorithm to begin with. Sure, sounds great. I wouldn't spend any time trying to make it work with a 3rd backend, though. The goal is to have two merge backends only long enough for people to become comfortable with the new backend and discover any unknown issues with it that we can fix, then we'll rip it out the old "recursive" backend and we'll translate any requests for "recursive" to mean "ort". We'll also rip the test_expect_merge_success() function out since it'll be unneeded (so efforts towards future proofing of that function will be wasted). Then years will go by before another merge backend comes along, if one ever does.
Elijah Newren <newren@gmail.com> writes: >> When I wrote the patch, I was expecting something like >> >> test_expect_merge_success recursive=failure,other=failure ... >> >> in order to merge all algorithm into single parameters. >> >> How about something like: >> >> test_expect_merge_success exception=recursive,other ... >> >> Not that we have "other" algorithm to begin with. > > Sure, sounds great. I wouldn't spend any time trying to make it work > with a 3rd backend, though. Yup. I'd appreciate if the lines become a bit shorter while at it, though. test_merge_both [<exception>] '<title>' ' <body> ' that expects success unless otherwise told, and <exception> like "failure=recursive" can be used to tell us to expect differently, would work well?
diff --git a/t/lib-merge.sh b/t/lib-merge.sh new file mode 100644 index 0000000000..fac2bc5919 --- /dev/null +++ b/t/lib-merge.sh @@ -0,0 +1,15 @@ +# Helper functions used by merge tests. + +test_expect_merge_algorithm () { + status_for_recursive=$1 + shift + status_for_ort=$1 + shift + + if test "$GIT_TEST_MERGE_ALGORITHM" = ort + then + test_expect_${status_for_ort} "$@" + else + test_expect_${status_for_recursive} "$@" + fi +} diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index fd98989b14..8b3a4fc843 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -3,6 +3,7 @@ test_description='recursive merge corner cases involving criss-cross merges' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh # # L1 L2 @@ -1069,7 +1070,7 @@ test_expect_success 'setup symlink modify/modify' ' ) ' -test_expect_failure 'check symlink modify/modify' ' +test_expect_merge_algorithm failure success 'check symlink modify/modify' ' ( cd symlink-modify-modify && @@ -1135,7 +1136,7 @@ test_expect_success 'setup symlink add/add' ' ) ' -test_expect_failure 'check symlink add/add' ' +test_expect_merge_algorithm failure success 'check symlink add/add' ' ( cd symlink-add-add && @@ -1223,7 +1224,7 @@ test_expect_success 'setup submodule modify/modify' ' ) ' -test_expect_failure 'check submodule modify/modify' ' +test_expect_merge_algorithm failure success 'check submodule modify/modify' ' ( cd submodule-modify-modify && @@ -1311,7 +1312,7 @@ test_expect_success 'setup submodule add/add' ' ) ' -test_expect_failure 'check submodule add/add' ' +test_expect_merge_algorithm failure success 'check submodule add/add' ' ( cd submodule-add-add && @@ -1386,7 +1387,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' ' ) ' -test_expect_failure 'check conflicting entry types (submodule vs symlink)' ' +test_expect_merge_algorithm failure success 'check conflicting entry types (submodule vs symlink)' ' ( cd submodule-symlink-add-add && diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh index 3375eaf4e7..58729593ba 100755 --- a/t/t6422-merge-rename-corner-cases.sh +++ b/t/t6422-merge-rename-corner-cases.sh @@ -4,6 +4,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses" # t6036 has corner cases that involve both criss-cross merges and renames . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh test_setup_rename_delete_untracked () { test_create_repo rename-delete-untracked && @@ -878,7 +879,7 @@ test_setup_rad () { ) } -test_expect_failure 'rad-check: rename/add/delete conflict' ' +test_expect_merge_algorithm failure success 'rad-check: rename/add/delete conflict' ' test_setup_rad && ( cd rad && @@ -951,7 +952,7 @@ test_setup_rrdd () { ) } -test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' +test_expect_merge_algorithm failure success 'rrdd-check: rename/rename(2to1)/delete/delete conflict' ' test_setup_rrdd && ( cd rrdd && @@ -1040,7 +1041,7 @@ test_setup_mod6 () { ) } -test_expect_failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' +test_expect_merge_algorithm failure success 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' ' test_setup_mod6 && ( cd mod6 && diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 06b46af765..807a424a52 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -26,6 +26,7 @@ test_description="recursive merge with directory renames" # files that might be renamed into each other's paths.) . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh ########################################################################### @@ -1339,7 +1340,7 @@ test_setup_6b1 () { ) } -test_expect_failure '6b1: Same renames done on both sides, plus another rename' ' +test_expect_merge_algorithm failure success '6b1: Same renames done on both sides, plus another rename' ' test_setup_6b1 && ( cd 6b1 && @@ -1412,7 +1413,7 @@ test_setup_6b2 () { ) } -test_expect_failure '6b2: Same rename done on both sides' ' +test_expect_merge_algorithm failure success '6b2: Same rename done on both sides' ' test_setup_6b2 && ( cd 6b2 && @@ -3471,7 +3472,7 @@ test_setup_10e () { ) } -test_expect_failure '10e: Does git complain about untracked file that is not really in the way?' ' +test_expect_merge_algorithm failure success '10e: Does git complain about untracked file that is not really in the way?' ' test_setup_10e && ( cd 10e && @@ -4104,7 +4105,7 @@ test_setup_12b1 () { ) } -test_expect_failure '12b1: Moving two directory hierarchies into each other' ' +test_expect_merge_algorithm failure success '12b1: Moving two directory hierarchies into each other' ' test_setup_12b1 && ( cd 12b1 && @@ -4272,7 +4273,7 @@ test_setup_12c1 () { ) } -test_expect_failure '12c1: Moving one directory hierarchy into another w/ content merge' ' +test_expect_merge_algorithm failure success '12c1: Moving one directory hierarchy into another w/ content merge' ' test_setup_12c1 && ( cd 12c1 && @@ -4632,7 +4633,7 @@ test_setup_12f () { ) } -test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' ' +test_expect_merge_algorithm failure success '12f: Trivial directory resolve, caching, all kinds of fun' ' test_setup_12f && ( cd 12f && diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh index 699813671c..d7eeee4310 100755 --- a/t/t6426-merge-skip-unneeded-updates.sh +++ b/t/t6426-merge-skip-unneeded-updates.sh @@ -23,6 +23,7 @@ test_description="merge cases" # files that might be renamed into each other's paths.) . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh ########################################################################### @@ -666,7 +667,7 @@ test_setup_4a () { # correct requires doing the merge in-memory first, then realizing that no # updates to the file are necessary, and thus that we can just leave the path # alone. -test_expect_failure '4a: Change on A, change on B subset of A, dirty mods present' ' +test_expect_merge_algorithm failure success '4a: Change on A, change on B subset of A, dirty mods present' ' test_setup_4a && ( cd 4a && diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh index a328260d42..9c08e63af2 100755 --- a/t/t6430-merge-recursive.sh +++ b/t/t6430-merge-recursive.sh @@ -3,6 +3,7 @@ test_description='merge-recursive backend test' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-merge.sh test_expect_success 'setup 1' ' @@ -641,7 +642,7 @@ test_expect_success 'merge-recursive copy vs. rename' ' test_cmp expected actual ' -test_expect_failure 'merge-recursive rename vs. rename/symlink' ' +test_expect_merge_algorithm failure success 'merge-recursive rename vs. rename/symlink' ' git checkout -f rename && git merge rename-ln &&