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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 4, 2018, 00:52 UTC
Message-ID: <20180104005223.13566-1-szeder.dev@gmail.com>
In-Reply-To: <20171228041352.27880-14-newren@gmail.com>

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

Show 18 quoted lines

> +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?

Show 7 quoted lines

> +		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)
> +	)
> +'