Re: [PATCH v5 13/34] directory rename detection: tests for handling overwriting untracked files

["SZEDER Gábor" <szeder.dev@gmail.com>]

> ---
>  t/t6043-merge-rename-directories.sh | 337 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 337 insertions(+)

> +test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' '
> +	(
> +		cd 10b &&
> +
> +		git checkout A^0 &&
> +		echo very >y/c &&
> +		echo important >y/d &&
> +		echo contents >y/e &&
> +
> +		test_must_fail git merge -s recursive B^0 >out 2>err &&
> +		test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d left in tree at y/d~B^0" out &&
> +		test_i18ngrep "Error: Refusing to lose untracked file at y/e; writing to y/e~B^0 instead" out &&
> +
> +		test 3 -eq $(git ls-files -s | wc -l) &&
> +		test 2 -eq $(git ls-files -u | wc -l) &&
> +		test 5 -eq $(git ls-files -o | wc -l) &&
> +
> +		test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&

There is a test helper for that :)

  test_cmp_rev :0:y/b O:z/b

Note, that this is not only a matter of useful output on failure, but
also that of correctness and robustness.  A failing command inside
those command substitutions won't cause the whole command above to
fail, and if both 'git rev-parse' were to fail without writing
anything to stdout, the whole condition would still be fulfilled:

  $ test $(false) = $(false) && echo true
  true

I noticed that this patch series adds several similar

  test $(git hash-object this) = $(git rev-parse that)

conditions.  Well, for that we don't have a test helper
function.  Similar 'hash-object = rev-parse' comparisons are already
present in two other test scripts, so perhaps it's worth adding a
helper function.  Or you could perhaps

  git cat-file -p that >out &&
  test_cmp this out

I also noticed that all existing 'hash-object = rev-parse' conditions
came from you, so I would leave it up to you to decide which is easier
to work with and whether it's worth it.


> +		test "very" = "$(cat y/c)" &&
> +
> +		test "important" = "$(cat y/d)" &&

The 'verbose' helper could make conditions like these more, well,
verbose about their failure:

  verbose test "very" = "$(cat y/c)" &&

> +		test "important" != "$(git rev-parse :3:y/d)" &&

I'm not sure what this condition is supposed to check.

I'm not particularly well versed in the intricacies of 'git rev-parse'
operating on different stages of the index, but to my understanding
'git rev-parse rev' either outputs the object name pointed by 'rev',
or 'rev' verbatim if that doesn't resolve to a valid object.  IOW, it
would never output "important" and the condition would always be
fulfilled.
What am I missing?

> +		test $(git rev-parse :3:y/d) = $(git rev-parse O:z/c) &&
> +
> +		test "contents" = "$(cat y/e)" &&
> +		test "contents" != "$(git rev-parse :3:y/e)" &&
> +		test $(git rev-parse :3:y/e) = $(git rev-parse B:z/e)
> +	)
> +'