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

[Junio C Hamano <gitster@pobox.com>]

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