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

[Eric Sunshine <sunshine@sunshineco.com>]

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).

> +                       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;
    }

> +       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.

> +               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?

> +                       return;
> +
> +               if (!num_ours && !num_theirs)
> +                       *s = "=";
> +               else if (!num_ours)
> +                       *s = "<";
> +               else if (!num_theirs)
> +                       *s = ">";
> +               else
> +                       *s = "<>";
> +       } else if (atom->u.remote_ref == RR_NORMAL)
> +               *s = refname;
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -889,6 +946,8 @@ static void populate_value(struct ref_array_item *ref)
>                         refname = branch_get_upstream(branch, NULL);
>                         if (!refname)
>                                 continue;
> +                       fill_remote_ref_details(atom, refname, branch, &v->s);
> +                       continue;
>                 } else if (starts_with(name, "push")) {
>                         const char *branch_name;
>                         if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -899,6 +958,8 @@ static void populate_value(struct ref_array_item *ref)
>                         refname = branch_get_push(branch, NULL);
>                         if (!refname)
>                                 continue;
> +                       fill_remote_ref_details(atom, refname, branch, &v->s);
> +                       continue;
>                 } else if (starts_with(name, "color:")) {
>                         char color[COLOR_MAXLEN] = "";
>
> @@ -944,49 +1005,11 @@ static void populate_value(struct ref_array_item *ref)
>
>                 formatp = strchr(name, ':');
>                 if (formatp) {
> -                       int num_ours, num_theirs;
> -
>                         formatp++;
>                         if (!strcmp(formatp, "short"))
>                                 refname = shorten_unambiguous_ref(refname,
>                                                       warn_ambiguous_refs);
> -                       else if (!strcmp(formatp, "track") &&
> -                                (starts_with(name, "upstream") ||
> -                                 starts_with(name, "push"))) {
> -
> -                               if (stat_tracking_info(branch, &num_ours,
> -                                                      &num_theirs, NULL))
> -                                       continue;
> -
> -                               if (!num_ours && !num_theirs)
> -                                       v->s = "";
> -                               else if (!num_ours)
> -                                       v->s = xstrfmt("[behind %d]", num_theirs);
> -                               else if (!num_theirs)
> -                                       v->s = xstrfmt("[ahead %d]", num_ours);
> -                               else
> -                                       v->s = xstrfmt("[ahead %d, behind %d]",
> -                                                      num_ours, num_theirs);
> -                               continue;
> -                       } else if (!strcmp(formatp, "trackshort") &&
> -                                  (starts_with(name, "upstream") ||
> -                                   starts_with(name, "push"))) {
> -                               assert(branch);
> -
> -                               if (stat_tracking_info(branch, &num_ours,
> -                                                       &num_theirs, NULL))
> -                                       continue;
> -
> -                               if (!num_ours && !num_theirs)
> -                                       v->s = "=";
> -                               else if (!num_ours)
> -                                       v->s = "<";
> -                               else if (!num_theirs)
> -                                       v->s = ">";
> -                               else
> -                                       v->s = "<>";
> -                               continue;
> -                       } else
> +                       else
>                                 die("unknown %.*s format %s",
>                                     (int)(formatp - name), name, formatp);
>                 }
> --
> 2.6.4