tracking branches: add advice to ambiguous refspec error

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?

advice.h +1 -0

index 601265fd107..3d68c1a6cb4 100644
@@ -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,

branch.c +0 -0

index 5d20a2e8484..243f6d8b362 100644

branch.c +1 -0

 /*
  * 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;

branch.c +3 -0

+			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

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?

Quoted 8 lines

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;

Quoted 4 lines

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

Quoted 6 lines

advice.c +0 -0

index 1dfc91d1767..686612590ec 100644
@@ -39,6 +39,7 @@ static struct {
      [ADVICE_ADD_EMPTY_PATHSPEC]                     = { "addEmptyPathspec", 1 },

advice.c static struct { · line 45

Quoted 6 lines

advice.c +1 -0

      [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

Quoted 22 lines

advice.h +1 -0

index 601265fd107..3d68c1a6cb4 100644
@@ -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,

branch.c +1 -0

index 5d20a2e8484..243f6d8b362 100644
@@ -12,6 +12,7 @@
 struct tracking {
      struct refspec_item spec;
      struct string_list *srcs;
+     struct string_list *remotes;
 

branch.c static int inherit_tracking(struct tracking *tracking, const char *orig_ref) · line 226

Quoted 2 lines

branch.c +2 -0

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

Quoted 8 lines

branch.c +1 -0

 /*
  * 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;

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 254

Quoted 10 lines

branch.c +2 -0

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

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 273

Quoted 12 lines

branch.c +6 -1

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

From: Tao Klerks <[email protected]>
[...]

/*
* 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

Documentation/config/advice.txt +5 -4

@@ -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);

From: Tao Klerks <[email protected]>

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.

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

Documentation/config/advice.txt +0 -0

index c40eb09cb7e..90f7dbd03aa 100644

branch.c +7 -1

index 6b31df539a5..5c28d432103 100644
@@ -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) {

branch.c +0 -0

 			string_list_append(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
 		} else {
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);

Quoted 26 lines

branch.c +7 -1

index 6b31df539a5..5c28d432103 100644
@@ -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) {

branch.c +0 -0

 			string_list_append(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
 		} else {
 			free(tracking->spec.src);
 			string_list_clear(tracking->srcs, 0);

branch.c static int find_tracked_branch(struct remote *remote, void *priv) · line 47

Quoted 10 lines

branch.c +7 -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.

Quoted 2 lines · Glen Choo

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

Quoted 2 lines

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

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.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Glen Choo

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

[...]

Quoted 2 lines

Quoted 2 lines

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?

Quoted 6 lines

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

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

Small things accumulate.

I believe this third version incorporates all Ævar's suggestions, and might be usable. Removed "RFC" prefix.

From: Tao Klerks <[email protected]>

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 <[email protected]>

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

branch.c +5 -1

 /*
  * 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;
 

branch.c +1 -0

 	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

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

Nit: I think it's been Junio's preference not to introduce C99 comments,

branch.c static int inherit_tracking(struct tracking *tracking, const char *orig_ref) · line 236

Quoted 5 lines

branch.c +1 -0

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

Stray whitespace?

Quoted 26 lines

branch.c +5 -1

 /*
  * 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;
              }
 

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 276

Quoted 6 lines

branch.c +3 -3

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

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 281

Quoted 5 lines

branch.c +5 -0

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

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 289

Quoted 9 lines

branch.c +8 -0

+                             /*
+                              * 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.
*/

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 301

Quoted 12 lines

branch.c +12 -0

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

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 305

Quoted 4 lines

branch.c +4 -0

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

Quoted 8 lines · Ævar Arnfjörð Bjarmason

Quoted 3 lines · Tao Klerks via GitGitGadget

branch.c +3 -0

+     if (tracking.matches > 1) {
+             int status = die_message(_("not tracking: ambiguous information for ref %s"),
+                                         orig_ref);

Quoted 4 lines

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)

Quoted 9 lines

Quoted 8 lines

branch.c +7 -0

+                              * 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);
+                     }
+

Quoted 11 lines · Tao Klerks

Quoted 8 lines · Ævar Arnfjörð Bjarmason

Quoted 3 lines · Tao Klerks via GitGitGadget

branch.c +3 -0

+     if (tracking.matches > 1) {
+             int status = die_message(_("not tracking: ambiguous information for ref %s"),
+                                         orig_ref);

Quoted 8 lines

Quoted 4 lines

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.

branch.c struct tracking { · line 38

Quoted 4 lines · Tao Klerks via GitGitGadget

From: Tao Klerks <[email protected]>

branch.c +2 -0

+		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/[email protected]/)

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 276

Quoted 3 lines

branch.c +3 -0

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

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 301

Quoted 6 lines

branch.c +6 -0

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

Quoted 8 lines · Ævar Arnfjörð Bjarmason

branch.c struct tracking { · line 38

Quoted 4 lines · Tao Klerks via GitGitGadget

From: Tao Klerks <[email protected]>

branch.c +2 -0

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

Quoted 3 lines

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/[email protected]/)

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

Quoted 7 lines

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 276

Quoted 3 lines

branch.c +3 -0

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

Quoted 11 lines

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 301

Quoted 6 lines

branch.c +6 -0

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

Quoted 12 lines

Quoted 11 lines

branch.c static void setup_tracking(const char *new_ref, const char *orig_ref, · line 301

Quoted 6 lines

branch.c +6 -0

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

Quoted 3 lines

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.

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 12 lines

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

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

Quoted 6 lines

Quoted 3 lines

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

branch.c +2 -2

 	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 {

From: Tao Klerks <[email protected]>

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

Quoted 7 lines · Tao Klerks via GitGitGadget

branch.c +2 -2

 	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 {

branch.c struct tracking { · line 45

Quoted 10 lines

branch.c +8 -0

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

Quoted 41 lines · Junio C Hamano

Quoted 7 lines · Tao Klerks via GitGitGadget

     if (!remote_find_tracking(remote, &tracking->spec)) {

branch.c +2 -2

-            if (++tracking->matches == 1) {
+            switch (++tracking->matches) {
+            case 1:
                     string_list_append(tracking->srcs, tracking->spec.src);
                     tracking->remote = remote->name;
-            } else {

branch.c struct tracking { · line 45

Quoted 10 lines

branch.c +8 -0

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

Quoted 1 line

Thanks.

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

Quoted 44 lines · Glen Choo

Quoted 41 lines · Junio C Hamano

Quoted 7 lines · Tao Klerks via GitGitGadget

     if (!remote_find_tracking(remote, &tracking->spec)) {

branch.c +2 -2

-            if (++tracking->matches == 1) {
+            switch (++tracking->matches) {
+            case 1:
                     string_list_append(tracking->srcs, tracking->spec.src);
                     tracking->remote = remote->name;
-            } else {

branch.c struct tracking { · line 45

Quoted 10 lines

branch.c +8 -0

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

Quoted 4 lines

Quoted 1 line

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.

branch.c +12 -3

index 6b31df539a5..182f1c5a556 100644
@@ -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 */

branch.c +8 -2

-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: ''

Quoted 29 lines · Tao Klerks via GitGitGadget

branch.c +12 -3

index 6b31df539a5..182f1c5a556 100644
@@ -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 */

branch.c struct tracking { · line 48

Quoted 10 lines

branch.c +5 -0

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

Quoted 15 lines

branch.c +8 -2

-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: ''

branch.c

Quoted 10 lines

branch.c +8 -0

+	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 <[email protected]>