Re: [PATCH 8/9] branch: use ref-filter printing APIs

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 7, 2015, 16:47 UTC
Message-ID: <CAOLa=ZS4xVfsQ+F+KGD8Gz9NHzvJtuVvW16RSLV08ZiEv8DhKw@mail.gmail.com>
In-Reply-To: <vpqmvvvhl9m.fsf@grenoble-inp.fr>

On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 16 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> If you look closely, thats only for the local branches, the remotes
>> have `align` atom when
>> printing in verbose.
>
> Yes, but that's already one thing factored out of the if, even if it's
> just for local.
>
> Actually, I think you can also factor some parts out of the
> %(if:notequals=remotes). In 'local', you have an %(if) to display either
> "* " or "  ", and in remote you always start with "  ". Why not always
> apply the %(if), and let it display "  " if not displaying the current
> branch? Similarly, the "verbose" part of remote branches seems like a
> particular case of the one for local ones (remotes don't have tracking
> branches, so the tracking info should expand to the empty string).

We could factor out the " " and the "* " printing. I'll do that

Show 6 quoted lines

>
> To go a bit further, you can pre-build a string or strbuf aligned_short
> with value like "%%(align:20,left)%%(refname:short)%%(end)" and use it
> where needed (it's not a constant because you have to introduce maxwidth
> into the string, so it's not a candidate for #define).
>

Again, the remote has a remote prefix here, so we can't really factor it out much.

Show 7 quoted lines

>> I could cook up this:
>
> Your mailer broke the formatting, so it looks terrible, but from what I
> could parse, it's already much better than the previous one. It's not a
> matter of size of the function, I very much prefer reading 10 lines of
> nice code than 4 lines like
>

I get what you're saying I'll see if I can factor out more.

Show 13 quoted lines

>> +                        local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s"
>> +                                        " %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)"
>> +                                        "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)",
>> +                                        branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),
>> +                                        branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
>
> ;-).
>
> One obvious issue with the initial version was this big hard-to-parse
> block, but another one is that the code did not make it easy to
> understand what was changing depending on which branch of the if, and
> depending on local/remote. It's getting much easier already.
>

Yea, we can say that :)

-- 
Regards,
Karthik Nayak