Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 18, 2022, 04:51 UTC
Message-ID: <221118.864juwhkcc.gmgdl@evledraar.gmail.com>
In-Reply-To: <762c1e8f-fd0c-3b4b-94a0-709d8c9431e4@gmail.com>

On Thu, Nov 17 2022, Rubén Justo wrote:

Show 13 quoted lines

> There are two problems with -m (rename) and -c (copy) branch operations.
>
>  1. If we force-rename or force-copy a branch to overwrite another
>  branch that already has configuration, the resultant branch ends up
>  with the source configuration (if any) mixed with the configuration for
>  the overwritten branch.
>
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch bar              # bar has not
> 	$ git branch -M bar foo       # force-rename bar to foo
> 	$ git config branch.foo.merge # must return clear
> 	refs/heads/upstream

I'm fuzzy on whether Sahil and I discussed these edge cases at the time, but my first reaction was surprise that you thought this was purely a bug, I'd have thought it was a feature.

I.e. yes there's bugs & edge cases here, but fundimentally doesn't it make sense to think about "branch -c" as being mostly equivalent to a hypothetical:

	git branch --just-the-ref-operations -c <old> <new>
	git config --rename-section branch.<old> branch.<new>

And not:

	git config --remove-section branch.<new>
	git branch --just-the-ref-operations -c <old> <new>
	git config --rename-section branch.<old> branch.<new>

From reading the initial thread I see the "delete first" seems to have
been a TODO item of Sahil's[1], but the "copy branch" initally (I
mentored Sahil on it) from a shell one-liner I still have in my
.gitconfig history, which was a mostly-rename-section.

Show 12 quoted lines

>  2. If we repeatedly force-copy a branch to the same name, the branch
>  configuration is repeatedly copied each time.
>
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch -c foo bar       # bar is a copy of foo
> 	$ git branch -C foo bar       # again
> 	$ git branch -C foo bar       # ..
> 	$ git config --get-all branch.bar.merge # must return one value
> 	refs/heads/upstream
> 	refs/heads/upstream
> 	refs/heads/upstream

Yeah, you came about this conclusion because you're looking at the tracking config, of which there should be only one.

Our config space is multi-value in general, although most (all?) of our branch.* space is one-value.

But users can also stick things in there, so....

> Whenever we copy or move (forced or not) we must make sure that there is
> no residual configuration that will be, probably erroneously, inherited
> by the new branch.  To avoid confusions, clear any branch configuration
> before setting the configuration from the copied or moved branch.

So, whatever tea leaves we read into the history, or whether this was a good or bad design in the first place, I think we should probably lean towards not having this be a bug fix, but a new feature. Both modes are clearly easy to support.

And then document it in terms of some new switch being the equivalent to --remove-section followed by a rename, the existing thing a rename etc.

Show 20 quoted lines

> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  
>  	strbuf_release(&logmsg);
>  
> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> -		die(_("Branch is renamed, but update of config-file failed"));
> -	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"));
> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> +
> +		delete_branch_config(interpreted_newname);
> +
> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is renamed, but update of config-file failed"));
> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is copied, but update of config-file failed"));

Aside from any question of a hypothetical "should", your implementation is running head-first into a major caveat in our config API.

Which is that we don't have transactions or rollbacks, and we don't even carry a lock forward for all of these.

So, there's crappy edge cases in the old implementation as you've found, but at least it mostly failed-safe.

But here we'll delete_branch_config(), then release the lock, and then try to rename the new branch to that location, which might fail.

So, we'll be left with no config for the thing we tried to clobber, nor the new config.