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

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

[Ævar Arnfjörð Bjarmason]

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.

This changes the output message from e.g.:

    fatal: remote origin already exists.

To:

    error: remote origin already exists.

Which I believe is a feature, since we generally use "fatal" for the
generic errors, and "error" for the more specific ones with a custom
exit code, but this part of the change may break code that already
relies on stderr parsing (not that we ever supported that...).

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

It's worth noting as an aside that a method of checking this that
doesn't rely on that is to check with "git config" whether the value
in question does or doesn't exist. That introduces a TOCTOU race
condition, but on the other hand this code (e.g. "git remote add")
already has a TOCTOU race.

We go through the config.lock for the actual setting of the config,
but the pseudocode logic is:

    read_config();
    check_config_and_arg_sanity();
    save_config();

So e.g. if a sleep() is added right after the remote_is_configured()
check in add() we'll clobber rermote.NAME.url, and add
another (usually duplicate) remote.NAME.fetch entry (and other values,
depending on invocation).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-remote.txt | 11 ++++++++++
 builtin/remote.c             | 42 ++++++++++++++++++++++++------------
 t/t5505-remote.sh            | 16 +++++++-------
 3 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 9659abbf8e..c1f1355f52 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -203,6 +203,17 @@ The remote configuration is achieved using the `remote.origin.url` and
 `remote.origin.fetch` configuration variables.  (See
 linkgit:git-config[1]).
 
+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`.
+
+On any other error, the exit status may be any other non-zero value.
+
 EXAMPLES
 --------
 
diff --git a/builtin/remote.c b/builtin/remote.c
index 64b4b551eb..c1828ca7d2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -191,8 +191,10 @@ static int add(int argc, const char **argv)
 	url = argv[1];
 
 	remote = remote_get(name);
-	if (remote_is_configured(remote, 1))
-		die(_("remote %s already exists."), name);
+	if (remote_is_configured(remote, 1)) {
+		error(_("remote %s already exists."), name);
+		exit(3);
+	}
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
 	if (!valid_fetch_refspec(buf2.buf))
@@ -686,15 +688,19 @@ static int mv(int argc, const char **argv)
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old_name);
-	if (!remote_is_configured(oldremote, 1))
-		die(_("No such remote: '%s'"), rename.old_name);
+	if (!remote_is_configured(oldremote, 1)) {
+		error(_("No such remote: '%s'"), rename.old_name);
+		exit(2);
+	}
 
 	if (!strcmp(rename.old_name, rename.new_name) && oldremote->origin != REMOTE_CONFIG)
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new_name);
-	if (remote_is_configured(newremote, 1))
-		die(_("remote %s already exists."), rename.new_name);
+	if (remote_is_configured(newremote, 1)) {
+		error(_("remote %s already exists."), rename.new_name);
+		exit(3);
+	}
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name);
 	if (!valid_fetch_refspec(buf.buf))
@@ -829,8 +835,10 @@ static int rm(int argc, const char **argv)
 		usage_with_options(builtin_remote_rm_usage, options);
 
 	remote = remote_get(argv[1]);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote: '%s'"), argv[1]);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote: '%s'"), argv[1]);
+		exit(2);
+	}
 
 	known_remotes.to_delete = remote;
 	for_each_remote(add_known_remote, &known_remotes);
@@ -1511,8 +1519,10 @@ static int set_remote_branches(const char *remotename, const char **branches,
 	strbuf_addf(&key, "remote.%s.fetch", remotename);
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
 		strbuf_release(&key);
@@ -1565,8 +1575,10 @@ static int get_url(int argc, const char **argv)
 	remotename = argv[0];
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	url_nr = 0;
 	if (push_mode) {
@@ -1633,8 +1645,10 @@ static int set_url(int argc, const char **argv)
 		oldurl = newurl;
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8d62edd98b..c99659d39e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -145,8 +145,8 @@ test_expect_success 'remove remote protects local branches' '
 test_expect_success 'remove errors out early when deleting non-existent branch' '
 	(
 		cd test &&
-		echo "fatal: No such remote: '\''foo'\''" >expect &&
-		test_must_fail git remote rm foo 2>actual &&
+		echo "error: No such remote: '\''foo'\''" >expect &&
+		test_expect_code 2 git remote rm foo 2>actual &&
 		test_i18ncmp expect actual
 	)
 '
@@ -173,24 +173,24 @@ test_expect_success 'remove remote with a branch without configured merge' '
 test_expect_success 'rename errors out early when deleting non-existent branch' '
 	(
 		cd test &&
-		echo "fatal: No such remote: '\''foo'\''" >expect &&
-		test_must_fail git remote rename foo bar 2>actual &&
+		echo "error: No such remote: '\''foo'\''" >expect &&
+		test_expect_code 2 git remote rename foo bar 2>actual &&
 		test_i18ncmp expect actual
 	)
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
-	echo "fatal: remote foo already exists." >expect &&
-	test_must_fail git remote add foo bar 2>actual &&
+	echo "error: remote foo already exists." >expect &&
+	test_expect_code 3 git remote add foo bar 2>actual &&
 	test_i18ncmp expect actual
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
 	test_config remote.bar.vcs bar &&
-	echo "fatal: remote bar already exists." >expect &&
-	test_must_fail git remote rename foo bar 2>actual &&
+	echo "error: remote bar already exists." >expect &&
+	test_expect_code 3 git remote rename foo bar 2>actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.28.0.297.g1956fa8f8d

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

[Eric Sunshine]

On Mon, Oct 26, 2020 at 10:48 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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.
> [...]
> So e.g. if a sleep() is added right after the remote_is_configured()
> check in add() we'll clobber rermote.NAME.url, and add

s/rermote/remote/

> another (usually duplicate) remote.NAME.fetch entry (and other values,
> depending on invocation).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.
>
> This changes the output message from e.g.:
>
>     fatal: remote origin already exists.
>
> To:
>
>     error: remote origin already exists.
>
> Which I believe is a feature, since we generally use "fatal" for the
> generic errors, and "error" for the more specific ones with a custom
> exit code, but this part of the change may break code that already
> relies on stderr parsing (not that we ever supported that...).

Sounds like a worthy goal.  One thing that disturbs me somewhat,
which the time will solve without any of our effort, is how the
calling scripts and machinery decides when they can start relying on
the exit codes.  A "git remote tell-me-what-you-can-do" command that
may or may not say "exitcode" to its standard output is added?
"test $(git version) -ge 2.30 || old_way"?

The changes to the code all looked quite straight-forward and 
the tests look sensible, too.

Thanks.

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

[Jeff King]

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:

> +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

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

[Bert Wesarg]

On Mon, Oct 26, 2020 at 3:48 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 9659abbf8e..c1f1355f52 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -203,6 +203,17 @@ The remote configuration is achieved using the `remote.origin.url` and
>  `remote.origin.fetch` configuration variables.  (See
>  linkgit:git-config[1]).
>
> +EXIT STATUS
> +-----------
> +
> +On success, the exit status is `0`.
> +
> +When subcommands such as 'add', 'rename' and 'remove' can't find the

at least a comma before 'and', but I consider 'or' better here anyway:

When subcommands such as 'add', 'rename', or 'remove' can't find the

Bert

> +remote in question the exit status is `2`, when the remote already
> +exists the exit status is `3`.
> +
> +On any other error, the exit status may be any other non-zero value.
> +
>  EXAMPLES
>  --------
>

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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. :)

Will queue with this squashed in.

 Documentation/git-remote.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git c/Documentation/git-remote.txt w/Documentation/git-remote.txt
index 7c84631343..31c29c9b31 100644
--- c/Documentation/git-remote.txt
+++ w/Documentation/git-remote.txt
@@ -208,9 +208,9 @@ 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`.
+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`.
 
 On any other error, the exit status may be any other non-zero value.
 

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

[Ævar Arnfjörð Bjarmason]

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.

This changes the output message from e.g.:

    fatal: remote origin already exists.

To:

    error: remote origin already exists.

Which I believe is a feature, since we generally use "fatal" for the
generic errors, and "error" for the more specific ones with a custom
exit code, but this part of the change may break code that already
relies on stderr parsing (not that we ever supported that...).

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

It's worth noting as an aside that a method of checking this that
doesn't rely on that is to check with "git config" whether the value
in question does or doesn't exist. That introduces a TOCTOU race
condition, but on the other hand this code (e.g. "git remote add")
already has a TOCTOU race.

We go through the config.lock for the actual setting of the config,
but the pseudocode logic is:

    read_config();
    check_config_and_arg_sanity();
    save_config();

So e.g. if a sleep() is added right after the remote_is_configured()
check in add() we'll clobber remote.NAME.url, and add another (usually
duplicate) remote.NAME.fetch entry (and other values, depending on
invocation).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Oct 26 2020, Junio C Hamano wrote:
  
> Jeff King <peff@peff.net> writes:
>
>> 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. :)
>
> Will queue with this squashed in.

Thanks, here's that version plus Eric's suggested typo fix in the
commit message.

Range-diff:
1:  26037f6294 ! 1:  4e44aed049 remote: add meaningful exit code on missing/existing
    @@ Commit message
             save_config();
     
         So e.g. if a sleep() is added right after the remote_is_configured()
    -    check in add() we'll clobber rermote.NAME.url, and add
    -    another (usually duplicate) remote.NAME.fetch entry (and other values,
    -    depending on invocation).
    +    check in add() we'll clobber remote.NAME.url, and add another (usually
    +    duplicate) remote.NAME.fetch entry (and other values, depending on
    +    invocation).
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Documentation/git-remote.txt: The remote configuration is achieved using the `re
     +
     +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`.
    ++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`.
     +
     +On any other error, the exit status may be any other non-zero value.
     +

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

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index ea73386c81..31c29c9b31 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -203,6 +203,17 @@ The remote configuration is achieved using the `remote.origin.url` and
 `remote.origin.fetch` configuration variables.  (See
 linkgit:git-config[1]).
 
+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`.
+
+On any other error, the exit status may be any other non-zero value.
+
 EXAMPLES
 --------
 
diff --git a/builtin/remote.c b/builtin/remote.c
index 64b4b551eb..c1828ca7d2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -191,8 +191,10 @@ static int add(int argc, const char **argv)
 	url = argv[1];
 
 	remote = remote_get(name);
-	if (remote_is_configured(remote, 1))
-		die(_("remote %s already exists."), name);
+	if (remote_is_configured(remote, 1)) {
+		error(_("remote %s already exists."), name);
+		exit(3);
+	}
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
 	if (!valid_fetch_refspec(buf2.buf))
@@ -686,15 +688,19 @@ static int mv(int argc, const char **argv)
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old_name);
-	if (!remote_is_configured(oldremote, 1))
-		die(_("No such remote: '%s'"), rename.old_name);
+	if (!remote_is_configured(oldremote, 1)) {
+		error(_("No such remote: '%s'"), rename.old_name);
+		exit(2);
+	}
 
 	if (!strcmp(rename.old_name, rename.new_name) && oldremote->origin != REMOTE_CONFIG)
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new_name);
-	if (remote_is_configured(newremote, 1))
-		die(_("remote %s already exists."), rename.new_name);
+	if (remote_is_configured(newremote, 1)) {
+		error(_("remote %s already exists."), rename.new_name);
+		exit(3);
+	}
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name);
 	if (!valid_fetch_refspec(buf.buf))
@@ -829,8 +835,10 @@ static int rm(int argc, const char **argv)
 		usage_with_options(builtin_remote_rm_usage, options);
 
 	remote = remote_get(argv[1]);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote: '%s'"), argv[1]);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote: '%s'"), argv[1]);
+		exit(2);
+	}
 
 	known_remotes.to_delete = remote;
 	for_each_remote(add_known_remote, &known_remotes);
@@ -1511,8 +1519,10 @@ static int set_remote_branches(const char *remotename, const char **branches,
 	strbuf_addf(&key, "remote.%s.fetch", remotename);
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
 		strbuf_release(&key);
@@ -1565,8 +1575,10 @@ static int get_url(int argc, const char **argv)
 	remotename = argv[0];
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	url_nr = 0;
 	if (push_mode) {
@@ -1633,8 +1645,10 @@ static int set_url(int argc, const char **argv)
 		oldurl = newurl;
 
 	remote = remote_get(remotename);
-	if (!remote_is_configured(remote, 1))
-		die(_("No such remote '%s'"), remotename);
+	if (!remote_is_configured(remote, 1)) {
+		error(_("No such remote '%s'"), remotename);
+		exit(2);
+	}
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8d62edd98b..c99659d39e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -145,8 +145,8 @@ test_expect_success 'remove remote protects local branches' '
 test_expect_success 'remove errors out early when deleting non-existent branch' '
 	(
 		cd test &&
-		echo "fatal: No such remote: '\''foo'\''" >expect &&
-		test_must_fail git remote rm foo 2>actual &&
+		echo "error: No such remote: '\''foo'\''" >expect &&
+		test_expect_code 2 git remote rm foo 2>actual &&
 		test_i18ncmp expect actual
 	)
 '
@@ -173,24 +173,24 @@ test_expect_success 'remove remote with a branch without configured merge' '
 test_expect_success 'rename errors out early when deleting non-existent branch' '
 	(
 		cd test &&
-		echo "fatal: No such remote: '\''foo'\''" >expect &&
-		test_must_fail git remote rename foo bar 2>actual &&
+		echo "error: No such remote: '\''foo'\''" >expect &&
+		test_expect_code 2 git remote rename foo bar 2>actual &&
 		test_i18ncmp expect actual
 	)
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
-	echo "fatal: remote foo already exists." >expect &&
-	test_must_fail git remote add foo bar 2>actual &&
+	echo "error: remote foo already exists." >expect &&
+	test_expect_code 3 git remote add foo bar 2>actual &&
 	test_i18ncmp expect actual
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
 	test_config remote.bar.vcs bar &&
-	echo "fatal: remote bar already exists." >expect &&
-	test_must_fail git remote rename foo bar 2>actual &&
+	echo "error: remote bar already exists." >expect &&
+	test_expect_code 3 git remote rename foo bar 2>actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.28.0.297.g1956fa8f8d