git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 18, 2022, 03:58 UTC
Message-ID
<221118.868rk8hpk4.gmgdl@evledraar.gmail.com>
In-Reply-To
<db182322-1383-4311-8baa-c4a9aeed3b4d@gmail.com>
On Thu, Nov 17 2022, Rubén Justo wrote:
Show 31 quoted lines
> 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.

Show 11 quoted lines
> @@ -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.

Show 5 quoted lines
>  		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.

Previous: Rubén JustoNext: Rubén Justo
Message 5 of 15 in “branch: fix some malfunctions in -m/-c”
  1. 0/2 branch: fix some malfunctions in -m/-cRubén Justo, Nov 17, 2022
  2. 1/2 branch: force-copy a branch to itself via @{-1} is a no-opRubén Justo, Nov 17, 2022
  3. Victoria DyeNov 17, 2022
  4. Rubén JustoNov 20, 2022
  5. Ævar Arnfjörð BjarmasonNov 18, 2022
  6. Rubén JustoNov 20, 2022
  7. 2/2 branch: clear target branch configuration before copying or renamingRubén Justo, Nov 17, 2022
  8. Victoria DyeNov 18, 2022
  9. Rubén JustoNov 20, 2022
  10. Victoria DyeNov 20, 2022
  11. Rubén JustoNov 21, 2022
  12. Ævar Arnfjörð BjarmasonNov 18, 2022
  13. Victoria DyeNov 18, 2022
  14. Ævar Arnfjörð BjarmasonNov 18, 2022
  15. Rubén JustoNov 20, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.