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

[Victoria Dye <vdye@github.com>]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Nov 17 2022, Rubén Justo wrote:
>> 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.

I've noticed a bit of a pattern on the mailing list where, if the desired
user experience is unclear, the suggestion is "add an option! then users can
choose for themselves." But then, at the same time, we'll complain about
option bloat (as you [1] and Taylor [2] did on another recent series).

I don't see a compelling enough reason to introduce an option variant here.
Could I imagine a user wanting such a feature? Yes. But it also isn't clear
what users would practically use. I think a more conservative approach would
be appropriate: in this case, come to an agreement on a sane default (i.e.,
should we preserve the source's, or the destination's, tracking config?),
then wait for feedback to indicate whether there's a desire for an
alternative to that default.

[1] https://lore.kernel.org/git/221117.86k03tiudl.gmgdl@evledraar.gmail.com/
[2] https://lore.kernel.org/git/Y3ave2+kEwLTvtE6@nand.local/

> 
>> @@ -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.

This is a good point, so to add to it: I think a fairly unobtrusive solution
would be to move the config deletion after the rename is 100% complete.