On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren [off-list ref] wrote:

(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@gitster.mtv.corp.google.com/

+       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