On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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
>
> 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.
>> 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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html