Re: [PATCH 04/30] directory rename detection: basic testcases

[Stefan Beller <sbeller@google.com>]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@gmail.com> wrote:
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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@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