On Wed, Oct 12, 2016 at 9:20 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>>> I am not sure if the updates to the callers fulfill that purpose.
>>> For example, look at this hunk.
>>>
>>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>>> struct archiver_args *args = c->args;
>>>> write_archive_entry_fn_t write_entry = c->write_entry;
>>>> static struct git_attr_check *check;
>>>> + static struct git_attr_result result;
>>>
>>> As we discussed, this caller, even when threaded, will always want
>>> to ask for a fixed two attributes, so "check" being static and
>>> shared across threads is perfectly fine. But we do not want to see
>>> "result" shared, do we?
>>
>> Well all of the hunks in the patch are not threaded, so they
>> don't follow a threading pattern, but the static pattern to not be
>> more expensive than needed.
>
> Is it too invasive a change to make them as if they are thread-ready
> users of API that happen to know their callers are not threading?
> It would be ideal if we can prepare them so that the way they
> interact with the attr subsystem will not have to change after this
> step.
As far as I see the future, we do not need to change those in the future,
unless we add the threading to the current callers, which is usually a very
invasive thing?
>
>>> In other words, ideally, I think this part of the patch should
>>> rather read like this:
>>>
>>> static struct git_attr_check *check;
>>> struct git_attr_result result[2];
>>>
>>> ...
>>> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>>> if (!git_check_attr(path_without_prefix, check, result)) {
>>> ... use result[0] and result[1] ...
>>>
>>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>>> and extra parameter to git_check_attr() so that the function can
>>> make sure it matches (or exceeds) check->nr.
>>
>> That seems tempting from a callers perspective; I'll look into that.
>
> For callers that prepare "check" and "result" before asking
> check-attr about the attributes in "check" for many paths, it is OK
> to use your "allocate with attr_result_init()" pattern. The "result"
> still needs to be made non-static, though.
I do not see why we would want to have a non static result for
non threaded callers.
>
> But many callers do not follow that; rather they do
>
> loop to iterate over paths {
> call a helper func to learn attr X for path
> use the value of attr X
> }
>
> using a callchain that embeds a helper function deep inside, and
> "check" is kept in the helper, check-attr function is called from
> there, and "result" is not passed from the caller to the helper
> (obviously, because it does not exist in the current API). See the
> callchain that leads down to convert.c::convert_attrs() for a
> typical example. When converted to the new API, it needs to have a
> new "result" structure every time it is called, and cannot reuse the
> one that was used in its previous call.
Why would that be? i.e. I do not understand the reasoning/motivation
as well as what you propose to change here. Do you think of the result
being held at the top of the call chain (and there it may be just allocated
on the stack) and passed down all the way to convert_attrs, that writes
into that result?
Currently the static result in convert_attrs just works fine (as all tests pass)
and that is not the place where we'd add threading to?
Threading would be added in dir.c, a new place that will use attrs.
Maybe instead of discussing the meaning of this patch further, I should
rebase the "pathspecs can ask for attributes" patches to either experience
the problems myself or at least have a series that has a meaningful endgoal.
I thought this preparatory series may be better than sending an even
larger series.
Thanks,
Stefan