Re: [PATCH 1/1] config: support a default remote tracking setup upon branch creation

From: Ben Boeckel <mathstuf@gmail.com>
Date: Jul 30, 2021, 14:07 UTC
Message-ID: <YQQHv/suvHohpFBe@erythro.dev.benboeckel.internal>
In-Reply-To: <bcbe13a8-7f22-7564-77cd-674c7b2acbf6@gmail.com>

On Fri, Jul 30, 2021 at 09:35:49 -0400, Philippe Blain wrote:

Show 10 quoted lines

> Le 2021-07-28 à 22:01, Ben Boeckel a écrit :
> >      $ git config remote.pushDefault myfork    # push to `myfork` by default
> >      $ git config push.default simple          # the default
> >      $ git config branch.autoSetupMerge always # always setup tracking
> 
> OK, so if I understand correctly this exisiting setting has to be changed
> to 'always' for the new settings you are adding to take effect, right ?
> I think this does make sense, reading the description of 'branch.autoSetupMerge',
> but maybe it should be spelled explicitely in the text of the commit message,
> and not just mentioned here in this terminal session excerpt.

Good point. I'll add it.

Show 7 quoted lines

> >      $ git config branch.defaultRemote origin  # track from `origin`
> >      $ git config branch.defaultMerge main     # track the `main` branch
> 
> Small nit: maybe I would invert these two, so it can read:
> 
>        $ git config branch.defaultMerge main     # track the `main` branch ...
>        $ git config branch.defaultRemote origin  # ... from `origin`

Agreed.

Show 8 quoted lines

> > Additionally, with the `extensions.worktreeConfig` setting, when a
> > separate work tree which is used for changes against a different branch
> > (e.g., a branch tracking a prior release), the `branch.defaultMerge`
> > setting may be changed independently, e.g., to `maint`.
> 
> This last paragraph is not explicitely needed, as nothing relating to
> 'extensions.worktreeConfig' is changed here right ? It's just the normal
> way that this setting works.

Yes. I'll mention more explicitly that this is the reason for preserving split settings (rather than a single `branch.defaultTrack = origin/main` setting that I had thought about until I saw the `--worktree` flag to `git config` and was intrigued).

Show 16 quoted lines

> > +branch.defaultRemote::
> > +	Tells 'git branch', 'git switch' and 'git checkout' to set up new branches
> > +	so that it will track a branch on the specified remote. This can be
> > +	used, in conjunction with `branch.defaultMerge`, in projects where
> > +	branches tend to target a single branch. This will be used to
> > +	initialize the "branch.<name>.remote" setting.
> > +
> > +branch.defaultMerge::
> > +	Tells 'git branch', 'git switch' and 'git checkout' to set up new branches
> > +	so that it will track a branch with this name. This can be used, in
> > +	conjunction with `branch.defaultRemote` in projects where branches tend
> > +	to target a single branch. This will be used to initialize the
> > +	"branch.<name>.merge" setting.
> 
> For the two setting above, if 'branch.autoSetupMerge' must be set to 'always' for
> the settings to work, I think it should be explicitely mentioned.

Will update.

Show 14 quoted lines

> > diff --git a/environment.c b/environment.c
> > index 2f27008424..d550deabbd 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -60,6 +60,8 @@ int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
> >   char *check_roundtrip_encoding = "SHIFT-JIS";
> >   unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
> >   enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
> > +const char* git_branch_remote = NULL;
> > +const char* git_branch_merge = NULL;
> 
> Can the new settings be implemented without adding more global variables ?
> I think we are trying to move away from these. Apart from that the code
> looks OK to me.

I'm all for that, but didn't see guidance on where such things should be stored. There's not a "context" object passed around, but I guess stuffing it into `repository` is fine? This also gives a nice place to free them rather than just leaking them too.

Alas, after some cursory investigation, `config.c@@git_default_branch_config` does not have access to "the repository" and the `cb` in `git_default_config` seems to be ~always be `NULL`. So maybe that will have to wait for further refactoring of the configuration tracking logic?

Show 14 quoted lines

> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> > index cc4b10236e..82137e8451 100755
> > --- a/t/t3200-branch.sh
> > +++ b/t/t3200-branch.sh
> > @@ -797,6 +797,45 @@ test_expect_success 'test tracking setup via --track but deeper' '
> >   	test "$(git config branch.my7.merge)" = refs/heads/o/o
> >   '
> >   
> > +test_expect_success 'test tracking setup via branch.default* and --track' '
> > +	git config branch.autosetupmerge always &&
> 
> You can use 'test_config branch.autosetupmerge always' so that the
> config is only active for the duration of the 'test_expect_success' block
> (see t/test-lib-functions.sh).

Nifty.

Show 68 quoted lines

> > +	git config branch.defaultremote local &&
> > +	git config branch.defaultmerge main &&
> > +	git config remote.local.url . &&
> > +	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> > +	(git show-ref -q refs/remotes/local/main || git fetch local) &&
> > +	git branch --track other/foo my2 &&
> > +	git config branch.autosetupmerge false &&
> > +	test "$(git config branch.my2.remote)" = other &&
> 
> Here and for the following line you can use 'test_cmp_config'.
> 
> > +	test "$(git config branch.my2.merge)" = refs/heads/foo
> > +'
> 
> This tests checks that an explicit '--track' argument overrides the new configs.
> I would name it something like "'--track overrides 'branch.default{merge,remote}'"
> or something like this. I would also add another test before this one that just
> checks that the new settings by themselves work as expected;
> i.e. a simple 'git checkout -b' and verifying that the
> tracking info is correctly configured according to 'branch.default{merge,remote}'.
> 
> > +
> > +test_expect_success 'test tracking setup via branch.default* and --no-track' '
> > +	git config branch.autosetupmerge always &&
> > +	git config branch.defaultremote local &&
> > +	git config branch.defaultmerge main &&
> > +	git config remote.local.url . &&
> > +	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> > +	(git show-ref -q refs/remotes/local/main || git fetch local) &&
> > +	git branch --no-track my2 &&
> > +	git config branch.autosetupmerge false &&
> > +	! test "$(git config branch.my2.remote)" = local &&
> > +	! test "$(git config branch.my2.merge)" = refs/heads/main
> 
> Here you expect the configs to be absent, so for more clarity you could
> do
> 
>      git config branch.my2.remote >remote &&
>      test_must_be_empty remote
> 
> and the same for merge.
> 
> > +'
> > +
> > +test_expect_success 'test tracking setup via branch.default*' '
> > +	git config branch.autosetupmerge always &&
> > +	git config branch.defaultremote local &&
> > +	git config branch.defaultmerge main &&
> > +	git config remote.local.url . &&
> > +	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> > +	(git show-ref -q refs/remotes/local/main || git fetch local) &&
> > +	git branch my2 &&
> > +	git config branch.autosetupmerge false &&
> > +	test "$(git config branch.my2.remote)" = local &&
> > +	test "$(git config branch.my2.merge)" = refs/heads/main
> > +'
> > +
> >   test_expect_success 'test deleting branch deletes branch config' '
> >   	git branch -d my7 &&
> >   	test -z "$(git config branch.my7.remote)" &&
> > 
> 
> Oh, so here is the 'just test the new settings' test. Let's move that
> one to be before the two others.
> 
> Another test that could be added is one that does not change
> 'branch.autosetupmerge' but sets the new configs, and checks that the
> tracking info is *not* set.

I'll make the test suite updates as well.

Thanks for the review,

--Ben