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

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?

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;

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 missing the relevant Documentation/config/advice.txt update

"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)

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...

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.