Update tracking behaviour for branches created from remote branches

[part22]

Overview

Current Behaviour	New Behaviour
Local branches will always track the remote branches they were created from.	Local branches will track the remote branches they were created from only if their names match.

Description

The current behaviour when creating a new branch off of a remote branch is to always track the branch it was created from.

For example, if a branch 'my_branch' is created off of the remote branch 'fix_crash_13', then 'my_branch' will be tracking the remote 'fix_crash_13' branch.
It is common practice to have both the local and remote branches named the same when the local is tracking the remote one. Therefore, it is reasonable to expect that 'my_branch' should not track the remote 'fix_crash_13' branch.

The new behaviour when creating a new branch off of a remote branch is to track the branch it was created from only if the branch names match. If the branch names DO NOT match then the newly created branch will not track the remote branch it was created from.

For example, if a user creates a new branch 'fix_crash_13' off of the remote branch 'fix_crash_13', then the local 'fix_crash_13' branch will track the remote 'fix_crash_13' branch.
However, if the user creates a new branch called 'other_branch_name' off of the remote branch 'fix_crash_13', then the local 'other_branch_name' branch will NOT track the remote 'fix_crash_13' branch.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

Before I start reviewing this: do we actually need it? In #3675 my suggestion was to never set up tracking when pressing n, letting people use space (or press c in the local branches panel to checkout by name) if they do want a tracking branch. Jesse agreed. This is much simpler, so I prefer that solution.

I'm not too worried about the added code complexity, but I am concerned about adding config options that we don't absolutely need. We have so many options already, it just makes it harder to find the really important ones among them.

[part22]

I could just disable the tracking when using the n keybinding but I think it's always tricky to remove something people might have got used to.
But you and Jesse know the software and the codebase better than I do, so I will defer to your good judgement! 😉

[stefanhaller]

You mean people who are used to pressing n enter to create a tracking branch? How about we keep your logic of setting the tracking branch only if the name matches, but not if it doesn't? This should cover that case but also address the problem raised in #3675. Just without the configurability. What do you think?

[part22]

That seems like a good compromise but if you feel that is still too much added complexity I am fine with just disabling the tracking when using n on remote branches.

[stefanhaller]

No, I like this middle-ground version best. It's just so unlikely that someone wants to create a branch that has the same name but does not track the remote. As I said, the added code complexity doesn't bother me so much.

[part22]

I have removed the configuration part and kept the new behaviour part.

Recap

Current Behaviour	New Behaviour
Local branches will always track the remote branches they were created from.	Local branches will track the remote branches they were created from only if their names match.

[stefanhaller]

Great work, I like the test coverage.

I pushed a fixup with a very minor simplification of the test setup (we usually assume that master is the main branch, and it already exists).

Apart from that, just some thoughts below.

Oh, and it would be good to update the PR description, because it will end up in the merge commit message.

[stefanhaller]

The duplicated if statement bothers me slightly; how do you feel about doing this instead?

	newBranchFunc := self.c.Git().Branch.New
	if newBranchName != suggestedBranchName {
		newBranchFunc = self.c.Git().Branch.NewWithoutTracking
	}
	if err := newBranchFunc(newBranchName, from); err != nil {
		return err
	}

Maybe that's too obscure, I'm not sure.

Another option would be to add an argument to the New function; it could be called something like inhibitAutoTracking bool. I'm a little on the fence here, but I think I slightly prefer that. It has the downside that you need to touch all other call sites, but on the other hand that may be seen as a pro argument: it forces you to consider what's the right thing to do at all other call sites.

Just input, let me know what you think.

[part22]

The duplicated if statement bothers me slightly; how do you feel about doing this instead?

newBranchFunc := self.c.Git().Branch.New
if newBranchName != suggestedBranchName {
	newBranchFunc = self.c.Git().Branch.NewWithoutTracking
}
if err := newBranchFunc(newBranchName, from); err != nil {
	return err
}

I am fine with that, I don't think it's too obscure.

[stefanhaller]

Instead of doing all this, you could simply test that "other_branch" has a checkmark in the branches list (and doesn't in the other test). That's a bit less work, but maybe not quite as explicit. Unsure, just a thought.

[part22]

I don't mind changing it, I like that it is more explicit but I can see at least two good reasons to change it.

1. Less code can be easier to read, understand and modify
2. Less code can be faster to run

I think having a tight feedback loop on tests is important since they should be run often.

[part22]

So, let me know what you prefer, please.

[stefanhaller]

I agree that less code is better, so I prefer to change it to the shorter version.

[part22]

It's done.

[part22]

Oh, and it would be good to update the PR description, because it will end up in the merge commit message.

I have updated it.

[stk-ableton]

Thanks! Very happy with the current state. Merging.

[stefanhaller]

(Grr, I keep commenting with the wrong account. That was me. 😄)