On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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).
>
I'm not how translation works, thanks for the hint, I'll look around.
>> +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.
>
Makes sense.
>> +{
>> + 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.
>
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. Just
that instead of using variables like "local", "remote" and "final" we'd just
make multiple calls to strbuf_addf().
>> @@ -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.
>
Wasn't that hard to do, I got this working, I was just reluctant on making
a new subvalue like "%(upstream:track,nobracket)" (no real reason, just didn't
want to ).
--
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