On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> +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)
>
> 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?)
> - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html