On Thu, Dec 17, 2015 at 2:52 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> 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.
>> + 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.
>> + 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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html