Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 19, 2015, 23:49 UTC
- Message-ID
- <CAPig+cRXsmi=UxRr-3rnt919d86jD6uMuTqdDxCComYLXk1TYw@mail.gmail.com>
- In-Reply-To
- <1437246749-14423-2-git-send-email-Karthik.188@gmail.com>
On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 16 quoted lines
> 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@gmail.com>
> ---
> 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.
Show 13 quoted lines
> };
>
> /*
> @@ -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.
Show 12 quoted lines
> + 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.
Show 5 quoted lines
> + 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.
Show 6 quoted lines
> + strbuf_addchars(&buf, ' ', ref->align_value - len); > + v->s = strbuf_detach(&buf, NULL); > + } > + ref->align_value = 0; > + } > +}