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.
