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

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

Le 2021-07-28 à 22:01, Ben Boeckel a écrit :

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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?

quoted

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.

quoted

+	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'.

quoted

+	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}'.

quoted

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

quoted

+'
+
+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