| Message ID | xmqq8rm1mz1d.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] branch: do not fail a no-op --edit-desc | expand |
On Thu, Sep 29 2022, Junio C Hamano wrote: > Imagine running "git branch --edit-description" on a branch without > a branch description, and then exit the editor after emptying the > edit buffer, which is the way to tell the command that you changed > your mind and you do not want the description after all. > > The command should just happily oblige, adding no branch description > for the current branch, and exit successfully. But it fails to do > so: > > $ git init -b main > $ git commit --allow-empty -m commit > $ GIT_EDITOR=: git branch --edit-description > fatal: could not unset 'branch.main.description' > > The end result is OK in that the configuration variable does not > exist in the resulting repository, but we should do better, by using > git_config_set_gently() and ignoring only the specific error that is > returned when removing a missing configuration variable. > > A nice side effect is that it allows us to give a pair of messages > that are tailored to the situation. Instead of reporting a failure > to set or unset a configuration variable "branch.X.description", we > can report that we failed to set or unset the description for branch > X, allowing the user to be oblivious to the irrelevant detail that > the branch description is implemented as a configuration variable. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * So, this is the "other" implementation. It is a bit more code > than the simpler one, but may be OK. I labeled this as v2 but I > do not mean I consider this one is an improved version of the > other one. They are merely alternatives. > > builtin/branch.c | 13 ++++++++++++- > t/t3200-branch.sh | 3 +++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git c/builtin/branch.c w/builtin/branch.c > index 5d00d0b8d3..033d8bd29b 100644 > --- c/builtin/branch.c > +++ w/builtin/branch.c > @@ -606,6 +606,7 @@ static int edit_branch_description(const char *branch_name) > { > struct strbuf buf = STRBUF_INIT; > struct strbuf name = STRBUF_INIT; > + int status; > > read_branch_desc(&buf, branch_name); > if (!buf.len || buf.buf[buf.len-1] != '\n') > @@ -624,7 +625,17 @@ static int edit_branch_description(const char *branch_name) > strbuf_stripspace(&buf, 1); > > strbuf_addf(&name, "branch.%s.description", branch_name); > - git_config_set(name.buf, buf.len ? buf.buf : NULL); > + > + status = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL); > + if (status && !(status == CONFIG_NOTHING_SET && !buf.len)) { > + if (buf.len) > + die(_("failed to set description for branch '%s'"), > + branch_name); > + else > + die(_("failed to unset description for branch '%s'"), > + branch_name); > + } > + I was curious to follow up on your suggestion in your 3rd paragraph, so I tried implementing this in the config API, results below, if you're interested in it assume my SOB. But, having done that I discovered that your code here has a bug, admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on "set" doesn't mean "nothing to set, because it's there already", it means "either <that>, or the key is multi-value and I'm bailing out". Which, with my patch below we can see in action with the then-one remaining user of CONFIG_NOTHING_SET: 0 $ git grep -n -C3 CONFIG_NOTHING_SET builtin/config.c-862- value = normalize_value(argv[0], argv[1]); builtin/config.c-863- UNLEAK(value); builtin/config.c-864- ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); builtin/config.c:865: if (ret == CONFIG_NOTHING_SET) builtin/config.c-866- error(_("cannot overwrite multiple values with a single value\n" builtin/config.c-867- " Use a regexp, --add or --replace-all to change %s."), argv[0]); builtin/config.c-868- return ret; The way that's buggy with your patch is if you have a config like: [branch "master"] description = foo description = bar Then --edit-description and get "bar" in your $EDITOR, change it to an empty buffer and save it, previously we'd error out, after we'll report success, but leave the double-value'd config in place. I think the obvious fix is to continue with what I have below, and then simply change what the CONFIG_NOTHING_SET flag means, and to have it only mean "the config was in this state already, nothing to do". Which, come to think of it also means that all the existing callers except that one caller in config.c are probably buggy. diff --git a/builtin/branch.c b/builtin/branch.c index a1bbdd79458..fa8f08290ea 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -601,7 +601,6 @@ static int edit_branch_description(const char *branch_name) { struct strbuf buf = STRBUF_INIT; struct strbuf name = STRBUF_INIT; - int status; read_branch_desc(&buf, branch_name); if (!buf.len || buf.buf[buf.len-1] != '\n') @@ -621,16 +620,8 @@ static int edit_branch_description(const char *branch_name) strbuf_addf(&name, "branch.%s.description", branch_name); - status = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL); - if (status && !(status == CONFIG_NOTHING_SET && !buf.len)) { - if (buf.len) - die(_("failed to set description for branch '%s'"), - branch_name); - else - die(_("failed to unset description for branch '%s'"), - branch_name); - } - + git_config_set_multivar(name.buf, buf.len ? buf.buf : NULL, NULL, + CONFIG_FLAGS_IGNORE_NOTHING_SET); strbuf_release(&name); strbuf_release(&buf); diff --git a/builtin/remote.c b/builtin/remote.c index 985b845a18b..54163cd3a78 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -665,12 +665,8 @@ static void handle_push_default(const char* old_name, const char* new_name) if (push_default.scope >= CONFIG_SCOPE_COMMAND) ; /* pass */ else if (push_default.scope >= CONFIG_SCOPE_LOCAL) { - int result = git_config_set_gently("remote.pushDefault", - new_name); - if (new_name && result && result != CONFIG_NOTHING_SET) - die(_("could not set '%s'"), "remote.pushDefault"); - else if (!new_name && result && result != CONFIG_NOTHING_SET) - die(_("could not unset '%s'"), "remote.pushDefault"); + git_config_set_multivar("remote.pushDefault", new_name, NULL, + CONFIG_FLAGS_IGNORE_NOTHING_SET); } else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) { /* warn */ warning(_("The %s configuration remote.pushDefault in:\n" @@ -889,17 +885,15 @@ static int rm(int argc, const char **argv, const char *prefix) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - result = git_config_set_gently(buf.buf, NULL); - if (result && result != CONFIG_NOTHING_SET) - die(_("could not unset '%s'"), buf.buf); + git_config_set_multivar(buf.buf, NULL, NULL, + CONFIG_FLAGS_IGNORE_NOTHING_SET); } } if (info->push_remote_name && !strcmp(info->push_remote_name, remote->name)) { strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.pushremote", item->string); - result = git_config_set_gently(buf.buf, NULL); - if (result && result != CONFIG_NOTHING_SET) - die(_("could not unset '%s'"), buf.buf); + git_config_set_multivar(buf.buf, NULL, NULL, + CONFIG_FLAGS_IGNORE_NOTHING_SET); } } diff --git a/config.c b/config.c index cbb5a3bab74..57ec50208b3 100644 --- a/config.c +++ b/config.c @@ -3245,7 +3245,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, } /* if nothing to unset, error out */ if (!value) { - ret = CONFIG_NOTHING_SET; + ret = flags & CONFIG_FLAGS_IGNORE_NOTHING_SET ? + 0 : CONFIG_NOTHING_SET; goto out_free; } @@ -3309,7 +3310,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, /* if nothing to unset, or too many matches, error out */ if ((store.seen_nr == 0 && value == NULL) || (store.seen_nr > 1 && !store.multi_replace)) { - ret = CONFIG_NOTHING_SET; + ret = flags & CONFIG_FLAGS_IGNORE_NOTHING_SET ? + 0 : CONFIG_NOTHING_SET; goto out_free; } diff --git a/config.h b/config.h index ca994d77147..b491479a863 100644 --- a/config.h +++ b/config.h @@ -299,6 +299,12 @@ int git_config_parse_key(const char *, char **, size_t *); */ #define CONFIG_FLAGS_FIXED_VALUE (1 << 1) +/* + * When CONFIG_FLAGS_NOTHING_SET is specified the non-gentle + * git_config_set_*() functions will ignore a CONFIG_NOTHING_SET. + */ +#define CONFIG_FLAGS_IGNORE_NOTHING_SET (1 << 2) + int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned); void git_config_set_multivar(const char *, const char *, const char *, unsigned); int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> The end result is OK in that the configuration variable does not >> exist in the resulting repository, but we should do better, by using >> git_config_set_gently() and ignoring only the specific error that is >> returned when removing a missing configuration variable. > ... > I was curious to follow up on your suggestion in your 3rd paragraph, so > I tried implementing this in the config API, results below, if you're > interested in it assume my SOB. Did I make any suggestion? I am assuming that what I left in the quote above is the paragraph you are referring to, and that is not a suggestion but a description of what the patch did, so I am puzzled. > But, having done that I discovered that your code here has a bug, > admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on "set" > doesn't mean "nothing to set, because it's there already", it means > "either <that>, or the key is multi-value and I'm bailing out". Ah, OK, so in short, _gently() is still unusable to use for that. I guess it means that the approach taken by v1 would be a better solution, then. Thanks.
On Fri, Sep 30 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> The end result is OK in that the configuration variable does not >>> exist in the resulting repository, but we should do better, by using >>> git_config_set_gently() and ignoring only the specific error that is >>> returned when removing a missing configuration variable. >> ... >> I was curious to follow up on your suggestion in your 3rd paragraph, so >> I tried implementing this in the config API, results below, if you're >> interested in it assume my SOB. > > Did I make any suggestion? I am assuming that what I left in the > quote above is the paragraph you are referring to, and that is not a > suggestion but a description of what the patch did, so I am puzzled. I think I just misread "by using git_config_set_gently() and ignoring only the specific error that is returned when removing a missing configuration variable." as "an alternative of this might do this via the config API"... >> But, having done that I discovered that your code here has a bug, >> admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on "set" >> doesn't mean "nothing to set, because it's there already", it means >> "either <that>, or the key is multi-value and I'm bailing out". > > Ah, OK, so in short, _gently() is still unusable to use for that. I > guess it means that the approach taken by v1 would be a better > solution, then. As you noted it's got a TOCTOU instead, so we might wipe away good config entirely. I think between the two I'd go with v2, bugs and all, it's pretty unlikely that someone has two "description" entries.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Ah, OK, so in short, _gently() is still unusable to use for that. I >> guess it means that the approach taken by v1 would be a better >> solution, then. > > As you noted it's got a TOCTOU instead, so we might wipe away good > config entirely. I do not think that is the case. What you have in mind is probably something entirely different. If you open two windows, start "edit description" in both, write a description in one of them, then write a different description in another, depending on the order you save the buffer and cause the "git branch --edit-desc" that invoked the editor to use the edited result, the description written in the editor that was later closed will win, instead of telling you "you started from state X and you want to update to state Y, but in the meantime somebody else made it state Z, so I won't overwrite it with state Y". But that has always been with us, IIUC. The only "funny" thing the change makes it do is leave the good config as-is, if the user starts without an existing branch description and exits with an empty edit buffer, instead of removing the description written in the other window in the meantime. That is quite far from wiping away good config entirely.
diff --git c/builtin/branch.c w/builtin/branch.c index 5d00d0b8d3..033d8bd29b 100644 --- c/builtin/branch.c +++ w/builtin/branch.c @@ -606,6 +606,7 @@ static int edit_branch_description(const char *branch_name) { struct strbuf buf = STRBUF_INIT; struct strbuf name = STRBUF_INIT; + int status; read_branch_desc(&buf, branch_name); if (!buf.len || buf.buf[buf.len-1] != '\n') @@ -624,7 +625,17 @@ static int edit_branch_description(const char *branch_name) strbuf_stripspace(&buf, 1); strbuf_addf(&name, "branch.%s.description", branch_name); - git_config_set(name.buf, buf.len ? buf.buf : NULL); + + status = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL); + if (status && !(status == CONFIG_NOTHING_SET && !buf.len)) { + if (buf.len) + die(_("failed to set description for branch '%s'"), + branch_name); + else + die(_("failed to unset description for branch '%s'"), + branch_name); + } + strbuf_release(&name); strbuf_release(&buf); diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh index 9723c2827c..5f72fd7453 100755 --- c/t/t3200-branch.sh +++ w/t/t3200-branch.sh @@ -1381,6 +1381,9 @@ test_expect_success 'branch --delete --force removes dangling branch' ' ' test_expect_success 'use --edit-description' ' + EDITOR=: git branch --edit-description && + test_must_fail git config branch.main.description && + write_script editor <<-\EOF && echo "New contents" >"$1" EOF
Imagine running "git branch --edit-description" on a branch without a branch description, and then exit the editor after emptying the edit buffer, which is the way to tell the command that you changed your mind and you do not want the description after all. The command should just happily oblige, adding no branch description for the current branch, and exit successfully. But it fails to do so: $ git init -b main $ git commit --allow-empty -m commit $ GIT_EDITOR=: git branch --edit-description fatal: could not unset 'branch.main.description' The end result is OK in that the configuration variable does not exist in the resulting repository, but we should do better, by using git_config_set_gently() and ignoring only the specific error that is returned when removing a missing configuration variable. A nice side effect is that it allows us to give a pair of messages that are tailored to the situation. Instead of reporting a failure to set or unset a configuration variable "branch.X.description", we can report that we failed to set or unset the description for branch X, allowing the user to be oblivious to the irrelevant detail that the branch description is implemented as a configuration variable. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * So, this is the "other" implementation. It is a bit more code than the simpler one, but may be OK. I labeled this as v2 but I do not mean I consider this one is an improved version of the other one. They are merely alternatives. builtin/branch.c | 13 ++++++++++++- t/t3200-branch.sh | 3 +++ 2 files changed, 15 insertions(+), 1 deletion(-)