Re: [PATCH] Allow git-diff exit with codes similar to diff(1)
- From
Junio C Hamano <junkio@cox.net>
- Date
- Mar 14, 2007, 04:56 UTC
- Message-ID
- <7v8xe0h19a.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <81b0412b0703131717k7106ee1cg964628f0bda2c83e@mail.gmail.com>
"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 24 quoted lines
> 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.
Show 27 quoted lines
> diff --git a/diff-lib.c b/diff-lib.c
> index 6abb981..f943b6f 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -433,8 +437,9 @@ int run_diff_files(struct rev_info *revs, int silent_on_removed)
>
> }
> diffcore_std(&revs->diffopt);
> + result = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
> diff_flush(&revs->diffopt);
> - return 0;
> + return result;
> }
>
> /*
> @@ -664,9 +669,10 @@ int run_diff_index(struct rev_info *revs, int cached)
> return error("bad tree object %s", tree_name);
> if (read_tree(tree, 1, revs->prune_data))
> return error("unable to read tree object %s", tree_name);
> - ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
> + diff_cache(revs, active_cache, active_nr, revs->prune_data,
> cached, match_missing);
> 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.
Show 8 quoted lines
> +test_expect_failure 'git diff-tree HEAD^ HEAD' ' > + git diff-tree --exit-code HEAD^ HEAD > +' > ... > +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".