diff mbox series

[v2,4/7] diff: die when failing to read index in git-diff builtin

Message ID 20230821201727.GD1798590@coredump.intra.peff.net (mailing list archive)
State Accepted
Commit 3755077b506356017d942dbcc2ffe9cb17ecc606
Headers show
Series cleaning up diff_result_code() | expand

Commit Message

Jeff King Aug. 21, 2023, 8:17 p.m. UTC
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(-)

Comments

Junio C Hamano Aug. 22, 2023, 11:27 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

> lib-ification purposes, at which point this code would already be doing
> the right thing).

Yup, I very much like that approach and attitude.  Proactively doing
the right thing even if the other parts of the code is not yet
ready.  Good.

Thanks.
diff mbox series

Patch

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);
 }