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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 5, 2015, 18:43 UTC
Message-ID: <vpq7fn1qhp2.fsf@grenoble-inp.fr>
In-Reply-To: <CAOLa=ZSk8-6nkfEd+Kz1srOJxPLj6+zLEU9DnLgW3rW1O6kZGg@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 24 quoted lines

> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Actually, this is not a performance-cricical piece of code at all, so I
>> think it's even better to build an strbuf little by little using
>> repeated strbuf_addf calls. This way you can do things like
>>
>> strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
>>             branch_get_color(BRANCH_COLOR_CURRENT));
>> strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
>>
>> which makes it much easier to pair the %s and the corresponding
>> branch_get_color() or the %d and maxwidth.
>>
>> But fundamentally, I think this function is doing the right thing. The
>> function is a bit complex (I think it will be much nicer to read when
>> better factored), but 1) it allows getting rid of a lot of specific code
>> and 2) it's a proof that --format is expressive enough.
>>
>
> I like the idea of using "#define" it might make things simpler.
>
> Not sure about using a strbuf cause I don't see how that could make things
> simpler, we'll end up with something similar to what we have
> currently.

It allows you to really break the way you build the string into several small steps, and use if/then locally on steps which require it.

For example, you currently have

+        if (filter->verbose) {
+                if (filter->verbose > 1)
+                        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));
+
+                else
+                        local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(align:%d,left)%"
+                                        "%(refname:short)%%(end)%s %%(objectname:short,7) %%(if)%%(upstream:track)%"
+                                        "%(then)%%(upstream:track) %%(end)%%(contents:subject)",
+                                        branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET));
+
+                remote = xstrfmt("  %s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
+                                 "%%(else) %%(objectname:short,7) %%(contents:subject)%%(end)",
+                                 branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
+                                 remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
+                final = xstrfmt("%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)", local, remote);
+
+        } else {
+                local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(refname:short)%s",

The first bits of local are identical in all branches of the two-level if/else. You could use something like

	strbuf_addf(format, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
		    branch_get_color(BRANCH_COLOR_CURRENT));
	if (filter->verbose) {
		...
	}

to factor it out of the if/else. Similarly, the "if (filter->verbose > 1)" could be much smaller, and probably doesn't require an else branch (just say "if very verbose, then add XYZ at this point in the format string").

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