On Mon, Aug 21, 2023 at 11:39:36AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > That would make the code simpler. It does feel a bit like going in the
> > opposite direction of recent "pass errors up the stack rather than
> > dying" libification efforts. I think that's OK for the builtin_* helpers
> > in diff.c, which are just serving the diff porcelain. But things like
> > run_diff_files(), while pretty big operations, are something we might
> > call as small part of another operation (like git-describe).
>
> True, for things in diff-lib.c we likely would want to go in the
> opposite "return an error to be handled by the caller" route.
After poking at it a bit, I think it actually is OK to drop the return
codes here. There are some immediate benefits, and I'm not sure it
actually hampers libification that much; see patch 5 for my reasoning.
So here's what I came up with.
[1/7]: diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()
Your patch verbatim, since there's tons of textual conflicts
otherwise.
[2/7]: diff-files: avoid negative exit value
A bonus fix that I noticed. It's the same problem as found
elsewhere, but using a different code path, so it seemed easiest to
fix on its own.
[3/7]: diff: show usage for unknown builtin_diff_files() options
This directly fixes the bug found by Romain (but in a different way
than v1).
[4/7]: diff: die when failing to read index in git-diff builtin
Obvious cleanups.
[5/7]: diff: drop useless return from run_diff_{files,index} functions
Possibly controversial cleanups. ;)
[6/7]: diff: drop useless return values in git-diff helpers
[7/7]: diff: drop useless "status" parameter from diff_result_code()
And then these are the payoff cleanups enabled by patch 5.
add-interactive.c | 2 +-
builtin/add.c | 3 +-
builtin/am.c | 4 +-
builtin/describe.c | 6 +--
builtin/diff-files.c | 12 ++----
builtin/diff-index.c | 4 +-
builtin/diff-tree.c | 2 +-
builtin/diff.c | 79 +++++++++++++++++--------------------
builtin/log.c | 2 +-
builtin/stash.c | 16 +++-----
builtin/submodule--helper.c | 7 ++--
diff-lib.c | 8 ++--
diff-no-index.c | 2 +-
diff.c | 6 +--
diff.h | 6 +--
t/t4017-diff-retval.sh | 5 +++
wt-status.c | 12 +++---
17 files changed, 81 insertions(+), 95 deletions(-)
No range-diff since it's effectively a brand-new series.
BTW, I know you were looking at --exit-code bugs recently in another
series. But I think this should all be orthogonal (both semantically
and textually). I'll try to give another review on that series, as well.
-Peff
