On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<[email protected]> wrote:
> Karthik Nayak <[email protected]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
