Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak <karthik.188@gmail.com>]

On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
>> +This prefixes the current branch with a star.
>> +
>> +------------
>> +#!/bin/sh
>> +
>> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/
>
> I don't think the #!/bin/sh adds any value here. Just the 'git
> for-each-ref' line is sufficient IMHO.
>

Sure, we can remove that.

>> +An example to show the usage of %(if)...%(then)...%(end).
>> +This adds a red color to authorname, if present
>
> I don't think this is such a good example.
> %(color:red)%(authorname)%(color:reset) just works even if %(authorname)
> is empty.
>
> A better example would be
>
> git for-each-ref --format='%(if)%(authorname)%(then)Authored by %(authorname)%(end)'
>
> which avoids writting "Authored by " with no author.
>

Yeah, will include this.

>> -static int is_empty(const char * s){
>> +static int is_empty(const char *s){
>
> You still have the { on the declaration line, it should be on the next
> line.
>

Oops, will change that.

>> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
>>  static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>>  {
>>       struct ref_formatting_stack *cur = state->stack;
>> -     struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> +     struct if_then_else *if_then_else = NULL;
>>
>> +     if (cur->at_end == if_then_else_handler)
>> +             if_then_else = (struct if_then_else *)cur->at_end_data;
>
> OK, now the cast is safe since at_end_data has to be of type struct
> if_then_else * if at_end is if_then_else_handler.
>
>> +                             unsigned int nobracket = 0;
>> +
>> +                             if (!strcmp(valp, ",nobracket"))
>> +                                     nobracket = 1;
>
> The code to parse comma-separated values is different here and
> elsewhere. I'd rather have the same code (possibly factored into a
> helper function), both to get consistent behavior (you're not allowing
> %(upstream:nobracket,track) for example, right?) and consistent code.
>

Eh, okay, will do that!

>>                               if (!num_ours && !num_theirs)
>>                                       v->s = "";
>>                               else if (!num_ours) {
>> -                                     sprintf(buf, "[behind %d]", num_theirs);
>> +                                     if (nobracket)
>> +                                             sprintf(buf, "behind %d", num_theirs);
>> +                                     else
>> +                                             sprintf(buf, "[behind %d]", num_theirs);
>
> Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
> unconditionnally, and set obracket = "" or obracket = "[" once and for
> all when you test for "nobracket" above. This avoids these "if
> (nobracket)" spread accross the code, but at the price of extra %s in
> the format strings.
>

Okay! will do that.

>> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
>>                               else
>>                                       v->s = "<>";
>>                               continue;
>> +                     } else if (!strcmp(formatp, "dir") &&
>> +                                (starts_with(name, "refname"))) {
>> +                             const char *sp, *ep, *tmp;
>> +
>> +                             sp = tmp = ref->refname;
>> +                             /*  Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
>> +                             do {
>> +                                     ep = tmp;
>> +                                     tmp = strchr(ep + 1, '/');
>> +                             } while (tmp);
>
> To look for the last occurence of '/' you can also use strrchr().
> Doesn't it do what you want here?
>

Didn't know that, thanks will do that.

>> --- a/t/t6040-tracking-info.sh
>> +++ b/t/t6040-tracking-info.sh
>> @@ -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
>>  b6 [origin/master] c
>>  EOF
>
> Cool!
>
> I didn't go through the patches themselves, but modulo my remarks above
> the interdiff looks good. Thanks.
>

Thanks for the review.

-- 
Regards,
Karthik Nayak