Re: [PATCH 3/3] for-each-ref: introduce %(upstream:track[short])

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sep 27, 2013, 14:27 UTC
Message-ID: <CALkWK0nFjuPprR3kcrCwJFj6j43HVwVTjHo-=__TfSpbcX912g@mail.gmail.com>
In-Reply-To: <CABURp0rrMV2Em=acG80KqqByo1nurC5azU5UBWB0L5p3Zcra8A@mail.gmail.com>

Phil Hord wrote:

Show 10 quoted lines

>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -631,6 +631,7 @@ static void populate_value(struct refinfo *ref)
>>         int eaten, i;
>>         unsigned long size;
>>         const unsigned char *tagged;
>> +       int upstream_present = 0;
>
> This flag is out of place.  It should be in the same scope as 'branch'
> since the code which depends on this flag also depends on '!!branch'.

Agreed. Fixed.

> However, I don't think it is even necessary.  The only way to reach
> the places where this flag is tested is when (name="upstream") and
> (upstream exists).  In all other cases, the parser loops before
> reaching the track/trackshort code or else it doesn't enter it.

Yeah, you're right. I was setting upstream_present in this snippet:

  else if (!prefixcmp(name, "upstream")) {
  /* only local branches may have an upstream */
    if (prefixcmp(ref->refname, "refs/heads/"))
      continue;

If the refname doesn't begin with "refs/heads" in the first place (which is what I was guarding against), the code will loop and never reach the track[short] code anyway.

upstream_present factored out now.

Show 22 quoted lines

>> @@ -698,11 +701,48 @@ static void populate_value(struct refinfo *ref)
>>                 formatp = strchr(name, ':');
>>                 /* look for "short" refname format */
>>                 if (formatp) {
>> +                       int num_ours, num_theirs;
>> +
>>                         formatp++;
>>                         if (!strcmp(formatp, "short"))
>>                                 refname = shorten_unambiguous_ref(refname,
>>                                                       warn_ambiguous_refs);
>> -                       else
>> +                       else if (!strcmp(formatp, "track") &&
>> +                               !prefixcmp(name, "upstream")) {
>> +                               char buf[40];
>> +
>> +                               if (!upstream_present)
>> +                                       continue;
>> +                               stat_tracking_info(branch, &num_ours, &num_theirs);
>> +                               if (!num_ours && !num_theirs)
>> +                                       v->s = "";
>
> Is this the same as 'continue'?

I'll leave this as it is for readability reasons.

Thanks for the review.