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

On Fri, Nov 18 2022, Victoria Dye wrote:

I think [1] and [2] aren't comparable in at least a couple of ways to this case:

 a) [1] and [2] are discussions about something you can trivially do
    outside of git itself, which isn't the case with most behavior and
    options we implement, including this "branch move/copy".

    I mean you can of course script it with the refs primatives and "git
    config", but it's not "just use grep" or "just use head".

 b) [1] and [2] are a discussion about the trade-offs of adding new
    behavior, which by definition nobody relies on us for. So I think
    "a" is kind of moot to begin with.

    There's UX behavior in Git that I think sucks, but which I think
    would be irresponsible to simply change, much of it I think we'd be
    better off not having, if we had a convenient time machine.

    But that's not realistic, even if behavior is dumb or we'd like to
    change it in hindsight, we'll always need to keep the potential
    impact in mind if it's in released versions.

    Here we're talking about changing the semantics of a feature that's
    already been in released version for 5 years.

Now, I honestly don't know where I stand on "b" yet, maybe this is a thing we can simply change.

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

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.

It's also the nature of the fix, i.e. an existing option being changed without a doc change to be destructive when it wasn't before.

For "git remote" we error out if "remote_is_configured()" before we call the very same underlying function that we do for the branch copy/move. Maybe that would make more sense? I don't know...

Right now we do it in one go under one *.lock in git_config_copy_section(), which has a stateful loop that mutates the config.

So rather than "after", why not all at once? I don't see why we'd need two calls & lock/unlock operations.

It's not like we're calling a generic function, it already has special-cased code for "here's what I do when I'm doing the branch copy thing", so we could implement any new behavior there.

Aside from anything else, I think one thing this proposed change
absolutely need is a documentation update. I.e. now we say:
	
	With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.
	If <oldbranch> had a corresponding reflog, it is renamed to match
	<newbranch>, and a reflog entry is created to remember the branch
	renaming. If <newbranch> exists, -M must be used to force the rename
	to happen.
	
	The `-c` and `-C` options have the exact same semantics as `-m` and
	`-M`, except instead of the branch being renamed, it will be copied to a
	new name, along with its config and reflog.

Which to my reading really leaves this ambiguous either way. I.e. we do say that we'll refuse to operate if the *ref* already exists, but say nothing about existing config.

Between that and using "renamed" (which I think a user might reasonably assume means '"git config"'s idea of rename") it seems to me to suggest that the pre-flight checking explicitly doesn't include the config.

But it's really quite ambiguous, and isn't spelled out explicitly. So if we're going to wipe out the existing config, we really should update the docs.

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.e. we don't wipe away existing branch.foo.* config for new branch creation, so why would copying or moving to the "foo" name act differently?

If you set "git config branch.foo.merge refs/heads/next" beforehand instead we'll wipe away *that config key*, but as noted above I think that might be sensible.

I haven't explored other existing behavior fully, but maybe the proposed change can look into that further ... :)

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

Now, personally I don't use it like that, but it works now. Whatever anyone's stance on the sensibility of existing established behavior, I'd think (sans perhaps obvious bugs) that we'd start out with testing how the existing behavior acts before changing it.

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.