Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

> "Alex Riesen" <raa.lkml@gmail.com> 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.

> 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.

> >       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.
>

> > +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".