* [PATCH] tracking branches: add advice to ambiguous refspec error @ 2022-03-21 10:23 Tao Klerks via GitGitGadget 2022-03-21 14:11 ` Ævar Arnfjörð Bjarmason 2022-03-22 9:18 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget 0 siblings, 2 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-21 10:23 UTC (permalink / raw) To: git; +Cc: Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- RFC: tracking branches: add advice to ambiguous refspec error I am submitting this as an RFC because of a couple of things I'm not sure about: 1. In this proposed patch the advice is emitted before the existing die(), in order to avoid changing the exact error behavior and only add extra/new hint lines, but in other advise() calls I see the error being emitted before the advise() hint. Given that error() and die() display different message prefixes, I'm not sure whether it's possible to follow the existing pattern and avoid changing the error itself. Should I just accept that with the new advice, the error message can and should change? 2. In order to include the names of the conflicting remotes I am calling advise() multiple times - this is not a pattern I see elsewhere - should I be building a single message and calling advise() only once? Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 advice.c | 1 + advice.h | 1 + branch.c | 27 ++++++++++++++++++++++++++- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/advice.c b/advice.c index 1dfc91d1767..686612590ec 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 601265fd107..3d68c1a6cb4 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 5d20a2e8484..243f6d8b362 100644 --- a/branch.c +++ b/branch.c @@ -12,6 +12,7 @@ struct tracking { struct refspec_item spec; struct string_list *srcs; + struct string_list *remotes; const char *remote; int matches; }; @@ -28,6 +29,7 @@ static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } + string_list_append(tracking->remotes, remote->name); tracking->spec.src = NULL; } @@ -217,6 +219,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) return 0; } + +static const char ambiguous_refspec_advice_pre[] = +N_("\n" +"There are multiple remotes with fetch refspecs mapping to\n" +"the tracking ref %s:\n";) +static const char ambiguous_refspec_advice_post[] = +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.\n"); + /* * 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 @@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, { struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; + struct string_list tracking_remotes = STRING_LIST_INIT_DUP; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + struct string_list_item *item; memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; tracking.srcs = &tracking_srcs; + tracking.remotes = &tracking_remotes; if (track != BRANCH_TRACK_INHERIT) for_each_remote(find_tracked_branch, &tracking); else if (inherit_tracking(&tracking, orig_ref)) @@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, return; } - if (tracking.matches > 1) + if (tracking.matches > 1) { + if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { + advise(_(ambiguous_refspec_advice_pre), orig_ref); + for_each_string_list_item(item, &tracking_remotes) { + advise(" %s", item->string); + } + advise(_(ambiguous_refspec_advice_post)); + } die(_("not tracking: ambiguous information for ref %s"), orig_ref); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] tracking branches: add advice to ambiguous refspec error 2022-03-21 10:23 [PATCH] tracking branches: add advice to ambiguous refspec error Tao Klerks via GitGitGadget @ 2022-03-21 14:11 ` Ævar Arnfjörð Bjarmason 2022-03-22 9:09 ` Tao Klerks 2022-03-22 9:18 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget 1 sibling, 1 reply; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-21 14:11 UTC (permalink / raw) To: Tao Klerks via GitGitGadget; +Cc: git, Tao Klerks On Mon, Mar 21 2022, Tao Klerks via GitGitGadget wrote: Re $subject (and you've got another outstanding patch on list that's the same), please add "RFC PATCH" to the subject for RFC patches, doesn't GGG have some way to do that? > 1. In this proposed patch the advice is emitted before the existing > die(), in order to avoid changing the exact error behavior and only > add extra/new hint lines, but in other advise() calls I see the > error being emitted before the advise() hint. Given that error() and > die() display different message prefixes, I'm not sure whether it's > possible to follow the existing pattern and avoid changing the error > itself. Should I just accept that with the new advice, the error > message can and should change? You can and should use die_message() in this case, it's exactly what it's intended for: int code = die_message(...); /* maybe advice */ return code; > 2. In order to include the names of the conflicting remotes I am > calling advise() multiple times - this is not a pattern I see > elsewhere - should I be building a single message and calling > advise() only once? That would make me very happy, yes :) I have some not-yet-sent patches to make a lot of this advice API less sucky, mainly to ensure that we always have a 1=1=1 mapping between config=docs=code, and to have the API itself emit the "and you can turn this off with XYZ config". I recently fixed the only in-tree message where we incrementally constructed advice() because of that, so not having another one sneak in would be good :) > diff --git a/advice.c b/advice.c > index 1dfc91d1767..686612590ec 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 }, This is missing the relevant Documentation/config/advice.txt update > diff --git a/advice.h b/advice.h > index 601265fd107..3d68c1a6cb4 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 5d20a2e8484..243f6d8b362 100644 > --- a/branch.c > +++ b/branch.c > @@ -12,6 +12,7 @@ > struct tracking { > struct refspec_item spec; > struct string_list *srcs; > + struct string_list *remotes; > > +"There are multiple remotes with fetch refspecs mapping to\n" > +"the tracking ref %s:\n";) "with" and "mapping to" is a bit confusing, should this say: There are multiple remotes whole fetch refspecs map to the remote tracking ref '%s'? (Should also have '' quotes for that in any case) > /* > * 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 > @@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > { > struct tracking tracking; > struct string_list tracking_srcs = STRING_LIST_INIT_DUP; > + struct string_list tracking_remotes = STRING_LIST_INIT_DUP; > int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; > + struct string_list_item *item; > > memset(&tracking, 0, sizeof(tracking)); > tracking.spec.dst = (char *)orig_ref; > tracking.srcs = &tracking_srcs; > + tracking.remotes = &tracking_remotes; > if (track != BRANCH_TRACK_INHERIT) > for_each_remote(find_tracked_branch, &tracking); > else if (inherit_tracking(&tracking, orig_ref)) FWIW I find the flow with something like this a lot clearer, i.e. not adding the new thing to a widely-used struct, just have a CB struct for the one thing that needs it: diff --git a/branch.c b/branch.c index 7958a2cb08f..55520eec6bd 100644 --- a/branch.c +++ b/branch.c @@ -14,14 +14,19 @@ struct tracking { struct refspec_item spec; struct string_list *srcs; - struct string_list *remotes; const char *remote; int matches; }; +struct find_tracked_branch_cb { + struct tracking *tracking; + struct string_list 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) { @@ -31,7 +36,7 @@ static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } - string_list_append(tracking->remotes, remote->name); + string_list_append(&ftb->remotes, remote->name); tracking->spec.src = NULL; } @@ -245,16 +250,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, { struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; - struct string_list tracking_remotes = STRING_LIST_INIT_DUP; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; struct string_list_item *item; + struct find_tracked_branch_cb ftb_cb = { + .tracking = &tracking, + .remotes = STRING_LIST_INIT_DUP, + }; memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; tracking.srcs = &tracking_srcs; - tracking.remotes = &tracking_remotes; 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; @@ -272,7 +279,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, if (tracking.matches > 1) { if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { advise(_(ambiguous_refspec_advice_pre), orig_ref); - for_each_string_list_item(item, &tracking_remotes) { + for_each_string_list_item(item, &ftb_cb.remotes) { advise(" %s", item->string); } advise(_(ambiguous_refspec_advice_post)); Also missing a string_list_clear() before/after this... > @@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > return; > } > > - if (tracking.matches > 1) > + if (tracking.matches > 1) { > + if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { > + advise(_(ambiguous_refspec_advice_pre), orig_ref); > + for_each_string_list_item(item, &tracking_remotes) { > + advise(" %s", item->string); > + } See: git show --first-parent 268e6b8d4d9 For how you can avoid incrementally constructing the message. I.e. we could just add a strbuf to the callback struct, have the callback add to it. Then on second thought we get rid of the string_list entirely don't we? Since we just need the \n-delimited list of remotes te inject into the message. > + advise(_(ambiguous_refspec_advice_post)); > + } > die(_("not tracking: ambiguous information for ref %s"), > orig_ref); > + } > > if (tracking.srcs->nr < 1) > string_list_append(tracking.srcs, orig_ref); > > base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] tracking branches: add advice to ambiguous refspec error 2022-03-21 14:11 ` Ævar Arnfjörð Bjarmason @ 2022-03-22 9:09 ` Tao Klerks 0 siblings, 0 replies; 36+ messages in thread From: Tao Klerks @ 2022-03-22 9:09 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Tao Klerks via GitGitGadget, git On Mon, Mar 21, 2022 at 3:33 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Mon, Mar 21 2022, Tao Klerks via GitGitGadget wrote: > > Re $subject (and you've got another outstanding patch on list that's the > same), please add "RFC PATCH" to the subject for RFC patches, doesn't > GGG have some way to do that? > Not as far as I can tell, not exactly. What I *could* have done, and will do next time, is add the "RFC: " prefix to the commit subject, after the "[PATCH]" prefix. The email subject lines are a bit surprising (to me): When it is a single commit, the email gets the subject line of that commit, and the subject of the "pull request" gets buried under the triple dash, whereas a series of several commits has the leading 0/N email with the pull request subject as email subject. > > 1. In this proposed patch the advice is emitted before the existing > > die(), in order to avoid changing the exact error behavior and only > > add extra/new hint lines, but in other advise() calls I see the > > error being emitted before the advise() hint. Given that error() and > > die() display different message prefixes, I'm not sure whether it's > > possible to follow the existing pattern and avoid changing the error > > itself. Should I just accept that with the new advice, the error > > message can and should change? > > You can and should use die_message() in this case, it's exactly what > it's intended for: > > int code = die_message(...); > /* maybe advice */ > return code; > Got it, thx. > > 2. In order to include the names of the conflicting remotes I am > > calling advise() multiple times - this is not a pattern I see > > elsewhere - should I be building a single message and calling > > advise() only once? > > That would make me very happy, yes :) > > I have some not-yet-sent patches to make a lot of this advice API less > sucky, mainly to ensure that we always have a 1=1=1 mapping between > config=docs=code, and to have the API itself emit the "and you can turn > this off with XYZ config". > > I recently fixed the only in-tree message where we incrementally > constructed advice() because of that, so not having another one sneak in > would be good :) > This is more or less sorted, although the result (the bit I did!) is still a bit ugly, I suspect there is a cleaner way. > > diff --git a/advice.c b/advice.c > > index 1dfc91d1767..686612590ec 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 }, > > This is missing the relevant Documentation/config/advice.txt update > Argh, thx. > > diff --git a/advice.h b/advice.h > > index 601265fd107..3d68c1a6cb4 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 5d20a2e8484..243f6d8b362 100644 > > --- a/branch.c > > +++ b/branch.c > > @@ -12,6 +12,7 @@ > > struct tracking { > > struct refspec_item spec; > > struct string_list *srcs; > > + struct string_list *remotes; > > > > +"There are multiple remotes with fetch refspecs mapping to\n" > > +"the tracking ref %s:\n";) > > "with" and "mapping to" is a bit confusing, should this say: > > There are multiple remotes whole fetch refspecs map to the remote > tracking ref '%s'? Works for me. > > (Should also have '' quotes for that in any case) > > > /* > > * 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 > > @@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > > { > > struct tracking tracking; > > struct string_list tracking_srcs = STRING_LIST_INIT_DUP; > > + struct string_list tracking_remotes = STRING_LIST_INIT_DUP; > > int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; > > + struct string_list_item *item; > > > > memset(&tracking, 0, sizeof(tracking)); > > tracking.spec.dst = (char *)orig_ref; > > tracking.srcs = &tracking_srcs; > > + tracking.remotes = &tracking_remotes; > > if (track != BRANCH_TRACK_INHERIT) > > for_each_remote(find_tracked_branch, &tracking); > > else if (inherit_tracking(&tracking, orig_ref)) > > FWIW I find the flow with something like this a lot clearer, i.e. not > adding the new thing to a widely-used struct, just have a CB struct for > the one thing that needs it: > > diff --git a/branch.c b/branch.c > index 7958a2cb08f..55520eec6bd 100644 > --- a/branch.c > +++ b/branch.c > @@ -14,14 +14,19 @@ > struct tracking { > struct refspec_item spec; > struct string_list *srcs; > - struct string_list *remotes; > const char *remote; > int matches; > }; > > +struct find_tracked_branch_cb { > + struct tracking *tracking; > + struct string_list 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) { > @@ -31,7 +36,7 @@ static int find_tracked_branch(struct remote *remote, void *priv) > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > } > - string_list_append(tracking->remotes, remote->name); > + string_list_append(&ftb->remotes, remote->name); > tracking->spec.src = NULL; > } > > @@ -245,16 +250,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > { > struct tracking tracking; > struct string_list tracking_srcs = STRING_LIST_INIT_DUP; > - struct string_list tracking_remotes = STRING_LIST_INIT_DUP; > int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; > struct string_list_item *item; > + struct find_tracked_branch_cb ftb_cb = { > + .tracking = &tracking, > + .remotes = STRING_LIST_INIT_DUP, > + }; > > memset(&tracking, 0, sizeof(tracking)); > tracking.spec.dst = (char *)orig_ref; > tracking.srcs = &tracking_srcs; > - tracking.remotes = &tracking_remotes; > 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; > > @@ -272,7 +279,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > if (tracking.matches > 1) { > if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { > advise(_(ambiguous_refspec_advice_pre), orig_ref); > - for_each_string_list_item(item, &tracking_remotes) { > + for_each_string_list_item(item, &ftb_cb.remotes) { > advise(" %s", item->string); > } > advise(_(ambiguous_refspec_advice_post)); Lovely, applied, thx. > > Also missing a string_list_clear() before/after this... Yes, sorry, I looked for "tracking_srcs" because I suspected some cleanup was needed, but did not think to look for "tracking.srcs", or even just scroll to the end. C takes some getting used to, for me at least. > > > @@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > > return; > > } > > > > - if (tracking.matches > 1) > > + if (tracking.matches > 1) { > > + if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { > > + advise(_(ambiguous_refspec_advice_pre), orig_ref); > > + for_each_string_list_item(item, &tracking_remotes) { > > + advise(" %s", item->string); > > + } > > See: > > git show --first-parent 268e6b8d4d9 > > For how you can avoid incrementally constructing the message. I.e. we > could just add a strbuf to the callback struct, have the callback add to > it. > > Then on second thought we get rid of the string_list entirely don't we? > Since we just need the \n-delimited list of remotes te inject into the > message. Yep, applied (with some icky argument awkwardness in the final advise() call) Reroll on the way. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2] RFC: tracking branches: add advice to ambiguous refspec error 2022-03-21 10:23 [PATCH] tracking branches: add advice to ambiguous refspec error Tao Klerks via GitGitGadget 2022-03-21 14:11 ` Ævar Arnfjörð Bjarmason @ 2022-03-22 9:18 ` Tao Klerks via GitGitGadget 2022-03-22 10:04 ` Ævar Arnfjörð Bjarmason 2022-03-28 6:51 ` [PATCH v3] " Tao Klerks via GitGitGadget 1 sibling, 2 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-22 9:18 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- RFC: tracking branches: add advice to ambiguous refspec error Second go at adding some advice around ambiguous remote refspecs, incorporating Ævar's suggestions. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v1: 1: 9ec19e1c48e ! 1: 6c1cd885dda tracking branches: add advice to ambiguous refspec error @@ Metadata Author: Tao Klerks <tao@klerks.biz> ## Commit message ## - tracking branches: add advice to ambiguous refspec error + RFC: tracking branches: add advice to ambiguous refspec error The error "not tracking: ambiguous information for ref" is raised when we are evaluating what tracking information to set on a branch, @@ Commit message Signed-off-by: Tao Klerks <tao@klerks.biz> + ## Documentation/config/advice.txt ## +@@ Documentation/config/advice.txt: advice.*:: + Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] + is asked to update index entries outside the current sparse + checkout. ++ 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.c ## @@ advice.c: static struct { [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 }, @@ advice.h: struct string_list; ADVICE_DETACHED_HEAD, ## branch.c ## -@@ - struct tracking { - struct refspec_item spec; - struct string_list *srcs; -+ struct string_list *remotes; - const char *remote; +@@ branch.c: 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) { @@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } -+ string_list_append(tracking->remotes, remote->name); ++ strbuf_addf(&ftb->remotes_advice, " %s\n", remote->name); tracking->spec.src = NULL; } @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori + +static const char ambiguous_refspec_advice_pre[] = +N_("\n" -+"There are multiple remotes with fetch refspecs mapping to\n" -+"the tracking ref %s:\n";) ++"There are multiple remotes whose fetch refspecs map to the remote\n" ++"tracking ref";) +static const char ambiguous_refspec_advice_post[] = +N_("This is typically a configuration error.\n" +"\n" @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori * 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 @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, - { struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; -+ struct string_list tracking_remotes = STRING_LIST_INIT_DUP; int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; -+ struct string_list_item *item; ++ 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; -+ tracking.remotes = &tracking_remotes; if (track != BRANCH_TRACK_INHERIT) - for_each_remote(find_tracked_branch, &tracking); +- for_each_remote(find_tracked_branch, &tracking); ++ for_each_remote(find_tracked_branch, &ftb_cb); else if (inherit_tracking(&tracking, orig_ref)) + return; + @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, return; } - if (tracking.matches > 1) +- die(_("not tracking: ambiguous information for ref %s"), +- orig_ref); + if (tracking.matches > 1) { -+ if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) { -+ advise(_(ambiguous_refspec_advice_pre), orig_ref); -+ for_each_string_list_item(item, &tracking_remotes) { -+ advise(" %s", item->string); -+ } -+ advise(_(ambiguous_refspec_advice_post)); -+ } - die(_("not tracking: ambiguous information for ref %s"), - orig_ref); ++ int status = die_message(_("not tracking: ambiguous information for ref %s"), ++ orig_ref); ++ if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) ++ advise("%s %s:\n%s\n%s", ++ _(ambiguous_refspec_advice_pre), ++ orig_ref, ++ ftb_cb.remotes_advice.buf, ++ _(ambiguous_refspec_advice_post) ++ ); ++ exit(status); + } if (tracking.srcs->nr < 1) Documentation/config/advice.txt | 4 ++++ advice.c | 1 + advice.h | 1 + branch.c | 42 +++++++++++++++++++++++++++++---- 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 063eec2511d..abfac4f664b 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -126,4 +126,8 @@ advice.*:: Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] is asked to update index entries outside the current sparse checkout. + ambiguousFetchRefspec:: + Advice shown when branch tracking relationship setup fails due + to multiple remotes' refspecs mapping to the same remote + tracking namespace in the repo. -- diff --git a/advice.c b/advice.c index 1dfc91d1767..686612590ec 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 601265fd107..3d68c1a6cb4 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 5d20a2e8484..4a3a77aa338 100644 --- a/branch.c +++ b/branch.c @@ -16,9 +16,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) { @@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } + strbuf_addf(&ftb->remotes_advice, " %s\n", remote->name); tracking->spec.src = NULL; } @@ -217,6 +224,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) return 0; } + +static const char ambiguous_refspec_advice_pre[] = +N_("\n" +"There are multiple remotes whose fetch refspecs map to the remote\n" +"tracking ref";) +static const char ambiguous_refspec_advice_post[] = +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.\n"); + /* * 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 @@ -228,12 +247,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)) return; @@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, return; } - 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("%s %s:\n%s\n%s", + _(ambiguous_refspec_advice_pre), + orig_ref, + ftb_cb.remotes_advice.buf, + _(ambiguous_refspec_advice_post) + ); + exit(status); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2] RFC: tracking branches: add advice to ambiguous refspec error 2022-03-22 9:18 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget @ 2022-03-22 10:04 ` Ævar Arnfjörð Bjarmason 2022-03-28 6:51 ` [PATCH v3] " Tao Klerks via GitGitGadget 1 sibling, 0 replies; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-22 10:04 UTC (permalink / raw) To: Tao Klerks via GitGitGadget; +Cc: git, Tao Klerks On Tue, Mar 22 2022, Tao Klerks via GitGitGadget wrote: > From: Tao Klerks <tao@klerks.biz> > [...] Looking much better! > diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt > index 063eec2511d..abfac4f664b 100644 > --- a/Documentation/config/advice.txt > +++ b/Documentation/config/advice.txt > @@ -126,4 +126,8 @@ advice.*:: > Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] > is asked to update index entries outside the current sparse > checkout. > + ambiguousFetchRefspec:: > + Advice shown when branch tracking relationship setup fails due > + to multiple remotes' refspecs mapping to the same remote > + tracking namespace in the repo. Let's add this in alphabetical section order. It's *somewhat* true of the order now, but for some, but in any case I've got a WIP patch to sort these, so since it's new adding it at the top would save us the churn :) > +struct find_tracked_branch_cb { > + struct tracking *tracking; > + struct strbuf remotes_advice; > +}; Nice, thanks! > 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) { > @@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv) > free(tracking->spec.src); > string_list_clear(tracking->srcs, 0); > } > + strbuf_addf(&ftb->remotes_advice, " %s\n", remote->name); Do this as: strbuf_addf([...], _(" %s\n"), [...]); And add a TRANSLATORS comment similar to show_ambiguous_object() in object-name.c. I.e. the alignment needs to be translatable for RTL languages. > +static const char ambiguous_refspec_advice_pre[] = > +N_("\n" > +"There are multiple remotes whose fetch refspecs map to the remote\n" > +"tracking ref";) > +static const char ambiguous_refspec_advice_post[] = > +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.\n"); These were split up before since we incrementally built the message.... Also, is the \n at the beginning/end really needed? > /* > * 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 > @@ -228,12 +247,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)) > return; > > @@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, > return; > } > > - 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("%s %s:\n%s\n%s", > + _(ambiguous_refspec_advice_pre), > + orig_ref, > + ftb_cb.remotes_advice.buf, > + _(ambiguous_refspec_advice_post) > + ); ...but now we don't, so this should just be inlined here. I.e. made it one big translatable _() message, the only parameters need to be the orig_ref and the "remote_advice" buf. We're also missing a strbuf_release() here. You can just add it to the "cleanup" omitted from the context: strbuf_release(&ftb_cb.remotes_advice); ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-22 9:18 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget 2022-03-22 10:04 ` Ævar Arnfjörð Bjarmason @ 2022-03-28 6:51 ` Tao Klerks via GitGitGadget 2022-03-28 16:23 ` Junio C Hamano 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget 1 sibling, 2 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-28 6:51 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- tracking branches: add advice to ambiguous refspec error I believe this third version incorporates all Ævar's suggestions, and might be usable. Removed "RFC" prefix. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v2: 1: 6c1cd885dda ! 1: 22ffe81ac26 RFC: tracking branches: add advice to ambiguous refspec error @@ Metadata Author: Tao Klerks <tao@klerks.biz> ## Commit message ## - RFC: tracking branches: add advice to ambiguous refspec error + tracking branches: add advice to ambiguous refspec error The error "not tracking: ambiguous information for ref" is raised when we are evaluating what tracking information to set on a branch, @@ Commit message ## Documentation/config/advice.txt ## @@ Documentation/config/advice.txt: advice.*:: - Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] - is asked to update index entries outside the current sparse - checkout. + 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 ## advice.c ## @@ advice.c: static struct { @@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv) free(tracking->spec.src); string_list_clear(tracking->srcs, 0); } -+ strbuf_addf(&ftb->remotes_advice, " %s\n", remote->name); ++ /* ++ * 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; } @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori return 0; } -+ -+static const char ambiguous_refspec_advice_pre[] = -+N_("\n" -+"There are multiple remotes whose fetch refspecs map to the remote\n" -+"tracking ref";) -+static const char ambiguous_refspec_advice_post[] = -+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.\n"); + /* - * 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 + * Used internally to set the branch.<new_ref>.{remote,merge} config + * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, - for_each_remote(find_tracked_branch, &tracking); + for_each_remote(find_tracked_branch, &ftb_cb); else if (inherit_tracking(&tracking, orig_ref)) - return; + goto cleanup; @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, - return; + goto cleanup; } - if (tracking.matches > 1) @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, + int status = die_message(_("not tracking: ambiguous information for ref %s"), + orig_ref); + if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) -+ advise("%s %s:\n%s\n%s", -+ _(ambiguous_refspec_advice_pre), ++ 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, -+ _(ambiguous_refspec_advice_post) ++ ftb_cb.remotes_advice.buf + ); + exit(status); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); +@@ branch.c: 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); + } + 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. 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); } base-commit: abf474a5dd901f28013c52155411a48fd4c09922 -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 6:51 ` [PATCH v3] " Tao Klerks via GitGitGadget @ 2022-03-28 16:23 ` Junio C Hamano 2022-03-28 17:12 ` Glen Choo 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget 1 sibling, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-28 16:23 UTC (permalink / raw) To: Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks "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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 16:23 ` Junio C Hamano @ 2022-03-28 17:12 ` Glen Choo 2022-03-28 17:23 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Glen Choo @ 2022-03-28 17:12 UTC (permalink / raw) To: Junio C Hamano, Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 17:12 ` Glen Choo @ 2022-03-28 17:23 ` Junio C Hamano 2022-03-28 18:02 ` Tao Klerks 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-28 17:23 UTC (permalink / raw) To: Glen Choo Cc: Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason, Tao Klerks 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 17:23 ` Junio C Hamano @ 2022-03-28 18:02 ` Tao Klerks 2022-03-28 18:50 ` Ævar Arnfjörð Bjarmason 2022-03-28 20:27 ` Junio C Hamano 0 siblings, 2 replies; 36+ messages in thread From: Tao Klerks @ 2022-03-28 18:02 UTC (permalink / raw) To: Junio C Hamano Cc: Glen Choo, Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason 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? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 18:02 ` Tao Klerks @ 2022-03-28 18:50 ` Ævar Arnfjörð Bjarmason 2022-03-28 20:32 ` Junio C Hamano 2022-03-28 20:27 ` Junio C Hamano 1 sibling, 1 reply; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-28 18:50 UTC (permalink / raw) To: Tao Klerks; +Cc: Junio C Hamano, Glen Choo, Tao Klerks via GitGitGadget, git 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" ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 18:50 ` Ævar Arnfjörð Bjarmason @ 2022-03-28 20:32 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2022-03-28 20:32 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Tao Klerks, Glen Choo, Tao Klerks via GitGitGadget, git Æ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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 18:02 ` Tao Klerks 2022-03-28 18:50 ` Ævar Arnfjörð Bjarmason @ 2022-03-28 20:27 ` Junio C Hamano 2022-03-29 11:26 ` Tao Klerks 1 sibling, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-28 20:27 UTC (permalink / raw) To: Tao Klerks Cc: Glen Choo, Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason 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). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error 2022-03-28 20:27 ` Junio C Hamano @ 2022-03-29 11:26 ` Tao Klerks 0 siblings, 0 replies; 36+ messages in thread From: Tao Klerks @ 2022-03-29 11:26 UTC (permalink / raw) To: Junio C Hamano Cc: Glen Choo, Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v4] tracking branches: add advice to ambiguous refspec error 2022-03-28 6:51 ` [PATCH v3] " Tao Klerks via GitGitGadget 2022-03-28 16:23 ` Junio C Hamano @ 2022-03-29 11:26 ` Tao Klerks via GitGitGadget 2022-03-29 11:31 ` Tao Klerks ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-29 11:26 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo, Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- tracking branches: add advice to ambiguous refspec error I believe this third version incorporates all Ævar's suggestions, and might be usable. Removed "RFC" prefix. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v3: 1: 22ffe81ac26 = 1: ac6c782f566 tracking branches: add advice to ambiguous refspec error 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. 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); } base-commit: abf474a5dd901f28013c52155411a48fd4c09922 -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget @ 2022-03-29 11:31 ` Tao Klerks 2022-03-29 15:49 ` Junio C Hamano 2022-03-30 7:20 ` [PATCH v5] " Tao Klerks via GitGitGadget 2 siblings, 0 replies; 36+ messages in thread From: Tao Klerks @ 2022-03-29 11:31 UTC (permalink / raw) To: Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Glen Choo On Tue, Mar 29, 2022 at 1:26 PM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote: > > I believe this third version incorporates all Ævar's suggestions, and > might be usable. Removed "RFC" prefix. Argh, sorry about the broken cover letter. This *fourth* version attempts to address Junio's concerns over unnecessary extra error-preparation work being done in the very-vastly-more-common "normal" case. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget 2022-03-29 11:31 ` Tao Klerks @ 2022-03-29 15:49 ` Junio C Hamano 2022-03-30 4:17 ` Tao Klerks 2022-03-30 7:20 ` [PATCH v5] " Tao Klerks via GitGitGadget 2 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-29 15:49 UTC (permalink / raw) To: Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo "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. > > Signed-off-by: Tao Klerks <tao@klerks.biz> > --- > tracking branches: add advice to ambiguous refspec error > > I believe this third version incorporates all Ævar's suggestions, and > might be usable. Removed "RFC" prefix. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4 > Pull-Request: https://github.com/gitgitgadget/git/pull/1183 > > Range-diff vs v3: > > 1: 22ffe81ac26 = 1: ac6c782f566 tracking branches: add advice to ambiguous refspec error > > > Documentation/config/advice.txt | 4 +++ > advice.c | 1 + > advice.h | 1 + > branch.c | 44 +++++++++++++++++++++++++++++---- > 4 files changed, 45 insertions(+), 5 deletions(-) Sent a wrong version? The patch text seems to be identical to that of v3 message archived at https://lore.kernel.org/git/pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error 2022-03-29 15:49 ` Junio C Hamano @ 2022-03-30 4:17 ` Tao Klerks 0 siblings, 0 replies; 36+ messages in thread From: Tao Klerks @ 2022-03-30 4:17 UTC (permalink / raw) To: Junio C Hamano Cc: Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason, Glen Choo On Tue, Mar 29, 2022 at 5:49 PM Junio C Hamano <gitster@pobox.com> wrote: [...] > > Sent a wrong version? [...] Yes indeed. My apologies once again. V5 coming as soon as the test run completes. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget 2022-03-29 11:31 ` Tao Klerks 2022-03-29 15:49 ` Junio C Hamano @ 2022-03-30 7:20 ` Tao Klerks via GitGitGadget 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget 2 siblings, 2 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-30 7:20 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo, Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- tracking branches: add advice to ambiguous refspec error This fifth version attempts to address Junio's concerns over unnecessary extra error-preparation work being done in the very-vastly-more-common "normal" case. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v5 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v4: 1: ac6c782f566 ! 1: 4478eaed6df tracking branches: add advice to ambiguous refspec error @@ Documentation/config/advice.txt: advice.*:: + -- + 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. fetchShowForcedUpdates:: Advice shown when linkgit:git-fetch[1] takes a long time to calculate forced updates after ref updates, or to warn @@ branch.c: struct tracking { +struct find_tracked_branch_cb { + struct tracking *tracking; -+ struct strbuf remotes_advice; ++ struct string_list ambiguous_remotes; +}; + static int find_tracked_branch(struct remote *remote, void *priv) @@ branch.c: struct tracking { + struct tracking *tracking = ftb->tracking; if (!remote_find_tracking(remote, &tracking->spec)) { - if (++tracking->matches == 1) { -@@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv) +- 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; } -+ /* -+ * 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; } - @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref) return 0; } @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; + struct find_tracked_branch_cb ftb_cb = { + .tracking = &tracking, -+ .remotes_advice = STRBUF_INIT, ++ .ambiguous_remotes = STRING_LIST_INIT_DUP, + }; memset(&tracking, 0, sizeof(tracking)); @@ branch.c: 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 (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); ++ } ++ + advise(_("There are multiple remotes whose fetch refspecs map to the remote\n" + "tracking ref %s:\n" + "%s" @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, + "different remotes' fetch refspecs map into different\n" + "tracking namespaces."), + orig_ref, -+ ftb_cb.remotes_advice.buf ++ remotes_advice.buf + ); ++ strbuf_release(&remotes_advice); ++ } + exit(status); + } if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); @@ branch.c: 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); ++ string_list_clear(&ftb_cb.ambiguous_remotes, 0); } + int read_branch_desc(struct strbuf *buf, const char *branch_name) Documentation/config/advice.txt | 4 +++ advice.c | 1 + advice.h | 1 + branch.c | 63 +++++++++++++++++++++++++++++---- 4 files changed, 62 insertions(+), 7 deletions(-) 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..e6ab50fff41 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; } @@ -219,6 +233,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 +247,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 +271,38 @@ 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); + } + + 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) base-commit: abf474a5dd901f28013c52155411a48fd4c09922 -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 7:20 ` [PATCH v5] " Tao Klerks via GitGitGadget @ 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason 2022-03-30 14:23 ` Tao Klerks 2022-03-30 20:37 ` Junio C Hamano 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget 1 sibling, 2 replies; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-30 13:19 UTC (permalink / raw) To: Tao Klerks via GitGitGadget; +Cc: git, Glen Choo, Tao Klerks On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: > From: Tao Klerks <tao@klerks.biz> > + case 2: > + // there are at least two remotes; backfill the first one Nit: I think it's been Junio's preference not to introduce C99 comments, despite other C99 features now being used (and I think it should work in practice as far as portability goes, see https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/) > @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) > return 0; > } > > + Stray whitespace? > /* > * Used internally to set the branch.<new_ref>.{remote,merge} config > * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike > @@ -232,12 +247,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 +271,38 @@ 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); This isn't per-se new, but I wonder if while we're at it we shold just quote '%s' here, which we'd usually do. I.e. this message isn't new, but referring again to "ref %s" (and not "ref '%s'") below is. > + 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) { Nit: drop braces, not needed. > + /* > + * 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); > + } > + Per the TRANSLATORS comments in get_short_oid(), it's also good to have one here in front of advice(). E.g.: /* * 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 > + ); Nit: The usual style for multi-line arguments is to "fill" lines until you're at 79 characters, so these last three lines (including the ");") can all go on the "tracking namespaces" line (until they're at 79, then wrap)> > + strbuf_release(&remotes_advice); > + } > + exit(status); > + } Other than the minor nits noted above this version looks good to me, and I think addresses both the translation concerns I brought up, and the "let's not do advice() work if we don't need it" concern by Junio et al. > 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) > > base-commit: abf474a5dd901f28013c52155411a48fd4c09922 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason @ 2022-03-30 14:23 ` Tao Klerks 2022-03-30 15:18 ` Tao Klerks 2022-03-30 20:37 ` Junio C Hamano 1 sibling, 1 reply; 36+ messages in thread From: Tao Klerks @ 2022-03-30 14:23 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Tao Klerks via GitGitGadget, git, Glen Choo On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: > > Nit: I think it's been Junio's preference not to introduce C99 comments, Argh, my apologies, bad habits. I wonder whether anyone has any tooling to help catch this kind of thing - I'll ask in GitGitGadget, anyway. > > @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref) > > return 0; > > } > > > > + > > Stray whitespace? Thx, I will check more carefully next time. > > > /* > > * Used internally to set the branch.<new_ref>.{remote,merge} config > > * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike > > @@ -232,12 +247,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 +271,38 @@ 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); > > This isn't per-se new, but I wonder if while we're at it we shold just > quote '%s' here, which we'd usually do. I.e. this message isn't new, but > referring again to "ref %s" (and not "ref '%s'") below is. > I will fix the below, but I would default to not changing the above unless someone thinks we should (not sure what the expectations are around changing error messages unnecessarily) > > + 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) { > > Nit: drop braces, not needed. Thx, will do. > > > + /* > > + * 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); > > + } > > + > > Per the TRANSLATORS comments in get_short_oid(), it's also good to have > one here in front of advice(). E.g.: > > /* > * TRANSLATORS: The second argument is a \n-delimited list of > * duplicate refspecs, composed above. > */ > Will do, thx. > > + 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 > > + ); > > Nit: The usual style for multi-line arguments is to "fill" lines until > you're at 79 characters, so these last three lines (including the ");") > can all go on the "tracking namespaces" line (until they're at 79, then > wrap)> > Yep, another oversight, thx. > > + strbuf_release(&remotes_advice); > > + } > > + exit(status); > > + } > > Other than the minor nits noted above this version looks good to me, and > I think addresses both the translation concerns I brought up, and the > "let's not do advice() work if we don't need it" concern by Junio et al. > Awesome, thx! ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 14:23 ` Tao Klerks @ 2022-03-30 15:18 ` Tao Klerks 2022-03-30 17:14 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 36+ messages in thread From: Tao Klerks @ 2022-03-30 15:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Tao Klerks via GitGitGadget, git, Glen Choo On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote: > > On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > > > > > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: > > > > > + if (tracking.matches > 1) { > > > + int status = die_message(_("not tracking: ambiguous information for ref %s"), > > > + orig_ref); > > > > This isn't per-se new, but I wonder if while we're at it we shold just > > quote '%s' here, which we'd usually do. I.e. this message isn't new, but > > referring again to "ref %s" (and not "ref '%s'") below is. > > > > I will fix the below, but I would default to not changing the above > unless someone thinks we should (not sure what the expectations are > around changing error messages unnecessarily) I take this back. I will update both - the change is in a "variable" part of the message anyway, and it's hard to imagine any tool actively/purposefully parsing the ref out of the message and being caught out by the new quotes. Any coordinating tool would already know what ref was being branched / having its tracking remote info updated. > > > > + * 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); > > > + } > > > + > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 15:18 ` Tao Klerks @ 2022-03-30 17:14 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-30 17:14 UTC (permalink / raw) To: Tao Klerks; +Cc: Tao Klerks via GitGitGadget, git, Glen Choo On Wed, Mar 30 2022, Tao Klerks wrote: > On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote: >> >> On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason >> <avarab@gmail.com> wrote: >> > >> > >> > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: >> > >> > > + if (tracking.matches > 1) { >> > > + int status = die_message(_("not tracking: ambiguous information for ref %s"), >> > > + orig_ref); >> > >> > This isn't per-se new, but I wonder if while we're at it we shold just >> > quote '%s' here, which we'd usually do. I.e. this message isn't new, but >> > referring again to "ref %s" (and not "ref '%s'") below is. >> > >> >> I will fix the below, but I would default to not changing the above >> unless someone thinks we should (not sure what the expectations are >> around changing error messages unnecessarily) > > I take this back. I will update both - the change is in a "variable" > part of the message anyway, and it's hard to imagine any tool > actively/purposefully parsing the ref out of the message and being > caught out by the new quotes. Any coordinating tool would already know > what ref was being branched / having its tracking remote info updated. Thanks, I'd be fine either way, it was just a suggestion. Aside from what we do here we don't support third-party tooling that's grepping our specific human-readable output, so changing any such messaging is OK. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason 2022-03-30 14:23 ` Tao Klerks @ 2022-03-30 20:37 ` Junio C Hamano 2022-03-30 21:09 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-30 20:37 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Tao Klerks via GitGitGadget, git, Glen Choo, Tao Klerks Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: > >> From: Tao Klerks <tao@klerks.biz> > >> + case 2: >> + // there are at least two remotes; backfill the first one > > Nit: I think it's been Junio's preference not to introduce C99 comments, I often mention the rule to new contributors, simply because it has been that way in our code base, regardless of what my personal preference might be, and sticking to the style will be more consistent. It's not like I am forcing my personal preference on developers. Do not mislead new people into thinking so. It is especially irritating to see ... > despite other C99 features now being used (and I think it should work in > practice as far as portability goes, see > https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/) ... a mention like this, when you know that it is not about portability but is about consistency, and also you know I've mentioned more than once on the list if we want to loosen some written CodingGuidelines rules, especially those that tools do not necessarily catch. >> + if (tracking.matches > 1) { >> + int status = die_message(_("not tracking: ambiguous information for ref %s"), >> + orig_ref); > > This isn't per-se new, but I wonder if while we're at it we shold just > quote '%s' here, which we'd usually do. I.e. this message isn't new, but > referring again to "ref %s" (and not "ref '%s'") below is. Good. >> + "To support setting up tracking branches, ensure that\n" >> + "different remotes' fetch refspecs map into different\n" >> + "tracking namespaces."), >> + orig_ref, >> + remotes_advice.buf >> + ); > > Nit: The usual style for multi-line arguments is to "fill" lines until > you're at 79 characters, so these last three lines (including the ");") > can all go on the "tracking namespaces" line (until they're at 79, then > wrap)> I didn't know about the magic "79" number. It makes the resulting source code extremely hard to read, though, while making it easier to grep for specific messages. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 20:37 ` Junio C Hamano @ 2022-03-30 21:09 ` Ævar Arnfjörð Bjarmason 2022-03-30 22:07 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-30 21:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tao Klerks via GitGitGadget, git, Glen Choo, Tao Klerks On Wed, Mar 30 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote: >> >>> From: Tao Klerks <tao@klerks.biz> >> >>> + case 2: >>> + // there are at least two remotes; backfill the first one >> >> Nit: I think it's been Junio's preference not to introduce C99 comments, > > I often mention the rule to new contributors, simply because it has > been that way in our code base, regardless of what my personal > preference might be, and sticking to the style will be more > consistent. It's not like I am forcing my personal preference on > developers. Do not mislead new people into thinking so. It is > especially irritating to see ... > >> despite other C99 features now being used (and I think it should work in >> practice as far as portability goes, see >> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/) > > ... a mention like this, when you know that it is not about > portability but is about consistency, and also you know I've > mentioned more than once on the list if we want to loosen some > written CodingGuidelines rules, especially those that tools do not > necessarily catch. I know, and I didn't mean to imply that you were standing in the way of C99 progress or anything like that. Personally, I much prefer not to have //-style comments introduced. I merely mentioned the portability concern because I thought it helped give a relatively new contributor some context. I.e. that despite any new developments on the C99 front they might discover this particular thing is stylistic and not about portability (although that may also have been a reason in the past). >>> + if (tracking.matches > 1) { >>> + int status = die_message(_("not tracking: ambiguous information for ref %s"), >>> + orig_ref); >> >> This isn't per-se new, but I wonder if while we're at it we shold just >> quote '%s' here, which we'd usually do. I.e. this message isn't new, but >> referring again to "ref %s" (and not "ref '%s'") below is. > > Good. > >>> + "To support setting up tracking branches, ensure that\n" >>> + "different remotes' fetch refspecs map into different\n" >>> + "tracking namespaces."), >>> + orig_ref, >>> + remotes_advice.buf >>> + ); >> >> Nit: The usual style for multi-line arguments is to "fill" lines until >> you're at 79 characters, so these last three lines (including the ");") >> can all go on the "tracking namespaces" line (until they're at 79, then >> wrap)> > > I didn't know about the magic "79" number. It makes the resulting > source code extremely hard to read, though, while making it easier > to grep for specific messages. I'm referring to the "80 characters per line", but omitted the \n, but yeah, I should have just said 80. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 21:09 ` Ævar Arnfjörð Bjarmason @ 2022-03-30 22:07 ` Junio C Hamano 2022-03-30 22:51 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-30 22:07 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Tao Klerks via GitGitGadget, git, Glen Choo, Tao Klerks Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>>> + "To support setting up tracking branches, ensure that\n" >>>> + "different remotes' fetch refspecs map into different\n" >>>> + "tracking namespaces."), >>>> + orig_ref, >>>> + remotes_advice.buf >>>> + ); >>> >>> Nit: The usual style for multi-line arguments is to "fill" lines until >>> you're at 79 characters, so these last three lines (including the ");") >>> can all go on the "tracking namespaces" line (until they're at 79, then >>> wrap)> >> >> I didn't know about the magic "79" number. It makes the resulting >> source code extremely hard to read, though, while making it easier >> to grep for specific messages. > > I'm referring to the "80 characters per line", but omitted the \n, but > yeah, I should have just said 80. No, what I meant was that you do not want the rule to be to cut *AT* exactly the column whatever random rule specifies, which would result in funny wrapping in the middle of the word, e.g. "To support setting up tracking branches, ensure that diff" "erent remotes' fetch refspecs map into different tracking" " namespaces." and "at 79, then wrap" somehow sounded to me like that. I do not think you meant to imply that (instead, I think you meant to suggest "wrap the line so that the string constant is not longer than 79 columns"), but it risks to be mistaken by new contributors. FWIW, I'd actually prefer to see both the sources to be readable by wrapping to keep the source code line length under certain limit (the current guideline being 70-something), counting the leading indentation, and at the same time keep it possible and easy to grep messages in the source. That requires us to notice when our code has too deeply nested, resulting in overly indented lines, and maintain the readability (refatoring the code may be a way to help in such cases). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error 2022-03-30 22:07 ` Junio C Hamano @ 2022-03-30 22:51 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 36+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-30 22:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tao Klerks via GitGitGadget, git, Glen Choo, Tao Klerks On Wed, Mar 30 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>>>> + "To support setting up tracking branches, ensure that\n" >>>>> + "different remotes' fetch refspecs map into different\n" >>>>> + "tracking namespaces."), >>>>> + orig_ref, >>>>> + remotes_advice.buf >>>>> + ); >>>> >>>> Nit: The usual style for multi-line arguments is to "fill" lines until >>>> you're at 79 characters, so these last three lines (including the ");") >>>> can all go on the "tracking namespaces" line (until they're at 79, then >>>> wrap)> >>> >>> I didn't know about the magic "79" number. It makes the resulting >>> source code extremely hard to read, though, while making it easier >>> to grep for specific messages. >> >> I'm referring to the "80 characters per line", but omitted the \n, but >> yeah, I should have just said 80. > > No, what I meant was that you do not want the rule to be to cut *AT* > exactly the column whatever random rule specifies, which would > result in funny wrapping in the middle of the word, e.g. > > "To support setting up tracking branches, ensure that diff" > "erent remotes' fetch refspecs map into different tracking" > " namespaces." > > and "at 79, then wrap" somehow sounded to me like that. I do not > think you meant to imply that (instead, I think you meant to suggest > "wrap the line so that the string constant is not longer than 79 > columns"), but it risks to be mistaken by new contributors. > > FWIW, I'd actually prefer to see both the sources to be readable by > wrapping to keep the source code line length under certain limit > (the current guideline being 70-something), counting the leading > indentation, and at the same time keep it possible and easy to grep > messages in the source. > > That requires us to notice when our code has too deeply nested, > resulting in overly indented lines, and maintain the readability > (refatoring the code may be a way to help in such cases). Yes, I didn't mean to say it was a hard rule. In particular as this code has the strings themselves over 80 characters. It can be good to break that guideline when it doesn't help readability. I just meant that this made sense as a fix-up, in this case: diff --git a/branch.c b/branch.c index 5c28d432103..4ccf5f79e83 100644 --- a/branch.c +++ b/branch.c @@ -282,10 +282,8 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, "\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 - ); + "tracking namespaces."), orig_ref, + ftb_cb.remotes_advice.buf); exit(status); } Which I'd also be tempted to do as: diff --git a/branch.c b/branch.c index 5c28d432103..b9f6fda980b 100644 --- a/branch.c +++ b/branch.c @@ -283,9 +283,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, "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 - ); + orig_ref, ftb_cb.remotes_advice.buf); exit(status); } But I find it generally helpful to do it consistently when possible, as when running into merge conflicts it makes things easier. ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-03-30 7:20 ` [PATCH v5] " Tao Klerks via GitGitGadget 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason @ 2022-03-31 16:01 ` Tao Klerks via GitGitGadget 2022-03-31 19:32 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-03-31 16:01 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo, Tao Klerks, Tao Klerks 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. Signed-off-by: Tao Klerks <tao@klerks.biz> --- tracking branches: add advice to ambiguous refspec error v6 addresses some formatting and commenting issues Ævar noticed. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v6 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v6 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v5: 1: 4478eaed6df ! 1: 2408ab0ccb3 tracking branches: add advice to ambiguous refspec error @@ branch.c: struct tracking { - } else { + break; + case 2: -+ // there are at least two remotes; backfill the first one ++ /* there are at least two remotes; backfill the first one */ + string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); + /* fall through */ + default: @@ branch.c: struct tracking { } tracking->spec.src = NULL; } -@@ branch.c: 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 @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, struct tracking tracking; struct string_list tracking_srcs = STRING_LIST_INIT_DUP; @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, - 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"), ++ 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) { ++ 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" ++ "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 -+ ); ++ "tracking namespaces."), orig_ref, ++ remotes_advice.buf); + strbuf_release(&remotes_advice); + } + exit(status); Documentation/config/advice.txt | 4 +++ advice.c | 1 + advice.h | 1 + branch.c | 63 +++++++++++++++++++++++++++++---- 4 files changed, 62 insertions(+), 7 deletions(-) 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) base-commit: abf474a5dd901f28013c52155411a48fd4c09922 -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget @ 2022-03-31 19:32 ` Junio C Hamano 2022-03-31 23:57 ` Glen Choo 2022-03-31 19:33 ` Junio C Hamano 2022-04-01 6:05 ` [PATCH v7] " Tao Klerks via GitGitGadget 2 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2022-03-31 19:32 UTC (permalink / raw) To: Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo "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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-03-31 19:32 ` Junio C Hamano @ 2022-03-31 23:57 ` Glen Choo 2022-04-01 4:30 ` Tao Klerks 0 siblings, 1 reply; 36+ messages in thread From: Glen Choo @ 2022-03-31 23:57 UTC (permalink / raw) To: Junio C Hamano, Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-03-31 23:57 ` Glen Choo @ 2022-04-01 4:30 ` Tao Klerks 2022-04-01 16:41 ` Glen Choo 0 siblings, 1 reply; 36+ messages in thread From: Tao Klerks @ 2022-04-01 4:30 UTC (permalink / raw) To: Glen Choo Cc: Junio C Hamano, Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-04-01 4:30 ` Tao Klerks @ 2022-04-01 16:41 ` Glen Choo 0 siblings, 0 replies; 36+ messages in thread From: Glen Choo @ 2022-04-01 16:41 UTC (permalink / raw) To: Tao Klerks Cc: Junio C Hamano, Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget 2022-03-31 19:32 ` Junio C Hamano @ 2022-03-31 19:33 ` Junio C Hamano 2022-04-01 6:05 ` [PATCH v7] " Tao Klerks via GitGitGadget 2 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2022-03-31 19:33 UTC (permalink / raw) To: Tao Klerks via GitGitGadget Cc: git, Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo "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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v7] tracking branches: add advice to ambiguous refspec error 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget 2022-03-31 19:32 ` Junio C Hamano 2022-03-31 19:33 ` Junio C Hamano @ 2022-04-01 6:05 ` Tao Klerks via GitGitGadget 2022-04-01 16:53 ` Glen Choo 2 siblings, 1 reply; 36+ messages in thread From: Tao Klerks via GitGitGadget @ 2022-04-01 6:05 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks, Glen Choo, Tao Klerks, Tao Klerks 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. Also update a test to explicitly expect that advice. Signed-off-by: Tao Klerks <tao@klerks.biz> --- tracking branches: add advice to ambiguous refspec error In v7 address a wrong-value-printed bug that Junio and Glen noticed, and add testing for the emitted advice. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v7 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v7 Pull-Request: https://github.com/gitgitgadget/git/pull/1183 Range-diff vs v6: 1: 2408ab0ccb3 ! 1: 4fc5d2b6d13 tracking branches: add advice to ambiguous refspec error @@ Commit message 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. + are involved and suggesting how to correct the situation. Also + update a test to explicitly expect that advice. Signed-off-by: Tao Klerks <tao@klerks.biz> @@ branch.c: struct tracking { + break; + case 2: + /* there are at least two remotes; backfill the first one */ -+ string_list_append(&ftb->ambiguous_remotes, tracking->spec.src); ++ string_list_append(&ftb->ambiguous_remotes, tracking->remote); + /* fall through */ + default: + string_list_append(&ftb->ambiguous_remotes, remote->name); @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, } int read_branch_desc(struct strbuf *buf, const char *branch_name) + + ## t/t3200-branch.sh ## +@@ t/t3200-branch.sh: test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates + git rev-parse --verify gamma@{0} + ' + +-test_expect_success 'avoid ambiguous track' ' ++test_expect_success 'avoid ambiguous track and advise' ' + git config branch.autosetupmerge true && + git config remote.ambi1.url lalala && + git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main && + git config remote.ambi2.url lilili && + git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main && +- test_must_fail git branch all1 main && ++ cat <<-EOF >expected && ++ fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\'' ++ hint: There are multiple remotes whose fetch refspecs map to the remote ++ hint: tracking ref '\''refs/heads/main'\'': ++ hint: ambi1 ++ hint: ambi2 ++ hint: '' ++ hint: This is typically a configuration error. ++ hint: '' ++ hint: To support setting up tracking branches, ensure that ++ hint: different remotes'\'' fetch refspecs map into different ++ hint: tracking namespaces. ++ EOF ++ test_must_fail git branch all1 main 2>actual && ++ test_cmp expected actual && + test -z "$(git config branch.all1.merge)" + ' + Documentation/config/advice.txt | 4 +++ advice.c | 1 + advice.h | 1 + branch.c | 63 +++++++++++++++++++++++++++++---- t/t3200-branch.sh | 18 ++++++++-- 5 files changed, 78 insertions(+), 9 deletions(-) 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..182f1c5a556 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->remote); + /* 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) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 7a0ff75ba86..e12db593615 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1039,13 +1039,27 @@ test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates git rev-parse --verify gamma@{0} ' -test_expect_success 'avoid ambiguous track' ' +test_expect_success 'avoid ambiguous track and advise' ' git config branch.autosetupmerge true && git config remote.ambi1.url lalala && git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main && git config remote.ambi2.url lilili && git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main && - test_must_fail git branch all1 main && + cat <<-EOF >expected && + fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\'' + hint: There are multiple remotes whose fetch refspecs map to the remote + hint: tracking ref '\''refs/heads/main'\'': + hint: ambi1 + hint: ambi2 + hint: '' + hint: This is typically a configuration error. + hint: '' + hint: To support setting up tracking branches, ensure that + hint: different remotes'\'' fetch refspecs map into different + hint: tracking namespaces. + EOF + test_must_fail git branch all1 main 2>actual && + test_cmp expected actual && test -z "$(git config branch.all1.merge)" ' base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b -- gitgitgadget ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v7] tracking branches: add advice to ambiguous refspec error 2022-04-01 6:05 ` [PATCH v7] " Tao Klerks via GitGitGadget @ 2022-04-01 16:53 ` Glen Choo 2022-04-01 19:57 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Glen Choo @ 2022-04-01 16:53 UTC (permalink / raw) To: Tao Klerks via GitGitGadget, git Cc: Ævar Arnfjörð Bjarmason, Tao Klerks "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/branch.c b/branch.c > index 6b31df539a5..182f1c5a556 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->remote); > + /* 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; > } Ah I see, on the first iteration, we set the first remote's name to tracking->remote, and on the second iteration, we copy that value to the list before copying the _current_ remote's name to the list. > -test_expect_success 'avoid ambiguous track' ' > +test_expect_success 'avoid ambiguous track and advise' ' > git config branch.autosetupmerge true && > git config remote.ambi1.url lalala && > git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main && > git config remote.ambi2.url lilili && > git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main && > - test_must_fail git branch all1 main && > + cat <<-EOF >expected && > + fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\'' > + hint: There are multiple remotes whose fetch refspecs map to the remote > + hint: tracking ref '\''refs/heads/main'\'': > + hint: ambi1 > + hint: ambi2 > + hint: '' > + hint: This is typically a configuration error. > + hint: '' > + hint: To support setting up tracking branches, ensure that > + hint: different remotes'\'' fetch refspecs map into different > + hint: tracking namespaces. > + EOF > + test_must_fail git branch all1 main 2>actual && > + test_cmp expected actual && > test -z "$(git config branch.all1.merge)" > ' And this test shows that this indeed does what we think it does. Nicely done. I notice that we there is an instance of test -z "$(some git command)", which IIRC is discouraged because it would mask a failure in the git command, but that's not new and I don't think it needs to be fixed in this series anyway. Reviewed-by: Glen Choo <chooglen@google.com> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v7] tracking branches: add advice to ambiguous refspec error 2022-04-01 16:53 ` Glen Choo @ 2022-04-01 19:57 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2022-04-01 19:57 UTC (permalink / raw) To: Glen Choo Cc: Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason, Tao Klerks Glen Choo <chooglen@google.com> writes: > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/branch.c b/branch.c >> index 6b31df539a5..182f1c5a556 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->remote); >> + /* 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; >> } > > Ah I see, on the first iteration, we set the first remote's name to > tracking->remote, and on the second iteration, we copy that value to the > list before copying the _current_ remote's name to the list. > >> -test_expect_success 'avoid ambiguous track' ' >> +test_expect_success 'avoid ambiguous track and advise' ' >> git config branch.autosetupmerge true && >> git config remote.ambi1.url lalala && >> git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main && >> git config remote.ambi2.url lilili && >> git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main && >> - test_must_fail git branch all1 main && >> + cat <<-EOF >expected && >> + fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\'' >> + hint: There are multiple remotes whose fetch refspecs map to the remote >> + hint: tracking ref '\''refs/heads/main'\'': >> + hint: ambi1 >> + hint: ambi2 >> + hint: '' >> + hint: This is typically a configuration error. >> + hint: '' >> + hint: To support setting up tracking branches, ensure that >> + hint: different remotes'\'' fetch refspecs map into different >> + hint: tracking namespaces. >> + EOF >> + test_must_fail git branch all1 main 2>actual && >> + test_cmp expected actual && >> test -z "$(git config branch.all1.merge)" >> ' > > And this test shows that this indeed does what we think it does. Nicely > done. > > I notice that we there is an instance of test -z "$(some git command)", > which IIRC is discouraged because it would mask a failure in the git > command, but that's not new and I don't think it needs to be fixed in > this series anyway. > > Reviewed-by: Glen Choo <chooglen@google.com> Thanks for giving the patch a careful read. Will queue. Thanks. ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-04-01 19:57 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-21 10:23 [PATCH] tracking branches: add advice to ambiguous refspec error Tao Klerks via GitGitGadget 2022-03-21 14:11 ` Ævar Arnfjörð Bjarmason 2022-03-22 9:09 ` Tao Klerks 2022-03-22 9:18 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget 2022-03-22 10:04 ` Ævar Arnfjörð Bjarmason 2022-03-28 6:51 ` [PATCH v3] " Tao Klerks via GitGitGadget 2022-03-28 16:23 ` Junio C Hamano 2022-03-28 17:12 ` Glen Choo 2022-03-28 17:23 ` Junio C Hamano 2022-03-28 18:02 ` Tao Klerks 2022-03-28 18:50 ` Ævar Arnfjörð Bjarmason 2022-03-28 20:32 ` Junio C Hamano 2022-03-28 20:27 ` Junio C Hamano 2022-03-29 11:26 ` Tao Klerks 2022-03-29 11:26 ` [PATCH v4] " Tao Klerks via GitGitGadget 2022-03-29 11:31 ` Tao Klerks 2022-03-29 15:49 ` Junio C Hamano 2022-03-30 4:17 ` Tao Klerks 2022-03-30 7:20 ` [PATCH v5] " Tao Klerks via GitGitGadget 2022-03-30 13:19 ` Ævar Arnfjörð Bjarmason 2022-03-30 14:23 ` Tao Klerks 2022-03-30 15:18 ` Tao Klerks 2022-03-30 17:14 ` Ævar Arnfjörð Bjarmason 2022-03-30 20:37 ` Junio C Hamano 2022-03-30 21:09 ` Ævar Arnfjörð Bjarmason 2022-03-30 22:07 ` Junio C Hamano 2022-03-30 22:51 ` Ævar Arnfjörð Bjarmason 2022-03-31 16:01 ` [PATCH v6] " Tao Klerks via GitGitGadget 2022-03-31 19:32 ` Junio C Hamano 2022-03-31 23:57 ` Glen Choo 2022-04-01 4:30 ` Tao Klerks 2022-04-01 16:41 ` Glen Choo 2022-03-31 19:33 ` Junio C Hamano 2022-04-01 6:05 ` [PATCH v7] " Tao Klerks via GitGitGadget 2022-04-01 16:53 ` Glen Choo 2022-04-01 19:57 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.