From: Ben Boeckel <mathstuf@gmail.com>
To: Philippe Blain <levraiphilippeblain@gmail.com>
Cc: git@vger.kernel.org, "Martin Ågren" <martin.agren@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Jeff Hostetler" <jeffhost@microsoft.com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH 1/1] config: support a default remote tracking setup upon branch creation
Date: Fri, 30 Jul 2021 10:07:59 -0400 [thread overview]
Message-ID: <YQQHv/suvHohpFBe@erythro.dev.benboeckel.internal> (raw)
In-Reply-To: <bcbe13a8-7f22-7564-77cd-674c7b2acbf6@gmail.com>
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
next prev parent reply other threads:[~2021-07-30 14:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-28 13:50 [PATCH 0/1] Improve automatic setup of tracking for new branches Ben Boeckel
2021-07-28 13:50 ` [PATCH 1/1] config: support setting up a remote tracking branch upon creation Ben Boeckel
2021-07-28 17:48 ` Junio C Hamano
2021-07-28 18:26 ` Ben Boeckel
2021-07-28 18:39 ` Junio C Hamano
2021-07-29 2:01 ` [PATCH 0/1] Improve automatic setup of tracking for new branches Ben Boeckel
2021-07-29 2:01 ` [PATCH 1/1] config: support a default remote tracking setup upon branch creation Ben Boeckel
2021-07-30 13:35 ` Philippe Blain
2021-07-30 14:07 ` Ben Boeckel [this message]
2021-07-30 17:32 ` Junio C Hamano
2021-08-02 13:02 ` Ævar Arnfjörð Bjarmason
2021-08-02 13:16 ` Ben Boeckel
2021-08-02 15:20 ` Ævar Arnfjörð Bjarmason
2021-07-30 1:04 ` [PATCH 0/1] Improve automatic setup of tracking for new branches Junio C Hamano
2021-07-30 1:33 ` Ben Boeckel
2021-07-30 13:35 ` Philippe Blain
2021-07-30 13:57 ` Ben Boeckel
2021-07-30 16:01 ` Philippe Blain
2021-07-30 17:45 ` Ben Boeckel
2021-08-02 21:17 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YQQHv/suvHohpFBe@erythro.dev.benboeckel.internal \
--to=mathstuf@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=levraiphilippeblain@gmail.com \
--cc=martin.agren@gmail.com \
--cc=me@ttaylorr.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).