Karthik Nayak <karthik....@gmail.com> writes:
> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
> <matthieu....@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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
