From: Alex Riesen Date: Wed, 14 Mar 2007 08:28:55 GMT Subject: Re: [PATCH] Allow git-diff exit with codes similar to diff(1) Message-ID: <81b0412b0703140128y46ff6bb6m503eeae00c043ddf@mail.gmail.com> In-Reply-To: <7v8xe0h19a.fsf@assigned-by-dhcp.cox.net> On 3/14/07, Junio C Hamano wrote: > "Alex Riesen" writes: > > > diff --git a/builtin-diff.c b/builtin-diff.c > > index 4efbb82..5e6265f 100644 > > --- a/builtin-diff.c > > +++ b/builtin-diff.c > > @@ -130,6 +130,7 @@ static int builtin_diff_tree(struct rev_info *revs, > > { > > const unsigned char *(sha1[2]); > > int swap = 0; > > + int result = 0; > > > > if (argc > 1) > > usage(builtin_diff_usage); > > @@ -141,9 +142,9 @@ static int builtin_diff_tree(struct rev_info *revs, > > swap = 1; > > sha1[swap] = ent[0].item->sha1; > > sha1[1-swap] = ent[1].item->sha1; > > - diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt); > > + result = diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt); > > log_tree_diff_flush(revs); > > - return 0; > > + return result; > > } > > > > static int builtin_diff_combined(struct rev_info *revs, > > The change to diff-tree side is completely borked. (1) You did > not notice compare_tree_entry() in tree-diff.c returns 0 only to > signal that it has dealt with an entry from both sides (so the > caller can do update_tree_entry() on both), and the return value > does not mean they are the same. (2) You are checking if there > are differences at wrong level, before letting diffcore_std() to > process the queue. Because of the bug (1) I cannot test that > but after you fix (1) you would notice that it would not work if > you say "-Spickaxe"; your changes to diff-files and diff-index > are correct on this regard. Challenging... Now, if someone just told me where to look for differences in diff-tree case... > A slight tangent, but what Linus recalled he thought he did but > he didn't is related to the parts you touched in diff-tree > above. Because of the interaction with diffcore, these changes > should not be used for the purpose of -exit-code, but catching > the tree level change in the above places and leaving early > would be the right thing to do for comparing the whole tree for > the purpose of simplifying the parents. Tomorrow will be my git > day so I might whip up a patch to speed that up. Can it eventually be wired to "-s" (DIFF_FORMAT_NO_OUTPUT)? > > diffcore_std(&revs->diffopt); > > + ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0; > > diff_flush(&revs->diffopt); > > return ret; > > } > > This side looks correct, as you are counting queued_diff.nr after > letting diffcore_std() to filter the results. > And it will continue to work if the diffing is left early because of no output needed. Err, will it? > > +test_expect_failure 'git diff-index --cached HEAD' ' > > + git update-index c && > > + git diff-index --exit-code --cached HEAD > > +' > > In general, expect_failure should not be used for complex cases > like this. The first one I quoted is fine, but the latter one > is not. update-index may fail (perhaps somebody screwed up > while updating read-cache.c or sha1_file.c) and the whole test > would say "happy that the command chain as a whole failed". Right. Fixed.