Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left
flat view
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:05:49
On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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.
quoted hunk ↗ jump to hunk
}; /*@@ -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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ strbuf_addchars(&buf, ' ', ref->align_value - len); + v->s = strbuf_detach(&buf, NULL); + } + ref->align_value = 0; + } +}