Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> Add a new atom "align" and support %(align:X) where X is a number.
> This will align the preceeding atom value to the left followed by
> spaces for a total length of X characters. If X is less than the item
> size, the entire atom value is printed.
>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..b81a08d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -53,6 +55,7 @@ static struct {
>         { "flag" },
>         { "HEAD" },
>         { "color" },
> +       { "align" },

Not a new issue, but some compilers (Solaris?) complain about the
trailing comma.

>  };
>
>  /*
> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;
> +
> +                       skip_prefix(name, "align:", &valp);
> +                       if (!valp[0])
> +                               die(_("No value given with 'align='"));

The parser expects "align:", but the error message talks about
"align=". Also, current trend is to drop capitalization from the error
message.

> +                       strtoul_ui(valp, 10, &ref->align_value);
> +                       if (ref->align_value < 1)
> +                               die(_("Value should be greater than zero"));

Drop capitalization. Also, the user seeing this message won't
necessarily know to which value this refers. Better would be to
provide context ("'align:' value should be..."), and even better would
be to show the actual value at fault:

    die(_("value should be greater than zero: align:%u\n",
        ref_align_value);

or something.

> +                       v->s = "";
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>         }
>  }
>
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +       if (v->s[0] && ref->align_value) {

Mental note: v->s[0] is not NUL ('\0').

Also, in this code base, this is typically written *v->s rather than v->s[0].

> +               unsigned int len = 0;
> +               len = utf8_strwidth(v->s);

You initialize 'len' to 0 but then immediately re-assign it.

> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       strbuf_addstr(&buf, v->s);
> +                       if (!v->s[0])
> +                               free((char *)v->s);

We know from the "mental note" above that v->s[0] is not NUL ('\0'),
so this 'if' statement can never be true, thus is dead code.

> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
> +                       v->s = strbuf_detach(&buf, NULL);
> +               }
> +               ref->align_value = 0;
> +       }
> +}
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]