| Message ID | de200fa0-379d-c1ce-8446-9e4292d0b66a@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] branch: support for shortcuts like @{-1}, completed | expand |
Rubén Justo <rjusto@gmail.com> writes: > ... I think this is a good stop point for this change if no new problems > are detected. > > Un saludo. Looks good. Thanks.
Rubén Justo <rjusto@gmail.com> writes: > @@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > } else if (edit_description) { > const char *branch_name; > struct strbuf branch_ref = STRBUF_INIT; > + struct strbuf buf = STRBUF_INIT; > > if (!argc) { > if (filter.detached) > die(_("Cannot give description to detached HEAD")); > branch_name = head; > - } else if (argc == 1) > - branch_name = argv[0]; > + } else if (argc == 1) { > + strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL); > + branch_name = buf.buf; > + } > else > die(_("cannot edit description of more than one branch")); Here branch_name could be pointing at buf.buf (or head). > strbuf_addf(&branch_ref, "refs/heads/%s", branch_name); > if (!ref_exists(branch_ref.buf)) { > strbuf_release(&branch_ref); > + strbuf_release(&buf); But the contents of the buf becomes invalid at this point. > if (!argc) > return error(_("No commit on branch '%s' yet."), In the post context of this hunk, we see that the '%s' above is filled with branch_name, accessing the potentially invalid contents. I'll put a squashable band-aid on top of the topic for now. --- >8 --- * cmd_foo() should not return an negative value. * branch_name used in the calls to error() could point at buf.buf that holds the expansion of @{-1}, but buf was released way too early, leading to a use-after-free. * Style: if/else if/else cascade whose one arm has multiple statements and requires {braces} around it should have {braces} around all of its arms. * each arm in the top-level if/else if/else cascade for "git branch" subcommands were more or less independent, and there wasn't anything common that they need to execute after exiting the cascade. Unconditionally returning from the arm for the edit-description subcommand seems to make the logic flow easier to read. builtin/branch.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 197603241d..17853225fa 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -792,6 +792,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) const char *branch_name; struct strbuf branch_ref = STRBUF_INIT; struct strbuf buf = STRBUF_INIT; + int ret = 1; /* assume failure */ if (!argc) { if (filter.detached) @@ -800,29 +801,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } else if (argc == 1) { strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL); branch_name = buf.buf; - } - else + } else { die(_("cannot edit description of more than one branch")); + } strbuf_addf(&branch_ref, "refs/heads/%s", branch_name); - if (!ref_exists(branch_ref.buf)) { - strbuf_release(&branch_ref); - strbuf_release(&buf); - - if (!argc) - return error(_("No commit on branch '%s' yet."), - branch_name); - else - return error(_("No branch named '%s'."), - branch_name); - } - strbuf_release(&branch_ref); + if (!ref_exists(branch_ref.buf)) + error(!argc + ? _("No commit on branch '%s' yet.") + : _("No branch named '%s'."), + branch_name); + else if (!edit_branch_description(branch_name)) + ret = 0; /* happy */ - if (edit_branch_description(branch_name)) { - strbuf_release(&buf); - return 1; - } + strbuf_release(&branch_ref); strbuf_release(&buf); + + return ret; } else if (copy) { if (!argc) die(_("branch name required"));
On 9/10/22 21:11, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > * cmd_foo() should not return an negative value. Yes, the refactoring we already discussed early in this thread. > > * branch_name used in the calls to error() could point at buf.buf > that holds the expansion of @{-1}, but buf was released way too > early, leading to a use-after-free. :-( good catch, thanks. Removing the refactoring commit was not carefully done. > > * Style: if/else if/else cascade whose one arm has multiple > statements and requires {braces} around it should have {braces} > around all of its arms. Ok. > > * each arm in the top-level if/else if/else cascade for "git > branch" subcommands were more or less independent, and there > wasn't anything common that they need to execute after exiting > the cascade. Unconditionally returning from the arm for the > edit-description subcommand seems to make the logic flow easier > to read. Mmm, I don't feel the same here, we already discussed about this. Maybe?: diff --git a/builtin/branch.c b/builtin/branch.c index 17853225fa..307073cc47 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_release(&branch_ref); strbuf_release(&buf); - return ret; + if (ret) + return ret; /* some failure happened */ } else if (copy) { if (!argc) die(_("branch name required")); not much important, though. You can squash the changes in the commit or if you need me to send a v6, please let me know. Thank you for your careful review.
Rubén Justo <rjusto@gmail.com> writes: > Mmm, I don't feel the same here, we already discussed about this. Maybe?: > > diff --git a/builtin/branch.c b/builtin/branch.c > index 17853225fa..307073cc47 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > strbuf_release(&branch_ref); > strbuf_release(&buf); > > - return ret; > + if (ret) > + return ret; /* some failure happened */ > } else if (copy) { > if (!argc) > die(_("branch name required")); Before the above change, the body of the "else if" clause for the option was self contained. With the above change, the reader has to follow to the end of the long top-level cascade to see the rest of the function does not do anything funny. If we have a big common clean-up after each operation, then, falling through in the success case might be good, but that is not what I am seeing here. So...
On 10/10/22 2:38, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > >> Mmm, I don't feel the same here, we already discussed about this. Maybe?: >> >> diff --git a/builtin/branch.c b/builtin/branch.c >> index 17853225fa..307073cc47 100644 >> --- a/builtin/branch.c >> +++ b/builtin/branch.c >> @@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) >> strbuf_release(&branch_ref); >> strbuf_release(&buf); >> >> - return ret; >> + if (ret) >> + return ret; /* some failure happened */ >> } else if (copy) { >> if (!argc) >> die(_("branch name required")); > > Before the above change, the body of the "else if" clause for the > option was self contained. With the above change, the reader has to > follow to the end of the long top-level cascade to see the rest of > the function does not do anything funny. > > If we have a big common clean-up after each operation, then, falling > through in the success case might be good, but that is not what I am > seeing here. So... > I would like to see some kind of free(head) in a clean-up to not get distracted with that. Not a proper leak though and the leak checkers does not refer to that as leak. So not important. We can go with the unconditional return and let the dust settle.
Rubén Justo <rjusto@gmail.com> writes: >> If we have a big common clean-up after each operation, then, falling >> through in the success case might be good, but that is not what I am >> seeing here. So... >> > > I would like to see some kind of free(head) in a clean-up to not get > distracted with that. Not a proper leak though and the leak checkers > does not refer to that as leak. So not important. We can go with the > unconditional return and let the dust settle. "head" is not leaking, as a pointer to it is head in a location that is still in scope (namely, a file-scope global variable) when the program exits. In fact, the only thing the code before or after this patch does after leaving this top-level if/elseif/else cascade is to return 0 and doing nothing else. Inserting free(head) would be an unneeded distraction for human developers (doing such a patch, reviewing, and even worse, having to read the resulting code in the coming years) and waste of computer resources (exiting the process will reclaim such a piece of memory just fine).
On 10/10/22 18:55, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > >>> If we have a big common clean-up after each operation, then, falling >>> through in the success case might be good, but that is not what I am >>> seeing here. So... >>> >> >> I would like to see some kind of free(head) in a clean-up to not get >> distracted with that. Not a proper leak though and the leak checkers >> does not refer to that as leak. So not important. We can go with the >> unconditional return and let the dust settle. > > "head" is not leaking, as a pointer to it is head in a location that > is still in scope (namely, a file-scope global variable) when the > program exits. > > In fact, the only thing the code before or after this patch does > after leaving this top-level if/elseif/else cascade is to return 0 > and doing nothing else. Inserting free(head) would be an unneeded > distraction for human developers (doing such a patch, reviewing, and > even worse, having to read the resulting code in the coming years) > and waste of computer resources (exiting the process will reclaim > such a piece of memory just fine). > Just to say that I truly agree. Un saludo.
diff --git a/builtin/branch.c b/builtin/branch.c index 55cd9a6e99..197603241d 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } else if (edit_description) { const char *branch_name; struct strbuf branch_ref = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; if (!argc) { if (filter.detached) die(_("Cannot give description to detached HEAD")); branch_name = head; - } else if (argc == 1) - branch_name = argv[0]; + } else if (argc == 1) { + strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL); + branch_name = buf.buf; + } else die(_("cannot edit description of more than one branch")); strbuf_addf(&branch_ref, "refs/heads/%s", branch_name); if (!ref_exists(branch_ref.buf)) { strbuf_release(&branch_ref); + strbuf_release(&buf); if (!argc) return error(_("No commit on branch '%s' yet."), @@ -814,8 +818,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } strbuf_release(&branch_ref); - if (edit_branch_description(branch_name)) + if (edit_branch_description(branch_name)) { + strbuf_release(&buf); return 1; + } + strbuf_release(&buf); } else if (copy) { if (!argc) die(_("branch name required")); @@ -835,9 +842,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix) else die(_("too many arguments for a rename operation")); } else if (new_upstream) { - struct branch *branch = branch_get(argv[0]); + struct branch *branch; + struct strbuf buf = STRBUF_INIT; - if (argc > 1) + if (!argc) + branch = branch_get(NULL); + else if (argc == 1) { + strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL); + branch = branch_get(buf.buf); + } else die(_("too many arguments to set new upstream")); if (!branch) { @@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix) dwim_and_setup_tracking(the_repository, branch->name, new_upstream, BRANCH_TRACK_OVERRIDE, quiet); + strbuf_release(&buf); } else if (unset_upstream) { - struct branch *branch = branch_get(argv[0]); + struct branch *branch; struct strbuf buf = STRBUF_INIT; - if (argc > 1) + if (!argc) + branch = branch_get(NULL); + else if (argc == 1) { + strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL); + branch = branch_get(buf.buf); + } else die(_("too many arguments to unset upstream")); if (!branch) { @@ -871,6 +890,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (!branch_has_merge_config(branch)) die(_("Branch '%s' has no upstream information"), branch->name); + strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.remote", branch->name); git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_reset(&buf); diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index 993a6b5eff..793bf4d269 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' ' expect_branch HEAD one ' +test_expect_success 'edit-description via @{-1}' ' + git checkout -b desc-branch && + git checkout -b non-desc-branch && + write_script editor <<-\EOF && + echo "Branch description" >"$1" + EOF + EDITOR=./editor git branch --edit-description @{-1} && + test_must_fail git config branch.non-desc-branch.description && + git config branch.desc-branch.description >actual && + printf "Branch description\n\n" >expect && + test_cmp expect actual +' + +test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' ' + git checkout -b upstream-branch && + git checkout -b upstream-other -t upstream-branch && + git branch --set-upstream-to upstream-other @{-1} && + git config branch.upstream-branch.merge >actual && + echo "refs/heads/upstream-other" >expect && + test_cmp expect actual && + git branch --unset-upstream @{-1}@{upstream} && + test_must_fail git config branch.upstream-other.merge +' + test_done
branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments. We can modify the description of the previously checked out branch with: $ git branch --edit--description @{-1} We can modify the upstream of the previously checked out branch with: $ git branch --set-upstream-to upstream @{-1} $ git branch --unset-upstream @{-1} Signed-off-by: Rubén Justo <rjusto@gmail.com> --- Changes from v4: - Fix non POSIX compatible usage of echo. Junio, I have taken two approaches to reduce the repetitive code, merging the two cases "new_upstream" and "unset_upstream". First keeping the different error messages to each case, which leads to multiple ugly if/elses. Second approach, merging the error messages with generics, which resolves the if/elses but leads to changes in t/t3200-branch.sh (at least). I haven't finished the tests yet, but I already have the same feeling as with the other patch for branch. I think this is a good stop point for this change if no new problems are detected. Un saludo. Range-diff: 1: 72a17039e0 ! 1: 393250bbac branch: support for shortcuts like @{-1}, completed @@ t/t3204-branch-name-interpretation.sh: test_expect_success 'checkout does not tr + EDITOR=./editor git branch --edit-description @{-1} && + test_must_fail git config branch.non-desc-branch.description && + git config branch.desc-branch.description >actual && -+ echo "Branch description\n" >expect && ++ printf "Branch description\n\n" >expect && + test_cmp expect actual +' + builtin/branch.c | 34 +++++++++++++++++++++------ t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)