[PATCH] branch: do not fail a no-op --edit-desc

[PATCH] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

In a repository on a branch without branch description, try running
"git branch --edit-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.

One way to solve this is to replace the git_config_set() call that
is also used to unset the variable when the edited buffer is empty
with git_config_set_gently(), so that we do not consider it an error
that we fail to unset something that does not exist in the first
place.

But there is even a simpler way, which is to take advantage of the
fact that we _know_ if the variable existed in the first place.  If
we know we didn't have description, and if we are asked not to have
a description by the editor, we can just return doing nothing.

The simpler solution of course introduces TOCTOU, but you are
fooling yourself in your own repository.  Not overwriting the branch
description on the same branch you added in another window, while
you had this other editor open, may even be a feature ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/branch.c  | 6 ++++--
 t/t3200-branch.sh | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git c/builtin/branch.c w/builtin/branch.c
index 5d00d0b8d3..dcd847158a 100644
--- c/builtin/branch.c
+++ w/builtin/branch.c
@@ -604,10 +604,11 @@ static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
 
 static int edit_branch_description(const char *branch_name)
 {
+	int exists;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf name = STRBUF_INIT;
 
-	read_branch_desc(&buf, branch_name);
+	exists = !read_branch_desc(&buf, branch_name);
 	if (!buf.len || buf.buf[buf.len-1] != '\n')
 		strbuf_addch(&buf, '\n');
 	strbuf_commented_addf(&buf,
@@ -624,7 +625,8 @@ 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);
+	if (buf.len || exists)
+		git_config_set(name.buf, buf.len ? buf.buf : NULL);
 	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

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Rubén Justo]

On 28/9/22 21:15, Junio C Hamano wrote:
> In a repository on a branch without branch description, try running

On a branch without description, ..

> The simpler solution of course introduces TOCTOU, but you are

Nice to introduce a term that can generate curiosity.

> fooling yourself in your own repository.  Not overwriting the branch
> description on the same branch you added in another window, while
> you had this other editor open, may even be a feature ;-)

Not overwriting if there was no description in the first place, otherwise
will clear.. ¿?

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/branch.c  | 6 ++++--
>  t/t3200-branch.sh | 3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git c/builtin/branch.c w/builtin/branch.c
> index 5d00d0b8d3..dcd847158a 100644
> --- c/builtin/branch.c
> +++ w/builtin/branch.c
> @@ -604,10 +604,11 @@ static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
>  
>  static int edit_branch_description(const char *branch_name)
>  {
> +	int exists;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct strbuf name = STRBUF_INIT;
>  
> -	read_branch_desc(&buf, branch_name);
> +	exists = !read_branch_desc(&buf, branch_name);
>  	if (!buf.len || buf.buf[buf.len-1] != '\n')
>  		strbuf_addch(&buf, '\n');
>  	strbuf_commented_addf(&buf,
> @@ -624,7 +625,8 @@ 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);
> +	if (buf.len || exists)
> +		git_config_set(name.buf, buf.len ? buf.buf : NULL);
>  	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
> 

The change looks fine and removes a confusing error. Good.

Thanks.

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> On 28/9/22 21:15, Junio C Hamano wrote:
>> In a repository on a branch without branch description, try running
>
> On a branch without description, ..
>
>> The simpler solution of course introduces TOCTOU, but you are
>
> Nice to introduce a term that can generate curiosity.
>
>> fooling yourself in your own repository.  Not overwriting the branch
>> description on the same branch you added in another window, while
>> you had this other editor open, may even be a feature ;-)
>
> Not overwriting if there was no description in the first place, otherwise
> will clear.. ¿?

Time-of-check-time-of-use is an established term to describe classes
of "bugs" that arises if you do

    1. check if something is in one state

    2. perform an action to change that something's state

in separate steps, expecting that the result of the check done in
the earlier step is unchanged.  A "bug" is what happens in the
second step, when that expectation is violated.

In our case, in the "check" stage, we set "exists" based on whether
we had the branch.<current>.description configuration variable.

Then later, we use that "exists" condition and expect that nobody
messed with the state of the variable in the meantime.

An example of what happens in the updated code is

 1. We saw the description did not exist.

 2. The user told us to abort editing/setting the description.  We
    assume description does not still exist, and skip the call to
    git_config_set().

Now, what if you added a description for the branch between these
two points in time, perhaps from another window?  Because 2. above
bases its decision not to bother calling git_config_set() (because
!buf.len and !exists are both true), the description you set from
the other window will be kept.  That is the comment "feature ;-)"
refers to.

Of course, this can bite the other way.  If we did have a
description, and the user told us to remove it by giving an empty
editor result, then

 1. We see that the description does exist.

 2. We see the buf.len == 0.  Because we think the description
    exists and the user asks to remove it, we call git_config_set()
    will NULL as the value to attempt to remove the variable.

What if you removed the description for the branch between these two
points in time from another window?  We end up triggering the exact
bug that comes from the fact that we use git_config_set(), not the
_gently variant, because we are now removing what does not exist.

But at that point, the user deserves it and the problem falls
squarely into "doctor, it hurts when I twist my arm in this unusual
way! -- well, don't do it then", I would think.

Thinking what happens in the remaining two combinations is left as
an exercise to readers ;-).

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Rubén Justo]

Let me try again, I think my review was not good :-)

On 28/9/22 21:15, Junio C Hamano wrote:
> In a repository on a branch without branch description, try running

It is a bit confusing the construction "a repository on a branch
without branch description" as "branch" have "repository" inherent.
So "On a branch without description.." holds the same meaning with less
distracting words.

> The simpler solution of course introduces TOCTOU, but you are

I like that the message introduces an appropriate term that also can
be a trigger for some to learn something without distracting others.
Instead of just using: "BUG"

> fooling yourself in your own repository.  Not overwriting the branch
> description on the same branch you added in another window, while
> you had this other editor open, may even be a feature ;-)

But.. do we want to implement this this way? Maybe we will have to
implement on purpose this feature in some future refactorization?

And.. the message does not make it clear the situation: if there is
a previous description, will clear; if not, will keep.

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

If we want that feature, should we test for it? (do not take
the snippet as tested...):

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index d5a1fc1375..aa5ee14bae 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1393,6 +1393,16 @@ test_expect_success 'use --edit-description' '
        EOF
        EDITOR=./editor git branch --edit-description &&
        echo "New contents" >expect &&
+       write_script editor <<-\EOF &&
+               if [ -z "$NA" ]; then
+                       NA=description GIT_EDITOR=./$0 git branch --edit-description
+               fi
+               echo $NA >$1
+       EOF
+       EDITOR=./editor git branch --edit-description &&
+       test_must_fail git config branch.main.description &&
+       EDITOR=./editor git branch --edit-description &&
+       git config branch.main.description &&
        test_cmp expect EDITOR_OUTPUT

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Let me try again, I think my review was not good :-)
>
> On 28/9/22 21:15, Junio C Hamano wrote:
>> In a repository on a branch without branch description, try running
>
> It is a bit confusing the construction "a repository on a branch
> without branch description" as "branch" have "repository" inherent.
> So "On a branch without description.." holds the same meaning with less
> distracting words.

Ah, no, I did not mean a repository is on any branch.  I meant

    Go into a repository and be on a branch.  That branch has to be
    without branch description set.  Now, try running these...

I think the easiest clarification is to drop "In a repository"
altogether.

> But.. do we want to implement this this way? Maybe we will have to
> implement on purpose this feature in some future refactorization?

Absolutely.  It is simpler and there is not much downside.

> And.. the message does not make it clear the situation: if there is
> a previous description, will clear; if not, will keep.

If there is a previous description, then we use the behaviour we
have had for ages (i.e. will remove).  If there is not a previous
description, then we use the new behaviour (i.e. not attempt to
remove and hence we do not show an error).  Either way, the end
result is "the user indicated they do not want description by giving
us an empty edit result.  After the command exits, the branch has no
description".  

> If we want that feature, should we test for it? (do not take
> the snippet as tested...):

Notice the air-quotes around "feature" ;-) 

The official stance is "if it hurts, do not do it then".  The side
discussion about TOCTOU was to see how much hurt we are making the
user responsible for, and explaining that it is not that much.

[PATCH v2] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

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);
+	}
+
 	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

Re: [PATCH v2] branch: do not fail a no-op --edit-desc

[Ævar Arnfjörð Bjarmason]

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);

Re: [PATCH v2] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

Æ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.

Re: [PATCH v2] branch: do not fail a no-op --edit-desc

[Ævar Arnfjörð Bjarmason]

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.

[PATCH v3] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

Imagine running "git branch --edit-description" while on a branch
without the 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.  If we
know we didn't have a description, and if we are asked not to have a
description by the editor, we can just return doing nothing.

This of course introduces TOCTOU.  If you add a branch description
to the same branch from another window, while you had the editor
open to edit the description, and then exit the editor without
writing anything there, we'd end up not removing the description you
added in the other window.  But you are fooling yourself in your own
repository at that point, and if it hurts, you'd be better off not
doing so ;-).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Ævar reports that CONFIG_NOTHING_SET is not a usable error status
   for this purpose, unfortunately.  So let's go back to the approach
   taken by the initial implementation, but with the proposed log
   message clarified.

 builtin/branch.c  | 6 ++++--
 t/t3200-branch.sh | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 8c0b428104..ae08147572 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,10 +576,11 @@ static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
 
 static int edit_branch_description(const char *branch_name)
 {
+	int exists;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf name = STRBUF_INIT;
 
-	read_branch_desc(&buf, branch_name);
+	exists = !read_branch_desc(&buf, branch_name);
 	if (!buf.len || buf.buf[buf.len-1] != '\n')
 		strbuf_addch(&buf, '\n');
 	strbuf_commented_addf(&buf,
@@ -596,7 +597,8 @@ 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);
+	if (buf.len || exists)
+		git_config_set(name.buf, buf.len ? buf.buf : NULL);
 	strbuf_release(&name);
 	strbuf_release(&buf);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 3ec3e1d730..30dff9e712 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1268,6 +1268,9 @@ test_expect_success 'attempt to delete a branch merged to its base' '
 '
 
 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
-- 
2.38.0-rc2-134-gd449533db0

Re: [PATCH v2] branch: do not fail a no-op --edit-desc

[Junio C Hamano]

Æ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.

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Rubén Justo]

On 30/9/22 0:26, Junio C Hamano wrote:

>> And.. the message does not make it clear the situation: if there is
>> a previous description, will clear; if not, will keep.
> 
> If there is a previous description, then we use the behaviour we
> have had for ages (i.e. will remove).  If there is not a previous
> description, then we use the new behaviour (i.e. not attempt to
> remove and hence we do not show an error).  Either way, the end
> result is "the user indicated they do not want description by giving
> us an empty edit result.  After the command exits, the branch has no
> description".  

I was not clear, sorry.  I was referring just to the concurrent-editors
circumstance.

I find your last paragraph in the message might suggest that in case of 
concurrent editions, the last one won't overwrite if exits with an 
empty description.  But it will if the branch had a previous branch
description when that last execution started.  You might want the reader
to assume that the example follows the initial description "on a branch
without branch description", in that case everything is fine.

That was what I was trying to review.  The usage and why the error is
avoided is well explained.  And better in your v3, imo.

Thank you.

Un saludo.

Re: [PATCH] branch: do not fail a no-op --edit-desc

[Rubén Justo]

Let me add, to my other reply to this..

On 30/9/22 0:26, Junio C Hamano wrote:

>> But.. do we want to implement this this way? Maybe we will have to
>> implement on purpose this feature in some future refactorization?
> 
> Absolutely.  It is simpler and there is not much downside.

Once I realize this is porcelain, I strongly agree with this.

The TOCTOU might be resolved, perhaps outside the scope of this patch,
warning the user or not allowing two concurrent editions.

Maybe... :-) even I would dig deeper in the TOCTOU, avoiding the call
to git_config_set if no change has been made to the branch description.

Anyway, as it is, it's good, imo. Simple and resolves a confuse error
removing it.  Nice.

> The official stance is "if it hurts, do not do it then".  The side
> discussion about TOCTOU was to see how much hurt we are making the
> user responsible for, and explaining that it is not that much.
>