[PATCH v2 0/7] cleaning up diff_result_code()

From: Jeff King <peff@peff.net>
Date: Aug 21, 2023, 20:13 UTC
Message-ID: <20230821201358.GA2663749@coredump.intra.peff.net>
In-Reply-To: <xmqqfs4cp7dj.fsf@gitster.g>

On Mon, Aug 21, 2023 at 11:39:36AM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> Jeff King <peff@peff.net> 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