Thread (2 messages) 2 messages, 2 authors, 2022-03-28

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

From: Glen Choo <hidden>
Date: 2022-03-28 17:12:14

Junio C Hamano [off-list ref] writes:
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help