Re: [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()

From: Karthik Nayak <karthik.188@gmail.com>
Date: Dec 24, 2015, 07:42 UTC
Message-ID: <CAOLa=ZQ1SXm6GG0jzokq7shrm0=4B8vvME13Qbh_QCEWt2X=0A@mail.gmail.com>
In-Reply-To: <CAPig+cRG_HTWL=VNqOxDnv_M0NbYZSReCHSsGe1CJiFQS8h-XQ@mail.gmail.com>

On Thu, Dec 17, 2015 at 2:52 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 19 quoted lines

> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -37,6 +37,8 @@ static struct used_atom {
>>         union {
>>                 const char *color;
>>                 struct align align;
>> +               enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
>
> Nit: I'd have expected to see the normal/plain case first rather than
> last (but not itself worth a re-roll).
>

Will add it in. That'll put it in an alphabetical order too.

Show 31 quoted lines

>> +                       remote_ref;
>>         } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)
>> +static void remote_ref_atom_parser(struct used_atom *atom)
>> +{
>> +       const char *buf;
>> +
>> +       buf = strchr(atom->str, ':');
>> +       atom->u.remote_ref = RR_NORMAL;
>> +       if (!buf)
>> +               return;
>
> This code is not as clear as it could be due to the way the 'buf'
> assignment and check for NULL are split apart. It can be made clearer
> either by doing this:
>
>     atom->u.remote_ref = RR_NORMAL;
>     buf = strchr(...);
>     if (!buf)
>         return;
>
> or (even better) this:
>
>     buf = strchr(...);
>     if (!buf) {
>         atom->u.remote_ref = RR_NORMAL;
>         return;
>     }
>

Will do the latter, thanks.

Show 26 quoted lines

>> +       buf++;
>> +       if (!strcmp(buf, "short"))
>> +               atom->u.remote_ref = RR_SHORTEN;
>> +       else if (!strcmp(buf, "track"))
>> +               atom->u.remote_ref = RR_TRACK;
>> +       else if (!strcmp(buf, "trackshort"))
>> +               atom->u.remote_ref = RR_TRACKSHORT;
>> +       else
>> +               die(_("unrecognized format: %%(%s)"), atom->str);
>> +}
>> +
>> @@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)
>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> +                                   struct branch *branch, const char **s)
>> +{
>> +       int num_ours, num_theirs;
>> +       if (atom->u.remote_ref == RR_SHORTEN)
>> +               *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>> +       else if (atom->u.remote_ref == RR_TRACK) {
>> +               if (stat_tracking_info(branch, &num_ours,
>> +                                      &num_theirs, NULL))
>> +                       return;
>
> The RR_TRACKSHORT branch below has a blank line following the
> 'return', but this branch lacks it, which is inconsistent.
>

will add.

Show 16 quoted lines

>> +               if (!num_ours && !num_theirs)
>> +                       *s = "";
>> +               else if (!num_ours)
>> +                       *s = xstrfmt("[behind %d]", num_theirs);
>> +               else if (!num_theirs)
>> +                       *s = xstrfmt("[ahead %d]", num_ours);
>> +               else
>> +                       *s = xstrfmt("[ahead %d, behind %d]",
>> +                                    num_ours, num_theirs);
>> +       } else if (atom->u.remote_ref == RR_TRACKSHORT) {
>> +               if (stat_tracking_info(branch, &num_ours,
>> +                                      &num_theirs, NULL))
>
> What happened to the assert(branch) which was in the original code
> from which this was derived (below)? Is it no longer needed?
>

stat_tracking_info() takes care of that, instead of aborting, we gracefully continue while leaving that value empty.

-- 
Regards,
Karthik Nayak