Re: [PATCH 1/5] attr.c: add push_stack() helper

From: Stefan Beller <sbeller@google.com>
Date: Jun 8, 2016, 23:43 UTC
Message-ID: <CAGZ79ka+BXOAMr7JxJXmhsmXTwTcdidJ72=9mbCkHiycj-t6AQ@mail.gmail.com>
In-Reply-To: <20160608225818.726-2-gitster@pobox.com>

On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> +static void push_stack(struct attr_stack **attr_stack_p,
> +                      struct attr_stack *elem, char *origin, size_t originlen)
> +{
> +       if (elem) {
> +               elem->origin = origin;
> +               if (origin)
> +                       elem->originlen = originlen;

Why do we need to be conditional on origin for setting the originlen, in all occurrences below, we pass in a reasonable number (0), so I would leave out the condition here.

We make use of the `originlen` in `fill` only, so maybe we can even get rid of the length and when we need it compute it with strlen? (I am unsure on this tradeoff)

Show 8 quoted lines

>
>         if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
>                 elem = read_attr(GITATTRIBUTES_FILE, 1);
> -               elem->origin = xstrdup("");
> -               elem->originlen = 0;
> -               elem->prev = attr_stack;
> -               attr_stack = elem;
> +               push_stack(&attr_stack, elem, xstrdup(""), 0);

I wonder why we need to pass an empty-but-not-null string here? In `fill` we use

  const char *base = stk->origin ? stk->origin : "";

so there it would be the same. In prepare_stack we have
       /*
        * Pop the ones from directories that are not the prefix of
        * the path we are checking. Break out of the loop when we see
        * the root one (whose origin is an empty string "") or the builtin
        * one (whose origin is NULL) without popping it.
        */
        while (attr_stack->origin) {

which seems to answer my question that we make a difference between empty and null `origin`. However I wonder if that could be made more clear? (By a well named bit flag in the attr_stack?)

Show 5 quoted lines

> -                       strbuf_add(&pathbuf, path, cp - path);
> -                       strbuf_addch(&pathbuf, '/');
> -                       strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
> +                       strbuf_addf(&pathbuf,
> +                                   "%.*s/%s", (int)(cp - path), path,

This is neat way of handling strings that are not null terminated! I have the suspicion I could have used this before.

As this is meant as a refactoring patch, which doesn't want to change semantics, it looks good to me, the questions are rather meant for followups.

Thanks, Stefan