git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] tracking branches: add advice to ambiguous refspec error

From
Tao Klerks <tao@klerks.biz>
Date
Mar 22, 2022, 09:09 UTC
Message-ID
<CAPMMpohn59X5CWOrhGD8gGiMumUCVcTWL=4dXE92AUsHJaRvoQ@mail.gmail.com>
In-Reply-To
<220321.86ee2v9xzd.gmgdl@evledraar.gmail.com>

On Mon, Mar 21, 2022 at 3:33 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 8 quoted lines
>
>
> 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.

Show 16 quoted lines
> >      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.
Show 16 quoted lines
> >      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.

Show 15 quoted lines
> > 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.
Show 29 quoted lines
> > 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.
Show 92 quoted lines
>
> (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.

Show 24 quoted lines
>
> > @@ -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.
Previous: Ævar Arnfjörð BjarmasonNext: Tao Klerks via GitGitGadget
Message 3 of 36 in “tracking branches: add advice to ambiguous refspec error”
  1. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 21, 2022
  2. Ævar Arnfjörð BjarmasonMar 21, 2022
  3. Tao KlerksMar 22, 2022
  4. RFC: tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 22, 2022
  5. Ævar Arnfjörð BjarmasonMar 22, 2022
  6. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 28, 2022
  7. Junio C HamanoMar 28, 2022
  8. Glen ChooMar 28, 2022
  9. Junio C HamanoMar 28, 2022
  10. Tao KlerksMar 28, 2022
  11. Ævar Arnfjörð BjarmasonMar 28, 2022
  12. Junio C HamanoMar 28, 2022
  13. Junio C HamanoMar 28, 2022
  14. Tao KlerksMar 29, 2022
  15. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 29, 2022
  16. Tao KlerksMar 29, 2022
  17. Junio C HamanoMar 29, 2022
  18. Tao KlerksMar 30, 2022
  19. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 30, 2022
  20. Ævar Arnfjörð BjarmasonMar 30, 2022
  21. Tao KlerksMar 30, 2022
  22. Tao KlerksMar 30, 2022
  23. Ævar Arnfjörð BjarmasonMar 30, 2022
  24. Junio C HamanoMar 30, 2022
  25. Ævar Arnfjörð BjarmasonMar 30, 2022
  26. Junio C HamanoMar 30, 2022
  27. Ævar Arnfjörð BjarmasonMar 30, 2022
  28. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Mar 31, 2022
  29. Junio C HamanoMar 31, 2022
  30. Glen ChooMar 31, 2022
  31. Tao KlerksApr 1, 2022
  32. Glen ChooApr 1, 2022
  33. Junio C HamanoMar 31, 2022
  34. tracking branches: add advice to ambiguous refspec errorTao Klerks via GitGitGadget, Apr 1, 2022
  35. Glen ChooApr 1, 2022
  36. Junio C HamanoApr 1, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.