{"thread":{"id":"58529","subject":"[PATCH] branch: do not fail a no-op --edit-desc","startedAt":"2022-09-28T19:15:43Z","lastAt":"2022-10-01T09:15:32Z","messageCount":13,"participants":["Junio C Hamano","Rubén Justo","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463872","messageId":"xmqqczbftina.fsf@gitster.g","threadId":"58529","inReplyTo":null,"subject":"[PATCH] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-28T19:15:37Z","receivedAt":"2022-09-28T19:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In a repository on a branch without branch description, try running\n\"git branch --edit-description\" and then exit the editor after\nemptying the edit buffer, which is the way to tell the command that\nyou changed your mind and you do not want the description after all.\n\nThe command should just happily oblige, adding no branch description\nfor the current branch, and exit successfully.  But it fails to do\nso:\n\n    $ git init -b main\n    $ git commit --allow-empty -m commit\n    $ GIT_EDITOR=: git branch --edit-description\n    fatal: could not unset 'branch.main.description'\n\nThe end result is OK in that the configuration variable does not\nexist in the resulting repository, but we should do better.\n\nOne way to solve this is to replace the git_config_set() call that\nis also used to unset the variable when the edited buffer is empty\nwith git_config_set_gently(), so that we do not consider it an error\nthat we fail to unset something that does not exist in the first\nplace.\n\nBut there is even a simpler way, which is to take advantage of the\nfact that we _know_ if the variable existed in the first place.  If\nwe know we didn't have description, and if we are asked not to have\na description by the editor, we can just return doing nothing.\n\nThe simpler solution of course introduces TOCTOU, but you are\nfooling yourself in your own repository.  Not overwriting the branch\ndescription on the same branch you added in another window, while\nyou had this other editor open, may even be a feature ;-)\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/branch.c  | 6 ++++--\n t/t3200-branch.sh | 3 +++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/branch.c w/builtin/branch.c\nindex 5d00d0b8d3..dcd847158a 100644\n--- c/builtin/branch.c\n+++ w/builtin/branch.c\n@@ -604,10 +604,11 @@ static GIT_PATH_FUNC(edit_description, \"EDIT_DESCRIPTION\")\n \n static int edit_branch_description(const char *branch_name)\n {\n+\tint exists;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf name = STRBUF_INIT;\n \n-\tread_branch_desc(&buf, branch_name);\n+\texists = !read_branch_desc(&buf, branch_name);\n \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n');\n \tstrbuf_commented_addf(&buf,\n@@ -624,7 +625,8 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_stripspace(&buf, 1);\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n+\tif (buf.len || exists)\n+\t\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n \tstrbuf_release(&name);\n \tstrbuf_release(&buf);\n \ndiff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\nindex 9723c2827c..5f72fd7453 100755\n--- c/t/t3200-branch.sh\n+++ w/t/t3200-branch.sh\n@@ -1381,6 +1381,9 @@ test_expect_success 'branch --delete --force removes dangling branch' '\n '\n \n test_expect_success 'use --edit-description' '\n+\tEDITOR=: git branch --edit-description &&\n+\ttest_must_fail git config branch.main.description &&\n+\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n \tEOF\n"},{"id":"463886","messageId":"1faa5c44-1a59-7a60-d29b-c4d4e8d0bf92@gmail.com","threadId":"58529","inReplyTo":"xmqqczbftina.fsf@gitster.g","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-28T23:04:21Z","receivedAt":"2022-09-28T23:04:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 28/9/22 21:15, Junio C Hamano wrote:\n> In a repository on a branch without branch description, try running\n\nOn a branch without description, ..\n\n> The simpler solution of course introduces TOCTOU, but you are\n\nNice to introduce a term that can generate curiosity.\n\n> fooling yourself in your own repository.  Not overwriting the branch\n> description on the same branch you added in another window, while\n> you had this other editor open, may even be a feature ;-)\n\nNot overwriting if there was no description in the first place, otherwise\nwill clear.. ¿?\n\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/branch.c  | 6 ++++--\n>  t/t3200-branch.sh | 3 +++\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git c/builtin/branch.c w/builtin/branch.c\n> index 5d00d0b8d3..dcd847158a 100644\n> --- c/builtin/branch.c\n> +++ w/builtin/branch.c\n> @@ -604,10 +604,11 @@ static GIT_PATH_FUNC(edit_description, \"EDIT_DESCRIPTION\")\n>  \n>  static int edit_branch_description(const char *branch_name)\n>  {\n> +\tint exists;\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct strbuf name = STRBUF_INIT;\n>  \n> -\tread_branch_desc(&buf, branch_name);\n> +\texists = !read_branch_desc(&buf, branch_name);\n>  \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n>  \t\tstrbuf_addch(&buf, '\\n');\n>  \tstrbuf_commented_addf(&buf,\n> @@ -624,7 +625,8 @@ static int edit_branch_description(const char *branch_name)\n>  \tstrbuf_stripspace(&buf, 1);\n>  \n>  \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n> -\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n> +\tif (buf.len || exists)\n> +\t\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n>  \tstrbuf_release(&name);\n>  \tstrbuf_release(&buf);\n>  \n> diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\n> index 9723c2827c..5f72fd7453 100755\n> --- c/t/t3200-branch.sh\n> +++ w/t/t3200-branch.sh\n> @@ -1381,6 +1381,9 @@ test_expect_success 'branch --delete --force removes dangling branch' '\n>  '\n>  \n>  test_expect_success 'use --edit-description' '\n> +\tEDITOR=: git branch --edit-description &&\n> +\ttest_must_fail git config branch.main.description &&\n> +\n>  \twrite_script editor <<-\\EOF &&\n>  \t\techo \"New contents\" >\"$1\"\n>  \tEOF\n> \n\nThe change looks fine and removes a confusing error. Good.\n\nThanks.\n"},{"id":"463889","messageId":"xmqq1qrvqd85.fsf@gitster.g","threadId":"58529","inReplyTo":"1faa5c44-1a59-7a60-d29b-c4d4e8d0bf92@gmail.com","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-28T23:40:58Z","receivedAt":"2022-09-28T23:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> On 28/9/22 21:15, Junio C Hamano wrote:\n>> In a repository on a branch without branch description, try running\n>\n> On a branch without description, ..\n>\n>> The simpler solution of course introduces TOCTOU, but you are\n>\n> Nice to introduce a term that can generate curiosity.\n>\n>> fooling yourself in your own repository.  Not overwriting the branch\n>> description on the same branch you added in another window, while\n>> you had this other editor open, may even be a feature ;-)\n>\n> Not overwriting if there was no description in the first place, otherwise\n> will clear.. ¿?\n\nTime-of-check-time-of-use is an established term to describe classes\nof \"bugs\" that arises if you do\n\n    1. check if something is in one state\n\n    2. perform an action to change that something's state\n\nin separate steps, expecting that the result of the check done in\nthe earlier step is unchanged.  A \"bug\" is what happens in the\nsecond step, when that expectation is violated.\n\nIn our case, in the \"check\" stage, we set \"exists\" based on whether\nwe had the branch.<current>.description configuration variable.\n\nThen later, we use that \"exists\" condition and expect that nobody\nmessed with the state of the variable in the meantime.\n\nAn example of what happens in the updated code is\n\n 1. We saw the description did not exist.\n\n 2. The user told us to abort editing/setting the description.  We\n    assume description does not still exist, and skip the call to\n    git_config_set().\n\nNow, what if you added a description for the branch between these\ntwo points in time, perhaps from another window?  Because 2. above\nbases its decision not to bother calling git_config_set() (because\n!buf.len and !exists are both true), the description you set from\nthe other window will be kept.  That is the comment \"feature ;-)\"\nrefers to.\n\nOf course, this can bite the other way.  If we did have a\ndescription, and the user told us to remove it by giving an empty\neditor result, then\n\n 1. We see that the description does exist.\n\n 2. We see the buf.len == 0.  Because we think the description\n    exists and the user asks to remove it, we call git_config_set()\n    will NULL as the value to attempt to remove the variable.\n\nWhat if you removed the description for the branch between these two\npoints in time from another window?  We end up triggering the exact\nbug that comes from the fact that we use git_config_set(), not the\n_gently variant, because we are now removing what does not exist.\n\nBut at that point, the user deserves it and the problem falls\nsquarely into \"doctor, it hurts when I twist my arm in this unusual\nway! -- well, don't do it then\", I would think.\n\nThinking what happens in the remaining two combinations is left as\nan exercise to readers ;-).\n\n\n\n\n"},{"id":"463921","messageId":"ba8a503b-76e2-5de9-1082-3b4c6ecd0cc3@gmail.com","threadId":"58529","inReplyTo":"xmqqczbftina.fsf@gitster.g","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-29T21:49:50Z","receivedAt":"2022-09-29T21:49:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Let me try again, I think my review was not good :-)\n\nOn 28/9/22 21:15, Junio C Hamano wrote:\n> In a repository on a branch without branch description, try running\n\nIt is a bit confusing the construction \"a repository on a branch\nwithout branch description\" as \"branch\" have \"repository\" inherent.\nSo \"On a branch without description..\" holds the same meaning with less\ndistracting words.\n\n> The simpler solution of course introduces TOCTOU, but you are\n\nI like that the message introduces an appropriate term that also can\nbe a trigger for some to learn something without distracting others.\nInstead of just using: \"BUG\"\n\n> fooling yourself in your own repository.  Not overwriting the branch\n> description on the same branch you added in another window, while\n> you had this other editor open, may even be a feature ;-)\n\nBut.. do we want to implement this this way? Maybe we will have to\nimplement on purpose this feature in some future refactorization?\n\nAnd.. the message does not make it clear the situation: if there is\na previous description, will clear; if not, will keep.\n\n>  test_expect_success 'use --edit-description' '\n> +\tEDITOR=: git branch --edit-description &&\n> +\ttest_must_fail git config branch.main.description &&\n> +\n>  \twrite_script editor <<-\\EOF &&\n>  \t\techo \"New contents\" >\"$1\"\n>  \tEOF\n> \n\nIf we want that feature, should we test for it? (do not take\nthe snippet as tested...):\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex d5a1fc1375..aa5ee14bae 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1393,6 +1393,16 @@ test_expect_success 'use --edit-description' '\n        EOF\n        EDITOR=./editor git branch --edit-description &&\n        echo \"New contents\" >expect &&\n+       write_script editor <<-\\EOF &&\n+               if [ -z \"$NA\" ]; then\n+                       NA=description GIT_EDITOR=./$0 git branch --edit-description\n+               fi\n+               echo $NA >$1\n+       EOF\n+       EDITOR=./editor git branch --edit-description &&\n+       test_must_fail git config branch.main.description &&\n+       EDITOR=./editor git branch --edit-description &&\n+       git config branch.main.description &&\n        test_cmp expect EDITOR_OUTPUT\n"},{"id":"463925","messageId":"xmqq7d1lom12.fsf@gitster.g","threadId":"58529","inReplyTo":"ba8a503b-76e2-5de9-1082-3b4c6ecd0cc3@gmail.com","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T22:26:01Z","receivedAt":"2022-09-29T22:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Let me try again, I think my review was not good :-)\n>\n> On 28/9/22 21:15, Junio C Hamano wrote:\n>> In a repository on a branch without branch description, try running\n>\n> It is a bit confusing the construction \"a repository on a branch\n> without branch description\" as \"branch\" have \"repository\" inherent.\n> So \"On a branch without description..\" holds the same meaning with less\n> distracting words.\n\nAh, no, I did not mean a repository is on any branch.  I meant\n\n    Go into a repository and be on a branch.  That branch has to be\n    without branch description set.  Now, try running these...\n\nI think the easiest clarification is to drop \"In a repository\"\naltogether.\n\n> But.. do we want to implement this this way? Maybe we will have to\n> implement on purpose this feature in some future refactorization?\n\nAbsolutely.  It is simpler and there is not much downside.\n\n> And.. the message does not make it clear the situation: if there is\n> a previous description, will clear; if not, will keep.\n\nIf there is a previous description, then we use the behaviour we\nhave had for ages (i.e. will remove).  If there is not a previous\ndescription, then we use the new behaviour (i.e. not attempt to\nremove and hence we do not show an error).  Either way, the end\nresult is \"the user indicated they do not want description by giving\nus an empty edit result.  After the command exits, the branch has no\ndescription\".  \n\n> If we want that feature, should we test for it? (do not take\n> the snippet as tested...):\n\nNotice the air-quotes around \"feature\" ;-) \n\nThe official stance is \"if it hurts, do not do it then\".  The side\ndiscussion about TOCTOU was to see how much hurt we are making the\nuser responsible for, and explaining that it is not that much.\n"},{"id":"463927","messageId":"xmqq8rm1mz1d.fsf@gitster.g","threadId":"58529","inReplyTo":"xmqqczbftina.fsf@gitster.g","subject":"[PATCH v2] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-30T01:27:58Z","receivedAt":"2022-09-30T01:28:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine running \"git branch --edit-description\" on a branch without\na branch description, and then exit the editor after emptying the\nedit buffer, which is the way to tell the command that you changed\nyour mind and you do not want the description after all.\n\nThe command should just happily oblige, adding no branch description\nfor the current branch, and exit successfully.  But it fails to do\nso:\n\n    $ git init -b main\n    $ git commit --allow-empty -m commit\n    $ GIT_EDITOR=: git branch --edit-description\n    fatal: could not unset 'branch.main.description'\n\nThe end result is OK in that the configuration variable does not\nexist in the resulting repository, but we should do better, by using\ngit_config_set_gently() and ignoring only the specific error that is\nreturned when removing a missing configuration variable.\n\nA nice side effect is that it allows us to give a pair of messages\nthat are tailored to the situation.  Instead of reporting a failure\nto set or unset a configuration variable \"branch.X.description\", we\ncan report that we failed to set or unset the description for branch\nX, allowing the user to be oblivious to the irrelevant detail that\nthe branch description is implemented as a configuration variable.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * So, this is the \"other\" implementation.  It is a bit more code\n   than the simpler one, but may be OK.  I labeled this as v2 but I\n   do not mean I consider this one is an improved version of the\n   other one.  They are merely alternatives.\n\n builtin/branch.c  | 13 ++++++++++++-\n t/t3200-branch.sh |  3 +++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/branch.c w/builtin/branch.c\nindex 5d00d0b8d3..033d8bd29b 100644\n--- c/builtin/branch.c\n+++ w/builtin/branch.c\n@@ -606,6 +606,7 @@ static int edit_branch_description(const char *branch_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf name = STRBUF_INIT;\n+\tint status;\n \n \tread_branch_desc(&buf, branch_name);\n \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n@@ -624,7 +625,17 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_stripspace(&buf, 1);\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n+\n+\tstatus = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL);\n+\tif (status && !(status == CONFIG_NOTHING_SET && !buf.len)) {\n+\t\tif (buf.len)\n+\t\t\tdie(_(\"failed to set description for branch '%s'\"), \n+\t\t\t    branch_name);\n+\t\telse\n+\t\t\tdie(_(\"failed to unset description for branch '%s'\"), \n+\t\t\t    branch_name);\n+\t}\n+\n \tstrbuf_release(&name);\n \tstrbuf_release(&buf);\n \ndiff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\nindex 9723c2827c..5f72fd7453 100755\n--- c/t/t3200-branch.sh\n+++ w/t/t3200-branch.sh\n@@ -1381,6 +1381,9 @@ test_expect_success 'branch --delete --force removes dangling branch' '\n '\n \n test_expect_success 'use --edit-description' '\n+\tEDITOR=: git branch --edit-description &&\n+\ttest_must_fail git config branch.main.description &&\n+\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n \tEOF\n"},{"id":"463931","messageId":"220930.86v8p5updv.gmgdl@evledraar.gmail.com","threadId":"58529","inReplyTo":"xmqq8rm1mz1d.fsf@gitster.g","subject":"Re: [PATCH v2] branch: do not fail a no-op --edit-desc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T10:21:48Z","receivedAt":"2022-09-30T11:00:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 29 2022, Junio C Hamano wrote:\n\n> Imagine running \"git branch --edit-description\" on a branch without\n> a branch description, and then exit the editor after emptying the\n> edit buffer, which is the way to tell the command that you changed\n> your mind and you do not want the description after all.\n>\n> The command should just happily oblige, adding no branch description\n> for the current branch, and exit successfully.  But it fails to do\n> so:\n>\n>     $ git init -b main\n>     $ git commit --allow-empty -m commit\n>     $ GIT_EDITOR=: git branch --edit-description\n>     fatal: could not unset 'branch.main.description'\n>\n> The end result is OK in that the configuration variable does not\n> exist in the resulting repository, but we should do better, by using\n> git_config_set_gently() and ignoring only the specific error that is\n> returned when removing a missing configuration variable.\n>\n> A nice side effect is that it allows us to give a pair of messages\n> that are tailored to the situation.  Instead of reporting a failure\n> to set or unset a configuration variable \"branch.X.description\", we\n> can report that we failed to set or unset the description for branch\n> X, allowing the user to be oblivious to the irrelevant detail that\n> the branch description is implemented as a configuration variable.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  * So, this is the \"other\" implementation.  It is a bit more code\n>    than the simpler one, but may be OK.  I labeled this as v2 but I\n>    do not mean I consider this one is an improved version of the\n>    other one.  They are merely alternatives.\n>\n>  builtin/branch.c  | 13 ++++++++++++-\n>  t/t3200-branch.sh |  3 +++\n>  2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git c/builtin/branch.c w/builtin/branch.c\n> index 5d00d0b8d3..033d8bd29b 100644\n> --- c/builtin/branch.c\n> +++ w/builtin/branch.c\n> @@ -606,6 +606,7 @@ static int edit_branch_description(const char *branch_name)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct strbuf name = STRBUF_INIT;\n> +\tint status;\n>  \n>  \tread_branch_desc(&buf, branch_name);\n>  \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n> @@ -624,7 +625,17 @@ static int edit_branch_description(const char *branch_name)\n>  \tstrbuf_stripspace(&buf, 1);\n>  \n>  \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n> -\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n> +\n> +\tstatus = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL);\n> +\tif (status && !(status == CONFIG_NOTHING_SET && !buf.len)) {\n> +\t\tif (buf.len)\n> +\t\t\tdie(_(\"failed to set description for branch '%s'\"), \n> +\t\t\t    branch_name);\n> +\t\telse\n> +\t\t\tdie(_(\"failed to unset description for branch '%s'\"), \n> +\t\t\t    branch_name);\n> +\t}\n> +\n\nI was curious to follow up on your suggestion in your 3rd paragraph, so\nI tried implementing this in the config API, results below, if you're\ninterested in it assume my SOB.\n\nBut, having done that I discovered that your code here has a bug,\nadmittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on \"set\"\ndoesn't mean \"nothing to set, because it's there already\", it means\n\"either <that>, or the key is multi-value and I'm bailing out\".\n\n\nWhich, with my patch below we can see in action with the then-one\nremaining user of CONFIG_NOTHING_SET:\n\t\n\t0 $ git grep -n -C3 CONFIG_NOTHING_SET\n\tbuiltin/config.c-862-           value = normalize_value(argv[0], argv[1]);\n\tbuiltin/config.c-863-           UNLEAK(value);\n\tbuiltin/config.c-864-           ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n\tbuiltin/config.c:865:           if (ret == CONFIG_NOTHING_SET)\n\tbuiltin/config.c-866-                   error(_(\"cannot overwrite multiple values with a single value\\n\"\n\tbuiltin/config.c-867-                   \"       Use a regexp, --add or --replace-all to change %s.\"), argv[0]);\n\tbuiltin/config.c-868-           return ret;\n\nThe way that's buggy with your patch is if you have a config like:\n\n\t[branch \"master\"]\n\tdescription = foo\n\tdescription = bar\n\nThen --edit-description and get \"bar\" in your $EDITOR, change it to an\nempty buffer and save it, previously we'd error out, after we'll report\nsuccess, but leave the double-value'd config in place.\n\nI think the obvious fix is to continue with what I have below, and then\nsimply change what the CONFIG_NOTHING_SET flag means, and to have it\nonly mean \"the config was in this state already, nothing to do\".\n\nWhich, come to think of it also means that all the existing callers\nexcept that one caller in config.c are probably buggy.\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex a1bbdd79458..fa8f08290ea 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -601,7 +601,6 @@ static int edit_branch_description(const char *branch_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf name = STRBUF_INIT;\n-\tint status;\n \n \tread_branch_desc(&buf, branch_name);\n \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n@@ -621,16 +620,8 @@ static int edit_branch_description(const char *branch_name)\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n \n-\tstatus = git_config_set_gently(name.buf, buf.len ? buf.buf : NULL);\n-\tif (status && !(status == CONFIG_NOTHING_SET && !buf.len)) {\n-\t\tif (buf.len)\n-\t\t\tdie(_(\"failed to set description for branch '%s'\"), \n-\t\t\t    branch_name);\n-\t\telse\n-\t\t\tdie(_(\"failed to unset description for branch '%s'\"), \n-\t\t\t    branch_name);\n-\t}\n-\n+\tgit_config_set_multivar(name.buf, buf.len ? buf.buf : NULL, NULL,\n+\t\t\t\tCONFIG_FLAGS_IGNORE_NOTHING_SET);\n \tstrbuf_release(&name);\n \tstrbuf_release(&buf);\n \ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 985b845a18b..54163cd3a78 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -665,12 +665,8 @@ static void handle_push_default(const char* old_name, const char* new_name)\n \tif (push_default.scope >= CONFIG_SCOPE_COMMAND)\n \t\t; /* pass */\n \telse if (push_default.scope >= CONFIG_SCOPE_LOCAL) {\n-\t\tint result = git_config_set_gently(\"remote.pushDefault\",\n-\t\t\t\t\t\t   new_name);\n-\t\tif (new_name && result && result != CONFIG_NOTHING_SET)\n-\t\t\tdie(_(\"could not set '%s'\"), \"remote.pushDefault\");\n-\t\telse if (!new_name && result && result != CONFIG_NOTHING_SET)\n-\t\t\tdie(_(\"could not unset '%s'\"), \"remote.pushDefault\");\n+\t\tgit_config_set_multivar(\"remote.pushDefault\", new_name, NULL,\n+\t\t\t\t\tCONFIG_FLAGS_IGNORE_NOTHING_SET);\n \t} else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) {\n \t\t/* warn */\n \t\twarning(_(\"The %s configuration remote.pushDefault in:\\n\"\n@@ -889,17 +885,15 @@ static int rm(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n-\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n-\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n-\t\t\t\t\tdie(_(\"could not unset '%s'\"), buf.buf);\n+\t\t\t\tgit_config_set_multivar(buf.buf, NULL, NULL,\n+\t\t\t\t\t\t\tCONFIG_FLAGS_IGNORE_NOTHING_SET);\n \t\t\t}\n \t\t}\n \t\tif (info->push_remote_name && !strcmp(info->push_remote_name, remote->name)) {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addf(&buf, \"branch.%s.pushremote\", item->string);\n-\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n-\t\t\tif (result && result != CONFIG_NOTHING_SET)\n-\t\t\t\tdie(_(\"could not unset '%s'\"), buf.buf);\n+\t\t\tgit_config_set_multivar(buf.buf, NULL, NULL,\n+\t\t\t\t\t\tCONFIG_FLAGS_IGNORE_NOTHING_SET);\n \t\t}\n \t}\n \ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..57ec50208b3 100644\n--- a/config.c\n+++ b/config.c\n@@ -3245,7 +3245,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t}\n \t\t/* if nothing to unset, error out */\n \t\tif (!value) {\n-\t\t\tret = CONFIG_NOTHING_SET;\n+\t\t\tret = flags & CONFIG_FLAGS_IGNORE_NOTHING_SET ?\n+\t\t\t\t0 : CONFIG_NOTHING_SET;\n \t\t\tgoto out_free;\n \t\t}\n \n@@ -3309,7 +3310,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen_nr == 0 && value == NULL) ||\n \t\t    (store.seen_nr > 1 && !store.multi_replace)) {\n-\t\t\tret = CONFIG_NOTHING_SET;\n+\t\t\tret = flags & CONFIG_FLAGS_IGNORE_NOTHING_SET ?\n+\t\t\t\t0 : CONFIG_NOTHING_SET;\n \t\t\tgoto out_free;\n \t\t}\n \ndiff --git a/config.h b/config.h\nindex ca994d77147..b491479a863 100644\n--- a/config.h\n+++ b/config.h\n@@ -299,6 +299,12 @@ int git_config_parse_key(const char *, char **, size_t *);\n  */\n #define CONFIG_FLAGS_FIXED_VALUE (1 << 1)\n \n+/*\n+ * When CONFIG_FLAGS_NOTHING_SET is specified the non-gentle\n+ * git_config_set_*() functions will ignore a CONFIG_NOTHING_SET.\n+ */\n+#define CONFIG_FLAGS_IGNORE_NOTHING_SET (1 << 2)\n+\n int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);\n void git_config_set_multivar(const char *, const char *, const char *, unsigned);\n int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);\n"},{"id":"463964","messageId":"xmqq7d1klrua.fsf@gitster.g","threadId":"58529","inReplyTo":"220930.86v8p5updv.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-30T17:01:01Z","receivedAt":"2022-09-30T17:01:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> The end result is OK in that the configuration variable does not\n>> exist in the resulting repository, but we should do better, by using\n>> git_config_set_gently() and ignoring only the specific error that is\n>> returned when removing a missing configuration variable.\n> ...\n> I was curious to follow up on your suggestion in your 3rd paragraph, so\n> I tried implementing this in the config API, results below, if you're\n> interested in it assume my SOB.\n\nDid I make any suggestion?  I am assuming that what I left in the\nquote above is the paragraph you are referring to, and that is not a\nsuggestion but a description of what the patch did, so I am puzzled.\n\n> But, having done that I discovered that your code here has a bug,\n> admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on \"set\"\n> doesn't mean \"nothing to set, because it's there already\", it means\n> \"either <that>, or the key is multi-value and I'm bailing out\".\n\nAh, OK, so in short, _gently() is still unusable to use for that.  I\nguess it means that the approach taken by v1 would be a better\nsolution, then.\n\nThanks.\n"},{"id":"463971","messageId":"220930.861qrsvj12.gmgdl@evledraar.gmail.com","threadId":"58529","inReplyTo":"xmqq7d1klrua.fsf@gitster.g","subject":"Re: [PATCH v2] branch: do not fail a no-op --edit-desc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-30T17:58:03Z","receivedAt":"2022-09-30T18:01:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 30 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> The end result is OK in that the configuration variable does not\n>>> exist in the resulting repository, but we should do better, by using\n>>> git_config_set_gently() and ignoring only the specific error that is\n>>> returned when removing a missing configuration variable.\n>> ...\n>> I was curious to follow up on your suggestion in your 3rd paragraph, so\n>> I tried implementing this in the config API, results below, if you're\n>> interested in it assume my SOB.\n>\n> Did I make any suggestion?  I am assuming that what I left in the\n> quote above is the paragraph you are referring to, and that is not a\n> suggestion but a description of what the patch did, so I am puzzled.\n\nI think I just misread \"by using git_config_set_gently() and ignoring\nonly the specific error that is returned when removing a missing\nconfiguration variable.\" as \"an alternative of this might do this via\nthe config API\"...\n\n>> But, having done that I discovered that your code here has a bug,\n>> admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on \"set\"\n>> doesn't mean \"nothing to set, because it's there already\", it means\n>> \"either <that>, or the key is multi-value and I'm bailing out\".\n>\n> Ah, OK, so in short, _gently() is still unusable to use for that.  I\n> guess it means that the approach taken by v1 would be a better\n> solution, then.\n\nAs you noted it's got a TOCTOU instead, so we might wipe away good\nconfig entirely.\n\nI think between the two I'd go with v2, bugs and all, it's pretty\nunlikely that someone has two \"description\" entries.\n\n"},{"id":"463972","messageId":"xmqqmtagka8x.fsf@gitster.g","threadId":"58529","inReplyTo":"xmqq8rm1mz1d.fsf@gitster.g","subject":"[PATCH v3] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-30T18:06:22Z","receivedAt":"2022-09-30T18:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine running \"git branch --edit-description\" while on a branch\nwithout the branch description, and then exit the editor after\nemptying the edit buffer, which is the way to tell the command that\nyou changed your mind and you do not want the description after all.\n\nThe command should just happily oblige, adding no branch description\nfor the current branch, and exit successfully.  But it fails to do\nso:\n\n    $ git init -b main\n    $ git commit --allow-empty -m commit\n    $ GIT_EDITOR=: git branch --edit-description\n    fatal: could not unset 'branch.main.description'\n\nThe end result is OK in that the configuration variable does not\nexist in the resulting repository, but we should do better.  If we\nknow we didn't have a description, and if we are asked not to have a\ndescription by the editor, we can just return doing nothing.\n\nThis of course introduces TOCTOU.  If you add a branch description\nto the same branch from another window, while you had the editor\nopen to edit the description, and then exit the editor without\nwriting anything there, we'd end up not removing the description you\nadded in the other window.  But you are fooling yourself in your own\nrepository at that point, and if it hurts, you'd be better off not\ndoing so ;-).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * Ævar reports that CONFIG_NOTHING_SET is not a usable error status\n   for this purpose, unfortunately.  So let's go back to the approach\n   taken by the initial implementation, but with the proposed log\n   message clarified.\n\n builtin/branch.c  | 6 ++++--\n t/t3200-branch.sh | 3 +++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8c0b428104..ae08147572 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -576,10 +576,11 @@ static GIT_PATH_FUNC(edit_description, \"EDIT_DESCRIPTION\")\n \n static int edit_branch_description(const char *branch_name)\n {\n+\tint exists;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf name = STRBUF_INIT;\n \n-\tread_branch_desc(&buf, branch_name);\n+\texists = !read_branch_desc(&buf, branch_name);\n \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n');\n \tstrbuf_commented_addf(&buf,\n@@ -596,7 +597,8 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_stripspace(&buf, 1);\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n+\tif (buf.len || exists)\n+\t\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n \tstrbuf_release(&name);\n \tstrbuf_release(&buf);\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 3ec3e1d730..30dff9e712 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1268,6 +1268,9 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n '\n \n test_expect_success 'use --edit-description' '\n+\tEDITOR=: git branch --edit-description &&\n+\ttest_must_fail git config branch.main.description &&\n+\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n \tEOF\n-- \n2.38.0-rc2-134-gd449533db0\n\n"},{"id":"464010","messageId":"xmqqill4k9wr.fsf@gitster.g","threadId":"58529","inReplyTo":"220930.861qrsvj12.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] branch: do not fail a no-op --edit-desc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-30T18:13:40Z","receivedAt":"2022-09-30T18:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Ah, OK, so in short, _gently() is still unusable to use for that.  I\n>> guess it means that the approach taken by v1 would be a better\n>> solution, then.\n>\n> As you noted it's got a TOCTOU instead, so we might wipe away good\n> config entirely.\n\nI do not think that is the case.\n\nWhat you have in mind is probably something entirely different.  If\nyou open two windows, start \"edit description\" in both, write a\ndescription in one of them, then write a different description in\nanother, depending on the order you save the buffer and cause the\n\"git branch --edit-desc\" that invoked the editor to use the edited\nresult, the description written in the editor that was later closed\nwill win, instead of telling you \"you started from state X and you\nwant to update to state Y, but in the meantime somebody else made it\nstate Z, so I won't overwrite it with state Y\".  But that has always\nbeen with us, IIUC.\n\nThe only \"funny\" thing the change makes it do is leave the good\nconfig as-is, if the user starts without an existing branch\ndescription and exits with an empty edit buffer, instead of removing\nthe description written in the other window in the meantime.  That\nis quite far from wiping away good config entirely.\n\n"},{"id":"464031","messageId":"0c875ba3-f2cd-7fe1-3192-7759e5cebdb7@gmail.com","threadId":"58529","inReplyTo":"xmqq7d1lom12.fsf@gitster.g","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-30T22:59:48Z","receivedAt":"2022-09-30T22:59:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 30/9/22 0:26, Junio C Hamano wrote:\n\n>> And.. the message does not make it clear the situation: if there is\n>> a previous description, will clear; if not, will keep.\n> \n> If there is a previous description, then we use the behaviour we\n> have had for ages (i.e. will remove).  If there is not a previous\n> description, then we use the new behaviour (i.e. not attempt to\n> remove and hence we do not show an error).  Either way, the end\n> result is \"the user indicated they do not want description by giving\n> us an empty edit result.  After the command exits, the branch has no\n> description\".  \n\nI was not clear, sorry.  I was referring just to the concurrent-editors\ncircumstance.\n\nI find your last paragraph in the message might suggest that in case of \nconcurrent editions, the last one won't overwrite if exits with an \nempty description.  But it will if the branch had a previous branch\ndescription when that last execution started.  You might want the reader\nto assume that the example follows the initial description \"on a branch\nwithout branch description\", in that case everything is fine.\n\nThat was what I was trying to review.  The usage and why the error is\navoided is well explained.  And better in your v3, imo.\n\nThank you.\n\nUn saludo.\n"},{"id":"464034","messageId":"c354947a-06d6-3852-6afe-bd560dd34d6e@gmail.com","threadId":"58529","inReplyTo":"xmqq7d1lom12.fsf@gitster.g","subject":"Re: [PATCH] branch: do not fail a no-op --edit-desc","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-01T09:15:14Z","receivedAt":"2022-10-01T09:15:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Let me add, to my other reply to this..\n\nOn 30/9/22 0:26, Junio C Hamano wrote:\n\n>> But.. do we want to implement this this way? Maybe we will have to\n>> implement on purpose this feature in some future refactorization?\n> \n> Absolutely.  It is simpler and there is not much downside.\n\nOnce I realize this is porcelain, I strongly agree with this.\n\nThe TOCTOU might be resolved, perhaps outside the scope of this patch,\nwarning the user or not allowing two concurrent editions.\n\nMaybe... :-) even I would dig deeper in the TOCTOU, avoiding the call\nto git_config_set if no change has been made to the branch description.\n\nAnyway, as it is, it's good, imo. Simple and resolves a confuse error\nremoving it.  Nice.\n\n> The official stance is \"if it hurts, do not do it then\".  The side\n> discussion about TOCTOU was to see how much hurt we are making the\n> user responsible for, and explaining that it is not that much.\n> \n"}]}