From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: Git List <git@vger.kernel.org>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op
Date: Fri, 18 Nov 2022 04:58:54 +0100 [thread overview]
Message-ID: <221118.868rk8hpk4.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <db182322-1383-4311-8baa-c4a9aeed3b4d@gmail.com>
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.
next prev parent reply other threads:[~2022-11-18 4:09 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-17 1:33 [PATCH 0/2] branch: fix some malfunctions in -m/-c Rubén Justo
2022-11-17 1:36 ` [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op Rubén Justo
2022-11-17 22:18 ` Victoria Dye
2022-11-20 8:10 ` Rubén Justo
2022-11-18 3:58 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-20 9:34 ` Rubén Justo
2022-11-17 1:44 ` [PATCH 2/2] branch: clear target branch configuration before copying or renaming Rubén Justo
2022-11-18 2:10 ` Victoria Dye
2022-11-20 9:20 ` Rubén Justo
2022-11-20 22:10 ` Victoria Dye
2022-11-21 23:13 ` Rubén Justo
2022-11-18 4:51 ` Ævar Arnfjörð Bjarmason
2022-11-18 16:36 ` Victoria Dye
2022-11-18 18:12 ` Ævar Arnfjörð Bjarmason
2022-11-20 14:55 ` Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221118.868rk8hpk4.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).