{"thread":{"id":"27507","subject":"diff: --quiet does not imply --exit-code if --diff-filter is present","startedAt":"2011-05-31T10:34:39Z","lastAt":"2011-06-01T09:41:48Z","messageCount":8,"participants":["Yasushi SHOJI","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"169047","messageId":"87wrh7jgzk.wl@dns1.atmark-techno.com","threadId":"27507","inReplyTo":null,"subject":"diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2011-05-31T10:34:39Z","receivedAt":"2011-05-31T10:34:39Z","isPatch":false,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"Hi,\n\njust noticed that quiet does not return exit code when I set\ndiff-filter. at the current tip (v1.7.5.3-401-gfb674d7):\n\n git diff --quiet --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 0\n git diff --exit-code --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 1\n\nthese two line returns different exit code.\n\nis this a bug or a feature?\n\nThanks,\n-- \n yashi\n"},{"id":"169052","messageId":"20110531153356.GB2594@sigill.intra.peff.net","threadId":"27507","inReplyTo":"87wrh7jgzk.wl@dns1.atmark-techno.com","subject":"Re: diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-31T15:33:56Z","receivedAt":"2011-05-31T15:33:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2011 at 07:34:39PM +0900, Yasushi SHOJI wrote:\n\n> just noticed that quiet does not return exit code when I set\n> diff-filter. at the current tip (v1.7.5.3-401-gfb674d7):\n> \n> git diff --quiet --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 0\n> git diff --exit-code --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 1\n> \n> these two line returns different exit code.\n> \n> is this a bug or a feature?\n\nIt's a bug.\n\n-- >8 --\nSubject: [PATCH] diff_tree: disable QUICK optimization with diff filter\n\nWe stop looking for changes early with QUICK, so our diff\nqueue contains only a subset of the changes. However, we\ndon't apply diff filters until later; it will appear at that\npoint as though there are no changes matching our filter,\nwhen in reality we simply didn't keep looking for changes\nlong enough.\n\nCommit 2cfe8a6 (diff --quiet: disable optimization when\n--diff-filter=X is used, 2011-03-16) fixes this in some\ncases by disabling the optimization when a filter is\npresent. However, it only tweaked run_diff_files, missing\nthe similar case in diff_tree. Thus the fix worked only for\ndiffing the working tree and index, but not between trees.\n\nNoticed by Yasushi SHOJI.\n\nSigned-off-by: Jeff King \n---\nGrepping around, I think this was the only other missed case.\n\n t/t4040-whitespace-status.sh | 5 +++++\n tree-diff.c | 1 +\n 2 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4040-whitespace-status.sh b/t/t4040-whitespace-status.sh\nindex abc4934..3c728a3 100755\n--- a/t/t4040-whitespace-status.sh\n+++ b/t/t4040-whitespace-status.sh\n@@ -67,4 +67,9 @@ test_expect_success 'diff-files --diff-filter --quiet' '\n \ttest_must_fail git diff-files --diff-filter=M --quiet\n '\n \n+test_expect_success 'diff-tree --diff-filter --quiet' '\n+\tgit commit -a -m \"worktree state\" &&\n+\ttest_must_fail git diff-tree --diff-filter=M --quiet HEAD^ HEAD\n+'\n+\n test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 7a79660..3d08f78 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -143,6 +143,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \n \tfor (;;) {\n \t\tif (DIFF_OPT_TST(opt, QUICK) &&\n+\t\t !opt->filter &&\n \t\t DIFF_OPT_TST(opt, HAS_CHANGES))\n \t\t\tbreak;\n \t\tif (opt->pathspec.nr) {\n-- \n1.7.5.3.7.g7dde6.dirty\n"},{"id":"169054","messageId":"7vvcwqkh4a.fsf@alter.siamese.dyndns.org","threadId":"27507","inReplyTo":"20110531153356.GB2594@sigill.intra.peff.net","subject":"Re: diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-31T15:46:29Z","receivedAt":"2011-05-31T15:46:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Commit 2cfe8a6 (diff --quiet: disable optimization when\n> --diff-filter=X is used, 2011-03-16) fixes this in some\n> cases by disabling the optimization when a filter is\n> present. However, it only tweaked run_diff_files, missing\n> the similar case in diff_tree. Thus the fix worked only for\n> diffing the working tree and index, but not between trees.\n\nThanks; a natural question is if we need the same for diff-index, then.\n\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 7a79660..3d08f78 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -143,6 +143,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n> \n> \tfor (;;) {\n> \t\tif (DIFF_OPT_TST(opt, QUICK) &&\n> +\t\t !opt->filter &&\n> \t\t DIFF_OPT_TST(opt, HAS_CHANGES))\n> \t\t\tbreak;\n> \t\tif (opt->pathspec.nr) {\n\nWe probably want to have a helper in diff.c that does\n\n\tint diff_can_quit_early(struct diff_options *opt)\n {\n \treturn (DIFF_OPT_TST(opt, QUICK) &&\n \t!opt->filter &&\n DIFF_OPT_TST(opt, HAS_CHANGES));\n\t}\n\nIt is possible for us to later add new diffcore transformations that need\na similar \"do not stop feeding early, as results may be filtered\".\n"},{"id":"169058","messageId":"20110531162546.GA11321@sigill.intra.peff.net","threadId":"27507","inReplyTo":"7vvcwqkh4a.fsf@alter.siamese.dyndns.org","subject":"Re: diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-31T16:25:46Z","receivedAt":"2011-05-31T16:25:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2011 at 08:46:29AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Commit 2cfe8a6 (diff --quiet: disable optimization when\n> > --diff-filter=X is used, 2011-03-16) fixes this in some\n> > cases by disabling the optimization when a filter is\n> > present. However, it only tweaked run_diff_files, missing\n> > the similar case in diff_tree. Thus the fix worked only for\n> > diffing the working tree and index, but not between trees.\n> \n> Thanks; a natural question is if we need the same for diff-index, then.\n\nNo, it calls straight into unpack_trees. So I think the question is\n\"should unpack_trees respect the QUICK optimization\". I suspect it\ndidn't happen simply because unpack_trees is so complex, and there are\nprobably corner cases with merging.\n\n> We probably want to have a helper in diff.c that does\n> \n> \tint diff_can_quit_early(struct diff_options *opt)\n> {\n> \treturn (DIFF_OPT_TST(opt, QUICK) &&\n> \t!opt->filter &&\n> DIFF_OPT_TST(opt, HAS_CHANGES));\n> \t}\n> \n> It is possible for us to later add new diffcore transformations that need\n> a similar \"do not stop feeding early, as results may be filtered\".\n\nYeah, that is a good refactoring. It's more readable, and it would have\nprevented this bug. :)\n\n-Peff\n"},{"id":"169060","messageId":"7vd3iykdej.fsf_-_@alter.siamese.dyndns.org","threadId":"27507","inReplyTo":"20110531162546.GA11321@sigill.intra.peff.net","subject":"Re* diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-31T17:06:44Z","receivedAt":"2011-05-31T17:06:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Thanks; a natural question is if we need the same for diff-index, then.\n>\n> No, it calls straight into unpack_trees. So I think the question is\n> \"should unpack_trees respect the QUICK optimization\". I suspect it\n> didn't happen simply because unpack_trees is so complex, and there are\n> probably corner cases with merging.\n\nYeah, a somewhat hacky version would look like this.\n\n-- >8 --\ndiff-index --quiet: learn the \"stop feeding the backend early\" logic\n\nA negative return from the unpack callback function usually means unpack\nfailed for the entry and signals the unpack_trees() machinery to fail the\nentire merge operation, immediately and there is no other way for the\ncallback to tell the machinery to exit early without reporting an error.\n\nThis is what we usually want to make a merge all-or-nothing operation, but\nthe machinery is also used for diff-index codepath by using a custom\nunpack callback function. And we do sometimes want to exit early without\nfailing, namely when we are under --quiet and can short-cut the diff upon\nfinding the first difference.\n\nAdd \"exiting_early\" field to unpack_trees_options structure, to signal the\nunpack_trees() machinery that the negative return value is not signaling\nan error but an early return from the unpack_trees() machinery. As this by\ndefinition hasn't unpacked everything, discard the resulting index just\nlike the failure codepath.\n\nSigned-off-by: Junio C Hamano \n---\n diff-lib.c | 7 ++++++-\n unpack-trees.c | 4 +++-\n unpack-trees.h | 1 +\n 3 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 9c29293..2e09500 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -433,8 +433,13 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)\n \tif (tree == o->df_conflict_entry)\n \t\ttree = NULL;\n \n-\tif (ce_path_match(idx ? idx : tree, &revs->prune_data))\n+\tif (ce_path_match(idx ? idx : tree, &revs->prune_data)) {\n \t\tdo_oneway_diff(o, idx, tree);\n+\t\tif (diff_can_quit_early(&revs->diffopt)) {\n+\t\t\to->exiting_early = 1;\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n \n \treturn 0;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 07f8364..3a61d82 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -593,7 +593,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n static int unpack_failed(struct unpack_trees_options *o, const char *message)\n {\n \tdiscard_index(&o->result);\n-\tif (!o->gently) {\n+\tif (!o->gently && !o->exiting_early) {\n \t\tif (message)\n \t\t\treturn error(\"%s\", message);\n \t\treturn -1;\n@@ -1133,6 +1133,8 @@ return_failed:\n \t\tdisplay_error_msgs(o);\n \tmark_all_ce_unused(o->src_index);\n \tret = unpack_failed(o, NULL);\n+\tif (o->exiting_early)\n+\t\tret = 0;\n \tgoto done;\n }\n \ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 64f02cb..4b07683 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -47,6 +47,7 @@ struct unpack_trees_options {\n \t\t skip_sparse_checkout,\n \t\t gently,\n \t\t show_all_errors,\n+\t\t exiting_early,\n \t\t dry_run;\n \tconst char *prefix;\n \tint cache_bottom;\n"},{"id":"169061","messageId":"20110531171401.GA12466@sigill.intra.peff.net","threadId":"27507","inReplyTo":"7vd3iykdej.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-31T17:14:01Z","receivedAt":"2011-05-31T17:14:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2011 at 10:06:44AM -0700, Junio C Hamano wrote:\n\n> Add \"exiting_early\" field to unpack_trees_options structure, to signal the\n> unpack_trees() machinery that the negative return value is not signaling\n> an error but an early return from the unpack_trees() machinery. As this by\n> definition hasn't unpacked everything, discard the resulting index just\n> like the failure codepath.\n\nMaybe it is just me, but I would think that it's a little more intuitive\nto set exiting_early and then return \"0\" to indicate \"success, but I\ndidn't look at everything\".\n\nI guess you did it this way to better share the discard-the-result\ncodepath. Maybe it wouldn't be too painful to refactor unpack_failed()\ninto the failed bits plus a call to a new unpack_discard() that does the\nnon-error bits. It may not be worth the trouble though.\n\n-Peff\n"},{"id":"169063","messageId":"7v8vtmkc1f.fsf@alter.siamese.dyndns.org","threadId":"27507","inReplyTo":"20110531171401.GA12466@sigill.intra.peff.net","subject":"Re: Re* diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-31T17:36:12Z","receivedAt":"2011-05-31T17:36:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess you did it this way to better share the discard-the-result\n> codepath.\n\nNo, I did it as a hack because many places already do:\n\n\tif (... sub helper function that eventually call_callback ... < 0)\n \tbreak; // or\n return; // or\n goto fail_return; // or whatever to exit recursion and loop\n\nand obviously it was too much pain to change everybody to also pay\nattention to the new flag.\n\nA possibly cleaner way would be to designate a single negative value that\nis not -1 as \"early return but not failure\" without using an extra bit,\nbut that also needs full vetting of the existing callchain, which I didn't\nwant to do just to write a \"it would be as little as this\" patch.\n"},{"id":"169106","messageId":"87pqmxkhwj.wl@dns1.atmark-techno.com","threadId":"27507","inReplyTo":"20110531153356.GB2594@sigill.intra.peff.net","subject":"Re: diff: --quiet does not imply --exit-code if --diff-filter is present","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2011-06-01T09:41:48Z","receivedAt":"2011-06-01T09:41:48Z","isPatch":false,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"Hi Jeff,\n\nAt Tue, 31 May 2011 11:33:56 -0400,\nJeff King wrote:\n> \n> On Tue, May 31, 2011 at 07:34:39PM +0900, Yasushi SHOJI wrote:\n> \n> > just noticed that quiet does not return exit code when I set\n> > diff-filter. at the current tip (v1.7.5.3-401-gfb674d7):\n> > \n> > git diff --quiet --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 0\n> > git diff --exit-code --diff-filter=A v1.7.5 v1.7.5.1 -- t #=> 1\n> > \n> > these two line returns different exit code.\n> > \n> > is this a bug or a feature?\n> \n> It's a bug.\n\nyour patch works like a charm. Thanks.\n\n> Commit 2cfe8a6 (diff --quiet: disable optimization when\n> --diff-filter=X is used, 2011-03-16) fixes this in some\n> cases by disabling the optimization when a filter is\n> present. However, it only tweaked run_diff_files, missing\n> the similar case in diff_tree. Thus the fix worked only for\n> diffing the working tree and index, but not between trees.\n\noh, sorry about not realizing the commit 2cfe8a6.\n\nregards,\n-- \n yashi\n"}]}