Re: [PATCH 27/36] attr: convert to new threadsafe API

From: Stefan Beller <sbeller@google.com>
Date: Oct 24, 2016, 19:18 UTC
Message-ID: <CAGZ79kb7PaqnyXZ7u0z8Q__ahTLKX8RQwV=dw7vbD4C9LnjOtw@mail.gmail.com>
In-Reply-To: <xmqqr37560gv.fsf@gitster.mtv.corp.google.com>

On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> Make that a double-asterisk.  The same problem appears in an updated
> example in technical/api-gitattributes.txt doc, but the example in
> the commit log message (below) is correct.

The implementation is actually using a double pointer, see below, I forgot commit message and documentation

>>     GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
>
> Are you sure about this?  We've called attr_check_initl() already so
> if this is declaring myresult, it would be decl-after-stmt.

I forgot to update the commit message and Documentation. GIT_ATTR_RESULT_INIT_FOR is gone in the header and in the implementation. I'll update that patch to be consistent throughout all of {Documentation, commit message, implementation}.

Show 12 quoted lines

>
> The latter half is questionable.  If it is "static" it wouldn't be
> thread safe, no?  I think the diff in this patch for archive.c shows
> that we only expect
>
>         struct git_attr_result result[2];
>
> upfront without RESULT_INIT_FOR(), and the reason why there is no
> need to free the result[] is because it is on the stack.  And each
> element in result[] may point at a string, but the string belongs to
> the attr subsystem and must not be freed.
>

Same as above, it's bogus.

Thanks, Stefan