Re: [PATCH] remote: add meaningful exit code on missing/existing

From: Jeff King <peff@peff.net>
Date: Oct 26, 2020, 19:45 UTC
Message-ID: <20201026194519.GB16150@coredump.intra.peff.net>
In-Reply-To: <20201026144825.26537-1-avarab@gmail.com>

On Mon, Oct 26, 2020 at 03:48:25PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Change the exit code for the likes of "git remote add/rename" to exit
> with 2 if the remote in question doesn't exist, and 3 if it
> does. Before we'd just die() and exit with the general 128 exit code.

I can't think of a particular reason not to do this. Though I suspect anything scripting may generally be better off using git-config directly for more flexibility and power.

> The motivation for this is a discussion around some code in GitLab's
> gitaly which wanted to check this, and had to parse stderr to do so:
> https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2695

I didn't immediately see there the use cases you'd put this to. If it's something like "add an origin remote for fetching from a mirror", then that it is probably reasonable. But if you're thinking of something that may grow without bound (e.g., one remote per fork of the repository), keep in mind that modifying the config file is linear to its size (so creating or deleting N remotes is N^2).

>  Documentation/git-remote.txt | 11 ++++++++++
>  builtin/remote.c             | 42 ++++++++++++++++++++++++------------
>  t/t5505-remote.sh            | 16 +++++++-------
>  3 files changed, 47 insertions(+), 22 deletions(-)

The patch itself looks good to me, with one minor nit:

Show 8 quoted lines

> +EXIT STATUS
> +-----------
> +
> +On success, the exit status is `0`.
> +
> +When subcommands such as 'add', 'rename' and 'remove' can't find the
> +remote in question the exit status is `2`, when the remote already
> +exists the exit status is `3`.

This is a run-on sentence; the comma after `2` should be a period.

I'd also put an Oxford comma after `rename`, but that's a stylistic choice. :)

-Peff