{"thread":{"id":"52650","subject":"[PATCH] t7800: don't rely on reuse_worktree_file()","startedAt":"2020-01-16T18:19:44Z","lastAt":"2020-01-16T22:48:28Z","messageCount":3,"participants":["Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389905","messageId":"20200116181940.GA2945961@coredump.intra.peff.net","threadId":"52650","inReplyTo":null,"subject":"[PATCH] t7800: don't rely on reuse_worktree_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-16T18:19:40Z","receivedAt":"2020-01-16T18:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A test in t7800 tries to make sure that when git-difftool runs an\nexternal tool that fails, it stops looking at files. Our fake failing\ntool prints the file name it was asked to diff before exiting non-zero,\nand then we confirm the output contains only that file.\n\nHowever, this subtly relies on our internal reuse_worktree_file().\nBecause we're diffing between branches, the command run by difftool\nmight see:\n\n  - the git-stored filename (e.g., \"file\"), if we decided that the\n    working tree contents were up-to-date with the object in the index\n    and HEAD, and we could reuse them\n\n  - a temporary filename (e.g. \"/tmp/abc123_file\") if we had to dump the\n    contents from the object database\n\nIf the latter case happens, then the test fails, because it's expecting\nthe string \"file\". I discovered this when debugging something unrelated\nwith reuse_worktree_file(). I _thought_ it should be able to be\ntriggered by a racy-git situation, but running:\n\n  ./t7800-difftool.sh --stress --run=2,13\n\nnever seems to fail. However, by my reading of reuse_worktree_file(),\nthis would probably always fail under Cygwin, because it sets\nNO_FAST_WORKING_DIRECTORY. At any rate, since reuse_worktree_file()\nis meant to be an optimization that may or may not trigger, our test\nshould be robust either way.\n\nInstead of checking the filename, let's just make sure we got a single\nline of output (which would not be true if we continued after the first\nfailure).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7800-difftool.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 6bac9ed180..29b92907e2 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -125,15 +125,14 @@ test_expect_success 'difftool stops on error with --trust-exit-code' '\n \ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n \ttest_when_finished \"git reset -- for-diff\" &&\n \twrite_script .git/fail-right-file <<-\\EOF &&\n-\techo \"$2\"\n+\techo failed\n \texit 1\n \tEOF\n \t>for-diff &&\n \tgit add for-diff &&\n-\techo file >expect &&\n \ttest_must_fail git difftool -y --trust-exit-code \\\n \t\t--extcmd .git/fail-right-file branch >actual &&\n-\ttest_cmp expect actual\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'difftool honors exit status if command not found' '\n-- \n2.25.0.318.gee4019ba55\n"},{"id":"389929","messageId":"nycvar.QRO.7.76.6.2001162207220.46@tvgsbejvaqbjf.bet","threadId":"52650","inReplyTo":"20200116181940.GA2945961@coredump.intra.peff.net","subject":"Re: [PATCH] t7800: don't rely on reuse_worktree_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-16T21:08:22Z","receivedAt":"2020-01-16T21:08:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 16 Jan 2020, Jeff King wrote:\n\n> A test in t7800 tries to make sure that when git-difftool runs an\n> external tool that fails, it stops looking at files. Our fake failing\n> tool prints the file name it was asked to diff before exiting non-zero,\n> and then we confirm the output contains only that file.\n>\n> However, this subtly relies on our internal reuse_worktree_file().\n> Because we're diffing between branches, the command run by difftool\n> might see:\n>\n>   - the git-stored filename (e.g., \"file\"), if we decided that the\n>     working tree contents were up-to-date with the object in the index\n>     and HEAD, and we could reuse them\n>\n>   - a temporary filename (e.g. \"/tmp/abc123_file\") if we had to dump the\n>     contents from the object database\n>\n> If the latter case happens, then the test fails, because it's expecting\n> the string \"file\". I discovered this when debugging something unrelated\n> with reuse_worktree_file(). I _thought_ it should be able to be\n> triggered by a racy-git situation, but running:\n>\n>   ./t7800-difftool.sh --stress --run=2,13\n>\n> never seems to fail. However, by my reading of reuse_worktree_file(),\n> this would probably always fail under Cygwin, because it sets\n> NO_FAST_WORKING_DIRECTORY. At any rate, since reuse_worktree_file()\n> is meant to be an optimization that may or may not trigger, our test\n> should be robust either way.\n>\n> Instead of checking the filename, let's just make sure we got a single\n> line of output (which would not be true if we continued after the first\n> failure).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nThe reasoning and the patch seem sound to me.\n\nThanks,\nDscho\n\n>  t/t7800-difftool.sh | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 6bac9ed180..29b92907e2 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -125,15 +125,14 @@ test_expect_success 'difftool stops on error with --trust-exit-code' '\n>  \ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n>  \ttest_when_finished \"git reset -- for-diff\" &&\n>  \twrite_script .git/fail-right-file <<-\\EOF &&\n> -\techo \"$2\"\n> +\techo failed\n>  \texit 1\n>  \tEOF\n>  \t>for-diff &&\n>  \tgit add for-diff &&\n> -\techo file >expect &&\n>  \ttest_must_fail git difftool -y --trust-exit-code \\\n>  \t\t--extcmd .git/fail-right-file branch >actual &&\n> -\ttest_cmp expect actual\n> +\ttest_line_count = 1 actual\n>  '\n>\n>  test_expect_success 'difftool honors exit status if command not found' '\n> --\n> 2.25.0.318.gee4019ba55\n>\n"},{"id":"389949","messageId":"xmqqo8v3f26k.fsf@gitster-ct.c.googlers.com","threadId":"52650","inReplyTo":"20200116181940.GA2945961@coredump.intra.peff.net","subject":"Re: [PATCH] t7800: don't rely on reuse_worktree_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T22:48:19Z","receivedAt":"2020-01-16T22:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... At any rate, since reuse_worktree_file()\n> is meant to be an optimization that may or may not trigger, our test\n> should be robust either way.\n>\n> Instead of checking the filename, let's just make sure we got a single\n> line of output (which would not be true if we continued after the first\n> failure).\n\nMakes sense.  Thanks for spotting.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t7800-difftool.sh | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 6bac9ed180..29b92907e2 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -125,15 +125,14 @@ test_expect_success 'difftool stops on error with --trust-exit-code' '\n>  \ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n>  \ttest_when_finished \"git reset -- for-diff\" &&\n>  \twrite_script .git/fail-right-file <<-\\EOF &&\n> -\techo \"$2\"\n> +\techo failed\n>  \texit 1\n>  \tEOF\n>  \t>for-diff &&\n>  \tgit add for-diff &&\n> -\techo file >expect &&\n>  \ttest_must_fail git difftool -y --trust-exit-code \\\n>  \t\t--extcmd .git/fail-right-file branch >actual &&\n> -\ttest_cmp expect actual\n> +\ttest_line_count = 1 actual\n>  '\n>  \n>  test_expect_success 'difftool honors exit status if command not found' '\n"}]}