On 2021.12.20 10:29, Glen Choo wrote:
> Josh Steadmon <steadmon@xxxxxxxxxx> writes:
>
> >> > @@ -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'.
Yeah, I think that's better. Fixed in V7, thanks.
> >> > } 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?
Yeah, the translation-unfriendliness is why I avoided this approach.
