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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> -     if (upstream_is_gone) {
>> -             if (show_upstream_ref)
>> -                     strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
>
> The old string was translated, and you're replacing it with one which
> isn't.
>
> I'm not a big fan of translation, so that change doesn't disturb me, but
> people used to having "git branch" talk their native language here may
> care.
>
> Be careful: translation should happen only in porcelain. Typicall,
> "for-each-ref" should anyway continue saying "gone" in english whatever
> the configuration is.
>
> See e.g. 7a76c28 (status: disable translation when --porcelain is used,
> 2014-03-20) for an example of translation only for porcelain (that was
> for status, but also for ahead/behind/gone).
>

>> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
>
> I'd call that "build_format", since "get_..." usually implies that the
> value exists already and you're retrieving it.
>

>> +{
>> +     char *remote = NULL;
>> +     char *local = NULL;
>> +     char *final = NULL;
>> +
>> +     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));
>
> OK, that is a hell of a block of code ;-).
>
> You should factor the common portions out of these if/else. C allows you
> to write several string litteral next to each other, so you can eg.
>
> #define STAR_IF_HEAD "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)"
> #define ALIGNED_REFNAME "%%(align:%d,left)%%(refname:short)%%(end)"
>
> and then
>
> STAR_IF_HEAD ALIGNED_REFNAME ...
>
> 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.
>

>> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
>>  '
>>
>>  cat >expect <<\EOF
>> -b1 [origin/master: ahead 1, behind 1] d
>> -b2 [origin/master: ahead 1, behind 1] d
>> -b3 [origin/master: behind 1] b
>> -b4 [origin/master: ahead 2] f
>> -b5 [brokenbase: gone] g
>> +b1 [origin/master] [ahead 1, behind 1] d
>> +b2 [origin/master] [ahead 1, behind 1] d
>> +b3 [origin/master] [behind 1] b
>> +b4 [origin/master] [ahead 2] f
>> +b5 [brokenbase] [gone] g
>
> This corresponds to this part of the commit message:
>
>> Since branch.c is being ported to use ref-filter APIs to print the
>> required data, it is constricted to the constraints of printing as per
>> ref-filter. Which means branch.c can only print as per the atoms
>> available in ref-filter. Hence the "-vv" option of 'git branch' now
>> prints the upstream and its tracking details separately as
>> "[<upstream>] [<tracking info>]" instead of "[<upstream>: <tracking
>> info>]".
>>
>> Make changes in /t/t6040-tracking-info.sh to reflect the change.
>
> nit: /t/t... -> t/t... (remove leading /)
>
> That is a behavior change, and I actually prefered the previous one.
>
> I actually don't understand why you can't get the old syntax: the []
> around the refname come from the format string it get_format(), so you
> could decide to change "[%s%%(upstream:short)%s]" to
> "[%s%%(upstream:short)%s: ". The brackets around the status are a bit
> more tricky, since they were here before your series. But we could have
> a %(upstream:track,nobracket) to display just the status without the
> brackets around.
>
> Then, the code must deal with up-to-date branches for which no
> " :ahead/behind" must be displayed. Probably one more if/then/else
> nested in the first one.
>
> So, it seems feasible to me to keep the old behavior with the new
> system, even though it's going to be a bit tricky.
>