Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
> <Matthieu.Moy@xxxxxxxxxxxxxxx> 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/
--
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