On Mon, Mar 21, 2022 at 3:33 PM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> 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.
