| Message ID | db182322-1383-4311-8baa-c4a9aeed3b4d@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cfbd173ccb4dbf9cbaae0640b17d96d7b2ee5a19 |
| Headers | show |
| Series | branch: fix some malfunctions in -m/-c | expand |
Rubén Justo wrote: > Since 52d59cc645 (branch: add a --copy (-c) option to go with --move > (-m), 2017-06-18) we can copy a branch to make a new branch with the > '-c' (copy) option or to overwrite an existing branch using the '-C' > (force copy) option. A no-op possibility is considered when we are > asked to copy a branch to itself, to follow the same no-op introduced > for the rename (-M) operation in 3f59481e33 (branch: allow a no-op > "branch -M <current-branch> HEAD", 2011-11-25). To check for this, in > 52d59cc645 we compared the branch names provided by the user, source > (HEAD if omitted) and destination, and a match is considered as this > no-op. > > Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th > last branch, 2009-01-17) a branch can be specified using shortcuts like > @{-1}. This allows this usage: > > $ git checkout -b test > $ git checkout - > $ git branch -C test test # no-op > $ git branch -C test @{-1} # oops > $ git branch -C @{-1} test # oops > > As we are using the branch name provided by the user to do the > comparison, if one of the branches is provided using a shortcut we are > not going to have a match and a call to git_config_copy_section() will > happen. This will make a duplicate of the configuration for that > branch, and with this progression the second call will produce four > copies of the configuration, and so on. This is a clear explanation of what the issue is and why it's happening. > > Let's use the interpreted branch name instead for this comparison. > > The rename operation is not affected. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > builtin/branch.c | 6 +++--- > t/t3204-branch-name-interpretation.sh | 10 ++++++++++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 15be0c03ef..a35e174aae 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int > strbuf_release(&logmsg); > > strbuf_addf(&oldsection, "branch.%s", interpreted_oldname); > - strbuf_release(&oldref); > strbuf_addf(&newsection, "branch.%s", interpreted_newname); > - strbuf_release(&newref); > if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0) > die(_("Branch is renamed, but update of config-file failed")); > - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > + if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) I double-checked that 'interpreted_oldname' and 'interpreted_newname' are always set (and not only when a shortcut name is used), and they are. So, this does exactly what you intend. > die(_("Branch is copied, but update of config-file failed")); > + strbuf_release(&oldref); > + strbuf_release(&newref); > strbuf_release(&oldsection); > strbuf_release(&newsection); > } > diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh > index 793bf4d269..3399344f25 100755 > --- a/t/t3204-branch-name-interpretation.sh > +++ b/t/t3204-branch-name-interpretation.sh > @@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' ' > expect_branch refs/heads/refs/heads/qualified two > ' > > +test_expect_success 'force-copy a branch to itself via @{-1} is no-op' ' > + git branch -t copiable main && > + git checkout copiable && > + git checkout - && > + git branch -C @{-1} copiable && > + git config --get-all branch.copiable.merge >actual && > + echo refs/heads/main >expect && > + test_cmp expect actual > +' > + And the test is straightforward and demonstrates the fix. Thanks for the well-written patch, this looks good to me!
On Thu, Nov 17 2022, Rubén Justo wrote: > Since 52d59cc645 (branch: add a --copy (-c) option to go with --move > (-m), 2017-06-18) we can copy a branch to make a new branch with the > '-c' (copy) option or to overwrite an existing branch using the '-C' > (force copy) option. A no-op possibility is considered when we are > asked to copy a branch to itself, to follow the same no-op introduced > for the rename (-M) operation in 3f59481e33 (branch: allow a no-op > "branch -M <current-branch> HEAD", 2011-11-25). To check for this, in > 52d59cc645 we compared the branch names provided by the user, source > (HEAD if omitted) and destination, and a match is considered as this > no-op. > > Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th > last branch, 2009-01-17) a branch can be specified using shortcuts like > @{-1}. This allows this usage: > > $ git checkout -b test > $ git checkout - > $ git branch -C test test # no-op > $ git branch -C test @{-1} # oops > $ git branch -C @{-1} test # oops > > As we are using the branch name provided by the user to do the > comparison, if one of the branches is provided using a shortcut we are > not going to have a match and a call to git_config_copy_section() will > happen. This will make a duplicate of the configuration for that > branch, and with this progression the second call will produce four > copies of the configuration, and so on. > > Let's use the interpreted branch name instead for this comparison. > > The rename operation is not affected. Good catch! Yes this definitely wasn't intended, and is just a failure of the config name v.s. ref names drifting from what the previous logic was assuming. > @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int > strbuf_release(&logmsg); > > strbuf_addf(&oldsection, "branch.%s", interpreted_oldname); > - strbuf_release(&oldref); > strbuf_addf(&newsection, "branch.%s", interpreted_newname); > - strbuf_release(&newref); > if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0) > die(_("Branch is renamed, but update of config-file failed")); > - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > + if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) We try to stay under 79 chars, see CodingGuidelines. The pre-image was already violating this, but the new one is really long. I think it would be good to just wrap this after the last && while at it. > die(_("Branch is copied, but update of config-file failed")); > + strbuf_release(&oldref); > + strbuf_release(&newref); > strbuf_release(&oldsection); > strbuf_release(&newsection); This moving around of destructors isn't needed, and is just some unrelated cleanup. Your change here only needs to be: - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) + if (copy && strcmp(interpreted_oldname, interpreted_newname) && + git_config_copy_section(oldsection.buf, newsection.buf) < 0) If you'd like to re-arrange some of this and e.g. free stuff at the end instead of after it's last used (which is what the current code is aiming for) that's arguably good, but let's do that in another commit then.
On 17/11/22 23:18, Victoria Dye wrote: > Rubén Justo wrote: >> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move >> (-m), 2017-06-18) we can copy a branch to make a new branch with the >> '-c' (copy) option or to overwrite an existing branch using the '-C' >> (force copy) option. A no-op possibility is considered when we are >> asked to copy a branch to itself, to follow the same no-op introduced >> for the rename (-M) operation in 3f59481e33 (branch: allow a no-op >> "branch -M <current-branch> HEAD", 2011-11-25). To check for this, in >> 52d59cc645 we compared the branch names provided by the user, source >> (HEAD if omitted) and destination, and a match is considered as this >> no-op. >> >> Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th >> last branch, 2009-01-17) a branch can be specified using shortcuts like >> @{-1}. This allows this usage: >> >> $ git checkout -b test >> $ git checkout - >> $ git branch -C test test # no-op >> $ git branch -C test @{-1} # oops >> $ git branch -C @{-1} test # oops >> >> As we are using the branch name provided by the user to do the >> comparison, if one of the branches is provided using a shortcut we are >> not going to have a match and a call to git_config_copy_section() will >> happen. This will make a duplicate of the configuration for that >> branch, and with this progression the second call will produce four >> copies of the configuration, and so on. > > This is a clear explanation of what the issue is and why it's happening. > >> >> Let's use the interpreted branch name instead for this comparison. >> >> The rename operation is not affected. >> >> Signed-off-by: Rubén Justo <rjusto@gmail.com> >> --- >> builtin/branch.c | 6 +++--- >> t/t3204-branch-name-interpretation.sh | 10 ++++++++++ >> 2 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/builtin/branch.c b/builtin/branch.c >> index 15be0c03ef..a35e174aae 100644 >> --- a/builtin/branch.c >> +++ b/builtin/branch.c >> @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int >> strbuf_release(&logmsg); >> >> strbuf_addf(&oldsection, "branch.%s", interpreted_oldname); >> - strbuf_release(&oldref); >> strbuf_addf(&newsection, "branch.%s", interpreted_newname); >> - strbuf_release(&newref); >> if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0) >> die(_("Branch is renamed, but update of config-file failed")); >> - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) >> + if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > > I double-checked that 'interpreted_oldname' and 'interpreted_newname' are > always set (and not only when a shortcut name is used), and they are. So, > this does exactly what you intend. > >> die(_("Branch is copied, but update of config-file failed")); >> + strbuf_release(&oldref); >> + strbuf_release(&newref); >> strbuf_release(&oldsection); >> strbuf_release(&newsection); >> } >> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh >> index 793bf4d269..3399344f25 100755 >> --- a/t/t3204-branch-name-interpretation.sh >> +++ b/t/t3204-branch-name-interpretation.sh >> @@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' ' >> expect_branch refs/heads/refs/heads/qualified two >> ' >> >> +test_expect_success 'force-copy a branch to itself via @{-1} is no-op' ' >> + git branch -t copiable main && >> + git checkout copiable && >> + git checkout - && >> + git branch -C @{-1} copiable && >> + git config --get-all branch.copiable.merge >actual && >> + echo refs/heads/main >expect && >> + test_cmp expect actual >> +' >> + > > And the test is straightforward and demonstrates the fix. Thanks for the > well-written patch, this looks good to me! > Thank you for reviewing this.
On 18-nov-2022 04:58:54, Ævar Arnfjörð Bjarmason wrote: > > @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int > > strbuf_release(&logmsg); > > > > strbuf_addf(&oldsection, "branch.%s", interpreted_oldname); > > - strbuf_release(&oldref); > > strbuf_addf(&newsection, "branch.%s", interpreted_newname); > > - strbuf_release(&newref); > > if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0) > > die(_("Branch is renamed, but update of config-file failed")); > > - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > > + if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > > We try to stay under 79 chars, see CodingGuidelines. The pre-image was > already violating this, but the new one is really long. I think it would > be good to just wrap this after the last && while at it. Yeah, thought about that. I preferred to not doing it mainly because my plan is to move out that strcmp() from there, but also wrapping that line induces to wrapping the previous one too, and there are many lines in that file above 79... I already have a series[1] to follow the CodingGuideLines in branch.c, currently focused in error messages but, maybe this change makes more sense there. Dunno. > > > die(_("Branch is copied, but update of config-file failed")); > > + strbuf_release(&oldref); > > + strbuf_release(&newref); > > strbuf_release(&oldsection); > > strbuf_release(&newsection); > > This moving around of destructors isn't needed, and is just some > unrelated cleanup. Your change here only needs to be: > > - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) > + if (copy && strcmp(interpreted_oldname, interpreted_newname) && > + git_config_copy_section(oldsection.buf, newsection.buf) < 0) 'interpreted_oldname' is a pointer to the 'oldref' buffer, and it is used in the next comparison, so the release for 'oldref' needs to be done later (same for interpreted_newname and newref). Maybe you are thinking in another change... I also thought comparing '{old,new}section.buf, the section names in the configuration, but I preferred to use interpreted_{old,new}name. It looks more clear what we are doing and in future commits that section names might not be composed at that point yet.
diff --git a/builtin/branch.c b/builtin/branch.c index 15be0c03ef..a35e174aae 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int strbuf_release(&logmsg); strbuf_addf(&oldsection, "branch.%s", interpreted_oldname); - strbuf_release(&oldref); strbuf_addf(&newsection, "branch.%s", interpreted_newname); - strbuf_release(&newref); if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0) die(_("Branch is renamed, but update of config-file failed")); - if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) + if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0) die(_("Branch is copied, but update of config-file failed")); + strbuf_release(&oldref); + strbuf_release(&newref); strbuf_release(&oldsection); strbuf_release(&newsection); } diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index 793bf4d269..3399344f25 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' ' expect_branch refs/heads/refs/heads/qualified two ' +test_expect_success 'force-copy a branch to itself via @{-1} is no-op' ' + git branch -t copiable main && + git checkout copiable && + git checkout - && + git branch -C @{-1} copiable && + git config --get-all branch.copiable.merge >actual && + echo refs/heads/main >expect && + test_cmp expect actual +' + test_expect_success 'delete branch via @{-1}' ' git branch previous-del &&
Since 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m), 2017-06-18) we can copy a branch to make a new branch with the '-c' (copy) option or to overwrite an existing branch using the '-C' (force copy) option. A no-op possibility is considered when we are asked to copy a branch to itself, to follow the same no-op introduced for the rename (-M) operation in 3f59481e33 (branch: allow a no-op "branch -M <current-branch> HEAD", 2011-11-25). To check for this, in 52d59cc645 we compared the branch names provided by the user, source (HEAD if omitted) and destination, and a match is considered as this no-op. Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. This allows this usage: $ git checkout -b test $ git checkout - $ git branch -C test test # no-op $ git branch -C test @{-1} # oops $ git branch -C @{-1} test # oops As we are using the branch name provided by the user to do the comparison, if one of the branches is provided using a shortcut we are not going to have a match and a call to git_config_copy_section() will happen. This will make a duplicate of the configuration for that branch, and with this progression the second call will produce four copies of the configuration, and so on. Let's use the interpreted branch name instead for this comparison. The rename operation is not affected. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/branch.c | 6 +++--- t/t3204-branch-name-interpretation.sh | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-)