On 3/14/07, Junio C Hamano <junkio@xxxxxxx> wrote:
"Alex Riesen" <raa.lkml@xxxxxxxxx> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html