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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 6, 2015, 19:03 UTC
Message-ID: <vpqmvvvhl9m.fsf@grenoble-inp.fr>
In-Reply-To: <CAOLa=ZS5x=ksfnBt1kLp5bJJHmqeBztR7Zn7U5VKZN-56T-_5A@mail.gmail.com>

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).

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).

> 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

Show 5 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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/