| Message ID | 20201026144825.26537-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remote: add meaningful exit code on missing/existing | expand |
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>
Æ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.
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
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 > -------- >
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.
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 '
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(-)