| Message ID | xmqqczbftina.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: do not fail a no-op --edit-desc | expand |
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.
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 ;-).
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
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.
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.
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. >
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
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(-)