| Message ID | pull.1183.v6.git.1648742485887.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6] tracking branches: add advice to ambiguous refspec error | expand |
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > if (!remote_find_tracking(remote, &tracking->spec)) { > - if (++tracking->matches == 1) { > + switch (++tracking->matches) { > + case 1: > string_list_append(tracking->srcs, tracking->spec.src); > tracking->remote = remote->name; > - } else { > + break; > + case 2: > + /* there are at least two remotes; backfill the first one */ > + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); > + /* fall through */ > + default: > + string_list_append(&ftb->ambiguous_remotes, remote->name); > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > + break; Just to sanity check this part, - During the first iteration, we append tracking->spec.src to tracking->srcs, and set tracking->remote to remote->name; - In later iterations, we do not want to touch tracking->srcs, and want to collect remote->name. And "backfill" assumes that tracking->spec.src at that point in the second iteration is the same as what we got in remote->name in the first round. If that were a correct assumption, then it is curious that the first iteration uses tracking->spec.src and remote->name separately for different purposes, which makes me want to double check if the assumption is indeed correct. If it were tracking->remote (which was assigned the value of remote->name during the first iteration) that is used to backfill before we append remote->name in the second iteration, I wouldn't find it "curious", but the use of tracking->spec.src there makes me feel confused. Thanks.
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Tao Klerks <tao@klerks.biz> > > The error "not tracking: ambiguous information for ref" is raised > when we are evaluating what tracking information to set on a branch, > and find that the ref to be added as tracking branch is mapped > under multiple remotes' fetch refspecs. > > This can easily happen when a user copy-pastes a remote definition > in their git config, and forgets to change the tracking path. > > Add advice in this situation, explicitly highlighting which remotes > are involved and suggesting how to correct the situation. > ... > Documentation/config/advice.txt | 4 +++ > advice.c | 1 + > advice.h | 1 + > branch.c | 63 +++++++++++++++++++++++++++++---- > 4 files changed, 62 insertions(+), 7 deletions(-) Can we add a test case for the new "advice" output? Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> if (!remote_find_tracking(remote, &tracking->spec)) { >> - if (++tracking->matches == 1) { >> + switch (++tracking->matches) { >> + case 1: >> string_list_append(tracking->srcs, tracking->spec.src); >> tracking->remote = remote->name; >> - } else { >> + break; >> + case 2: >> + /* there are at least two remotes; backfill the first one */ >> + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); >> + /* fall through */ >> + default: >> + string_list_append(&ftb->ambiguous_remotes, remote->name); >> free(tracking->spec.src); >> string_list_clear(tracking->srcs, 0); >> + break; > > Just to sanity check this part, > > - During the first iteration, we append tracking->spec.src to > tracking->srcs, and set tracking->remote to remote->name; > > - In later iterations, we do not want to touch tracking->srcs, and > want to collect remote->name. > > And "backfill" assumes that tracking->spec.src at that point in the > second iteration is the same as what we got in remote->name in the > first round. If that were a correct assumption, then it is curious > that the first iteration uses tracking->spec.src and remote->name > separately for different purposes, which makes me want to double > check if the assumption is indeed correct. > > If it were tracking->remote (which was assigned the value of > remote->name during the first iteration) that is used to backfill > before we append remote->name in the second iteration, I wouldn't > find it "curious", but the use of tracking->spec.src there makes me > feel confused. > > Thanks. Thanks for bringing this up, I also found this unusual when I was reading v5.
On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > >> if (!remote_find_tracking(remote, &tracking->spec)) { > >> - if (++tracking->matches == 1) { > >> + switch (++tracking->matches) { > >> + case 1: > >> string_list_append(tracking->srcs, tracking->spec.src); > >> tracking->remote = remote->name; > >> - } else { > >> + break; > >> + case 2: > >> + /* there are at least two remotes; backfill the first one */ > >> + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); > >> + /* fall through */ > >> + default: > >> + string_list_append(&ftb->ambiguous_remotes, remote->name); > >> free(tracking->spec.src); > >> string_list_clear(tracking->srcs, 0); > >> + break; > > > > Just to sanity check this part, > > > > - During the first iteration, we append tracking->spec.src to > > tracking->srcs, and set tracking->remote to remote->name; > > > > - In later iterations, we do not want to touch tracking->srcs, and > > want to collect remote->name. > > > > And "backfill" assumes that tracking->spec.src at that point in the > > second iteration is the same as what we got in remote->name in the > > first round. If that were a correct assumption, then it is curious > > that the first iteration uses tracking->spec.src and remote->name > > separately for different purposes, which makes me want to double > > check if the assumption is indeed correct. > > > > If it were tracking->remote (which was assigned the value of > > remote->name during the first iteration) that is used to backfill > > before we append remote->name in the second iteration, I wouldn't > > find it "curious", but the use of tracking->spec.src there makes me > > feel confused. > > > > Thanks. > > Thanks for bringing this up, I also found this unusual when I was > reading v5. If you never hear from me again, please know it's because I crawled back under the rock I had crawled out from. This is clearly a bug from a silly typo, and I've managed to look at the resulting output twice without noticing the wrong thing was printed. I'm guessing the use of the word "unusual" here is a polite euphemism for "you numskull, what you wrote makes no sense!" :) I did not think adding an automated test for advise() output made sense, but I guess I have proved myself wrong.
Tao Klerks <tao@klerks.biz> writes: > On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: >> > >> >> if (!remote_find_tracking(remote, &tracking->spec)) { >> >> - if (++tracking->matches == 1) { >> >> + switch (++tracking->matches) { >> >> + case 1: >> >> string_list_append(tracking->srcs, tracking->spec.src); >> >> tracking->remote = remote->name; >> >> - } else { >> >> + break; >> >> + case 2: >> >> + /* there are at least two remotes; backfill the first one */ >> >> + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); >> >> + /* fall through */ >> >> + default: >> >> + string_list_append(&ftb->ambiguous_remotes, remote->name); >> >> free(tracking->spec.src); >> >> string_list_clear(tracking->srcs, 0); >> >> + break; >> > >> > Just to sanity check this part, >> > >> > - During the first iteration, we append tracking->spec.src to >> > tracking->srcs, and set tracking->remote to remote->name; >> > >> > - In later iterations, we do not want to touch tracking->srcs, and >> > want to collect remote->name. >> > >> > And "backfill" assumes that tracking->spec.src at that point in the >> > second iteration is the same as what we got in remote->name in the >> > first round. If that were a correct assumption, then it is curious >> > that the first iteration uses tracking->spec.src and remote->name >> > separately for different purposes, which makes me want to double >> > check if the assumption is indeed correct. >> > >> > If it were tracking->remote (which was assigned the value of >> > remote->name during the first iteration) that is used to backfill >> > before we append remote->name in the second iteration, I wouldn't >> > find it "curious", but the use of tracking->spec.src there makes me >> > feel confused. >> > >> > Thanks. >> >> Thanks for bringing this up, I also found this unusual when I was >> reading v5. > > If you never hear from me again, please know it's because I crawled > back under the rock I had crawled out from. This is clearly a bug from > a silly typo, and I've managed to look at the resulting output twice > without noticing the wrong thing was printed. I'm guessing the use of > the word "unusual" here is a polite euphemism for "you numskull, what > you wrote makes no sense!" :) Please don't take it that way! I use it the way I think most others use it, which is a more charitable "Hm, I trust the author, but this looks confusing to me and let me ask just to be sure that all our bases are covered." After all, even the best of us make mistakes, so I don't think it's a big deal. Plus, if the mistake managed to sneak past review, the fault is on all of us :) > I did not think adding an automated test for advise() output made > sense, but I guess I have proved myself wrong. Heh, nearly every time I think that a test isn't necessary, I find a way to prove myself wrong too.
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index c40eb09cb7e..343d271c707 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -4,6 +4,10 @@ advice.*:: can tell Git that you do not need help by setting these to 'false': + -- + ambiguousFetchRefspec:: + Advice shown when fetch refspec for multiple remotes map to + the same remote-tracking branch namespace and causes branch + tracking set-up to fail. fetchShowForcedUpdates:: Advice shown when linkgit:git-fetch[1] takes a long time to calculate forced updates after ref updates, or to warn diff --git a/advice.c b/advice.c index 2e1fd483040..18ac8739519 100644 --- a/advice.c +++ b/advice.c @@ -39,6 +39,7 @@ static struct { [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 }, [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 }, [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 }, + [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 }, [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 }, [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, diff --git a/advice.h b/advice.h index a3957123a16..2d4c94f38eb 100644 --- a/advice.h +++ b/advice.h @@ -17,6 +17,7 @@ struct string_list; ADVICE_ADD_EMPTY_PATHSPEC, ADVICE_ADD_IGNORED_FILE, ADVICE_AM_WORK_DIR, + ADVICE_AMBIGUOUS_FETCH_REFSPEC, ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME, ADVICE_COMMIT_BEFORE_MERGE, ADVICE_DETACHED_HEAD, diff --git a/branch.c b/branch.c index 6b31df539a5..812ceae2d0f 100644 --- a/branch.c +++ b/branch.c @@ -18,17 +18,31 @@ struct tracking { int matches; }; +struct find_tracked_branch_cb { + struct tracking *tracking; + struct string_list ambiguous_remotes; +}; + static int find_tracked_branch(struct remote *remote, void *priv) { - struct tracking *tracking = priv; + struct find_tracked_branch_cb *ftb = priv; + struct tracking *tracking = ftb->tracking; if (!remote_find_tracking(remote, &tracking->spec)) { - if (++tracking->matches == 1) { + switch (++tracking->matches) { + case 1: string_list_append(tracking->srcs, tracking->spec.src); tracking->remote = remote->name; - } else { + break; + case 2: + /* there are at least two remotes; backfill the first one */ + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); + /* fall through */ + default: + string_list_append(&ftb->ambiguous_remotes, remote->name); free(tracking->spec.src); string_list_clear(tracking->srcs, 0); + break; } tracking->spec.src = NULL; } @@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + struct find_tracked_branch_cb ftb_cb = { + .tracking = &tracking, + .ambiguous_remotes = STRING_LIST_INIT_DUP, + }; memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; tracking.srcs = &tracking_srcs; if (track != BRANCH_TRACK_INHERIT) - for_each_remote(find_tracked_branch, &tracking); + for_each_remote(find_tracked_branch, &ftb_cb); else if (inherit_tracking(&tracking, orig_ref)) goto cleanup; @@ -252,9 +270,39 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, goto cleanup; } - if (tracking.matches > 1) - die(_("not tracking: ambiguous information for ref %s"), - orig_ref); + if (tracking.matches > 1) { + int status = die_message(_("not tracking: ambiguous information for ref '%s'"), + orig_ref); + if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { + struct strbuf remotes_advice = STRBUF_INIT; + struct string_list_item *item; + + for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) + /* + * TRANSLATORS: This is a line listing a remote with duplicate + * refspecs in the advice message below. For RTL languages you'll + * probably want to swap the "%s" and leading " " space around. + */ + strbuf_addf(&remotes_advice, _(" %s\n"), item->string); + + /* + * TRANSLATORS: The second argument is a \n-delimited list of + * duplicate refspecs, composed above. + */ + advise(_("There are multiple remotes whose fetch refspecs map to the remote\n" + "tracking ref '%s':\n" + "%s" + "\n" + "This is typically a configuration error.\n" + "\n" + "To support setting up tracking branches, ensure that\n" + "different remotes' fetch refspecs map into different\n" + "tracking namespaces."), orig_ref, + remotes_advice.buf); + strbuf_release(&remotes_advice); + } + exit(status); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); @@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, cleanup: string_list_clear(&tracking_srcs, 0); + string_list_clear(&ftb_cb.ambiguous_remotes, 0); } int read_branch_desc(struct strbuf *buf, const char *branch_name)