Re: [PATCH v6 1/3] branch: accept multiple upstream branches for tracking

Josh Steadmon [off-list ref] writes:

quoted

quoted

@@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
 	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
-		if (shortname) {
+		const char *name;
+		struct strbuf ref_string = STRBUF_INIT;
+
+		for_each_string_list_item(item, remotes) {
+			name = item->string;
+			skip_prefix(name, "refs/heads/", &name);
+			strbuf_addf(&ref_string, "  %s\n", name);
+		}
+
+		if (remotes->nr == 1) {
+			struct strbuf refname = STRBUF_INIT;
+
 			if (origin)
-				printf_ln(rebasing ?
-					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
-					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
-					  local, shortname, origin);
-			else
-				printf_ln(rebasing ?
-					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
-					  _("Branch '%s' set up to track local branch '%s'."),
-					  local, shortname);
+				strbuf_addf(&refname, "%s/", origin);
+			strbuf_addstr(&refname, remotes->items[0].string);
+
+			/*
+			 * Rebasing is only allowed in the case of a single
+			 * upstream branch.
+			 */
+			printf_ln(rebasing ?
+				_("branch '%s' set up to track '%s' by rebasing.") :
+				_("branch '%s' set up to track '%s'."),
+				local, refname.buf);
+
+			strbuf_release(&refname);
+		} else if (origin) {
+			printf_ln(_("branch '%s' set up to track from '%s':"),
+				local, origin);
+			printf("%s", ref_string.buf);

It's not clear to me why the hint contains the word 'from' when it is a
remote ref...

Because in the multiple-branch case, we don't prepend the origin to each
ref, so we need to let users know which remote the refs are coming from.

I see. So if I'm reading this correctly, the error message in the remote
case would read something like:

  branch 'main' set up to track from 'origin':
    main
    topic1
    topic2

Is there any reason why we couldn't append the origin to the ref to make
it consistent? I think this could be as simple as:


	for_each_string_list_item(item, remotes) {
		name = item->string;
		skip_prefix(name, "refs/heads/", &name);
			if (origin)
+         strbuf_addf(&ref_string, "%s/", origin);
		strbuf_addf(&ref_string, "  %s\n", name);
	}

and the resulting list could look like:

  branch 'main' set up to track from 'origin':
    origin/main
    origin/topic1
    origin/topic2

This looks repetitive, but I suggest this because, as I understand it,
we are omitting the "{local,remote} ref" phrase based on conventions
around ref names, like "origin/main" is probably a remote ref and not an
oddly named local ref. However, when we print the list like so,

  branch 'main' set up to track from 'origin':
    main
    topic1
    topic2

we now expect the user to understand that 'main', 'topic1' and 'topic2'
to implicitly have 'origin/' prepended to them. This behavior seems
inconsistent to me; I'd anticipate most users responding "Wait, I was
supposed to be tracking 'origin' branches right? Why am I looking at
local branches?". Some users would be able to recover because they can
figure out what we mean, but others might just give up.

Prepending 'origin/' would get rid of this problem altogether, and it
would let us drop the 'from'.

quoted

quoted

 		} else {
-			if (origin)
-				printf_ln(rebasing ?
-					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
-					  _("Branch '%s' set up to track remote ref '%s'."),
-					  local, remote);
-			else
-				printf_ln(rebasing ?
-					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
-					  _("Branch '%s' set up to track local ref '%s'."),
-					  local, remote);
+			printf_ln(_("branch '%s' set up to track:"), local);
+			printf("%s", ref_string.buf);

but does not have the word 'from' when it is a local ref. As far as I
can tell, this is the only difference between remote and local refs, and
adding the word 'from' does not seem like a good enough reason to add an
'if' condition. Maybe I missed something here?

This motivates my answer to the question you asked in [1]:

  I removed as many distinctions as possible, as most can still be
  inferred from context. [...] Likewise, we don't need to specify whether
  refs are remote or local: "some-remote/some-branch" vs.
  "a-local-branch" should be understandable without us spelling it out.

I agree that there is adequate context, so I would be ok with the
simplification if there was corresponding code simplification e.g.
dropping "if (origin)". But in its current form, I don't think there is
good enough reason to simplify the message.

I think the proper point of comparison is not the original code, but the
code from V5 where we try to preserve the same level of detail in output
as the original code. If we are committed to both having multiple
remotes and keeping similar styles of output as the original
implementation, then something like the massive conditional in V5 is
unavoidable.

I see. So for instance, post-simplification you have:

  printf_ln(rebasing ?
    _("branch '%s' set up to track '%s' by rebasing.") :
    _("branch '%s' set up to track '%s'."),
    local, refname.buf);

if you preserve the same amount of detail as before, you'd have to
distinguish between local/remote, which doubles the number of cases to
4, which is why the conditional v5 is so complicated.

That said, I think that it's already much simpler than v5 because you've
split the singular and plural cases. I wonder if you have considered
building the final string purely from format strings, like:

  char *message_format = _("branch %s set up to track %s%s%s%s");
  char *ref_type_clause = origin ? " remote ref " : " local ref ";
  char *rebasing_clause = rebasing ? " by rebasing." : ".";
  char *branch_names = "<branch names>";
  printf_ln(message_format, local, ref_type_clause, branch_names, rebasing_clause);

This sounds potentially unfriendly to i18n, but it would make the
conditional simpler. What do you think?