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

[Tao Klerks <tao@klerks.biz>]

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.