| Message ID | pull.1183.git.1647858238144.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracking branches: add advice to ambiguous refspec error | expand |
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
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.
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);