On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@xxxxxxxxx> wrote:
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> t/t6043-merge-rename-directories.sh | 391 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 391 insertions(+)
> create mode 100755 t/t6043-merge-rename-directories.sh
>
> diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
> new file mode 100755
> index 0000000000..b737b0a105
> --- /dev/null
> +++ b/t/t6043-merge-rename-directories.sh
> @@ -0,0 +1,391 @@
> +#!/bin/sh
> +
> +test_description="recursive merge with directory renames"
> +# includes checking of many corner cases, with a similar methodology to:
> +# t6042: corner cases with renames but not criss-cross merges
> +# t6036: corner cases with both renames and criss-cross merges
> +#
> +# The setup for all of them, pictorially, is:
> +#
> +# B
> +# o
> +# / \
> +# A o ?
> +# \ /
> +# o
> +# C
> +#
> +# To help make it easier to follow the flow of tests, they have been
> +# divided into sections and each test will start with a quick explanation
> +# of what commits A, B, and C contain.
> +#
> +# Notation:
> +# z/{b,c} means files z/b and z/c both exist
> +# x/d_1 means file x/d exists with content d1. (Purpose of the
> +# underscore notation is to differentiate different
> +# files that might be renamed into each other's paths.)
> +
> +. ./test-lib.sh
> +
> +
> +###########################################################################
> +# SECTION 1: Basic cases we should be able to handle
> +###########################################################################
> +
> +# Testcase 1a, Basic directory rename.
> +# Commit A: z/{b,c}
> +# Commit B: y/{b,c}
> +# Commit C: z/{b,c,d,e/f}
(minor thought:)
After rereading the docs above this is clear; I wonder if instead of A, B, C
a notation of Base, ours, theirs would be easier to understand?
> +test_expect_success '1a-setup: Simple directory rename detection' '
> +test_expect_failure '1a-check: Simple directory rename detection' '
Thanks for splitting the setup and the check into two different test cases!
> + git checkout B^0 &&
Any reason for ^0 ? (to make clear it is a branch?)
> +# Testcase 1b, Merge a directory with another
> +# Commit A: z/{b,c}, y/d
> +# Commit B: z/{b,c,e}, y/d
> +# Commit C: y/{b,c,d}
> +# Expected: y/{b,c,d,e}
> +
> +test_expect_success '1b-setup: Merge a directory with another' '
> + git rm -rf . &&
> + git clean -fdqx &&
> + rm -rf .git &&
> + git init &&
This is quite a strong statement to start a test with.
Nowadays we rather do
test_when_finished "some command" &&
than polluting the follow up tests. But as you split up the previous test
into 2 tests, it is not clear if this would bring any good.
Also these are four cleanup commands, I'd have expected fewer.
Maybe just "rm -rf ." ? Or as we make a new repo for each test case,
test_create_repo 1a &&
cd 1a
in the first setup, and here we do
test_create_repo 1b &&
cd 1b
relying on test_done to cleanup everything afterwards?
> +# Testcase 1c, Transitive renaming
> +# (Related to testcases 3a and 6d -- when should a transitive rename apply?)
> +# (Related to testcases 9c and 9d -- can transitivity repeat?)
> +# Commit A: z/{b,c}, x/d
> +# Commit B: y/{b,c}, x/d
> +# Commit C: z/{b,c,d}
So B is "Rename z to y" and C is "Move x/d to z/d"
> +# Expected: y/{b,c,d} (because x/d -> z/d -> y/d)
This is an excellent expectation for a clean case like this.
I have not reached 3, 9 yet, so I'll remember these questions.
> +test_expect_success '1c-setup: Transitive renaming' '
> + git rm -rf . &&
> + git clean -fdqx &&
> + rm -rf .git &&
> + git init &&
> +
> + mkdir z &&
> + echo b >z/b &&
> + echo c >z/c &&
> + mkdir x &&
> + echo d >x/d &&
> + git add z x &&
> + test_tick &&
> + git commit -m "A" &&
> +
> + git branch A &&
> + git branch B &&
> + git branch C &&
> +
> + git checkout B &&
> + git mv z y &&
> + test_tick &&
> + git commit -m "B" &&
> +
> + git checkout C &&
> + git mv x/d z/d &&
> + test_tick &&
> + git commit -m "C"
> +'
> +
> +test_expect_failure '1c-check: Transitive renaming' '
> + git checkout B^0 &&
> +
> + git merge -s recursive C^0 &&
> +
> + test 3 -eq $(git ls-files -s | wc -l) &&
git ls-files >out &&
test_line_count = 3 out &&
maybe? Piping output of git commands somewhere is an
anti-pattern as we cannot examine the return code of ls-files in this case.
> + test $(git rev-parse HEAD:y/b) = $(git rev-parse A:z/b) &&
> + test $(git rev-parse HEAD:y/c) = $(git rev-parse A:z/c) &&
> + test $(git rev-parse HEAD:y/d) = $(git rev-parse A:x/d) &&
Speaking of these, there are quite a lot of invocations of rev-parse,
though it looks clean; recently Junio had some good ideas regarding a
similar test[1].
So maybe
git rev-parse >actual \
HEAD:y/b HEAD:y/c HEAD:y/d &&
git rev-parse >expect \
A:z/b A:z/c A:x/d &&
test_cmp expect actual
Not sure if that is any nicer, but has fewer calls.
[1] https://public-inbox.org/git/xmqqa807ztx4.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
> + test_must_fail git rev-parse HEAD:x/d &&
> + test_must_fail git rev-parse HEAD:z/d &&
> + test ! -f z/d
> +'
> +
> +# Testcase 1d, Directory renames (merging two directories into one new one)
> +# cause a rename/rename(2to1) conflict
> +# (Related to testcases 1c and 7b)
> +# Commit A. z/{b,c}, y/{d,e}
> +# Commit B. x/{b,c}, y/{d,e,m,wham}
> +# Commit C. z/{b,c,n,wham}, x/{d,e}
> +# Expected: x/{b,c,d,e,m,n}, CONFLICT:(y/wham & z/wham -> x/wham)
> +# Note: y/m & z/n should definitely move into x. By the same token, both
> +# y/wham & z/wham should to...giving us a conflict.
If wham are equal (in oid), shouldn't this not conflict and only conflict
when z/wham and x/wham differ in oid, but have the same sub-path?
> +
> +# Testcase 1e, Renamed directory, with all filenames being renamed too
> +# Commit A: z/{oldb,oldc}
> +# Commit B: y/{newb,newc}
> +# Commit C: z/{oldb,oldc,d}
What about oldd ?
(expecting a pattern across many files of s/old/new/ isn't unreasonable,
but maybe too much for now?)
By having no "old" prefix there is only one thing to do, which is y/d
> +# Expected: y/{newb,newc,d}
ok.
> +# Testcase 1f, Split a directory into two other directories
> +# (Related to testcases 3a, all of section 2, and all of section 4)
> +# Commit A: z/{b,c,d,e,f}
> +# Commit B: z/{b,c,d,e,f,g}
> +# Commit C: y/{b,c}, x/{d,e,f}
> +# Expected: y/{b,c}, x/{d,e,f,g}
Why not y/g ? Because there are more files in x?
I can come up with a reasonable counter example:
A: src/{main.c, foo.c, bar.c, magic.py}
B: src/{main.c, foo.c, bar.c, magic.py, magic-helper.py}
C: src/{main.c, foo.c, bar.c} py/{magic.py}
Expect: src/{main.c, foo.c, bar.c} py/{magic.py, magic-helper.py}
> +
> +###########################################################################
> +# Rules suggested by testcases in section 1:
> +#
> +# We should still detect the directory rename even if it wasn't just
> +# the directory renamed, but the files within it. (see 1b)
> +#
> +# If renames split a directory into two or more others, the directory
> +# with the most renames, "wins" (see 1c). However, see the testcases
> +# in section 2, plus testcases 3a and 4a.
oh, ok. I presume testcases 2 follows in a later patch.
I'll go looking.
Thanks,
Stefan
