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

From: Rubén Justo <rjusto@gmail.com>
Date: Nov 20, 2022, 14:55 UTC
Message-ID: <660dc134-121c-11b1-f722-2b4946495c40@gmail.com>
In-Reply-To: <221118.86r0y0f5ff.gmgdl@evledraar.gmail.com>

On 18-nov-2022 19:12:10, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

> 
> On Fri, Nov 18 2022, Victoria Dye wrote:
> 
> > Æ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.

Mmm... the way I see this is as an unexpected _and unintentional_ result, not sure if I am happy describing this is as a bug.

Maybe we can see this as "completing the functionality of moving and copying a branch", a missing "NEEDSWORK". Though I think the message describing this patch as a problem is sensible because of the motivations and for future reference.

I might agree with a new option like "--mix-configuration" to add this "feature" (and document it). But I don't find sensible to consolidate this unexpected and unintentional result, imho, and making the expectable an option.

Show 5 quoted lines

> But my spidey sense is a bit tickled by the proposed change discussing
> the existing behavior as a pure bug, when it seems to me that it's
> approximately what you'd get if it was implemented in terms of "config
> --rename-section", v.s. the proposed new behavior of (as I understand
> it) a "config --remove-section" followed by "config --rename-section".

Explaining this as something you'd get modifying directly the configuration, might not be a good idea. A user can break the config, but "git branch" should not.

Show 6 quoted lines

> I think a conservative approach would be to focus narrowly on the
> tracking config, if that's the thing that's at issue. I.e. we read that
> ourselves, know that it's broken if it's transmuted in certain ways etc.
> 
> But the proposed patch suggests that we extend that fix to anything we
> might find in that config namespace.

Yes, as the expectation (again, imho) is that the resultant branch gets its original configuration not a mix of the original and the remanent.

Show 38 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.
> >
> > 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.

The update of the ref is already done when we operate on the configuration. I think that _even_ if the git_config_{rename/copy}_section fails, the delete_branch_config() is sensible.

Show 16 quoted lines

> Also, whatever this is supposed to do, shouldn't we make it consistent
> with other "git branch" modes. E.g.:
> 
> 	git branch -D foo
> 	git -P config --show-origin --get-regexp '^branch\.foo\.'
> 	git config branch.foo.rebase true
> 	git branch foo -t origin/seen
> 	git -P config --show-origin --get-regexp '^branch\.foo\.'
> 
> Will emit:
> 	
> 	Deleted branch foo (was ...).
> 	branch 'foo' set up to track 'origin/seen'.
> 	file:.git/config        branch.foo.rebase true
> 	file:.git/config        branch.foo.remote origin
> 	file:.git/config        branch.foo.merge refs/heads/seen

 
I'm not sure.  I did notice that, and I can think of ill but legit uses
there: a user might be setting the configuration and then creating the
reference.  It is tangential here, but we can issue a warning like:

	$ git branch -t foo
	$ git branch -t bar
	$ git branch -C foo bar
	$ git branch --set-upstream-to foo bar
	warning: branch.bar.remote has multiple values
	warning: branch.bar.merge has multiple values
	branch 'bar' set up to track 'foo'.

I have different unsent patches to address this too, but I'm not sure what we want there, and how.

Show 24 quoted lines

> The upthread commit message doesn't discuss it, but even multiple
> "merge" values are a valid state of affairs. E.g.:
> 	
> 	$ git branch seen -t origin/seen
> 	branch 'seen' set up to track 'origin/seen'.
> 	$ git branch next -t origin/next
> 	branch 'next' set up to track 'origin/next'.
> 	$ git branch -C seen next
> 	$ git config --get-regexp 'branch\.next'
> 	branch.next.remote origin
> 	branch.next.merge refs/heads/seen
> 	branch.next.remote origin
> 	branch.next.merge refs/heads/next
> 
> Now, if you do:
> 
> 	git checkout next
> 
> and:
> 	git pull --no-rebase
> 
> You'll get e.g.:
> 
>        Merge branches 'next' and 'seen' of github.com:git/git into next

I think this is unexpected and unintentional.

> And again, don't take any of that as an a-priori argument against
> changing it, I'm still undecided. I'd just prefer that we know what it
> is we're changing.

Thank you for your analysis.