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

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    RFC: tracking branches: add advice to ambiguous refspec error
    
    I am submitting this as an RFC because of a couple of things I'm not
    sure about:
    
     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?
     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?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

 advice.c |  1 +
 advice.h |  1 +
 branch.c | 27 ++++++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)

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 },
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;
 	const char *remote;
 	int matches;
 };
@@ -28,6 +29,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);
 		tracking->spec.src = NULL;
 	}
 
@@ -217,6 +219,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
+
+static const char ambiguous_refspec_advice_pre[] =
+N_("\n"
+"There are multiple remotes with fetch refspecs mapping to\n"
+"the tracking ref %s:\n";)
+static const char ambiguous_refspec_advice_post[] =
+N_("This is typically a configuration error.\n"
+"\n"
+"To support setting up tracking branches, ensure that\n"
+"different remotes' fetch refspecs map into different\n"
+"tracking namespaces.\n");
+
 /*
  * 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))
@@ -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);
+			}
+			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
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

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

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

[Tao Klerks]

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.

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    RFC: tracking branches: add advice to ambiguous refspec error
    
    Second go at adding some advice around ambiguous remote refspecs,
    incorporating Ævar's suggestions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v1:

 1:  9ec19e1c48e ! 1:  6c1cd885dda tracking branches: add advice to ambiguous refspec error
     @@ Metadata
      Author: Tao Klerks <tao@klerks.biz>
      
       ## Commit message ##
     -    tracking branches: add advice to ambiguous refspec error
     +    RFC: tracking branches: add advice to ambiguous refspec error
      
          The error "not tracking: ambiguous information for ref" is raised
          when we are evaluating what tracking information to set on a branch,
     @@ Commit message
      
          Signed-off-by: Tao Klerks <tao@klerks.biz>
      
     + ## Documentation/config/advice.txt ##
     +@@ Documentation/config/advice.txt: advice.*::
     + 		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
     + 		is asked to update index entries outside the current sparse
     + 		checkout.
     ++	ambiguousFetchRefspec::
     ++		Advice shown when branch tracking relationship setup fails due
     ++		to multiple remotes' refspecs mapping to the same remote
     ++		tracking namespace in the repo.
     + --
     +
       ## advice.c ##
      @@ advice.c: static struct {
       	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
     @@ advice.h: struct string_list;
       	ADVICE_DETACHED_HEAD,
      
       ## branch.c ##
     -@@
     - struct tracking {
     - 	struct refspec_item spec;
     - 	struct string_list *srcs;
     -+	struct string_list *remotes;
     - 	const char *remote;
     +@@ branch.c: struct tracking {
       	int matches;
       };
     + 
     ++struct find_tracked_branch_cb {
     ++	struct tracking *tracking;
     ++	struct strbuf remotes_advice;
     ++};
     ++
     + 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) {
      @@ branch.c: 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);
     ++		strbuf_addf(&ftb->remotes_advice, "  %s\n", remote->name);
       		tracking->spec.src = NULL;
       	}
       
     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori
      +
      +static const char ambiguous_refspec_advice_pre[] =
      +N_("\n"
     -+"There are multiple remotes with fetch refspecs mapping to\n"
     -+"the tracking ref %s:\n";)
     ++"There are multiple remotes whose fetch refspecs map to the remote\n"
     ++"tracking ref";)
      +static const char ambiguous_refspec_advice_post[] =
      +N_("This is typically a configuration error.\n"
      +"\n"
     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori
        * 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
      @@ branch.c: 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_advice = STRBUF_INIT,
     ++	};
       
       	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, &tracking);
     ++		for_each_remote(find_tracked_branch, &ftb_cb);
       	else if (inherit_tracking(&tracking, orig_ref))
     + 		return;
     + 
      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
       			return;
       		}
       
      -	if (tracking.matches > 1)
     +-		die(_("not tracking: ambiguous information for ref %s"),
     +-		    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) {
     -+				advise("  %s", item->string);
     -+			}
     -+			advise(_(ambiguous_refspec_advice_post));
     -+		}
     - 		die(_("not tracking: ambiguous information for ref %s"),
     - 		    orig_ref);
     ++		int status = die_message(_("not tracking: ambiguous information for ref %s"),
     ++					    orig_ref);
     ++		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
     ++			advise("%s %s:\n%s\n%s",
     ++			       _(ambiguous_refspec_advice_pre),
     ++			       orig_ref,
     ++			       ftb_cb.remotes_advice.buf,
     ++			       _(ambiguous_refspec_advice_post)
     ++			       );
     ++		exit(status);
      +	}
       
       	if (tracking.srcs->nr < 1)


 Documentation/config/advice.txt |  4 ++++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 42 +++++++++++++++++++++++++++++----
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 063eec2511d..abfac4f664b 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -126,4 +126,8 @@ advice.*::
 		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
 		is asked to update index entries outside the current sparse
 		checkout.
+	ambiguousFetchRefspec::
+		Advice shown when branch tracking relationship setup fails due
+		to multiple remotes' refspecs mapping to the same remote
+		tracking namespace in the repo.
 --
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 },
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..4a3a77aa338 100644
--- a/branch.c
+++ b/branch.c
@@ -16,9 +16,15 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct strbuf remotes_advice;
+};
+
 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) {
@@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
 		}
+		strbuf_addf(&ftb->remotes_advice, "  %s\n", remote->name);
 		tracking->spec.src = NULL;
 	}
 
@@ -217,6 +224,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
+
+static const char ambiguous_refspec_advice_pre[] =
+N_("\n"
+"There are multiple remotes whose fetch refspecs map to the remote\n"
+"tracking ref";)
+static const char ambiguous_refspec_advice_post[] =
+N_("This is typically a configuration error.\n"
+"\n"
+"To support setting up tracking branches, ensure that\n"
+"different remotes' fetch refspecs map into different\n"
+"tracking namespaces.\n");
+
 /*
  * 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
@@ -228,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.remotes_advice = STRBUF_INIT,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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))
 		return;
 
@@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			return;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref %s"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+			advise("%s %s:\n%s\n%s",
+			       _(ambiguous_refspec_advice_pre),
+			       orig_ref,
+			       ftb_cb.remotes_advice.buf,
+			       _(ambiguous_refspec_advice_post)
+			       );
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);

base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 22 2022, Tao Klerks via GitGitGadget wrote:

> From: Tao Klerks <tao@klerks.biz>
> [...]
Looking much better!

> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index 063eec2511d..abfac4f664b 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -126,4 +126,8 @@ advice.*::
>  		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
>  		is asked to update index entries outside the current sparse
>  		checkout.
> +	ambiguousFetchRefspec::
> +		Advice shown when branch tracking relationship setup fails due
> +		to multiple remotes' refspecs mapping to the same remote
> +		tracking namespace in the repo.

Let's add this in alphabetical section order. It's *somewhat* true of
the order now, but for some, but in any case I've got a WIP patch to
sort these, so since it's new adding it at the top would save us the
churn :)

> +struct find_tracked_branch_cb {
> +	struct tracking *tracking;
> +	struct strbuf remotes_advice;
> +};

Nice, thanks!

>  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) {
> @@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
>  			free(tracking->spec.src);
>  			string_list_clear(tracking->srcs, 0);
>  		}
> +		strbuf_addf(&ftb->remotes_advice, "  %s\n", remote->name);

Do this as:

    strbuf_addf([...], _("  %s\n"), [...]);

And add a TRANSLATORS comment similar to show_ambiguous_object() in
object-name.c. I.e. the alignment needs to be translatable for RTL
languages.

> +static const char ambiguous_refspec_advice_pre[] =
> +N_("\n"
> +"There are multiple remotes whose fetch refspecs map to the remote\n"
> +"tracking ref";)
> +static const char ambiguous_refspec_advice_post[] =
> +N_("This is typically a configuration error.\n"
> +"\n"
> +"To support setting up tracking branches, ensure that\n"
> +"different remotes' fetch refspecs map into different\n"
> +"tracking namespaces.\n");

These were split up before since we incrementally built the message....

Also, is the \n at the beginning/end really needed?

>  /*
>   * 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
> @@ -228,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  	struct tracking tracking;
>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> +	struct find_tracked_branch_cb ftb_cb = {
> +		.tracking = &tracking,
> +		.remotes_advice = STRBUF_INIT,
> +	};
>  
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
>  	tracking.srcs = &tracking_srcs;
>  	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))
>  		return;
>  
> @@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  			return;
>  		}
>  
> -	if (tracking.matches > 1)
> -		die(_("not tracking: ambiguous information for ref %s"),
> -		    orig_ref);
> +	if (tracking.matches > 1) {
> +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
> +					    orig_ref);
> +		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
> +			advise("%s %s:\n%s\n%s",
> +			       _(ambiguous_refspec_advice_pre),
> +			       orig_ref,
> +			       ftb_cb.remotes_advice.buf,
> +			       _(ambiguous_refspec_advice_post)
> +			       );

...but now we don't, so this should just be inlined here. I.e. made it
one big translatable _() message, the only parameters need to be the
orig_ref and the "remote_advice" buf.

We're also missing a strbuf_release() here. You can just add it to the
"cleanup" omitted from the context:

	strbuf_release(&ftb_cb.remotes_advice);

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    tracking branches: add advice to ambiguous refspec error
    
    I believe this third version incorporates all Ævar's suggestions, and
    might be usable. Removed "RFC" prefix.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v2:

 1:  6c1cd885dda ! 1:  22ffe81ac26 RFC: tracking branches: add advice to ambiguous refspec error
     @@ Metadata
      Author: Tao Klerks <tao@klerks.biz>
      
       ## Commit message ##
     -    RFC: tracking branches: add advice to ambiguous refspec error
     +    tracking branches: add advice to ambiguous refspec error
      
          The error "not tracking: ambiguous information for ref" is raised
          when we are evaluating what tracking information to set on a branch,
     @@ Commit message
      
       ## Documentation/config/advice.txt ##
      @@ Documentation/config/advice.txt: advice.*::
     - 		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
     - 		is asked to update index entries outside the current sparse
     - 		checkout.
     + 	can tell Git that you do not need help by setting these to 'false':
     + +
     + --
      +	ambiguousFetchRefspec::
      +		Advice shown when branch tracking relationship setup fails due
      +		to multiple remotes' refspecs mapping to the same remote
      +		tracking namespace in the repo.
     - --
     + 	fetchShowForcedUpdates::
     + 		Advice shown when linkgit:git-fetch[1] takes a long time
     + 		to calculate forced updates after ref updates, or to warn
      
       ## advice.c ##
      @@ advice.c: static struct {
     @@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv)
       			free(tracking->spec.src);
       			string_list_clear(tracking->srcs, 0);
       		}
     -+		strbuf_addf(&ftb->remotes_advice, "  %s\n", remote->name);
     ++		/*
     ++		 * TRANSLATORS: This is a line listing a remote with duplicate
     ++		 * refspecs, to be later included in advice message
     ++		 * ambiguousFetchRefspec. For RTL languages you'll probably want
     ++		 * to swap the "%s" and leading "  " space around.
     ++		 */
     ++		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
       		tracking->spec.src = NULL;
       	}
       
     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori
       	return 0;
       }
       
     -+
     -+static const char ambiguous_refspec_advice_pre[] =
     -+N_("\n"
     -+"There are multiple remotes whose fetch refspecs map to the remote\n"
     -+"tracking ref";)
     -+static const char ambiguous_refspec_advice_post[] =
     -+N_("This is typically a configuration error.\n"
     -+"\n"
     -+"To support setting up tracking branches, ensure that\n"
     -+"different remotes' fetch refspecs map into different\n"
     -+"tracking namespaces.\n");
      +
       /*
     -  * 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
     +  * Used internally to set the branch.<new_ref>.{remote,merge} config
     +  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
       	struct tracking tracking;
       	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
      -		for_each_remote(find_tracked_branch, &tracking);
      +		for_each_remote(find_tracked_branch, &ftb_cb);
       	else if (inherit_tracking(&tracking, orig_ref))
     - 		return;
     + 		goto cleanup;
       
      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
     - 			return;
     + 			goto cleanup;
       		}
       
      -	if (tracking.matches > 1)
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
      +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
      +					    orig_ref);
      +		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
     -+			advise("%s %s:\n%s\n%s",
     -+			       _(ambiguous_refspec_advice_pre),
     ++			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
     ++				 "tracking ref %s:\n"
     ++				 "%s"
     ++				 "\n"
     ++				 "This is typically a configuration error.\n"
     ++				 "\n"
     ++				 "To support setting up tracking branches, ensure that\n"
     ++				 "different remotes' fetch refspecs map into different\n"
     ++				 "tracking namespaces."),
      +			       orig_ref,
     -+			       ftb_cb.remotes_advice.buf,
     -+			       _(ambiguous_refspec_advice_post)
     ++			       ftb_cb.remotes_advice.buf
      +			       );
      +		exit(status);
      +	}
       
       	if (tracking.srcs->nr < 1)
       		string_list_append(tracking.srcs, orig_ref);
     +@@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
     + 		exit(-1);
     + 
     + cleanup:
     ++	strbuf_release(&ftb_cb.remotes_advice);
     + 	string_list_clear(&tracking_srcs, 0);
     + }
     + 


 Documentation/config/advice.txt |  4 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 44 +++++++++++++++++++++++++++++----
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..90f7dbd03aa 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,6 +4,10 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	ambiguousFetchRefspec::
+		Advice shown when branch tracking relationship setup fails due
+		to multiple remotes' refspecs mapping to the same remote
+		tracking namespace in the repo.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
diff --git a/advice.c b/advice.c
index 2e1fd483040..18ac8739519 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 },
diff --git a/advice.h b/advice.h
index a3957123a16..2d4c94f38eb 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 6b31df539a5..5c28d432103 100644
--- a/branch.c
+++ b/branch.c
@@ -18,9 +18,15 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct strbuf remotes_advice;
+};
+
 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) {
@@ -30,6 +36,13 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
 		}
+		/*
+		 * TRANSLATORS: This is a line listing a remote with duplicate
+		 * refspecs, to be later included in advice message
+		 * ambiguousFetchRefspec. For RTL languages you'll probably want
+		 * to swap the "%s" and leading "  " space around.
+		 */
+		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
 		tracking->spec.src = NULL;
 	}
 
@@ -219,6 +232,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
+
 /*
  * Used internally to set the branch.<new_ref>.{remote,merge} config
  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.remotes_advice = STRBUF_INIT,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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;
 
@@ -252,9 +270,24 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			goto cleanup;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref %s"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+				 "tracking ref %s:\n"
+				 "%s"
+				 "\n"
+				 "This is typically a configuration error.\n"
+				 "\n"
+				 "To support setting up tracking branches, ensure that\n"
+				 "different remotes' fetch refspecs map into different\n"
+				 "tracking namespaces."),
+			       orig_ref,
+			       ftb_cb.remotes_advice.buf
+			       );
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);
@@ -263,6 +296,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		exit(-1);
 
 cleanup:
+	strbuf_release(&ftb_cb.remotes_advice);
 	string_list_clear(&tracking_srcs, 0);
 }
 

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
-- 
gitgitgadget

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Tao Klerks <tao@klerks.biz>
>
> The error "not tracking: ambiguous information for ref" is raised
> when we are evaluating what tracking information to set on a branch,
> and find that the ref to be added as tracking branch is mapped
> under multiple remotes' fetch refspecs.

OK.

>  Documentation/config/advice.txt |  4 +++
>  advice.c                        |  1 +
>  advice.h                        |  1 +
>  branch.c                        | 44 +++++++++++++++++++++++++++++----
>  4 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index c40eb09cb7e..90f7dbd03aa 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -4,6 +4,10 @@ advice.*::
>  	can tell Git that you do not need help by setting these to 'false':
>  +
>  --
> +	ambiguousFetchRefspec::
> +		Advice shown when branch tracking relationship setup fails due
> +		to multiple remotes' refspecs mapping to the same remote
> +		tracking namespace in the repo.

	Advice shown when fetch refspec for multiple remotes map to
	the same remote-tracking branch namespace and causes branch
	tracking set-up to fail.

"remote-tracking" should be spelled as a single word.  There are
some existing mistakes in the code, but let's not make it worse.

> diff --git a/branch.c b/branch.c
> index 6b31df539a5..5c28d432103 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -18,9 +18,15 @@ struct tracking {
>  	int matches;
>  };
>  
> +struct find_tracked_branch_cb {
> +	struct tracking *tracking;
> +	struct strbuf remotes_advice;
> +};
> +
>  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) {
>  			string_list_append(tracking->srcs, tracking->spec.src);
>  			tracking->remote = remote->name;
>  		} else {
>  			free(tracking->spec.src);
>  			string_list_clear(tracking->srcs, 0);
>  		}
> +		/*
> +		 * TRANSLATORS: This is a line listing a remote with duplicate
> +		 * refspecs, to be later included in advice message
> +		 * ambiguousFetchRefspec. For RTL languages you'll probably want
> +		 * to swap the "%s" and leading "  " space around.
> +		 */
> +		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
>  		tracking->spec.src = NULL;
>  	}

This is wasteful.  remotes_advice does not have to be filled when we
are not going to give advice, i.e. there is only one matching remote
and no second or subsequent ones, which should be the majority case.
And we should not make majority of users who do not make a mistake
that needs the advice message pay the cost of giving advice to those
who screw up, if we can avoid it.

Instead, only when we are looking at the second one, we can stuff
tracking->remote (i.e. the first one) to remotes_advice, and then
append remote->name there.  Perhaps we can do it like so?

	struct strbuf *names = &ftb->remotes_advice;
	const char *namefmt = N_("  %s\n");

	switch (++tracking->matches) {
	case 1:
		string_list_append(tracking->srcs, tracking->spec.src);
		tracking->remote = remote->name;
		break;
	case 2:
		strbuf_addf(names, _(namefmt), tracking->remote);
		/* fall through */
	default:
		strbuf_addf(names, _(namefmt), remote->name);
                free(tracking->spec.src);
                string_list_clear(tracking->srcs, 0);
                break;
	}
	tracking->spec.src = NULL;

For a bonus point, remotes_advice member can be left empty without
paying the cost to strbuf_addf() when the advice configuration says
we are not going to show the message.

Thanks.

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

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/branch.c b/branch.c
>> index 6b31df539a5..5c28d432103 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -18,9 +18,15 @@ struct tracking {
>>  	int matches;
>>  };
>>  
>> +struct find_tracked_branch_cb {
>> +	struct tracking *tracking;
>> +	struct strbuf remotes_advice;
>> +};
>> +
>>  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) {
>>  			string_list_append(tracking->srcs, tracking->spec.src);
>>  			tracking->remote = remote->name;
>>  		} else {
>>  			free(tracking->spec.src);
>>  			string_list_clear(tracking->srcs, 0);
>>  		}
>> +		/*
>> +		 * TRANSLATORS: This is a line listing a remote with duplicate
>> +		 * refspecs, to be later included in advice message
>> +		 * ambiguousFetchRefspec. For RTL languages you'll probably want
>> +		 * to swap the "%s" and leading "  " space around.
>> +		 */
>> +		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
>>  		tracking->spec.src = NULL;
>>  	}
>
> This is wasteful.  remotes_advice does not have to be filled when we
> are not going to give advice, i.e. there is only one matching remote
> and no second or subsequent ones, which should be the majority case.
> And we should not make majority of users who do not make a mistake
> that needs the advice message pay the cost of giving advice to those
> who screw up, if we can avoid it.
>
> Instead, only when we are looking at the second one, we can stuff
> tracking->remote (i.e. the first one) to remotes_advice, and then
> append remote->name there.  Perhaps we can do it like so?
>
> 	struct strbuf *names = &ftb->remotes_advice;
> 	const char *namefmt = N_("  %s\n");
>
> 	switch (++tracking->matches) {
> 	case 1:
> 		string_list_append(tracking->srcs, tracking->spec.src);
> 		tracking->remote = remote->name;
> 		break;
> 	case 2:
> 		strbuf_addf(names, _(namefmt), tracking->remote);
> 		/* fall through */
> 	default:
> 		strbuf_addf(names, _(namefmt), remote->name);
>                 free(tracking->spec.src);
>                 string_list_clear(tracking->srcs, 0);
>                 break;
> 	}
> 	tracking->spec.src = NULL;
>
> For a bonus point, remotes_advice member can be left empty without
> paying the cost to strbuf_addf() when the advice configuration says
> we are not going to show the message.
>
> Thanks.

Hm, what do you think of an alternate approach of storing of the
matching remotes in a string_list, something like:

  struct find_tracked_branch_cb {
  	struct tracking *tracking;
  	struct string_list matching_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) {
  			string_list_append(tracking->srcs, tracking->spec.src);
  			tracking->remote = remote->name;
  		} else {
  			free(tracking->spec.src);
  			string_list_clear(tracking->srcs, 0);
  		}
  		string_list_append(&ftb->matching_remotes, remote->name);
  		tracking->spec.src = NULL;
  	}

then construct the advice message in setup_tracking()? To my untrained
eye, "case 2" requires a bit of extra work to understand.

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

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> Hm, what do you think of an alternate approach of storing of the
> matching remotes in a string_list, something like:
>
>   struct find_tracked_branch_cb {
>   	struct tracking *tracking;
>   	struct string_list matching_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) {
>   			string_list_append(tracking->srcs, tracking->spec.src);
>   			tracking->remote = remote->name;
>   		} else {
>   			free(tracking->spec.src);
>   			string_list_clear(tracking->srcs, 0);
>   		}
>   		string_list_append(&ftb->matching_remotes, remote->name);
>   		tracking->spec.src = NULL;
>   	}
>
> then construct the advice message in setup_tracking()? To my untrained
> eye, "case 2" requires a bit of extra work to understand.

If I were writing this code from scratch, I would have probably
collected the names first in this callback, and assembled them at
the end into a single string when we need a single string to show.

Having said that, as long as you do that lazily not to penalize
those who have sane setting without the need for advice/error to
trigger, I do not particularly care how the list of matching remote
names are kept.  Having string_list_append() unconditionally like
the above patch has, even for folks with just a single match without
need for the advice/error message is suboptimal, I would think.

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

[Tao Klerks]

On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Glen Choo <chooglen@google.com> writes:
>
> > Hm, what do you think of an alternate approach of storing of the
> > matching remotes in a string_list, something like:
[...]
> > then construct the advice message in setup_tracking()? To my untrained
> > eye, "case 2" requires a bit of extra work to understand.

Interestingly, that was what I had in the original RFC. I started using
the strbuf later, after Ævar confirmed that a single "advise()" call is
the way to go. I understood building the string as we go to lead to
simpler code, as it meant one less loop. On the other hand I
understand Junio is more concerned about performance than the
existence of a second loop that we should almost never hit.

I'm very happy to switch from strbuf-building to string_list-appending,
but I'm curious to understand how/why the performance of
strbuf_addf() would be notably worse than that of
string_list_append().

Is there public doc about this somewhere?

> Having said that, as long as you do that lazily not to penalize
> those who have sane setting without the need for advice/error to
> trigger, I do not particularly care how the list of matching remote
> names are kept.  Having string_list_append() unconditionally like
> the above patch has, even for folks with just a single match without
> need for the advice/error message is suboptimal, I would think.

Again, I'm new here, and not a great coder to start with, but I'm
having a hard time understanding why the single extra/gratuitous
strbuf_addf() or string_list_append() call that we stand to optimize
(I haven't understood whether there is a significant difference
between them) would ever be noticeable in the context of creating
a branch.

I of course completely understand optimizing anything that will
end up looping, but this is a max of 1 iteration's savings; I would
have thought that at these levels, readability/maintainability (and
succinctness) of the code would trump any marginal performance
savings.

To that end, I'd understand going back to string_list_append() as
Glen proposes, and building a formatted string in a single place
(setup_tracking()) only when required - both for readability, and
in case some aspect of strbuf_addf() is non-trivially expensive -
but is the "only append to the string_list() on the rare second
pass" optimization really worth the increase in amount of code?

Is "performance over succinctness" a general principle that
could or should be noted somewhere?

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 28 2022, Tao Klerks wrote:

> On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Glen Choo <chooglen@google.com> writes:
>>
>> > Hm, what do you think of an alternate approach of storing of the
>> > matching remotes in a string_list, something like:
> [...]
>> > then construct the advice message in setup_tracking()? To my untrained
>> > eye, "case 2" requires a bit of extra work to understand.
>
> Interestingly, that was what I had in the original RFC. I started using
> the strbuf later, after Ævar confirmed that a single "advise()" call is
> the way to go. I understood building the string as we go to lead to
> simpler code, as it meant one less loop. On the other hand I
> understand Junio is more concerned about performance than the
> existence of a second loop that we should almost never hit.
>
> I'm very happy to switch from strbuf-building to string_list-appending,
> but I'm curious to understand how/why the performance of
> strbuf_addf() would be notably worse than that of
> string_list_append().
>
> Is there public doc about this somewhere?

We could do a string_list as in your v1 and concat it as we're
formatting it, but pushing new strings to a string_list v.s. appending
to a strbuf is actually more efficient in favor of the strbuf.

But as to not penalizing those who don't have the advice enabled,
something like this (untested)?:

diff --git a/branch.c b/branch.c
index 5c28d432103..83545456c36 100644
--- a/branch.c
+++ b/branch.c
@@ -20,6 +20,7 @@ struct tracking {
 
 struct find_tracked_branch_cb {
 	struct tracking *tracking;
+	unsigned int do_advice:1;
 	struct strbuf remotes_advice;
 };
 
@@ -36,6 +37,9 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
 		}
+		tracking->spec.src = NULL;
+		if (!ftb->do_advice)
+			return 0;
 		/*
 		 * TRANSLATORS: This is a line listing a remote with duplicate
 		 * refspecs, to be later included in advice message
@@ -43,7 +47,6 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 		 * to swap the "%s" and leading "  " space around.
 		 */
 		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
-		tracking->spec.src = NULL;
 	}
 
 	return 0;
@@ -249,6 +252,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct find_tracked_branch_cb ftb_cb = {
 		.tracking = &tracking,
 		.remotes_advice = STRBUF_INIT,
+		.do_advice = advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC),
 	};
 
 	memset(&tracking, 0, sizeof(tracking));
@@ -273,7 +277,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	if (tracking.matches > 1) {
 		int status = die_message(_("not tracking: ambiguous information for ref %s"),
 					    orig_ref);
-		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+		if (ftb_cb.do_advice)
 			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
 				 "tracking ref %s:\n"
 				 "%s"

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

>> Having said that, as long as you do that lazily not to penalize
>> those who have sane setting without the need for advice/error to
>> trigger, I do not particularly care how the list of matching remote
>> names are kept.  Having string_list_append() unconditionally like
>> the above patch has, even for folks with just a single match without
>> need for the advice/error message is suboptimal, I would think.
>
> Again, I'm new here, and not a great coder to start with, but I'm
> having a hard time understanding why the single extra/gratuitous
> strbuf_addf() or string_list_append() call that we stand to optimize
> (I haven't understood whether there is a significant difference
> between them) would ever be noticeable in the context of creating
> a branch.

Small things accumulate.

Unless you have to bend over backwards to do so, avoid computing
unconditionally what you only need in an error case.  People do not
make mistake all the time, and they shouldn't be forced to pay for
the possibility that other people may make mistakes and may want to
get help.

When you see that you are wasting cycles "just in case I may see an
error", you may stop, take a deep breath, and think if you can push
the extra work to error code path with equally simple and easy code.
And in this case, I think it is just as equally easy and simple as
the unconditional code in your patch to optimize for the case where
there is *no* duplicate.

It is a good discipline to learn, especially if you are new, as it
is something that stays with you even after you move on to different
projects.

> I of course completely understand optimizing anything that will
> end up looping, but this is a max of 1 iteration's savings; I would

When there is no error, you do not even need to keep the "names that
will become necessary for advice" at all, so you are comparing 0
with 1.  And if you read the no-error case of the suggested rewrite,
you'd realize that it is much easier to reason about.  You do not
have to wonder what the remotes_advice strbuf (or string_list) is
doing in the no-error code path at all.  This is not about
performance, it is about clarity of the code (not doing unnecessary
thing means doing only essential thing, after all).

Between strbuf appending and string_list appending, I do not
personally care.  As long as the code can produce the same output,
it is OK.  Using string_list _may_ have an advantage that string
formatting logic will be concentrated in a single place, as opposed
to strbuf appending, where part of formatting decisions need to be
made in the callback and other part is left in the advise-string.
And because this should all happen only in error code path, the
performance between two APIs would not matter at all (and for that
to truly hold, you need to stick to "do not unconditionally compute 
what you need only in an error case" discipline).

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But as to not penalizing those who don't have the advice enabled,
> something like this (untested)?:

Unconditionally computing what you need only in the error path is
the primary sin in the patch, and that should be addressed first.

If we need to do new things (like adding the do_advice member), it
becomes questionable if it is worth doing.  In this case, I think
the update is simple enough and the control flow makes it clear what
is and what isn't needed only for advice-message generation, so it
might be an overall win.

Thanks.

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

[Tao Klerks]

OK, I

On Mon, Mar 28, 2022 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Small things accumulate.
>

OK. I've tried combining Junio's "only do something when needed" case
statement with Glen's "co-locate advice-related string manipulations"
proposal, and I believe this disqualifies and obviates the need for passing
around any new "are we going to be issuing advice" flag.

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    tracking branches: add advice to ambiguous refspec error
    
    I believe this third version incorporates all Ævar's suggestions, and
    might be usable. Removed "RFC" prefix.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v3:

 1:  22ffe81ac26 = 1:  ac6c782f566 tracking branches: add advice to ambiguous refspec error


 Documentation/config/advice.txt |  4 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 44 +++++++++++++++++++++++++++++----
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..90f7dbd03aa 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,6 +4,10 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	ambiguousFetchRefspec::
+		Advice shown when branch tracking relationship setup fails due
+		to multiple remotes' refspecs mapping to the same remote
+		tracking namespace in the repo.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
diff --git a/advice.c b/advice.c
index 2e1fd483040..18ac8739519 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 },
diff --git a/advice.h b/advice.h
index a3957123a16..2d4c94f38eb 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 6b31df539a5..5c28d432103 100644
--- a/branch.c
+++ b/branch.c
@@ -18,9 +18,15 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct strbuf remotes_advice;
+};
+
 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) {
@@ -30,6 +36,13 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
 		}
+		/*
+		 * TRANSLATORS: This is a line listing a remote with duplicate
+		 * refspecs, to be later included in advice message
+		 * ambiguousFetchRefspec. For RTL languages you'll probably want
+		 * to swap the "%s" and leading "  " space around.
+		 */
+		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
 		tracking->spec.src = NULL;
 	}
 
@@ -219,6 +232,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
+
 /*
  * Used internally to set the branch.<new_ref>.{remote,merge} config
  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.remotes_advice = STRBUF_INIT,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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;
 
@@ -252,9 +270,24 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			goto cleanup;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref %s"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+				 "tracking ref %s:\n"
+				 "%s"
+				 "\n"
+				 "This is typically a configuration error.\n"
+				 "\n"
+				 "To support setting up tracking branches, ensure that\n"
+				 "different remotes' fetch refspecs map into different\n"
+				 "tracking namespaces."),
+			       orig_ref,
+			       ftb_cb.remotes_advice.buf
+			       );
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);
@@ -263,6 +296,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		exit(-1);
 
 cleanup:
+	strbuf_release(&ftb_cb.remotes_advice);
 	string_list_clear(&tracking_srcs, 0);
 }
 

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
-- 
gitgitgadget

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

[Tao Klerks]

On Tue, Mar 29, 2022 at 1:26 PM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
>     I believe this third version incorporates all Ævar's suggestions, and
>     might be usable. Removed "RFC" prefix.

Argh, sorry about the broken cover letter. This *fourth* version
attempts to address Junio's concerns over unnecessary extra
error-preparation work being done in the very-vastly-more-common
"normal" case.

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Tao Klerks <tao@klerks.biz>
>
> The error "not tracking: ambiguous information for ref" is raised
> when we are evaluating what tracking information to set on a branch,
> and find that the ref to be added as tracking branch is mapped
> under multiple remotes' fetch refspecs.
>
> This can easily happen when a user copy-pastes a remote definition
> in their git config, and forgets to change the tracking path.
>
> Add advice in this situation, explicitly highlighting which remotes
> are involved and suggesting how to correct the situation.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     tracking branches: add advice to ambiguous refspec error
>     
>     I believe this third version incorporates all Ævar's suggestions, and
>     might be usable. Removed "RFC" prefix.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1183
>
> Range-diff vs v3:
>
>  1:  22ffe81ac26 = 1:  ac6c782f566 tracking branches: add advice to ambiguous refspec error
>
>
>  Documentation/config/advice.txt |  4 +++
>  advice.c                        |  1 +
>  advice.h                        |  1 +
>  branch.c                        | 44 +++++++++++++++++++++++++++++----
>  4 files changed, 45 insertions(+), 5 deletions(-)

Sent a wrong version?

The patch text seems to be identical to that of v3 message archived at
https://lore.kernel.org/git/pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com/

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

[Tao Klerks]

On Tue, Mar 29, 2022 at 5:49 PM Junio C Hamano <gitster@pobox.com> wrote:
[...]
>
> Sent a wrong version?
[...]

Yes indeed. My apologies once again. V5 coming as soon as the test run
completes.

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    tracking branches: add advice to ambiguous refspec error
    
    This fifth version attempts to address Junio's concerns over unnecessary
    extra error-preparation work being done in the very-vastly-more-common
    "normal" case.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v4:

 1:  ac6c782f566 ! 1:  4478eaed6df tracking branches: add advice to ambiguous refspec error
     @@ Documentation/config/advice.txt: advice.*::
       +
       --
      +	ambiguousFetchRefspec::
     -+		Advice shown when branch tracking relationship setup fails due
     -+		to multiple remotes' refspecs mapping to the same remote
     -+		tracking namespace in the repo.
     ++		Advice shown when fetch refspec for multiple remotes map to
     ++		the same remote-tracking branch namespace and causes branch
     ++		tracking set-up to fail.
       	fetchShowForcedUpdates::
       		Advice shown when linkgit:git-fetch[1] takes a long time
       		to calculate forced updates after ref updates, or to warn
     @@ branch.c: struct tracking {
       
      +struct find_tracked_branch_cb {
      +	struct tracking *tracking;
     -+	struct strbuf remotes_advice;
     ++	struct string_list ambiguous_remotes;
      +};
      +
       static int find_tracked_branch(struct remote *remote, void *priv)
     @@ branch.c: struct tracking {
      +	struct tracking *tracking = ftb->tracking;
       
       	if (!remote_find_tracking(remote, &tracking->spec)) {
     - 		if (++tracking->matches == 1) {
     -@@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv)
     +-		if (++tracking->matches == 1) {
     ++		switch (++tracking->matches) {
     ++		case 1:
     + 			string_list_append(tracking->srcs, tracking->spec.src);
     + 			tracking->remote = remote->name;
     +-		} else {
     ++			break;
     ++		case 2:
     ++			// there are at least two remotes; backfill the first one
     ++			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
     ++			/* fall through */
     ++		default:
     ++			string_list_append(&ftb->ambiguous_remotes, remote->name);
       			free(tracking->spec.src);
       			string_list_clear(tracking->srcs, 0);
     ++		break;
       		}
     -+		/*
     -+		 * TRANSLATORS: This is a line listing a remote with duplicate
     -+		 * refspecs, to be later included in advice message
     -+		 * ambiguousFetchRefspec. For RTL languages you'll probably want
     -+		 * to swap the "%s" and leading "  " space around.
     -+		 */
     -+		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
       		tracking->spec.src = NULL;
       	}
     - 
      @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
       	return 0;
       }
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
       	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
      +	struct find_tracked_branch_cb ftb_cb = {
      +		.tracking = &tracking,
     -+		.remotes_advice = STRBUF_INIT,
     ++		.ambiguous_remotes = STRING_LIST_INIT_DUP,
      +	};
       
       	memset(&tracking, 0, sizeof(tracking));
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
      +	if (tracking.matches > 1) {
      +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
      +					    orig_ref);
     -+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
     ++		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
     ++			struct strbuf remotes_advice = STRBUF_INIT;
     ++			struct string_list_item *item;
     ++
     ++			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {
     ++				/*
     ++				 * TRANSLATORS: This is a line listing a remote with duplicate
     ++				 * refspecs in the advice message below. For RTL languages you'll
     ++				 * probably want to swap the "%s" and leading "  " space around.
     ++				 */
     ++				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
     ++			}
     ++
      +			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
      +				 "tracking ref %s:\n"
      +				 "%s"
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
      +				 "different remotes' fetch refspecs map into different\n"
      +				 "tracking namespaces."),
      +			       orig_ref,
     -+			       ftb_cb.remotes_advice.buf
     ++			       remotes_advice.buf
      +			       );
     ++			strbuf_release(&remotes_advice);
     ++		}
      +		exit(status);
      +	}
       
       	if (tracking.srcs->nr < 1)
       		string_list_append(tracking.srcs, orig_ref);
      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
     - 		exit(-1);
       
       cleanup:
     -+	strbuf_release(&ftb_cb.remotes_advice);
       	string_list_clear(&tracking_srcs, 0);
     ++	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
       }
       
     + int read_branch_desc(struct strbuf *buf, const char *branch_name)


 Documentation/config/advice.txt |  4 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 63 +++++++++++++++++++++++++++++----
 4 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..343d271c707 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,6 +4,10 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	ambiguousFetchRefspec::
+		Advice shown when fetch refspec for multiple remotes map to
+		the same remote-tracking branch namespace and causes branch
+		tracking set-up to fail.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
diff --git a/advice.c b/advice.c
index 2e1fd483040..18ac8739519 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 },
diff --git a/advice.h b/advice.h
index a3957123a16..2d4c94f38eb 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 6b31df539a5..e6ab50fff41 100644
--- a/branch.c
+++ b/branch.c
@@ -18,17 +18,31 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct string_list ambiguous_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) {
+		switch (++tracking->matches) {
+		case 1:
 			string_list_append(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
-		} else {
+			break;
+		case 2:
+			// there are at least two remotes; backfill the first one
+			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
+			/* fall through */
+		default:
+			string_list_append(&ftb->ambiguous_remotes, remote->name);
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
+		break;
 		}
 		tracking->spec.src = NULL;
 	}
@@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	return 0;
 }
 
+
 /*
  * Used internally to set the branch.<new_ref>.{remote,merge} config
  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
@@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.ambiguous_remotes = STRING_LIST_INIT_DUP,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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;
 
@@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			goto cleanup;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref %s"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
+			struct strbuf remotes_advice = STRBUF_INIT;
+			struct string_list_item *item;
+
+			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {
+				/*
+				 * TRANSLATORS: This is a line listing a remote with duplicate
+				 * refspecs in the advice message below. For RTL languages you'll
+				 * probably want to swap the "%s" and leading "  " space around.
+				 */
+				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
+			}
+
+			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+				 "tracking ref %s:\n"
+				 "%s"
+				 "\n"
+				 "This is typically a configuration error.\n"
+				 "\n"
+				 "To support setting up tracking branches, ensure that\n"
+				 "different remotes' fetch refspecs map into different\n"
+				 "tracking namespaces."),
+			       orig_ref,
+			       remotes_advice.buf
+			       );
+			strbuf_release(&remotes_advice);
+		}
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);
@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);
+	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
 }
 
 int read_branch_desc(struct strbuf *buf, const char *branch_name)

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:

> From: Tao Klerks <tao@klerks.biz>

> +		case 2:
> +			// there are at least two remotes; backfill the first one

Nit: I think it's been Junio's preference not to introduce C99 comments,
despite other C99 features now being used (and I think it should work in
practice as far as portability goes, see
https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)

> @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
>  	return 0;
>  }
>  
> +

Stray whitespace?

>  /*
>   * Used internally to set the branch.<new_ref>.{remote,merge} config
>   * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
> @@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  	struct tracking tracking;
>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> +	struct find_tracked_branch_cb ftb_cb = {
> +		.tracking = &tracking,
> +		.ambiguous_remotes = STRING_LIST_INIT_DUP,
> +	};
>  
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
>  	tracking.srcs = &tracking_srcs;
>  	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;
>  
> @@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  			goto cleanup;
>  		}
>  
> -	if (tracking.matches > 1)
> -		die(_("not tracking: ambiguous information for ref %s"),
> -		    orig_ref);
> +	if (tracking.matches > 1) {
> +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
> +					    orig_ref);

This isn't per-se new, but I wonder if while we're at it we shold just
quote '%s' here, which we'd usually do. I.e. this message isn't new, but
referring again to "ref %s" (and not "ref '%s'") below is.

> +		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
> +			struct strbuf remotes_advice = STRBUF_INIT;
> +			struct string_list_item *item;
> +
> +			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {

Nit: drop braces, not needed.

> +				/*
> +				 * TRANSLATORS: This is a line listing a remote with duplicate
> +				 * refspecs in the advice message below. For RTL languages you'll
> +				 * probably want to swap the "%s" and leading "  " space around.
> +				 */
> +				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
> +			}
> +

Per the TRANSLATORS comments in get_short_oid(), it's also good to have
one here in front of advice(). E.g.:

	/*
	 * TRANSLATORS: The second argument is a \n-delimited list of
	 * duplicate refspecs, composed above.
	*/

> +			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
> +				 "tracking ref %s:\n"
> +				 "%s"
> +				 "\n"
> +				 "This is typically a configuration error.\n"
> +				 "\n"
> +				 "To support setting up tracking branches, ensure that\n"
> +				 "different remotes' fetch refspecs map into different\n"
> +				 "tracking namespaces."),
> +			       orig_ref,
> +			       remotes_advice.buf
> +			       );

Nit: The usual style for multi-line arguments is to "fill" lines until
you're at 79 characters, so these last three lines (including the ");")
can all go on the "tracking namespaces" line (until they're at 79, then
wrap)>

> +			strbuf_release(&remotes_advice);
> +		}
> +		exit(status);
> +	}

Other than the minor nits noted above this version looks good to me, and
I think addresses both the translation concerns I brought up, and the
"let's not do advice() work if we don't need it" concern by Junio et al.

>  	if (tracking.srcs->nr < 1)
>  		string_list_append(tracking.srcs, orig_ref);
> @@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  
>  cleanup:
>  	string_list_clear(&tracking_srcs, 0);
> +	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
>  }
>  
>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
>
> base-commit: abf474a5dd901f28013c52155411a48fd4c09922

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

[Tao Klerks]

On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
>
> Nit: I think it's been Junio's preference not to introduce C99 comments,

Argh, my apologies, bad habits.

I wonder whether anyone has any tooling to help catch this kind of
thing - I'll ask in GitGitGadget, anyway.

> > @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
> >       return 0;
> >  }
> >
> > +
>
> Stray whitespace?

Thx, I will check more carefully next time.

>
> >  /*
> >   * Used internally to set the branch.<new_ref>.{remote,merge} config
> >   * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
> > @@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
> >       struct tracking tracking;
> >       struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
> >       int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> > +     struct find_tracked_branch_cb ftb_cb = {
> > +             .tracking = &tracking,
> > +             .ambiguous_remotes = STRING_LIST_INIT_DUP,
> > +     };
> >
> >       memset(&tracking, 0, sizeof(tracking));
> >       tracking.spec.dst = (char *)orig_ref;
> >       tracking.srcs = &tracking_srcs;
> >       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;
> >
> > @@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
> >                       goto cleanup;
> >               }
> >
> > -     if (tracking.matches > 1)
> > -             die(_("not tracking: ambiguous information for ref %s"),
> > -                 orig_ref);
> > +     if (tracking.matches > 1) {
> > +             int status = die_message(_("not tracking: ambiguous information for ref %s"),
> > +                                         orig_ref);
>
> This isn't per-se new, but I wonder if while we're at it we shold just
> quote '%s' here, which we'd usually do. I.e. this message isn't new, but
> referring again to "ref %s" (and not "ref '%s'") below is.
>

I will fix the below, but I would default to not changing the above
unless someone thinks we should (not sure what the expectations are
around changing error messages unnecessarily)

> > +             if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
> > +                     struct strbuf remotes_advice = STRBUF_INIT;
> > +                     struct string_list_item *item;
> > +
> > +                     for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {
>
> Nit: drop braces, not needed.

Thx, will do.

>
> > +                             /*
> > +                              * TRANSLATORS: This is a line listing a remote with duplicate
> > +                              * refspecs in the advice message below. For RTL languages you'll
> > +                              * probably want to swap the "%s" and leading "  " space around.
> > +                              */
> > +                             strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
> > +                     }
> > +
>
> Per the TRANSLATORS comments in get_short_oid(), it's also good to have
> one here in front of advice(). E.g.:
>
>         /*
>          * TRANSLATORS: The second argument is a \n-delimited list of
>          * duplicate refspecs, composed above.
>         */
>

Will do, thx.

> > +                     advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
> > +                              "tracking ref %s:\n"
> > +                              "%s"
> > +                              "\n"
> > +                              "This is typically a configuration error.\n"
> > +                              "\n"
> > +                              "To support setting up tracking branches, ensure that\n"
> > +                              "different remotes' fetch refspecs map into different\n"
> > +                              "tracking namespaces."),
> > +                            orig_ref,
> > +                            remotes_advice.buf
> > +                            );
>
> Nit: The usual style for multi-line arguments is to "fill" lines until
> you're at 79 characters, so these last three lines (including the ");")
> can all go on the "tracking namespaces" line (until they're at 79, then
> wrap)>
>

Yep, another oversight, thx.

> > +                     strbuf_release(&remotes_advice);
> > +             }
> > +             exit(status);
> > +     }
>
> Other than the minor nits noted above this version looks good to me, and
> I think addresses both the translation concerns I brought up, and the
> "let's not do advice() work if we don't need it" concern by Junio et al.
>

Awesome, thx!

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

[Tao Klerks]

On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote:
>
> On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> >
> > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
> >
> > > +     if (tracking.matches > 1) {
> > > +             int status = die_message(_("not tracking: ambiguous information for ref %s"),
> > > +                                         orig_ref);
> >
> > This isn't per-se new, but I wonder if while we're at it we shold just
> > quote '%s' here, which we'd usually do. I.e. this message isn't new, but
> > referring again to "ref %s" (and not "ref '%s'") below is.
> >
>
> I will fix the below, but I would default to not changing the above
> unless someone thinks we should (not sure what the expectations are
> around changing error messages unnecessarily)

I take this back. I will update both - the change is in a "variable"
part of the message anyway, and it's hard to imagine any tool
actively/purposefully parsing the ref out of the message and being
caught out by the new quotes. Any coordinating tool would already know
what ref was being branched / having its tracking remote info updated.

>
> > > +                              * TRANSLATORS: This is a line listing a remote with duplicate
> > > +                              * refspecs in the advice message below. For RTL languages you'll
> > > +                              * probably want to swap the "%s" and leading "  " space around.
> > > +                              */
> > > +                             strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
> > > +                     }
> > > +
> >

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

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Tao Klerks wrote:

> On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote:
>>
>> On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> >
>> >
>> > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
>> >
>> > > +     if (tracking.matches > 1) {
>> > > +             int status = die_message(_("not tracking: ambiguous information for ref %s"),
>> > > +                                         orig_ref);
>> >
>> > This isn't per-se new, but I wonder if while we're at it we shold just
>> > quote '%s' here, which we'd usually do. I.e. this message isn't new, but
>> > referring again to "ref %s" (and not "ref '%s'") below is.
>> >
>>
>> I will fix the below, but I would default to not changing the above
>> unless someone thinks we should (not sure what the expectations are
>> around changing error messages unnecessarily)
>
> I take this back. I will update both - the change is in a "variable"
> part of the message anyway, and it's hard to imagine any tool
> actively/purposefully parsing the ref out of the message and being
> caught out by the new quotes. Any coordinating tool would already know
> what ref was being branched / having its tracking remote info updated.

Thanks, I'd be fine either way, it was just a suggestion.

Aside from what we do here we don't support third-party tooling that's
grepping our specific human-readable output, so changing any such
messaging is OK.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
>
>> From: Tao Klerks <tao@klerks.biz>
>
>> +		case 2:
>> +			// there are at least two remotes; backfill the first one
>
> Nit: I think it's been Junio's preference not to introduce C99 comments,

I often mention the rule to new contributors, simply because it has
been that way in our code base, regardless of what my personal
preference might be, and sticking to the style will be more
consistent.  It's not like I am forcing my personal preference on
developers.  Do not mislead new people into thinking so.  It is
especially irritating to see ...

> despite other C99 features now being used (and I think it should work in
> practice as far as portability goes, see
> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)

... a mention like this, when you know that it is not about
portability but is about consistency, and also you know I've
mentioned more than once on the list if we want to loosen some
written CodingGuidelines rules, especially those that tools do not
necessarily catch.

>> +	if (tracking.matches > 1) {
>> +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
>> +					    orig_ref);
>
> This isn't per-se new, but I wonder if while we're at it we shold just
> quote '%s' here, which we'd usually do. I.e. this message isn't new, but
> referring again to "ref %s" (and not "ref '%s'") below is.

Good.

>> +				 "To support setting up tracking branches, ensure that\n"
>> +				 "different remotes' fetch refspecs map into different\n"
>> +				 "tracking namespaces."),
>> +			       orig_ref,
>> +			       remotes_advice.buf
>> +			       );
>
> Nit: The usual style for multi-line arguments is to "fill" lines until
> you're at 79 characters, so these last three lines (including the ");")
> can all go on the "tracking namespaces" line (until they're at 79, then
> wrap)>

I didn't know about the magic "79" number.  It makes the resulting
source code extremely hard to read, though, while making it easier
to grep for specific messages.

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

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
>>
>>> From: Tao Klerks <tao@klerks.biz>
>>
>>> +		case 2:
>>> +			// there are at least two remotes; backfill the first one
>>
>> Nit: I think it's been Junio's preference not to introduce C99 comments,
>
> I often mention the rule to new contributors, simply because it has
> been that way in our code base, regardless of what my personal
> preference might be, and sticking to the style will be more
> consistent.  It's not like I am forcing my personal preference on
> developers.  Do not mislead new people into thinking so.  It is
> especially irritating to see ...
>
>> despite other C99 features now being used (and I think it should work in
>> practice as far as portability goes, see
>> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)
>
> ... a mention like this, when you know that it is not about
> portability but is about consistency, and also you know I've
> mentioned more than once on the list if we want to loosen some
> written CodingGuidelines rules, especially those that tools do not
> necessarily catch.

I know, and I didn't mean to imply that you were standing in the way of
C99 progress or anything like that.

Personally, I much prefer not to have //-style comments introduced. I
merely mentioned the portability concern because I thought it helped
give a relatively new contributor some context.

I.e. that despite any new developments on the C99 front they might
discover this particular thing is stylistic and not about portability
(although that may also have been a reason in the past).

>>> +	if (tracking.matches > 1) {
>>> +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
>>> +					    orig_ref);
>>
>> This isn't per-se new, but I wonder if while we're at it we shold just
>> quote '%s' here, which we'd usually do. I.e. this message isn't new, but
>> referring again to "ref %s" (and not "ref '%s'") below is.
>
> Good.
>
>>> +				 "To support setting up tracking branches, ensure that\n"
>>> +				 "different remotes' fetch refspecs map into different\n"
>>> +				 "tracking namespaces."),
>>> +			       orig_ref,
>>> +			       remotes_advice.buf
>>> +			       );
>>
>> Nit: The usual style for multi-line arguments is to "fill" lines until
>> you're at 79 characters, so these last three lines (including the ");")
>> can all go on the "tracking namespaces" line (until they're at 79, then
>> wrap)>
>
> I didn't know about the magic "79" number.  It makes the resulting
> source code extremely hard to read, though, while making it easier
> to grep for specific messages.

I'm referring to the "80 characters per line", but omitted the \n, but
yeah, I should have just said 80.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>>> +				 "To support setting up tracking branches, ensure that\n"
>>>> +				 "different remotes' fetch refspecs map into different\n"
>>>> +				 "tracking namespaces."),
>>>> +			       orig_ref,
>>>> +			       remotes_advice.buf
>>>> +			       );
>>>
>>> Nit: The usual style for multi-line arguments is to "fill" lines until
>>> you're at 79 characters, so these last three lines (including the ");")
>>> can all go on the "tracking namespaces" line (until they're at 79, then
>>> wrap)>
>>
>> I didn't know about the magic "79" number.  It makes the resulting
>> source code extremely hard to read, though, while making it easier
>> to grep for specific messages.
>
> I'm referring to the "80 characters per line", but omitted the \n, but
> yeah, I should have just said 80.

No, what I meant was that you do not want the rule to be to cut *AT*
exactly the column whatever random rule specifies, which would
result in funny wrapping in the middle of the word, e.g.

        "To support setting up tracking branches, ensure that diff"
        "erent remotes' fetch refspecs map into different tracking"
        " namespaces."

and "at 79, then wrap" somehow sounded to me like that.  I do not
think you meant to imply that (instead, I think you meant to suggest
"wrap the line so that the string constant is not longer than 79
columns"), but it risks to be mistaken by new contributors.

FWIW, I'd actually prefer to see both the sources to be readable by
wrapping to keep the source code line length under certain limit
(the current guideline being 70-something), counting the leading
indentation, and at the same time keep it possible and easy to grep
messages in the source.

That requires us to notice when our code has too deeply nested,
resulting in overly indented lines, and maintain the readability
(refatoring the code may be a way to help in such cases).

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

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 30 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>>> +				 "To support setting up tracking branches, ensure that\n"
>>>>> +				 "different remotes' fetch refspecs map into different\n"
>>>>> +				 "tracking namespaces."),
>>>>> +			       orig_ref,
>>>>> +			       remotes_advice.buf
>>>>> +			       );
>>>>
>>>> Nit: The usual style for multi-line arguments is to "fill" lines until
>>>> you're at 79 characters, so these last three lines (including the ");")
>>>> can all go on the "tracking namespaces" line (until they're at 79, then
>>>> wrap)>
>>>
>>> I didn't know about the magic "79" number.  It makes the resulting
>>> source code extremely hard to read, though, while making it easier
>>> to grep for specific messages.
>>
>> I'm referring to the "80 characters per line", but omitted the \n, but
>> yeah, I should have just said 80.
>
> No, what I meant was that you do not want the rule to be to cut *AT*
> exactly the column whatever random rule specifies, which would
> result in funny wrapping in the middle of the word, e.g.
>
>         "To support setting up tracking branches, ensure that diff"
>         "erent remotes' fetch refspecs map into different tracking"
>         " namespaces."
>
> and "at 79, then wrap" somehow sounded to me like that.  I do not
> think you meant to imply that (instead, I think you meant to suggest
> "wrap the line so that the string constant is not longer than 79
> columns"), but it risks to be mistaken by new contributors.
>
> FWIW, I'd actually prefer to see both the sources to be readable by
> wrapping to keep the source code line length under certain limit
> (the current guideline being 70-something), counting the leading
> indentation, and at the same time keep it possible and easy to grep
> messages in the source.
>
> That requires us to notice when our code has too deeply nested,
> resulting in overly indented lines, and maintain the readability
> (refatoring the code may be a way to help in such cases).

Yes, I didn't mean to say it was a hard rule. In particular as this code
has the strings themselves over 80 characters. It can be good to break
that guideline when it doesn't help readability.

I just meant that this made sense as a fix-up, in this case:

diff --git a/branch.c b/branch.c
index 5c28d432103..4ccf5f79e83 100644
--- a/branch.c
+++ b/branch.c
@@ -282,10 +282,8 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 				 "\n"
 				 "To support setting up tracking branches, ensure that\n"
 				 "different remotes' fetch refspecs map into different\n"
-				 "tracking namespaces."),
-			       orig_ref,
-			       ftb_cb.remotes_advice.buf
-			       );
+				 "tracking namespaces."), orig_ref,
+			       ftb_cb.remotes_advice.buf);
 		exit(status);
 	}
 

Which I'd also be tempted to do as:

diff --git a/branch.c b/branch.c
index 5c28d432103..b9f6fda980b 100644
--- a/branch.c
+++ b/branch.c
@@ -283,9 +283,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 				 "To support setting up tracking branches, ensure that\n"
 				 "different remotes' fetch refspecs map into different\n"
 				 "tracking namespaces."),
-			       orig_ref,
-			       ftb_cb.remotes_advice.buf
-			       );
+			       orig_ref, ftb_cb.remotes_advice.buf);
 		exit(status);
 	}
 
But I find it generally helpful to do it consistently when possible, as
when running into merge conflicts it makes things easier.

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    tracking branches: add advice to ambiguous refspec error
    
    v6 addresses some formatting and commenting issues Ævar noticed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v5:

 1:  4478eaed6df ! 1:  2408ab0ccb3 tracking branches: add advice to ambiguous refspec error
     @@ branch.c: struct tracking {
      -		} else {
      +			break;
      +		case 2:
     -+			// there are at least two remotes; backfill the first one
     ++			/* there are at least two remotes; backfill the first one */
      +			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
      +			/* fall through */
      +		default:
     @@ branch.c: struct tracking {
       		}
       		tracking->spec.src = NULL;
       	}
     -@@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
     - 	return 0;
     - }
     - 
     -+
     - /*
     -  * Used internally to set the branch.<new_ref>.{remote,merge} config
     -  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike
      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
       	struct tracking tracking;
       	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
      -		die(_("not tracking: ambiguous information for ref %s"),
      -		    orig_ref);
      +	if (tracking.matches > 1) {
     -+		int status = die_message(_("not tracking: ambiguous information for ref %s"),
     ++		int status = die_message(_("not tracking: ambiguous information for ref '%s'"),
      +					    orig_ref);
      +		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
      +			struct strbuf remotes_advice = STRBUF_INIT;
      +			struct string_list_item *item;
      +
     -+			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {
     ++			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes)
      +				/*
      +				 * TRANSLATORS: This is a line listing a remote with duplicate
      +				 * refspecs in the advice message below. For RTL languages you'll
      +				 * probably want to swap the "%s" and leading "  " space around.
      +				 */
      +				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
     -+			}
      +
     ++			/*
     ++			 * TRANSLATORS: The second argument is a \n-delimited list of
     ++			 * duplicate refspecs, composed above.
     ++			 */
      +			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
     -+				 "tracking ref %s:\n"
     ++				 "tracking ref '%s':\n"
      +				 "%s"
      +				 "\n"
      +				 "This is typically a configuration error.\n"
      +				 "\n"
      +				 "To support setting up tracking branches, ensure that\n"
      +				 "different remotes' fetch refspecs map into different\n"
     -+				 "tracking namespaces."),
     -+			       orig_ref,
     -+			       remotes_advice.buf
     -+			       );
     ++				 "tracking namespaces."), orig_ref,
     ++			       remotes_advice.buf);
      +			strbuf_release(&remotes_advice);
      +		}
      +		exit(status);


 Documentation/config/advice.txt |  4 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 63 +++++++++++++++++++++++++++++----
 4 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..343d271c707 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,6 +4,10 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	ambiguousFetchRefspec::
+		Advice shown when fetch refspec for multiple remotes map to
+		the same remote-tracking branch namespace and causes branch
+		tracking set-up to fail.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
diff --git a/advice.c b/advice.c
index 2e1fd483040..18ac8739519 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 },
diff --git a/advice.h b/advice.h
index a3957123a16..2d4c94f38eb 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 6b31df539a5..812ceae2d0f 100644
--- a/branch.c
+++ b/branch.c
@@ -18,17 +18,31 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct string_list ambiguous_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) {
+		switch (++tracking->matches) {
+		case 1:
 			string_list_append(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
-		} else {
+			break;
+		case 2:
+			/* there are at least two remotes; backfill the first one */
+			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
+			/* fall through */
+		default:
+			string_list_append(&ftb->ambiguous_remotes, remote->name);
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
+		break;
 		}
 		tracking->spec.src = NULL;
 	}
@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.ambiguous_remotes = STRING_LIST_INIT_DUP,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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;
 
@@ -252,9 +270,39 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			goto cleanup;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref '%s'"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
+			struct strbuf remotes_advice = STRBUF_INIT;
+			struct string_list_item *item;
+
+			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes)
+				/*
+				 * TRANSLATORS: This is a line listing a remote with duplicate
+				 * refspecs in the advice message below. For RTL languages you'll
+				 * probably want to swap the "%s" and leading "  " space around.
+				 */
+				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
+
+			/*
+			 * TRANSLATORS: The second argument is a \n-delimited list of
+			 * duplicate refspecs, composed above.
+			 */
+			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+				 "tracking ref '%s':\n"
+				 "%s"
+				 "\n"
+				 "This is typically a configuration error.\n"
+				 "\n"
+				 "To support setting up tracking branches, ensure that\n"
+				 "different remotes' fetch refspecs map into different\n"
+				 "tracking namespaces."), orig_ref,
+			       remotes_advice.buf);
+			strbuf_release(&remotes_advice);
+		}
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);
@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);
+	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
 }
 
 int read_branch_desc(struct strbuf *buf, const char *branch_name)

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
-- 
gitgitgadget

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  	if (!remote_find_tracking(remote, &tracking->spec)) {
> -		if (++tracking->matches == 1) {
> +		switch (++tracking->matches) {
> +		case 1:
>  			string_list_append(tracking->srcs, tracking->spec.src);
>  			tracking->remote = remote->name;
> -		} else {
> +			break;
> +		case 2:
> +			/* there are at least two remotes; backfill the first one */
> +			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
> +			/* fall through */
> +		default:
> +			string_list_append(&ftb->ambiguous_remotes, remote->name);
>  			free(tracking->spec.src);
>  			string_list_clear(tracking->srcs, 0);
> +		break;

Just to sanity check this part,

 - During the first iteration, we append tracking->spec.src to
   tracking->srcs, and set tracking->remote to remote->name;

 - In later iterations, we do not want to touch tracking->srcs, and
   want to collect remote->name.

And "backfill" assumes that tracking->spec.src at that point in the
second iteration is the same as what we got in remote->name in the
first round.  If that were a correct assumption, then it is curious
that the first iteration uses tracking->spec.src and remote->name
separately for different purposes, which makes me want to double
check if the assumption is indeed correct.

If it were tracking->remote (which was assigned the value of
remote->name during the first iteration) that is used to backfill
before we append remote->name in the second iteration, I wouldn't
find it "curious", but the use of tracking->spec.src there makes me
feel confused.

Thanks.

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Tao Klerks <tao@klerks.biz>
>
> The error "not tracking: ambiguous information for ref" is raised
> when we are evaluating what tracking information to set on a branch,
> and find that the ref to be added as tracking branch is mapped
> under multiple remotes' fetch refspecs.
>
> This can easily happen when a user copy-pastes a remote definition
> in their git config, and forgets to change the tracking path.
>
> Add advice in this situation, explicitly highlighting which remotes
> are involved and suggesting how to correct the situation.
> ...
>  Documentation/config/advice.txt |  4 +++
>  advice.c                        |  1 +
>  advice.h                        |  1 +
>  branch.c                        | 63 +++++++++++++++++++++++++++++----
>  4 files changed, 62 insertions(+), 7 deletions(-)

Can we add a test case for the new "advice" output?

Thanks.

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

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>  	if (!remote_find_tracking(remote, &tracking->spec)) {
>> -		if (++tracking->matches == 1) {
>> +		switch (++tracking->matches) {
>> +		case 1:
>>  			string_list_append(tracking->srcs, tracking->spec.src);
>>  			tracking->remote = remote->name;
>> -		} else {
>> +			break;
>> +		case 2:
>> +			/* there are at least two remotes; backfill the first one */
>> +			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
>> +			/* fall through */
>> +		default:
>> +			string_list_append(&ftb->ambiguous_remotes, remote->name);
>>  			free(tracking->spec.src);
>>  			string_list_clear(tracking->srcs, 0);
>> +		break;
>
> Just to sanity check this part,
>
>  - During the first iteration, we append tracking->spec.src to
>    tracking->srcs, and set tracking->remote to remote->name;
>
>  - In later iterations, we do not want to touch tracking->srcs, and
>    want to collect remote->name.
>
> And "backfill" assumes that tracking->spec.src at that point in the
> second iteration is the same as what we got in remote->name in the
> first round.  If that were a correct assumption, then it is curious
> that the first iteration uses tracking->spec.src and remote->name
> separately for different purposes, which makes me want to double
> check if the assumption is indeed correct.
>
> If it were tracking->remote (which was assigned the value of
> remote->name during the first iteration) that is used to backfill
> before we append remote->name in the second iteration, I wouldn't
> find it "curious", but the use of tracking->spec.src there makes me
> feel confused.
>
> Thanks.

Thanks for bringing this up, I also found this unusual when I was
reading v5.

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

[Tao Klerks]

On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >>      if (!remote_find_tracking(remote, &tracking->spec)) {
> >> -            if (++tracking->matches == 1) {
> >> +            switch (++tracking->matches) {
> >> +            case 1:
> >>                      string_list_append(tracking->srcs, tracking->spec.src);
> >>                      tracking->remote = remote->name;
> >> -            } else {
> >> +                    break;
> >> +            case 2:
> >> +                    /* there are at least two remotes; backfill the first one */
> >> +                    string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
> >> +                    /* fall through */
> >> +            default:
> >> +                    string_list_append(&ftb->ambiguous_remotes, remote->name);
> >>                      free(tracking->spec.src);
> >>                      string_list_clear(tracking->srcs, 0);
> >> +            break;
> >
> > Just to sanity check this part,
> >
> >  - During the first iteration, we append tracking->spec.src to
> >    tracking->srcs, and set tracking->remote to remote->name;
> >
> >  - In later iterations, we do not want to touch tracking->srcs, and
> >    want to collect remote->name.
> >
> > And "backfill" assumes that tracking->spec.src at that point in the
> > second iteration is the same as what we got in remote->name in the
> > first round.  If that were a correct assumption, then it is curious
> > that the first iteration uses tracking->spec.src and remote->name
> > separately for different purposes, which makes me want to double
> > check if the assumption is indeed correct.
> >
> > If it were tracking->remote (which was assigned the value of
> > remote->name during the first iteration) that is used to backfill
> > before we append remote->name in the second iteration, I wouldn't
> > find it "curious", but the use of tracking->spec.src there makes me
> > feel confused.
> >
> > Thanks.
>
> Thanks for bringing this up, I also found this unusual when I was
> reading v5.

If you never hear from me again, please know it's because I crawled
back under the rock I had crawled out from. This is clearly a bug from
a silly typo, and I've managed to look at the resulting output twice
without noticing the wrong thing was printed. I'm guessing the use of
the word "unusual" here is a polite euphemism for "you numskull, what
you wrote makes no sense!" :)

I did not think adding an automated test for advise() output made
sense, but I guess I have proved myself wrong.

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

[Tao Klerks via GitGitGadget]

From: Tao Klerks <tao@klerks.biz>

The error "not tracking: ambiguous information for ref" is raised
when we are evaluating what tracking information to set on a branch,
and find that the ref to be added as tracking branch is mapped
under multiple remotes' fetch refspecs.

This can easily happen when a user copy-pastes a remote definition
in their git config, and forgets to change the tracking path.

Add advice in this situation, explicitly highlighting which remotes
are involved and suggesting how to correct the situation. Also
update a test to explicitly expect that advice.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    tracking branches: add advice to ambiguous refspec error
    
    In v7 address a wrong-value-printed bug that Junio and Glen noticed, and
    add testing for the emitted advice.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1183

Range-diff vs v6:

 1:  2408ab0ccb3 ! 1:  4fc5d2b6d13 tracking branches: add advice to ambiguous refspec error
     @@ Commit message
          in their git config, and forgets to change the tracking path.
      
          Add advice in this situation, explicitly highlighting which remotes
     -    are involved and suggesting how to correct the situation.
     +    are involved and suggesting how to correct the situation. Also
     +    update a test to explicitly expect that advice.
      
          Signed-off-by: Tao Klerks <tao@klerks.biz>
      
     @@ branch.c: struct tracking {
      +			break;
      +		case 2:
      +			/* there are at least two remotes; backfill the first one */
     -+			string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
     ++			string_list_append(&ftb->ambiguous_remotes, tracking->remote);
      +			/* fall through */
      +		default:
      +			string_list_append(&ftb->ambiguous_remotes, remote->name);
     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
       }
       
       int read_branch_desc(struct strbuf *buf, const char *branch_name)
     +
     + ## t/t3200-branch.sh ##
     +@@ t/t3200-branch.sh: test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates
     + 	git rev-parse --verify gamma@{0}
     + '
     + 
     +-test_expect_success 'avoid ambiguous track' '
     ++test_expect_success 'avoid ambiguous track and advise' '
     + 	git config branch.autosetupmerge true &&
     + 	git config remote.ambi1.url lalala &&
     + 	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&
     + 	git config remote.ambi2.url lilili &&
     + 	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&
     +-	test_must_fail git branch all1 main &&
     ++	cat <<-EOF >expected &&
     ++	fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\''
     ++	hint: There are multiple remotes whose fetch refspecs map to the remote
     ++	hint: tracking ref '\''refs/heads/main'\'':
     ++	hint:   ambi1
     ++	hint:   ambi2
     ++	hint: ''
     ++	hint: This is typically a configuration error.
     ++	hint: ''
     ++	hint: To support setting up tracking branches, ensure that
     ++	hint: different remotes'\'' fetch refspecs map into different
     ++	hint: tracking namespaces.
     ++	EOF
     ++	test_must_fail git branch all1 main 2>actual &&
     ++	test_cmp expected actual &&
     + 	test -z "$(git config branch.all1.merge)"
     + '
     + 


 Documentation/config/advice.txt |  4 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        | 63 +++++++++++++++++++++++++++++----
 t/t3200-branch.sh               | 18 ++++++++--
 5 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..343d271c707 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,6 +4,10 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	ambiguousFetchRefspec::
+		Advice shown when fetch refspec for multiple remotes map to
+		the same remote-tracking branch namespace and causes branch
+		tracking set-up to fail.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
diff --git a/advice.c b/advice.c
index 2e1fd483040..18ac8739519 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 },
diff --git a/advice.h b/advice.h
index a3957123a16..2d4c94f38eb 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 6b31df539a5..182f1c5a556 100644
--- a/branch.c
+++ b/branch.c
@@ -18,17 +18,31 @@ struct tracking {
 	int matches;
 };
 
+struct find_tracked_branch_cb {
+	struct tracking *tracking;
+	struct string_list ambiguous_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) {
+		switch (++tracking->matches) {
+		case 1:
 			string_list_append(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
-		} else {
+			break;
+		case 2:
+			/* there are at least two remotes; backfill the first one */
+			string_list_append(&ftb->ambiguous_remotes, tracking->remote);
+			/* fall through */
+		default:
+			string_list_append(&ftb->ambiguous_remotes, remote->name);
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);
+		break;
 		}
 		tracking->spec.src = NULL;
 	}
@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	struct find_tracked_branch_cb ftb_cb = {
+		.tracking = &tracking,
+		.ambiguous_remotes = STRING_LIST_INIT_DUP,
+	};
 
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
 	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;
 
@@ -252,9 +270,39 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 			goto cleanup;
 		}
 
-	if (tracking.matches > 1)
-		die(_("not tracking: ambiguous information for ref %s"),
-		    orig_ref);
+	if (tracking.matches > 1) {
+		int status = die_message(_("not tracking: ambiguous information for ref '%s'"),
+					    orig_ref);
+		if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
+			struct strbuf remotes_advice = STRBUF_INIT;
+			struct string_list_item *item;
+
+			for_each_string_list_item(item, &ftb_cb.ambiguous_remotes)
+				/*
+				 * TRANSLATORS: This is a line listing a remote with duplicate
+				 * refspecs in the advice message below. For RTL languages you'll
+				 * probably want to swap the "%s" and leading "  " space around.
+				 */
+				strbuf_addf(&remotes_advice, _("  %s\n"), item->string);
+
+			/*
+			 * TRANSLATORS: The second argument is a \n-delimited list of
+			 * duplicate refspecs, composed above.
+			 */
+			advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+				 "tracking ref '%s':\n"
+				 "%s"
+				 "\n"
+				 "This is typically a configuration error.\n"
+				 "\n"
+				 "To support setting up tracking branches, ensure that\n"
+				 "different remotes' fetch refspecs map into different\n"
+				 "tracking namespaces."), orig_ref,
+			       remotes_advice.buf);
+			strbuf_release(&remotes_advice);
+		}
+		exit(status);
+	}
 
 	if (tracking.srcs->nr < 1)
 		string_list_append(tracking.srcs, orig_ref);
@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);
+	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
 }
 
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7a0ff75ba86..e12db593615 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1039,13 +1039,27 @@ test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates
 	git rev-parse --verify gamma@{0}
 '
 
-test_expect_success 'avoid ambiguous track' '
+test_expect_success 'avoid ambiguous track and advise' '
 	git config branch.autosetupmerge true &&
 	git config remote.ambi1.url lalala &&
 	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&
 	git config remote.ambi2.url lilili &&
 	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&
-	test_must_fail git branch all1 main &&
+	cat <<-EOF >expected &&
+	fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\''
+	hint: There are multiple remotes whose fetch refspecs map to the remote
+	hint: tracking ref '\''refs/heads/main'\'':
+	hint:   ambi1
+	hint:   ambi2
+	hint: ''
+	hint: This is typically a configuration error.
+	hint: ''
+	hint: To support setting up tracking branches, ensure that
+	hint: different remotes'\'' fetch refspecs map into different
+	hint: tracking namespaces.
+	EOF
+	test_must_fail git branch all1 main 2>actual &&
+	test_cmp expected actual &&
 	test -z "$(git config branch.all1.merge)"
 '
 

base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
-- 
gitgitgadget

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

[Glen Choo]

Tao Klerks <tao@klerks.biz> writes:

> On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> >
>> >>      if (!remote_find_tracking(remote, &tracking->spec)) {
>> >> -            if (++tracking->matches == 1) {
>> >> +            switch (++tracking->matches) {
>> >> +            case 1:
>> >>                      string_list_append(tracking->srcs, tracking->spec.src);
>> >>                      tracking->remote = remote->name;
>> >> -            } else {
>> >> +                    break;
>> >> +            case 2:
>> >> +                    /* there are at least two remotes; backfill the first one */
>> >> +                    string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);
>> >> +                    /* fall through */
>> >> +            default:
>> >> +                    string_list_append(&ftb->ambiguous_remotes, remote->name);
>> >>                      free(tracking->spec.src);
>> >>                      string_list_clear(tracking->srcs, 0);
>> >> +            break;
>> >
>> > Just to sanity check this part,
>> >
>> >  - During the first iteration, we append tracking->spec.src to
>> >    tracking->srcs, and set tracking->remote to remote->name;
>> >
>> >  - In later iterations, we do not want to touch tracking->srcs, and
>> >    want to collect remote->name.
>> >
>> > And "backfill" assumes that tracking->spec.src at that point in the
>> > second iteration is the same as what we got in remote->name in the
>> > first round.  If that were a correct assumption, then it is curious
>> > that the first iteration uses tracking->spec.src and remote->name
>> > separately for different purposes, which makes me want to double
>> > check if the assumption is indeed correct.
>> >
>> > If it were tracking->remote (which was assigned the value of
>> > remote->name during the first iteration) that is used to backfill
>> > before we append remote->name in the second iteration, I wouldn't
>> > find it "curious", but the use of tracking->spec.src there makes me
>> > feel confused.
>> >
>> > Thanks.
>>
>> Thanks for bringing this up, I also found this unusual when I was
>> reading v5.
>
> If you never hear from me again, please know it's because I crawled
> back under the rock I had crawled out from. This is clearly a bug from
> a silly typo, and I've managed to look at the resulting output twice
> without noticing the wrong thing was printed. I'm guessing the use of
> the word "unusual" here is a polite euphemism for "you numskull, what
> you wrote makes no sense!" :)

Please don't take it that way! I use it the way I think most others use
it, which is a more charitable "Hm, I trust the author, but this looks
confusing to me and let me ask just to be sure that all our bases are
covered."

After all, even the best of us make mistakes, so I don't think it's a
big deal. Plus, if the mistake managed to sneak past review, the fault
is on all of us :)

> I did not think adding an automated test for advise() output made
> sense, but I guess I have proved myself wrong.

Heh, nearly every time I think that a test isn't necessary, I find a way
to prove myself wrong too.

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

[Glen Choo]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/branch.c b/branch.c
> index 6b31df539a5..182f1c5a556 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -18,17 +18,31 @@ struct tracking {
>  	int matches;
>  };
>  
> +struct find_tracked_branch_cb {
> +	struct tracking *tracking;
> +	struct string_list ambiguous_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) {
> +		switch (++tracking->matches) {
> +		case 1:
>  			string_list_append(tracking->srcs, tracking->spec.src);
>  			tracking->remote = remote->name;
> -		} else {
> +			break;
> +		case 2:
> +			/* there are at least two remotes; backfill the first one */
> +			string_list_append(&ftb->ambiguous_remotes, tracking->remote);
> +			/* fall through */
> +		default:
> +			string_list_append(&ftb->ambiguous_remotes, remote->name);
>  			free(tracking->spec.src);
>  			string_list_clear(tracking->srcs, 0);
> +		break;
>  		}
>  		tracking->spec.src = NULL;
>  	}

Ah I see, on the first iteration, we set the first remote's name to
tracking->remote, and on the second iteration, we copy that value to the
list before copying the _current_ remote's name to the list.

> -test_expect_success 'avoid ambiguous track' '
> +test_expect_success 'avoid ambiguous track and advise' '
>  	git config branch.autosetupmerge true &&
>  	git config remote.ambi1.url lalala &&
>  	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&
>  	git config remote.ambi2.url lilili &&
>  	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&
> -	test_must_fail git branch all1 main &&
> +	cat <<-EOF >expected &&
> +	fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\''
> +	hint: There are multiple remotes whose fetch refspecs map to the remote
> +	hint: tracking ref '\''refs/heads/main'\'':
> +	hint:   ambi1
> +	hint:   ambi2
> +	hint: ''
> +	hint: This is typically a configuration error.
> +	hint: ''
> +	hint: To support setting up tracking branches, ensure that
> +	hint: different remotes'\'' fetch refspecs map into different
> +	hint: tracking namespaces.
> +	EOF
> +	test_must_fail git branch all1 main 2>actual &&
> +	test_cmp expected actual &&
>  	test -z "$(git config branch.all1.merge)"
>  '

And this test shows that this indeed does what we think it does. Nicely
done.

I notice that we there is an instance of test -z "$(some git command)",
which IIRC is discouraged because it would mask a failure in the git
command, but that's not new and I don't think it needs to be fixed in
this series anyway.

Reviewed-by: Glen Choo <chooglen@google.com>

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

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/branch.c b/branch.c
>> index 6b31df539a5..182f1c5a556 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -18,17 +18,31 @@ struct tracking {
>>  	int matches;
>>  };
>>  
>> +struct find_tracked_branch_cb {
>> +	struct tracking *tracking;
>> +	struct string_list ambiguous_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) {
>> +		switch (++tracking->matches) {
>> +		case 1:
>>  			string_list_append(tracking->srcs, tracking->spec.src);
>>  			tracking->remote = remote->name;
>> -		} else {
>> +			break;
>> +		case 2:
>> +			/* there are at least two remotes; backfill the first one */
>> +			string_list_append(&ftb->ambiguous_remotes, tracking->remote);
>> +			/* fall through */
>> +		default:
>> +			string_list_append(&ftb->ambiguous_remotes, remote->name);
>>  			free(tracking->spec.src);
>>  			string_list_clear(tracking->srcs, 0);
>> +		break;
>>  		}
>>  		tracking->spec.src = NULL;
>>  	}
>
> Ah I see, on the first iteration, we set the first remote's name to
> tracking->remote, and on the second iteration, we copy that value to the
> list before copying the _current_ remote's name to the list.
>
>> -test_expect_success 'avoid ambiguous track' '
>> +test_expect_success 'avoid ambiguous track and advise' '
>>  	git config branch.autosetupmerge true &&
>>  	git config remote.ambi1.url lalala &&
>>  	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&
>>  	git config remote.ambi2.url lilili &&
>>  	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&
>> -	test_must_fail git branch all1 main &&
>> +	cat <<-EOF >expected &&
>> +	fatal: not tracking: ambiguous information for ref '\''refs/heads/main'\''
>> +	hint: There are multiple remotes whose fetch refspecs map to the remote
>> +	hint: tracking ref '\''refs/heads/main'\'':
>> +	hint:   ambi1
>> +	hint:   ambi2
>> +	hint: ''
>> +	hint: This is typically a configuration error.
>> +	hint: ''
>> +	hint: To support setting up tracking branches, ensure that
>> +	hint: different remotes'\'' fetch refspecs map into different
>> +	hint: tracking namespaces.
>> +	EOF
>> +	test_must_fail git branch all1 main 2>actual &&
>> +	test_cmp expected actual &&
>>  	test -z "$(git config branch.all1.merge)"
>>  '
>
> And this test shows that this indeed does what we think it does. Nicely
> done.
>
> I notice that we there is an instance of test -z "$(some git command)",
> which IIRC is discouraged because it would mask a failure in the git
> command, but that's not new and I don't think it needs to be fixed in
> this series anyway.
>
> Reviewed-by: Glen Choo <chooglen@google.com>

Thanks for giving the patch a careful read.

Will queue.

Thanks.