On Fri, Jul 30, 2021 at 09:35:49 -0400, Philippe Blain wrote:
> 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.
> > $ 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.
> > 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).
> > +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.
> > 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?
> > 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.
> > + 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
