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

On Thu, Dec 17, 2015 at 2:52 PM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak [off-list ref] wrote:

quoted

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 [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---

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.

quoted

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

quoted

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

quoted

+               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