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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 20, 2015, 18:01 UTC
Message-ID: <CAPig+cQ1uAHxMRuiBja2ap-nVCU2dUMLcN40Xb=HnT_kN_MW_A@mail.gmail.com>
In-Reply-To: <1437412947-17555-1-git-send-email-Karthik.188@gmail.com>

On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 25 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..93f59aa 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -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:'"));
> +                       strtoul_ui(valp, 10, &ref->align_value);
> +                       if (ref->align_value < 1)
> +                               die(_("value should be greater than zero: align:%u"), ref->align_value);
> +                       v->s = "";

Mental note: v->s points at literal zero-length string ("").

Show 21 quoted lines

> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,40 @@ 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 (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
> +               unsigned int len = 0;
> +
> +               if (*v->s)
> +                       len = utf8_strwidth(v->s);
> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       if (*v->s)
> +                               strbuf_addstr(&buf, v->s);
> +                       if (*v->s && v->s[0] == '\0')
> +                               free((char *)v->s);

Is the "v->s[0] == '\0'" checking for the same literal zero-length string assigned above? If so, attempting to free() that string doesn't make sense, since it's not heap-allocated. Maybe you meant != '\0'?

Overall, this code is getting rather complex and difficult to follow (especially with all the 'v->s' checks thrown in). Junio's proposed 'pseudo_atom' and 'ref_formatting_state' would go a long way toward simplifying it.

Show 6 quoted lines

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