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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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