On Tue, Oct 6, 2015 at 12:13 AM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> 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").
>
If you look closely, thats only for the local branches, the remotes
have `align` atom when
printing in verbose.
So like I said, I couldn't really make it quite simpler. Maybe a line
or two, but using `#define`
I could cook up this:
char *remote = NULL;
char *final = NULL;
struct strbuf local = STRBUF_INIT;
strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)",
branch_get_color(BRANCH_COLOR_CURRENT));
if (filter->verbose) {
strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)",
maxwidth);
strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
strbuf_addf(&local, " %%(objectname:short=7) ");
if (filter->verbose > 1)
strbuf_addf(&local,
"%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
"%%(then): %%(upstream:track,nobracket)%%(end)]
%%(end)%%(contents:subject)",
branch_get_color(BRANCH_COLOR_UPSTREAM),
branch_get_color(BRANCH_COLOR_RESET));
else
strbuf_addf(&local,
"%%(if)%%(upstream:track)%%(then)%%(upstream:track)
%%(end)%%(contents:subject)");
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));
} else {
strbuf_addf(&local, "%%(refname:short)%s",
branch_get_color(BRANCH_COLOR_RESET));
remote = xstrfmt("
%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) ->
%%(symref:short)%%(end)",
branch_get_color(BRANCH_COLOR_REMOTE),
remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
}
final = xstrfmt("%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
local.buf, remote);
strbuf_release(&local);
free(remote);
return final;
Couldn't see anything else I could break down.
--
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