| Message ID | pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] tracking branches: add advice to ambiguous refspec error | expand |
"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. OK. > Documentation/config/advice.txt | 4 +++ > advice.c | 1 + > advice.h | 1 + > branch.c | 44 +++++++++++++++++++++++++++++---- > 4 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt > index c40eb09cb7e..90f7dbd03aa 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 branch tracking relationship setup fails due > + to multiple remotes' refspecs mapping to the same remote > + tracking namespace in the repo. Advice shown when fetch refspec for multiple remotes map to the same remote-tracking branch namespace and causes branch tracking set-up to fail. "remote-tracking" should be spelled as a single word. There are some existing mistakes in the code, but let's not make it worse. > diff --git a/branch.c b/branch.c > index 6b31df539a5..5c28d432103 100644 > --- a/branch.c > +++ b/branch.c > @@ -18,9 +18,15 @@ struct tracking { > int matches; > }; > > +struct find_tracked_branch_cb { > + struct tracking *tracking; > + struct strbuf remotes_advice; > +}; > + > 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) { > string_list_append(tracking->srcs, tracking->spec.src); > tracking->remote = remote->name; > } else { > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > } > + /* > + * TRANSLATORS: This is a line listing a remote with duplicate > + * refspecs, to be later included in advice message > + * ambiguousFetchRefspec. For RTL languages you'll probably want > + * to swap the "%s" and leading " " space around. > + */ > + strbuf_addf(&ftb->remotes_advice, _(" %s\n"), remote->name); > tracking->spec.src = NULL; > } This is wasteful. remotes_advice does not have to be filled when we are not going to give advice, i.e. there is only one matching remote and no second or subsequent ones, which should be the majority case. And we should not make majority of users who do not make a mistake that needs the advice message pay the cost of giving advice to those who screw up, if we can avoid it. Instead, only when we are looking at the second one, we can stuff tracking->remote (i.e. the first one) to remotes_advice, and then append remote->name there. Perhaps we can do it like so? struct strbuf *names = &ftb->remotes_advice; const char *namefmt = N_(" %s\n"); switch (++tracking->matches) { case 1: string_list_append(tracking->srcs, tracking->spec.src); tracking->remote = remote->name; break; case 2: strbuf_addf(names, _(namefmt), tracking->remote); /* fall through */ default: strbuf_addf(names, _(namefmt), remote->name); free(tracking->spec.src); string_list_clear(tracking->srcs, 0); break; } tracking->spec.src = NULL; For a bonus point, remotes_advice member can be left empty without paying the cost to strbuf_addf() when the advice configuration says we are not going to show the message. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> diff --git a/branch.c b/branch.c >> index 6b31df539a5..5c28d432103 100644 >> --- a/branch.c >> +++ b/branch.c >> @@ -18,9 +18,15 @@ struct tracking { >> int matches; >> }; >> >> +struct find_tracked_branch_cb { >> + struct tracking *tracking; >> + struct strbuf remotes_advice; >> +}; >> + >> 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) { >> string_list_append(tracking->srcs, tracking->spec.src); >> tracking->remote = remote->name; >> } else { >> free(tracking->spec.src); >> string_list_clear(tracking->srcs, 0); >> } >> + /* >> + * TRANSLATORS: This is a line listing a remote with duplicate >> + * refspecs, to be later included in advice message >> + * ambiguousFetchRefspec. For RTL languages you'll probably want >> + * to swap the "%s" and leading " " space around. >> + */ >> + strbuf_addf(&ftb->remotes_advice, _(" %s\n"), remote->name); >> tracking->spec.src = NULL; >> } > > This is wasteful. remotes_advice does not have to be filled when we > are not going to give advice, i.e. there is only one matching remote > and no second or subsequent ones, which should be the majority case. > And we should not make majority of users who do not make a mistake > that needs the advice message pay the cost of giving advice to those > who screw up, if we can avoid it. > > Instead, only when we are looking at the second one, we can stuff > tracking->remote (i.e. the first one) to remotes_advice, and then > append remote->name there. Perhaps we can do it like so? > > struct strbuf *names = &ftb->remotes_advice; > const char *namefmt = N_(" %s\n"); > > switch (++tracking->matches) { > case 1: > string_list_append(tracking->srcs, tracking->spec.src); > tracking->remote = remote->name; > break; > case 2: > strbuf_addf(names, _(namefmt), tracking->remote); > /* fall through */ > default: > strbuf_addf(names, _(namefmt), remote->name); > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > break; > } > tracking->spec.src = NULL; > > For a bonus point, remotes_advice member can be left empty without > paying the cost to strbuf_addf() when the advice configuration says > we are not going to show the message. > > Thanks. Hm, what do you think of an alternate approach of storing of the matching remotes in a string_list, something like: struct find_tracked_branch_cb { struct tracking *tracking; struct string_list matching_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) { string_list_append(tracking->srcs, tracking->spec.src); tracking->remote = remote->name; } else { free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } string_list_append(&ftb->matching_remotes, remote->name); tracking->spec.src = NULL; } then construct the advice message in setup_tracking()? To my untrained eye, "case 2" requires a bit of extra work to understand.
Glen Choo <chooglen@google.com> writes: > Hm, what do you think of an alternate approach of storing of the > matching remotes in a string_list, something like: > > struct find_tracked_branch_cb { > struct tracking *tracking; > struct string_list matching_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) { > string_list_append(tracking->srcs, tracking->spec.src); > tracking->remote = remote->name; > } else { > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > } > string_list_append(&ftb->matching_remotes, remote->name); > tracking->spec.src = NULL; > } > > then construct the advice message in setup_tracking()? To my untrained > eye, "case 2" requires a bit of extra work to understand. If I were writing this code from scratch, I would have probably collected the names first in this callback, and assembled them at the end into a single string when we need a single string to show. Having said that, as long as you do that lazily not to penalize those who have sane setting without the need for advice/error to trigger, I do not particularly care how the list of matching remote names are kept. Having string_list_append() unconditionally like the above patch has, even for folks with just a single match without need for the advice/error message is suboptimal, I would think.
On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote: > > Glen Choo <chooglen@google.com> writes: > > > Hm, what do you think of an alternate approach of storing of the > > matching remotes in a string_list, something like: [...] > > then construct the advice message in setup_tracking()? To my untrained > > eye, "case 2" requires a bit of extra work to understand. Interestingly, that was what I had in the original RFC. I started using the strbuf later, after Ævar confirmed that a single "advise()" call is the way to go. I understood building the string as we go to lead to simpler code, as it meant one less loop. On the other hand I understand Junio is more concerned about performance than the existence of a second loop that we should almost never hit. I'm very happy to switch from strbuf-building to string_list-appending, but I'm curious to understand how/why the performance of strbuf_addf() would be notably worse than that of string_list_append(). Is there public doc about this somewhere? > Having said that, as long as you do that lazily not to penalize > those who have sane setting without the need for advice/error to > trigger, I do not particularly care how the list of matching remote > names are kept. Having string_list_append() unconditionally like > the above patch has, even for folks with just a single match without > need for the advice/error message is suboptimal, I would think. Again, I'm new here, and not a great coder to start with, but I'm having a hard time understanding why the single extra/gratuitous strbuf_addf() or string_list_append() call that we stand to optimize (I haven't understood whether there is a significant difference between them) would ever be noticeable in the context of creating a branch. I of course completely understand optimizing anything that will end up looping, but this is a max of 1 iteration's savings; I would have thought that at these levels, readability/maintainability (and succinctness) of the code would trump any marginal performance savings. To that end, I'd understand going back to string_list_append() as Glen proposes, and building a formatted string in a single place (setup_tracking()) only when required - both for readability, and in case some aspect of strbuf_addf() is non-trivially expensive - but is the "only append to the string_list() on the rare second pass" optimization really worth the increase in amount of code? Is "performance over succinctness" a general principle that could or should be noted somewhere?
On Mon, Mar 28 2022, Tao Klerks wrote: > On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Glen Choo <chooglen@google.com> writes: >> >> > Hm, what do you think of an alternate approach of storing of the >> > matching remotes in a string_list, something like: > [...] >> > then construct the advice message in setup_tracking()? To my untrained >> > eye, "case 2" requires a bit of extra work to understand. > > Interestingly, that was what I had in the original RFC. I started using > the strbuf later, after Ævar confirmed that a single "advise()" call is > the way to go. I understood building the string as we go to lead to > simpler code, as it meant one less loop. On the other hand I > understand Junio is more concerned about performance than the > existence of a second loop that we should almost never hit. > > I'm very happy to switch from strbuf-building to string_list-appending, > but I'm curious to understand how/why the performance of > strbuf_addf() would be notably worse than that of > string_list_append(). > > Is there public doc about this somewhere? We could do a string_list as in your v1 and concat it as we're formatting it, but pushing new strings to a string_list v.s. appending to a strbuf is actually more efficient in favor of the strbuf. But as to not penalizing those who don't have the advice enabled, something like this (untested)?: diff --git a/branch.c b/branch.c index 5c28d432103..83545456c36 100644 --- a/branch.c +++ b/branch.c @@ -20,6 +20,7 @@ struct tracking { struct find_tracked_branch_cb { struct tracking *tracking; + unsigned int do_advice:1; struct strbuf remotes_advice; }; @@ -36,6 +37,9 @@ static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } + tracking->spec.src = NULL; + if (!ftb->do_advice) + return 0; /* * TRANSLATORS: This is a line listing a remote with duplicate * refspecs, to be later included in advice message @@ -43,7 +47,6 @@ static int find_tracked_branch(struct remote *remote, void *priv) * to swap the "%s" and leading " " space around. */ strbuf_addf(&ftb->remotes_advice, _(" %s\n"), remote->name); - tracking->spec.src = NULL; } return 0; @@ -249,6 +252,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, struct find_tracked_branch_cb ftb_cb = { .tracking = &tracking, .remotes_advice = STRBUF_INIT, + .do_advice = advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC), }; memset(&tracking, 0, sizeof(tracking)); @@ -273,7 +277,7 @@ static void setup_tracking(const char *new_ref, const char *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)) + if (ftb_cb.do_advice) advise(_("There are multiple remotes whose fetch refspecs map to the remote\n" "tracking ref %s:\n" "%s"
Tao Klerks <tao@klerks.biz> writes: >> Having said that, as long as you do that lazily not to penalize >> those who have sane setting without the need for advice/error to >> trigger, I do not particularly care how the list of matching remote >> names are kept. Having string_list_append() unconditionally like >> the above patch has, even for folks with just a single match without >> need for the advice/error message is suboptimal, I would think. > > Again, I'm new here, and not a great coder to start with, but I'm > having a hard time understanding why the single extra/gratuitous > strbuf_addf() or string_list_append() call that we stand to optimize > (I haven't understood whether there is a significant difference > between them) would ever be noticeable in the context of creating > a branch. Small things accumulate. Unless you have to bend over backwards to do so, avoid computing unconditionally what you only need in an error case. People do not make mistake all the time, and they shouldn't be forced to pay for the possibility that other people may make mistakes and may want to get help. When you see that you are wasting cycles "just in case I may see an error", you may stop, take a deep breath, and think if you can push the extra work to error code path with equally simple and easy code. And in this case, I think it is just as equally easy and simple as the unconditional code in your patch to optimize for the case where there is *no* duplicate. It is a good discipline to learn, especially if you are new, as it is something that stays with you even after you move on to different projects. > I of course completely understand optimizing anything that will > end up looping, but this is a max of 1 iteration's savings; I would When there is no error, you do not even need to keep the "names that will become necessary for advice" at all, so you are comparing 0 with 1. And if you read the no-error case of the suggested rewrite, you'd realize that it is much easier to reason about. You do not have to wonder what the remotes_advice strbuf (or string_list) is doing in the no-error code path at all. This is not about performance, it is about clarity of the code (not doing unnecessary thing means doing only essential thing, after all). Between strbuf appending and string_list appending, I do not personally care. As long as the code can produce the same output, it is OK. Using string_list _may_ have an advantage that string formatting logic will be concentrated in a single place, as opposed to strbuf appending, where part of formatting decisions need to be made in the callback and other part is left in the advise-string. And because this should all happen only in error code path, the performance between two APIs would not matter at all (and for that to truly hold, you need to stick to "do not unconditionally compute what you need only in an error case" discipline).
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But as to not penalizing those who don't have the advice enabled, > something like this (untested)?: Unconditionally computing what you need only in the error path is the primary sin in the patch, and that should be addressed first. If we need to do new things (like adding the do_advice member), it becomes questionable if it is worth doing. In this case, I think the update is simple enough and the control flow makes it clear what is and what isn't needed only for advice-message generation, so it might be an overall win. Thanks.
OK, I On Mon, Mar 28, 2022 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote: > > Small things accumulate. > OK. I've tried combining Junio's "only do something when needed" case statement with Glen's "co-locate advice-related string manipulations" proposal, and I believe this disqualifies and obviates the need for passing around any new "are we going to be issuing advice" flag.
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index c40eb09cb7e..90f7dbd03aa 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 branch tracking relationship setup fails due + to multiple remotes' refspecs mapping to the same remote + tracking namespace in the repo. 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..5c28d432103 100644 --- a/branch.c +++ b/branch.c @@ -18,9 +18,15 @@ struct tracking { int matches; }; +struct find_tracked_branch_cb { + struct tracking *tracking; + struct strbuf remotes_advice; +}; + 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) { @@ -30,6 +36,13 @@ static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } + /* + * TRANSLATORS: This is a line listing a remote with duplicate + * refspecs, to be later included in advice message + * ambiguousFetchRefspec. For RTL languages you'll probably want + * to swap the "%s" and leading " " space around. + */ + strbuf_addf(&ftb->remotes_advice, _(" %s\n"), remote->name); tracking->spec.src = NULL; } @@ -219,6 +232,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) return 0; } + /* * Used internally to set the branch.<new_ref>.{remote,merge} config * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike @@ -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, + .remotes_advice = STRBUF_INIT, + }; 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,24 @@ 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)) + 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, + ftb_cb.remotes_advice.buf + ); + exit(status); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); @@ -263,6 +296,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, exit(-1); cleanup: + strbuf_release(&ftb_cb.remotes_advice); string_list_clear(&tracking_srcs, 0); }