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

On Mon, Jul 20, 2015 at 5:19 AM, Eric Sunshine [off-list ref] wrote:

On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak [off-list ref] wrote:

quoted

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.

Ok will check.

quoted

 };

 /*

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

Thanks will change.

quoted

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

Makes sense, thanks :)

quoted

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

My bad, got confused with that :)

quoted

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

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

Will change.

quoted

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

Yes, my bad. Will change.

quoted

+                       strbuf_addchars(&buf, ' ', ref->align_value - len);
+                       v->s = strbuf_detach(&buf, NULL);
+               }
+               ref->align_value = 0;
+       }
+}

Thanks.

-- 
Regards,
Karthik Nayak