* [PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
@ 2023-08-21 20:14 ` Jeff King
2023-08-21 20:15 ` [PATCH v2 2/7] diff-files: avoid negative exit value Jeff King
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
From: Junio C Hamano <gitster@pobox.com>
Many callers of run_diff_index() passed literal "1" for the option
flag word, which should better be spelled out as DIFF_INDEX_CACHED
for readablity. Everybody else passes "0" that can stay as-is.
The other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but
curiously there is only one caller that can pass it, which is "git
diff-index --merge-base" itself---no internal callers uses the
feature.
A bit tricky call to the function is in builtin/submodule--helper.c
where the .cached member in a private struct is set/reset as a plain
Boolean flag, which happens to be "1" and happens to match the value
of DIFF_INDEX_CACHED.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
add-interactive.c | 2 +-
builtin/am.c | 4 ++--
builtin/stash.c | 2 +-
builtin/submodule--helper.c | 2 +-
diff-lib.c | 2 +-
wt-status.c | 6 +++---
6 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/add-interactive.c b/add-interactive.c
index add9a1ad43..7fd00c5e25 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -569,7 +569,7 @@ static int get_modified_files(struct repository *r,
copy_pathspec(&rev.prune_data, ps);
if (s.mode == FROM_INDEX)
- run_diff_index(&rev, 1);
+ run_diff_index(&rev, DIFF_INDEX_CACHED);
else {
rev.diffopt.flags.ignore_dirty_submodules = 1;
run_diff_files(&rev, 0);
diff --git a/builtin/am.c b/builtin/am.c
index 8bde034fae..202040b62e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1430,7 +1430,7 @@ static void write_index_patch(const struct am_state *state)
rev_info.diffopt.close_file = 1;
add_pending_object(&rev_info, &tree->object, "");
diff_setup_done(&rev_info.diffopt);
- run_diff_index(&rev_info, 1);
+ run_diff_index(&rev_info, DIFF_INDEX_CACHED);
release_revisions(&rev_info);
}
@@ -1593,7 +1593,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
rev_info.diffopt.filter |= diff_filter_bit('M');
add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
diff_setup_done(&rev_info.diffopt);
- run_diff_index(&rev_info, 1);
+ run_diff_index(&rev_info, DIFF_INDEX_CACHED);
release_revisions(&rev_info);
}
diff --git a/builtin/stash.c b/builtin/stash.c
index fe64cde9ce..fe5052f12f 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1111,7 +1111,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)
add_head_to_pending(&rev);
diff_setup_done(&rev.diffopt);
- result = run_diff_index(&rev, 1);
+ result = run_diff_index(&rev, DIFF_INDEX_CACHED);
if (diff_result_code(&rev.diffopt, result)) {
ret = 1;
goto done;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6871efd95..125ea80d21 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
}
if (diff_cmd == DIFF_INDEX)
- run_diff_index(&rev, info->cached);
+ run_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);
else
run_diff_files(&rev, 0);
prepare_submodule_summary(info, &list);
diff --git a/diff-lib.c b/diff-lib.c
index 6b0c6a7180..cfa3489111 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -682,7 +682,7 @@ int index_differs_from(struct repository *r,
rev.diffopt.flags.ignore_submodules = flags->ignore_submodules;
}
rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
- run_diff_index(&rev, 1);
+ run_diff_index(&rev, DIFF_INDEX_CACHED);
has_changes = rev.diffopt.flags.has_changes;
release_revisions(&rev);
return (has_changes != 0);
diff --git a/wt-status.c b/wt-status.c
index 5b1378965c..bf8687b357 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -675,7 +675,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
rev.diffopt.flags.recursive = 1;
copy_pathspec(&rev.prune_data, &s->pathspec);
- run_diff_index(&rev, 1);
+ run_diff_index(&rev, DIFF_INDEX_CACHED);
release_revisions(&rev);
}
@@ -1156,7 +1156,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
rev.diffopt.a_prefix = "c/";
rev.diffopt.b_prefix = "i/";
} /* else use prefix as per user config */
- run_diff_index(&rev, 1);
+ run_diff_index(&rev, DIFF_INDEX_CACHED);
if (s->verbose > 1 &&
wt_status_check_worktree_changes(s, &dirty_submodules)) {
status_printf_ln(s, c,
@@ -2614,7 +2614,7 @@ int has_uncommitted_changes(struct repository *r,
}
diff_setup_done(&rev_info.diffopt);
- result = run_diff_index(&rev_info, 1);
+ result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);
result = diff_result_code(&rev_info.diffopt, result);
release_revisions(&rev_info);
return result;
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 2/7] diff-files: avoid negative exit value
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
2023-08-21 20:14 ` [PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index() Jeff King
@ 2023-08-21 20:15 ` Jeff King
2023-08-21 20:16 ` [PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options Jeff King
` (4 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
If loading the index fails, we print an error and then return "-1" from
the function. But since this is a builtin, we end up with exit(-1),
which produces odd results since program exit codes are unsigned.
Because of integer conversion, it usually becomes 255, which is at least
still an error, but values above 128 are usually interpreted as signal
death.
Since we know the program is exiting immediately, we can just replace
the error return with a die().
Signed-off-by: Jeff King <peff@peff.net>
---
I'm actually not sure this can be triggered in practice; see patch 4 for
more discussion. Regardless, it seems like a strict improvement.
builtin/diff-files.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 50330b8dd2..2e3e948583 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -80,14 +80,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
(rev.diffopt.output_format & DIFF_FORMAT_PATCH))
diff_merges_set_dense_combined_if_unset(&rev);
- if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {
- perror("repo_read_index_preload");
- result = -1;
- goto cleanup;
- }
+ if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
+ die_errno("repo_read_index_preload");
result = run_diff_files(&rev, options);
result = diff_result_code(&rev.diffopt, result);
-cleanup:
release_revisions(&rev);
return result;
}
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
2023-08-21 20:14 ` [PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index() Jeff King
2023-08-21 20:15 ` [PATCH v2 2/7] diff-files: avoid negative exit value Jeff King
@ 2023-08-21 20:16 ` Jeff King
2023-08-21 20:17 ` [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin Jeff King
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
The git-diff command has many modes (comparing worktree to index, index
to HEAD, individual blobs, etc). As a result, it dispatches to many
helper functions and cannot completely parse its options until we're in
those helper functions.
Most of them, when seeing an unknown option, exit immediately by calling
usage(). But builtin_diff_files(), which is the default if no revision
or blob arguments are given, instead prints an error() and returns -1.
One obvious shortcoming here is that the user doesn't get to see the
usual usage message. But there's a much more important bug: the -1
return is fed to diff_result_code(), which is not ready to handle it.
By default, it passes the code along as an exit code. We try to avoid
negative exit codes because they get converted to unsigned values, but
it should at least consistently show up as non-zero (i.e., a failure).
But much worse is that when --exit-code is in effect, diff_result_code()
will _ignore_ the status passed in by the caller, and instead only
report on whether the diff found changes. It didn't, of course, because
we never ran the diff, and the program unexpectedly exits with success!
We can fix this bug by just calling usage(), like the other helpers do.
Another option would of course be to teach diff_result_code() to handle
this value. But as we'll see in the next few patches, it can be cleaned
up even further. Let's just fix this bug directly to start with.
Reported-by: Romain Chossart <romainchossart@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/diff.c | 6 ++++--
t/t4017-diff-retval.sh | 5 +++++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index b19530c996..d0e187ec18 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -269,8 +269,10 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
options |= DIFF_SILENT_ON_REMOVED;
else if (!strcmp(argv[1], "-h"))
usage(builtin_diff_usage);
- else
- return error(_("invalid option: %s"), argv[1]);
+ else {
+ error(_("invalid option: %s"), argv[1]);
+ usage(builtin_diff_usage);
+ }
argv++; argc--;
}
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 5bc28ad9f0..f439f469bd 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -138,4 +138,9 @@ test_expect_success 'check honors conflict marker length' '
git reset --hard
'
+test_expect_success 'option errors are not confused by --exit-code' '
+ test_must_fail git diff --exit-code --nonsense 2>err &&
+ grep '^usage:' err
+'
+
test_done
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
` (2 preceding siblings ...)
2023-08-21 20:16 ` [PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options Jeff King
@ 2023-08-21 20:17 ` Jeff King
2023-08-22 23:27 ` Junio C Hamano
2023-08-21 20:18 ` [PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions Jeff King
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
When the git-diff program fails to read the index in its diff-files or
diff-index helper functions, it propagates the error up the stack. This
eventually lands in diff_result_code(), which does not handle it well
(as discussed in the previous patch).
Since the only sensible thing here is to exit with an error code (and
what we were expecting the propagated error code to cause), let's just
do that directly.
There's no test here, as I'm not even sure this case can be triggered.
The index-reading functions tend to die() themselves when encountering
any errors, and the return value is just the number of entries in the
file (and so always 0 or positive). But let's err on the conservative
side and keep checking the return value. It may be worth digging into as
a separate topic (though index-reading is low-level enough that we
probably want to eventually teach it to propagate errors anyway for
lib-ification purposes, at which point this code would already be doing
the right thing).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/diff.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index d0e187ec18..005f415d36 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -163,12 +163,10 @@ static int builtin_diff_index(struct rev_info *revs,
setup_work_tree();
if (repo_read_index_preload(the_repository,
&revs->diffopt.pathspec, 0) < 0) {
- perror("repo_read_index_preload");
- return -1;
+ die_errno("repo_read_index_preload");
}
} else if (repo_read_index(the_repository) < 0) {
- perror("repo_read_cache");
- return -1;
+ die_errno("repo_read_cache");
}
return run_diff_index(revs, option);
}
@@ -289,8 +287,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
setup_work_tree();
if (repo_read_index_preload(the_repository, &revs->diffopt.pathspec,
0) < 0) {
- perror("repo_read_index_preload");
- return -1;
+ die_errno("repo_read_index_preload");
}
return run_diff_files(revs, options);
}
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
` (3 preceding siblings ...)
2023-08-21 20:17 ` [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin Jeff King
@ 2023-08-21 20:18 ` Jeff King
2023-08-21 20:19 ` [PATCH v2 6/7] diff: drop useless return values in git-diff helpers Jeff King
2023-08-21 20:20 ` [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code() Jeff King
6 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
Neither of these functions ever returns a value other than zero.
Instead, they expect unrecoverable errors to exit immediately, and
things like "--exit-code" are stored inside the diff_options struct to
be handled later via diff_result_code().
Some callers do check the return values, but many don't bother. Let's
drop the useless return values, which are misleading callers about how
the functions work. This could be seen as a step in the wrong direction,
as we might want to eventually "lib-ify" these to more cleanly return
errors up the stack, in which case we'd have to add the return values
back in. But there are some benefits to doing this now:
1. In the current code, somebody could accidentally add a "return -1"
to one of the functions, which would be erroneously ignored by many
callers. By removing the return code, the compiler can notice the
mismatch and force the developer to decide what to do.
Obviously the other option here is that we could start consistently
checking the error code in every caller. But it would be dead code,
and we wouldn't get any compile-time help in catching new cases.
2. It communicates the situation to callers, who may want to choose a
different function. These functions are really thin wrappers for
doing git-diff-files and git-diff-index within the process. But
callers who care about recovering from an error here are probably
better off using the underlying library functions, many of
which do return errors.
If somebody eventually wants to teach these functions to propagate
errors, they'll have to switch back to returning a value, effectively
reverting this patch. But at least then they will be starting with a
level playing field: they know that they will need to inspect each
caller to see how it should handle the error.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/add.c | 3 +--
builtin/describe.c | 6 +++---
builtin/diff-files.c | 4 ++--
builtin/diff-index.c | 4 ++--
builtin/diff.c | 6 ++++--
builtin/stash.c | 14 +++++---------
builtin/submodule--helper.c | 5 ++---
diff-lib.c | 6 ++----
diff.h | 4 ++--
wt-status.c | 8 ++++----
10 files changed, 27 insertions(+), 33 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..12c5aa6d1f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -194,8 +194,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
rev.diffopt.file = xfdopen(out, "w");
rev.diffopt.close_file = 1;
- if (run_diff_files(&rev, 0))
- die(_("Could not write patch"));
+ run_diff_files(&rev, 0);
if (launch_editor(file, NULL, NULL))
die(_("editing patch failed"));
diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..8cdc25b748 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -668,7 +668,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
struct lock_file index_lock = LOCK_INIT;
struct rev_info revs;
struct strvec args = STRVEC_INIT;
- int fd, result;
+ int fd;
setup_work_tree();
prepare_repo_settings(the_repository);
@@ -685,9 +685,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
strvec_pushv(&args, diff_index_args);
if (setup_revisions(args.nr, args.v, &revs, NULL) != 1)
BUG("malformed internal diff-index command line");
- result = run_diff_index(&revs, 0);
+ run_diff_index(&revs, 0);
- if (!diff_result_code(&revs.diffopt, result))
+ if (!diff_result_code(&revs.diffopt, 0))
suffix = NULL;
else
suffix = dirty;
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 2e3e948583..04070607b1 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -82,8 +82,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
die_errno("repo_read_index_preload");
- result = run_diff_files(&rev, options);
- result = diff_result_code(&rev.diffopt, result);
+ run_diff_files(&rev, options);
+ result = diff_result_code(&rev.diffopt, 0);
release_revisions(&rev);
return result;
}
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 9db7139b83..2c6a179832 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -72,8 +72,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
perror("repo_read_index");
return -1;
}
- result = run_diff_index(&rev, option);
- result = diff_result_code(&rev.diffopt, result);
+ run_diff_index(&rev, option);
+ result = diff_result_code(&rev.diffopt, 0);
release_revisions(&rev);
return result;
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 005f415d36..e1f7647c84 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -168,7 +168,8 @@ static int builtin_diff_index(struct rev_info *revs,
} else if (repo_read_index(the_repository) < 0) {
die_errno("repo_read_cache");
}
- return run_diff_index(revs, option);
+ run_diff_index(revs, option);
+ return 0;
}
static int builtin_diff_tree(struct rev_info *revs,
@@ -289,7 +290,8 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
0) < 0) {
die_errno("repo_read_index_preload");
}
- return run_diff_files(revs, options);
+ run_diff_files(revs, options);
+ return 0;
}
struct symdiff {
diff --git a/builtin/stash.c b/builtin/stash.c
index fe5052f12f..e799b660f0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1089,7 +1089,6 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
*/
static int check_changes_tracked_files(const struct pathspec *ps)
{
- int result;
struct rev_info rev;
struct object_id dummy;
int ret = 0;
@@ -1111,14 +1110,14 @@ static int check_changes_tracked_files(const struct pathspec *ps)
add_head_to_pending(&rev);
diff_setup_done(&rev.diffopt);
- result = run_diff_index(&rev, DIFF_INDEX_CACHED);
- if (diff_result_code(&rev.diffopt, result)) {
+ run_diff_index(&rev, DIFF_INDEX_CACHED);
+ if (diff_result_code(&rev.diffopt, 0)) {
ret = 1;
goto done;
}
- result = run_diff_files(&rev, 0);
- if (diff_result_code(&rev.diffopt, result)) {
+ run_diff_files(&rev, 0);
+ if (diff_result_code(&rev.diffopt, 0)) {
ret = 1;
goto done;
}
@@ -1309,10 +1308,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
add_pending_object(&rev, parse_object(the_repository, &info->b_commit),
"");
- if (run_diff_index(&rev, 0)) {
- ret = -1;
- goto done;
- }
+ run_diff_index(&rev, 0);
cp_upd_index.git_cmd = 1;
strvec_pushl(&cp_upd_index.args, "update-index",
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 125ea80d21..3764ed1f9c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -629,7 +629,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
char *displaypath;
struct strvec diff_files_args = STRVEC_INIT;
struct rev_info rev = REV_INFO_INIT;
- int diff_files_result;
struct strbuf buf = STRBUF_INIT;
const char *git_dir;
struct setup_revision_opt opt = {
@@ -669,9 +668,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
repo_init_revisions(the_repository, &rev, NULL);
rev.abbrev = 0;
setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
- diff_files_result = run_diff_files(&rev, 0);
+ run_diff_files(&rev, 0);
- if (!diff_result_code(&rev.diffopt, diff_files_result)) {
+ if (!diff_result_code(&rev.diffopt, 0)) {
print_status(flags, ' ', path, ce_oid,
displaypath);
} else if (!(flags & OPT_CACHED)) {
diff --git a/diff-lib.c b/diff-lib.c
index cfa3489111..d8aa777a73 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -96,7 +96,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
return changed;
}
-int run_diff_files(struct rev_info *revs, unsigned int option)
+void run_diff_files(struct rev_info *revs, unsigned int option)
{
int entries, i;
int diff_unmerged_stage = revs->max_count;
@@ -272,7 +272,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
trace_performance_since(start, "diff-files");
- return 0;
}
/*
@@ -606,7 +605,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
free_commit_list(merge_bases);
}
-int run_diff_index(struct rev_info *revs, unsigned int option)
+void run_diff_index(struct rev_info *revs, unsigned int option)
{
struct object_array_entry *ent;
int cached = !!(option & DIFF_INDEX_CACHED);
@@ -640,7 +639,6 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
trace_performance_leave("diff-index");
- return 0;
}
int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
diff --git a/diff.h b/diff.h
index 260c454155..528f00d5e1 100644
--- a/diff.h
+++ b/diff.h
@@ -637,11 +637,11 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
#define DIFF_SILENT_ON_REMOVED 01
/* report racily-clean paths as modified */
#define DIFF_RACY_IS_MODIFIED 02
-int run_diff_files(struct rev_info *revs, unsigned int option);
+void run_diff_files(struct rev_info *revs, unsigned int option);
#define DIFF_INDEX_CACHED 01
#define DIFF_INDEX_MERGE_BASE 02
-int run_diff_index(struct rev_info *revs, unsigned int option);
+void run_diff_index(struct rev_info *revs, unsigned int option);
int do_diff_cache(const struct object_id *, struct diff_options *);
int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
diff --git a/wt-status.c b/wt-status.c
index bf8687b357..545cea948f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2580,8 +2580,8 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
}
rev_info.diffopt.flags.quick = 1;
diff_setup_done(&rev_info.diffopt);
- result = run_diff_files(&rev_info, 0);
- result = diff_result_code(&rev_info.diffopt, result);
+ run_diff_files(&rev_info, 0);
+ result = diff_result_code(&rev_info.diffopt, 0);
release_revisions(&rev_info);
return result;
}
@@ -2614,8 +2614,8 @@ int has_uncommitted_changes(struct repository *r,
}
diff_setup_done(&rev_info.diffopt);
- result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);
- result = diff_result_code(&rev_info.diffopt, result);
+ run_diff_index(&rev_info, DIFF_INDEX_CACHED);
+ result = diff_result_code(&rev_info.diffopt, 0);
release_revisions(&rev_info);
return result;
}
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 6/7] diff: drop useless return values in git-diff helpers
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
` (4 preceding siblings ...)
2023-08-21 20:18 ` [PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions Jeff King
@ 2023-08-21 20:19 ` Jeff King
2023-08-21 20:20 ` [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code() Jeff King
6 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
Since git-diff has many diff modes, it dispatches to many helpers to
perform each one. But every helper simply returns "0", as it exits
directly if there are serious errors (and options like --exit-code are
handled afterwards). So let's get rid of these useless return values,
which makes the code flow more clear.
There's very little chance that we'd later want to propagate errors
instead of dying immediately. These are all static-local helpers for the
git-diff program implementing its various modes. More "lib-ified" code
would directly call the underlying functions.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/diff.c | 62 +++++++++++++++++++++++---------------------------
1 file changed, 28 insertions(+), 34 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index e1f7647c84..3eba691b82 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -77,9 +77,9 @@ static void stuff_change(struct diff_options *opt,
diff_queue(&diff_queued_diff, one, two);
}
-static int builtin_diff_b_f(struct rev_info *revs,
- int argc, const char **argv UNUSED,
- struct object_array_entry **blob)
+static void builtin_diff_b_f(struct rev_info *revs,
+ int argc, const char **argv UNUSED,
+ struct object_array_entry **blob)
{
/* Blob vs file in the working tree*/
struct stat st;
@@ -109,12 +109,11 @@ static int builtin_diff_b_f(struct rev_info *revs,
path);
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
- return 0;
}
-static int builtin_diff_blobs(struct rev_info *revs,
- int argc, const char **argv UNUSED,
- struct object_array_entry **blob)
+static void builtin_diff_blobs(struct rev_info *revs,
+ int argc, const char **argv UNUSED,
+ struct object_array_entry **blob)
{
const unsigned mode = canon_mode(S_IFREG | 0644);
@@ -134,11 +133,10 @@ static int builtin_diff_blobs(struct rev_info *revs,
blob_path(blob[0]), blob_path(blob[1]));
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
- return 0;
}
-static int builtin_diff_index(struct rev_info *revs,
- int argc, const char **argv)
+static void builtin_diff_index(struct rev_info *revs,
+ int argc, const char **argv)
{
unsigned int option = 0;
while (1 < argc) {
@@ -169,13 +167,12 @@ static int builtin_diff_index(struct rev_info *revs,
die_errno("repo_read_cache");
}
run_diff_index(revs, option);
- return 0;
}
-static int builtin_diff_tree(struct rev_info *revs,
- int argc, const char **argv,
- struct object_array_entry *ent0,
- struct object_array_entry *ent1)
+static void builtin_diff_tree(struct rev_info *revs,
+ int argc, const char **argv,
+ struct object_array_entry *ent0,
+ struct object_array_entry *ent1)
{
const struct object_id *(oid[2]);
struct object_id mb_oid;
@@ -208,13 +205,12 @@ static int builtin_diff_tree(struct rev_info *revs,
}
diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
log_tree_diff_flush(revs);
- return 0;
}
-static int builtin_diff_combined(struct rev_info *revs,
- int argc, const char **argv UNUSED,
- struct object_array_entry *ent,
- int ents, int first_non_parent)
+static void builtin_diff_combined(struct rev_info *revs,
+ int argc, const char **argv UNUSED,
+ struct object_array_entry *ent,
+ int ents, int first_non_parent)
{
struct oid_array parents = OID_ARRAY_INIT;
int i;
@@ -235,7 +231,6 @@ static int builtin_diff_combined(struct rev_info *revs,
}
diff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);
oid_array_clear(&parents);
- return 0;
}
static void refresh_index_quietly(void)
@@ -253,7 +248,7 @@ static void refresh_index_quietly(void)
repo_update_index_if_able(the_repository, &lock_file);
}
-static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
+static void builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
{
unsigned int options = 0;
@@ -291,7 +286,6 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
die_errno("repo_read_index_preload");
}
run_diff_files(revs, options);
- return 0;
}
struct symdiff {
@@ -405,7 +399,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
int blobs = 0, paths = 0;
struct object_array_entry *blob[2];
int nongit = 0, no_index = 0;
- int result = 0;
+ int result;
struct symdiff sdiff;
/*
@@ -584,17 +578,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
if (!ent.nr) {
switch (blobs) {
case 0:
- result = builtin_diff_files(&rev, argc, argv);
+ builtin_diff_files(&rev, argc, argv);
break;
case 1:
if (paths != 1)
usage(builtin_diff_usage);
- result = builtin_diff_b_f(&rev, argc, argv, blob);
+ builtin_diff_b_f(&rev, argc, argv, blob);
break;
case 2:
if (paths)
usage(builtin_diff_usage);
- result = builtin_diff_blobs(&rev, argc, argv, blob);
+ builtin_diff_blobs(&rev, argc, argv, blob);
break;
default:
usage(builtin_diff_usage);
@@ -603,18 +597,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
else if (blobs)
usage(builtin_diff_usage);
else if (ent.nr == 1)
- result = builtin_diff_index(&rev, argc, argv);
+ builtin_diff_index(&rev, argc, argv);
else if (ent.nr == 2) {
if (sdiff.warn)
warning(_("%s...%s: multiple merge bases, using %s"),
sdiff.left, sdiff.right, sdiff.base);
- result = builtin_diff_tree(&rev, argc, argv,
- &ent.objects[0], &ent.objects[1]);
+ builtin_diff_tree(&rev, argc, argv,
+ &ent.objects[0], &ent.objects[1]);
} else
- result = builtin_diff_combined(&rev, argc, argv,
- ent.objects, ent.nr,
- first_non_parent);
- result = diff_result_code(&rev.diffopt, result);
+ builtin_diff_combined(&rev, argc, argv,
+ ent.objects, ent.nr,
+ first_non_parent);
+ result = diff_result_code(&rev.diffopt, 0);
if (1 < rev.diffopt.skip_stat_unmatch)
refresh_index_quietly();
release_revisions(&rev);
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()
2023-08-21 20:13 ` [PATCH v2 0/7] cleaning up diff_result_code() Jeff King
` (5 preceding siblings ...)
2023-08-21 20:19 ` [PATCH v2 6/7] diff: drop useless return values in git-diff helpers Jeff King
@ 2023-08-21 20:20 ` Jeff King
2023-08-22 23:38 ` Junio C Hamano
6 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2023-08-21 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
Many programs use diff_result_code() to get a user-visible program exit
code from a diff result (e.g., checking opts.found_changes if
--exit-code was requested).
This function also takes a "status" parameter, which seems at first
glance that it could be used to propagate an error encountered when
computing the diff. But it doesn't work that way:
- negative values are passed through as-is, but are not appropriate as
program exit codes
- when --exit-code or --check is in effect, we _ignore_ the passed-in
status completely. So a failed diff which did not have a chance to
set opts.found_changes would erroneously report "success, no
changes" instead of propagating the error.
After recent cleanups, neither of these bugs is possible to trigger, as
every caller just passes in "0". So rather than fixing them, we can
simply drop the useless parameter instead.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/describe.c | 2 +-
builtin/diff-files.c | 2 +-
builtin/diff-index.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/diff.c | 2 +-
builtin/log.c | 2 +-
builtin/stash.c | 6 +++---
builtin/submodule--helper.c | 2 +-
diff-no-index.c | 2 +-
diff.c | 6 ++----
diff.h | 2 +-
wt-status.c | 4 ++--
12 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 8cdc25b748..a9e375882b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -687,7 +687,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
BUG("malformed internal diff-index command line");
run_diff_index(&revs, 0);
- if (!diff_result_code(&revs.diffopt, 0))
+ if (!diff_result_code(&revs.diffopt))
suffix = NULL;
else
suffix = dirty;
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 04070607b1..f38912cd40 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -83,7 +83,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
die_errno("repo_read_index_preload");
run_diff_files(&rev, options);
- result = diff_result_code(&rev.diffopt, 0);
+ result = diff_result_code(&rev.diffopt);
release_revisions(&rev);
return result;
}
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 2c6a179832..220f341ffa 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -73,7 +73,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
return -1;
}
run_diff_index(&rev, option);
- result = diff_result_code(&rev.diffopt, 0);
+ result = diff_result_code(&rev.diffopt);
release_revisions(&rev);
return result;
}
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index c9ba35f143..86be634286 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -232,5 +232,5 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
diff_free(&opt->diffopt);
}
- return diff_result_code(&opt->diffopt, 0);
+ return diff_result_code(&opt->diffopt);
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 3eba691b82..0b313549c7 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -608,7 +608,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
builtin_diff_combined(&rev, argc, argv,
ent.objects, ent.nr,
first_non_parent);
- result = diff_result_code(&rev.diffopt, 0);
+ result = diff_result_code(&rev.diffopt);
if (1 < rev.diffopt.skip_stat_unmatch)
refresh_index_quietly();
release_revisions(&rev);
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..5d808c92f4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -549,7 +549,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
rev->diffopt.flags.check_failed) {
return 02;
}
- return diff_result_code(&rev->diffopt, 0);
+ return diff_result_code(&rev->diffopt);
}
static int cmd_log_walk(struct rev_info *rev)
diff --git a/builtin/stash.c b/builtin/stash.c
index e799b660f0..53e8868ba1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -973,7 +973,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)
}
log_tree_diff_flush(&rev);
- ret = diff_result_code(&rev.diffopt, 0);
+ ret = diff_result_code(&rev.diffopt);
cleanup:
strvec_clear(&stash_args);
free_stash_info(&info);
@@ -1111,13 +1111,13 @@ static int check_changes_tracked_files(const struct pathspec *ps)
diff_setup_done(&rev.diffopt);
run_diff_index(&rev, DIFF_INDEX_CACHED);
- if (diff_result_code(&rev.diffopt, 0)) {
+ if (diff_result_code(&rev.diffopt)) {
ret = 1;
goto done;
}
run_diff_files(&rev, 0);
- if (diff_result_code(&rev.diffopt, 0)) {
+ if (diff_result_code(&rev.diffopt)) {
ret = 1;
goto done;
}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3764ed1f9c..6f3bf33e61 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -670,7 +670,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
run_diff_files(&rev, 0);
- if (!diff_result_code(&rev.diffopt, 0)) {
+ if (!diff_result_code(&rev.diffopt)) {
print_status(flags, ' ', path, ce_oid,
displaypath);
} else if (!(flags & OPT_CACHED)) {
diff --git a/diff-no-index.c b/diff-no-index.c
index 4771cf02aa..8aead3e332 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -364,7 +364,7 @@ int diff_no_index(struct rev_info *revs,
* The return code for --no-index imitates diff(1):
* 0 = no changes, 1 = changes, else error
*/
- ret = diff_result_code(&revs->diffopt, 0);
+ ret = diff_result_code(&revs->diffopt);
out:
for (i = 0; i < ARRAY_SIZE(to_free); i++)
diff --git a/diff.c b/diff.c
index ee3eb629e3..2de5d3d098 100644
--- a/diff.c
+++ b/diff.c
@@ -6973,16 +6973,14 @@ void diffcore_std(struct diff_options *options)
options->found_follow = 0;
}
-int diff_result_code(struct diff_options *opt, int status)
+int diff_result_code(struct diff_options *opt)
{
int result = 0;
diff_warn_rename_limit("diff.renameLimit",
opt->needed_rename_limit,
opt->degraded_cc_to_c);
- if (!opt->flags.exit_with_status &&
- !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
- return status;
+
if (opt->flags.exit_with_status &&
opt->flags.has_changes)
result |= 01;
diff --git a/diff.h b/diff.h
index 528f00d5e1..caf1528bf0 100644
--- a/diff.h
+++ b/diff.h
@@ -647,7 +647,7 @@ int do_diff_cache(const struct object_id *, struct diff_options *);
int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);
-int diff_result_code(struct diff_options *, int);
+int diff_result_code(struct diff_options *);
int diff_no_index(struct rev_info *,
int implicit_no_index, int, const char **);
diff --git a/wt-status.c b/wt-status.c
index 545cea948f..981adb09f3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2581,7 +2581,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
rev_info.diffopt.flags.quick = 1;
diff_setup_done(&rev_info.diffopt);
run_diff_files(&rev_info, 0);
- result = diff_result_code(&rev_info.diffopt, 0);
+ result = diff_result_code(&rev_info.diffopt);
release_revisions(&rev_info);
return result;
}
@@ -2615,7 +2615,7 @@ int has_uncommitted_changes(struct repository *r,
diff_setup_done(&rev_info.diffopt);
run_diff_index(&rev_info, DIFF_INDEX_CACHED);
- result = diff_result_code(&rev_info.diffopt, 0);
+ result = diff_result_code(&rev_info.diffopt);
release_revisions(&rev_info);
return result;
}
--
2.42.0.rc2.423.g967ecb4f2b
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()
2023-08-21 20:20 ` [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code() Jeff King
@ 2023-08-22 23:38 ` Junio C Hamano
2023-08-23 19:00 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-08-22 23:38 UTC (permalink / raw)
To: Jeff King; +Cc: Romain Chossart, git
Jeff King <peff@peff.net> writes:
> Many programs use diff_result_code() to get a user-visible program exit
> code from a diff result (e.g., checking opts.found_changes if
> --exit-code was requested).
>
> This function also takes a "status" parameter, which seems at first
> glance that it could be used to propagate an error encountered when
> computing the diff. But it doesn't work that way:
>
> - negative values are passed through as-is, but are not appropriate as
> program exit codes
>
> - when --exit-code or --check is in effect, we _ignore_ the passed-in
> status completely. So a failed diff which did not have a chance to
> set opts.found_changes would erroneously report "success, no
> changes" instead of propagating the error.
>
> After recent cleanups, neither of these bugs is possible to trigger, as
Here "after recent cleanups" refers to the changes to make them
die() upon seeing an error, instead of using it to call this
function with non-zero in the status parameter? At least, they were
signaling errors correctly when --exit-code is not in use, but now
all the callers are responsible for exiting with non-zero status to
signal an error even when --exit-code is *not* used.
> every caller just passes in "0". So rather than fixing them, we can
> simply drop the useless parameter instead.
OK.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()
2023-08-22 23:38 ` Junio C Hamano
@ 2023-08-23 19:00 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-08-23 19:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Romain Chossart, git
On Tue, Aug 22, 2023 at 04:38:17PM -0700, Junio C Hamano wrote:
> > This function also takes a "status" parameter, which seems at first
> > glance that it could be used to propagate an error encountered when
> > computing the diff. But it doesn't work that way:
> >
> > - negative values are passed through as-is, but are not appropriate as
> > program exit codes
> >
> > - when --exit-code or --check is in effect, we _ignore_ the passed-in
> > status completely. So a failed diff which did not have a chance to
> > set opts.found_changes would erroneously report "success, no
> > changes" instead of propagating the error.
> >
> > After recent cleanups, neither of these bugs is possible to trigger, as
>
> Here "after recent cleanups" refers to the changes to make them
> die() upon seeing an error, instead of using it to call this
> function with non-zero in the status parameter? At least, they were
> signaling errors correctly when --exit-code is not in use, but now
> all the callers are responsible for exiting with non-zero status to
> signal an error even when --exit-code is *not* used.
Sort of. Prior to this series, callers were responsible for ferrying
exit codes to diff_result_code(), which mis-handled them (a little for
the path without --exit-code, and badly with it). Now they are
responsible for handling the errors themselves. I'd say that is not much
more work than passing them along, and provides better outcomes (they
can produce more useful error messages, or die() as appropriate).
But what I really meant with "after recent cleanups" was just that
nobody is even bothering to pass anything but 0 to diff_result_code().
Which was already true before the series for every code path except the
few index/argument parsing paths in builtin/diff.c, and after the
cleanups is entirely true.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread