Re: [PATCH v4] branch: add flags and config to inherit tracking
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-19 06:57:00
On Tue, Nov 16 2021, Josh Steadmon wrote:
I've addressed Glen's feedback from V3. However, this brings up a new issue that was not obvious before: "branch.<name>.merge" can be specified more than once. On the other hand, the existing tracking setup code supports only a single merge entry. For now I'm defaulting to use the first merge entry listed in the branch struct, but I'm curious what people think the best solution would be. This may be another point in favor of Ævar's suggestion to reuse the copy-branch-config machinery.
I haven't looked in any detail now at the "should we copy the config?" questions. Just some quick comments/nits below:
+static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
+{
+ const char *bare_ref;
+ struct branch *branch;
+
+ bare_ref = orig_ref;
+ skip_prefix(orig_ref, "refs/heads/", &bare_ref);
+
+ branch = branch_get(bare_ref);
+ if (!branch->remote_name) {
+ warning(_("asked to inherit tracking from %s, but no remote is set"),
+ bare_ref);
+ return -1;
+ }
+
+ if (branch->merge_nr < 1 || !branch->merge_name || !branch->merge_name[0]) {
+ warning(_("asked to inherit tracking from %s, but no merge configuration is set"),
+ bare_ref);
Should quote ('%s') the %s in both here.
quoted hunk ↗ jump to hunk
+ return -1; + } + + tracking->remote = xstrdup(branch->remote_name); + tracking->src = xstrdup(branch->merge_name[0]); + tracking->matches = 1; + return 0; +} + /* * This is called when new_ref is branched off of orig_ref, and tries * to infer the settings for branch.<new_ref>.{remote,merge} from the@@ -139,7 +166,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; - if (for_each_remote(find_tracked_branch, &tracking)) + if (track != BRANCH_TRACK_INHERIT) { + for_each_remote(find_tracked_branch, &tracking); + } else if (inherit_tracking(&tracking, orig_ref)) return;
Style: Dangling braces, can just skip the braces here.
quoted hunk ↗ jump to hunk
@@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT__VERBOSE(&filter.verbose, N_("show hash and subject, give twice for upstream branch")), OPT__QUIET(&quiet, N_("suppress informational messages")), - OPT_SET_INT('t', "track", &track, N_("set up tracking mode (see git-pull(1))"), - BRANCH_TRACK_EXPLICIT), + OPT_CALLBACK_F('t', "track", &track, "direct|inherit", + N_("set up tracking mode (see git-pull(1))"),
Hrm, should we say "git help pull" here, on just not reference it at all and have a linkgit:git-pull[1]? Or maybe git-branch.txt and git-pull.txt should be including a template? As we do with Documentation/rev-list-options.txt, then this cross-reference wouldn't be needed.
quoted hunk ↗ jump to hunk
+ PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + parse_opt_tracking_mode), OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"), BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN), OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),diff --git a/builtin/checkout.c b/builtin/checkout.c index b5d477919a..45dab414ea 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c@@ -1532,8 +1532,10 @@ static struct option *add_common_switch_branch_options( { struct option options[] = { OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")), - OPT_SET_INT('t', "track", &opts->track, N_("set upstream info for new branch"), - BRANCH_TRACK_EXPLICIT), + OPT_CALLBACK_F('t', "track", &opts->track, "direct|inherit", + N_("set up tracking mode (see git-pull(1))"), + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + parse_opt_tracking_mode), OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"), PARSE_OPT_NOCOMPLETE), OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")),diff --git a/config.c b/config.c index cb4a8058bf..4bd5a18faf 100644 --- a/config.c +++ b/config.c@@ -1580,6 +1580,9 @@ static int git_default_branch_config(const char *var, const char *value) if (value && !strcasecmp(value, "always")) { git_branch_track = BRANCH_TRACK_ALWAYS; return 0; + } else if (value && !strcasecmp(value, "inherit")) { + git_branch_track = BRANCH_TRACK_INHERIT; + return 0; }
Looks like an existing issue, but we just document "inherit", not "INHERIT", "iNhErIt" etc. I.e. should it being strcasecmp() v.s. strcmp() be documented?
+ return error(_("option `--track' expects \"direct\" or \"inherit\""));Already commented-on by Junio.
+test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' ' + # Set up tracking config on main + git config branch.main.remote origin && + git config branch.main.merge refs/heads/main && + test_config branch.autoSetupMerge inherit && + # With --track=inherit, we copy the tracking config from main + git checkout --track=inherit -b b1 main && + test_cmp_config origin branch.b1.remote && + test_cmp_config refs/heads/main branch.b1.merge && + # With branch.autoSetupMerge=inherit, we do the same + git checkout -b b2 main && + test_cmp_config origin branch.b2.remote && + test_cmp_config refs/heads/main branch.b2.merge && + # But --track overrides this + git checkout --track -b b3 main && + test_cmp_config . branch.b3.remote && + test_cmp_config refs/heads/main branch.b3.merge && + # And --track=direct does as well + git checkout --track=direct -b b4 main && + test_cmp_config . branch.b4.remote && + test_cmp_config refs/heads/main branch.b4.merge +' +
This is the last test, we can use test_config instead of "git config" there I think, i.e. it's not setting up config for subseuent tests.