| Message ID | 2e164aea-7dd8-5018-474a-01643553ea49@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] branch: support for shortcuts like @{-1}, completed | expand |
On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote: > 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. > [...] > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > diff --git 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' ' > +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 && > + echo "Branch description\n" >expect && Is the intention here with the embedded "\n" that `echo` should emit two newlines? If so, interpreting "\n" specially is not POSIX behavior for `echo`, thus we probably don't want to rely upon it. > + test_cmp expect actual
Rubén Justo <rjusto@gmail.com> writes: > } 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")); The above two are a bit repetitious. A helper like static struct branch *interpret_local(int ac, const char **av) { struct branch *branch; if (!ac) { branch = branch_get(NULL); } else if (ac == 1) { struct strbuf buf = STRBUF_INIT; strbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL); branch = branch_get(buf.buf); strbuf_release(&buf); } else { die(_("too many arguments")); } return branch; } might be useful, and it frees the callers from worrying about temporary allocations. But the code written is OK as-is.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote: >> 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. >> [...] >> Signed-off-by: Rubén Justo <rjusto@gmail.com> >> --- >> diff --git 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' ' >> +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 && >> + echo "Branch description\n" >expect && > > Is the intention here with the embedded "\n" that `echo` should emit > two newlines? If so, interpreting "\n" specially is not POSIX behavior > for `echo`, thus we probably don't want to rely upon it. Good eyes. You are correct that "echo" and "\n" do not play well together. Thanks.
On 8/10/22 5:17, Eric Sunshine wrote: > On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote: >> 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. >> [...] >> Signed-off-by: Rubén Justo <rjusto@gmail.com> >> --- >> diff --git 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' ' >> +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 && >> + echo "Branch description\n" >expect && > > Is the intention here with the embedded "\n" that `echo` should emit > two newlines? If so, interpreting "\n" specially is not POSIX behavior > for `echo`, thus we probably don't want to rely upon it. > Oops. Thank you! I'll reroll back to using "git stripspace". >> + test_cmp expect actual
On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote: > On 8/10/22 5:17, Eric Sunshine wrote: > > On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote: > >> + echo "Branch description\n" >expect && > > > > Is the intention here with the embedded "\n" that `echo` should emit > > two newlines? If so, interpreting "\n" specially is not POSIX behavior > > for `echo`, thus we probably don't want to rely upon it. > > Oops. Thank you! I'll reroll back to using "git stripspace". `git stripspace` is perhaps unnecessarily heavyweight. Lightweight alternatives would include: printf "Branch description\n\n" >expect && test_write_lines "Branch description" "" >expect && { echo "Branch description" && echo; } >expect && cat >expect <<-\EOF && Branch description EOF
On 8/10/22 6:17, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > >> } 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")); > > The above two are a bit repetitious. A helper like > > static struct branch *interpret_local(int ac, const char **av) > { > struct branch *branch; > if (!ac) { > branch = branch_get(NULL); > } else if (ac == 1) { > struct strbuf buf = STRBUF_INIT; > strbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL); > branch = branch_get(buf.buf); > strbuf_release(&buf); > } else { > die(_("too many arguments")); > } > return branch; > } > > might be useful, and it frees the callers from worrying about > temporary allocations. Yes, it leaves a repetitive taste. I thought about joining the two cases, but the taste with multiple if/else was worse. Following what you suggest with the "too many arguments" error, I'll try to reduce that repetitive taste. Thanks > > But the code written is OK as-is. >
On 8/10/22 9:23, Eric Sunshine wrote: > On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote: >> On 8/10/22 5:17, Eric Sunshine wrote: >>> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote: >>>> + echo "Branch description\n" >expect && >>> >>> Is the intention here with the embedded "\n" that `echo` should emit >>> two newlines? If so, interpreting "\n" specially is not POSIX behavior >>> for `echo`, thus we probably don't want to rely upon it. >> >> Oops. Thank you! I'll reroll back to using "git stripspace". > > `git stripspace` is perhaps unnecessarily heavyweight. Lightweight > alternatives would include: > > printf "Branch description\n\n" >expect && > > test_write_lines "Branch description" "" >expect && > > { echo "Branch description" && echo; } >expect && > > cat >expect <<-\EOF && > Branch description > > EOF > Yeah, I thought about that. What convinced me to use "git stripspace" was that maybe that '\n' tail could be removed sometime from the description setting and this will be fine with that. I haven't found any reason for that '\n' and it bugs me a little seeing it in the config :-) But I agree with you about the unnecessarily heavyweight, though all involves a new process, probably echo, cat or printf are lightweight than another instance of git for that. All of this involves two files and that is how it is done almost everywhere except in some places where it looks like an 'older way' (test_i18ngrep) of doing it. Is there any reason to do it this way and not using variables, process substitution,..? Anyway I'll switch to one of your suggestions, as it is definitely easier to read, understand and therefore change if needed. Thanks!
On Sat, Oct 8, 2022 at 5:12 AM Rubén Justo <rjusto@gmail.com> wrote: > On 8/10/22 9:23, Eric Sunshine wrote: > > On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote: > >> Oops. Thank you! I'll reroll back to using "git stripspace". > > > > `git stripspace` is perhaps unnecessarily heavyweight. Lightweight > > alternatives would include: > > > > printf "Branch description\n\n" >expect && > > [...] > > Yeah, I thought about that. What convinced me to use "git stripspace" was > that maybe that '\n' tail could be removed sometime from the description > setting and this will be fine with that. I haven't found any reason for > that '\n' and it bugs me a little seeing it in the config :-) That reasoning occurred to me, as well, and I'd have no objection to git-stripspace if that's the motivation for using it. I don't feel strongly one way or the other, and my previous email was intended primarily to point out the lightweight alternatives in case you hadn't considered them. Feel free to use git-stripspace if you feel it is the more appropriate choice. > But I agree with you about the unnecessarily heavyweight, though all > involves a new process, probably echo, cat or printf are lightweight than > another instance of git for that. In most shells, `printf`, `echo`, `cat` are builtins, so no extra processes are involved (and `test_write_lines` is a shell function built atop `printf`). As a matter of personal preference, given the lightweight options, I find that `printf "...\n\n"` shows the intention of the code most plainly (but if you go with git-stripspace, then `echo` would be an idiomatic way to create the "expect" file). > All of this involves two files and that is how it is done almost everywhere > except in some places where it looks like an 'older way' (test_i18ngrep) of > doing it. Is there any reason to do it this way and not using variables, > process substitution,..? An invocation such as: test $(git foo) = $(git bar) && throws away the exit-code from the two commands, which means we'd miss if one or the other (or both) crashed, especially if the crash was after the command produced the correct output. These days we try to avoid losing the exit command of Git commands. It's possible to avoid losing the exit-code by using variables: expect=$(git foo) && actual=$(git bar) && test "$expect" = "$actual" && but, if the expected and actual output don't match, you don't learn much (other than that they failed). You could address that by showing a message saying what failed: expect=... && actual=... && if test "$expect" != "$actual" then echo "expect not match actual" # maybe emit $expect and $actual too fi However, `test_cmp` gives you that behavior for free, and it emits a helpful "diff" upon failure, so these days we usually go with `test_cmp`. > Anyway I'll switch to one of your suggestions, as it is definitely easier > to read, understand and therefore change if needed. It's fine to use git-stripspace if you feel it's more appropriate for the situation.
Eric Sunshine <sunshine@sunshineco.com> writes: >> Yeah, I thought about that. What convinced me to use "git stripspace" was >> that maybe that '\n' tail could be removed sometime from the description >> setting and this will be fine with that. I haven't found any reason for >> that '\n' and it bugs me a little seeing it in the config :-) > > That reasoning occurred to me, as well, and I'd have no objection to > git-stripspace if that's the motivation for using it. I don't feel > strongly one way or the other, and my previous email was intended > primarily to point out the lightweight alternatives in case you hadn't > considered them. Feel free to use git-stripspace if you feel it is the > more appropriate choice. I do not think I would agree with the line of reasoning. It all depends on why we anticipate that the terminating LF may go away someday, but if it is because we may do so by mistake and without a good reason when making some unrelated changes to the implementation of "git branch --edit-desc", we would want to know about it, and such a loose check that would miss it is definitely unwelcome. It is very likely that not just "git merge" but people's external scripts depend on the presence of final LF especially when the description has only one line, so unless we are doing deliberately so, we should prepare to catch such a change. If it is because we may gain a consensus that the description string (which by the way can well consist of multiple lines) is better without the LF on its final line, and we are "fixing" the code to do so very much on purpose, it would be good to have a test to protect such a change from future unintended breakages. Adding a loose test that won't break across such a change today may be OK, but whoever is making such a change in the future has to make sure there is a test that is not loose to protect the change. And it would very likely to be done by adding a new test, instead of noticing that this loosely written test can be tightened to serve the purpose. So if we start with a tight test that expects the exact number of LFs at the end, we would be better off in that case, too.
On 8/10/22 19:46, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> Yeah, I thought about that. What convinced me to use "git stripspace" was >>> that maybe that '\n' tail could be removed sometime from the description >>> setting and this will be fine with that. I haven't found any reason for >>> that '\n' and it bugs me a little seeing it in the config :-) >> >> That reasoning occurred to me, as well, and I'd have no objection to >> git-stripspace if that's the motivation for using it. I don't feel >> strongly one way or the other, and my previous email was intended >> primarily to point out the lightweight alternatives in case you hadn't >> considered them. Feel free to use git-stripspace if you feel it is the >> more appropriate choice. > > I do not think I would agree with the line of reasoning. > > It all depends on why we anticipate that the terminating LF may go > away someday, but if it is because we may do so by mistake and > without a good reason when making some unrelated changes to the > implementation of "git branch --edit-desc", we would want to know > about it, and such a loose check that would miss it is definitely > unwelcome. It is very likely that not just "git merge" but people's > external scripts depend on the presence of final LF especially when > the description has only one line, so unless we are doing > deliberately so, we should prepare to catch such a change. > > If it is because we may gain a consensus that the description string > (which by the way can well consist of multiple lines) is better > without the LF on its final line, and we are "fixing" the code to do > so very much on purpose, it would be good to have a test to protect > such a change from future unintended breakages. Adding a loose test > that won't break across such a change today may be OK, but whoever > is making such a change in the future has to make sure there is a > test that is not loose to protect the change. And it would very > likely to be done by adding a new test, instead of noticing that > this loosely written test can be tightened to serve the purpose. > > So if we start with a tight test that expects the exact number of > LFs at the end, we would be better off in that case, too. > Fair point. Thank you for being cautious.
On 8/10/22 19:10, Eric Sunshine wrote: >> All of this involves two files and that is how it is done almost everywhere >> except in some places where it looks like an 'older way' (test_i18ngrep) of >> doing it. Is there any reason to do it this way and not using variables, >> process substitution,..? > > An invocation such as: > > test $(git foo) = $(git bar) && > > throws away the exit-code from the two commands, which means we'd miss > if one or the other (or both) crashed, especially if the crash was > after the command produced the correct output. These days we try to > avoid losing the exit command of Git commands. It's possible to avoid > losing the exit-code by using variables: > > expect=$(git foo) && > actual=$(git bar) && > test "$expect" = "$actual" && > > but, if the expected and actual output don't match, you don't learn > much (other than that they failed). You could address that by showing > a message saying what failed: > > expect=... && > actual=... && > if test "$expect" != "$actual" > then > echo "expect not match actual" > # maybe emit $expect and $actual too > fi > > However, `test_cmp` gives you that behavior for free, and it emits a > helpful "diff" upon failure, so these days we usually go with > `test_cmp`. > This is already out of the subject, the patch, of this thread, so sorry, but the context is worth of it. I understand the possibility of losing exit codes or segfaults in subcommands or pipes, but I'm more thinking in the element to compare to. Having something like: test_cmp_str () { test -f "$1" || BUG "first argument must be a file" if test "$#" -gt 1 then local F=$1 shift printf "$@" | eval "$GIT_TEST_CMP" '"$F"' - else eval "$GIT_TEST_CMP" '"$1"' - fi } to allow writing simpler tests like: --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -142,8 +142,7 @@ test_expect_success 'edit-description via @{-1}' ' 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_cmp_str actual "Branch description\n\n" ' --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -142,8 +142,10 @@ test_expect_success 'edit-description via @{-1}' ' 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_cmp_str actual <<-\EOF + Branch description + + EOF ' My doubts are that maybe this can induce to some bad usages, is unusable in some systems, it has already been explored and discarded, using files gives the diff with nice names not "-",... Maybe this needs a new RFC thread. I dunno. Thanks for your comments and explanations.
On Sat, Oct 8, 2022 at 7:28 PM Rubén Justo <rjusto@gmail.com> wrote: > On 8/10/22 19:10, Eric Sunshine wrote: > > However, `test_cmp` gives you that behavior for free, and it emits a > > helpful "diff" upon failure, so these days we usually go with > > `test_cmp`. > > I understand the possibility of losing exit codes or segfaults in > subcommands or pipes, but I'm more thinking in the element to compare to. > Having something like: > > test_cmp_str () { > test -f "$1" || BUG "first argument must be a file" > if test "$#" -gt 1 > then > local F=$1 > shift > printf "$@" | eval "$GIT_TEST_CMP" '"$F"' - > else > eval "$GIT_TEST_CMP" '"$1"' - > fi > } > > to allow writing simpler tests like: > > + test_cmp_str actual "Branch description\n\n" > > + test_cmp_str actual <<-\EOF > + Branch description > + > + EOF > > My doubts are that maybe this can induce to some bad usages, is > unusable in some systems, it has already been explored and discarded, > using files gives the diff with nice names not "-",... I wouldn't be at all surprised if this sort of thing has been discussed already (it sounds familiar enough), or that such an improvement has already been submitted in the form of a patch (possibly by Ævar, Cc:'d). My knee-jerk reaction is that the form which takes a string as argument would hardly be used, thus an unnecessary complication. The form which accepts stdin could even be retrofitted onto the existing test_cmp, thus you don't even need to invent a new function name. A different approach would be to introduce a function `test_stdout` which both accepts a command to run, as well as the "expected" text on stdin with which to compare the output of the command; i.e. a combination of the existing `test_stdout_line_count` in t/test-lib-functions.sh and your proposed function above. That said, I'm not yet seeing all that much added value in such a function; at most, it seems to save only a single line of code, and it's not as if the code it's replacing was doing anything complicated or hard to grok. So, I'm pretty ambivalent about it. But, of course, I'm only one person expressing a knee-jerk reaction. Submitting the idea as an RFC patch might generate more feedback.
Eric Sunshine <sunshine@sunshineco.com> writes: > That said, I'm not yet seeing all that much added value in such a > function; at most, it seems to save only a single line of code, and > it's not as if the code it's replacing was doing anything complicated > or hard to grok. I share the sentiment. Leaving the result that was used in comparison in file(s) also helps debugging.
For me, the most value here is in the expressiveness, writing and reading
a simple test.
git config branch.desc-branch.description >actual &&
test_cmp actual "Branch description\n\n"
vs
git config branch.desc-branch.description >actual &&
printf "Branch description\n\n" >expect &&
test_cmp expect actual
vs
printf "Branch description\n\n" >expect &&
...
git config branch.desc-branch.description >actual &&
test_cmp expect actual
vs (oops)
printf "Branch description\n\n" >expect &&
...
git config branch.desc-branch.description >actual &&
printf "New branch description\n\n" >expect_ &&
test_cmp expect actual
Less lines, less files and less error prone are a plus.
On 9/10/22 8:46, Eric Sunshine wrote:
> My knee-jerk reaction is that the form which takes a string as
> argument would hardly be used, thus an unnecessary complication. The
A quick overview over a not much elaborated search:
git grep -B 3 'test_cmp expect' | grep '\(echo\|cat\).*expect'
gives many results that imo should be beneficial of this expressiveness.
> form which accepts stdin could even be retrofitted onto the existing
> test_cmp, thus you don't even need to invent a new function name. A
Nice. Maybe even the printf form with a "test ! -f", some risky though.
> different approach would be to introduce a function `test_stdout`
> which both accepts a command to run, as well as the "expected" text on
> stdin with which to compare the output of the command; i.e. a
> combination of the existing `test_stdout_line_count` in
> t/test-lib-functions.sh and your proposed function above.
Sounds good to me but maybe this goes in the direction of "inducing
bad usages", and the tests to cover starts to be not so simple...
On 9/10/22 21:33, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> That said, I'm not yet seeing all that much added value in such a
>> function; at most, it seems to save only a single line of code, and
>> it's not as if the code it's replacing was doing anything complicated
>> or hard to grok.
>
> I share the sentiment. Leaving the result that was used in comparison
> in file(s) also helps debugging.
>
And if this is fulfilled with a drop a file on failure? That resolves the
'-' in the diff output result too.
Thank you.
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..71afceac71 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 && + echo "Branch description\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> --- To simplify the patch and make it easier to review, I removed all refactoring related to return codes and errors. Leaving that and everything else discussed in the thread, to next series. This patch now only has one commit with the missing @{-N} handling completed and the tests for it. Some refactoring has been done in the test as well. Un saludo. builtin/branch.c | 34 +++++++++++++++++++++------ t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)