Re: [PATCH 31/35] pathspec: allow querying for attributes

From: Stefan Beller <sbeller@google.com>
Date: Nov 29, 2016, 17:37 UTC
Message-ID: <CAGZ79ka9K=VM7afW5E4=e52JOwpKe=o3TTYqWFVrf6TnptSHcA@mail.gmail.com>
In-Reply-To: <20161128221123.GC150448@google.com>

On Mon, Nov 28, 2016 at 2:11 PM, Brandon Williams <bmwill@google.com> wrote:

Show 24 quoted lines

> On 11/10, Stefan Beller wrote:
>> @@ -500,6 +586,18 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
>>
>>  void clear_pathspec(struct pathspec *pathspec)
>>  {
>> +     int i, j;
>> +     for (i = 0; i < pathspec->nr; i++) {
>> +             if (!pathspec->items[i].attr_match_nr)
>> +                     continue;
>> +             for (j = 0; j < pathspec->items[j].attr_match_nr; j++)
>> +                     free(pathspec->items[i].attr_match[j].value);
>> +             free(pathspec->items[i].attr_match);
>> +             if (pathspec->items[i].attr_check)
>> +                     git_attr_check_clear(pathspec->items[i].attr_check);
>> +             free(pathspec->items[i].attr_check);
>> +     }
>> +
>>       free(pathspec->items);
>>       pathspec->items = NULL;
>
> You may also want to add logic like this to the 'copy_pathspec' function
> so that when a pathspec struct is copied, the destination also has
> ownership of its own attribute items.
>

Thanks for the review comments, I'll plan on resending this series after the submodule checkout series (which is nowhere near done, but the foundation is set with sb/submodule-intern-gitdir as a preparation.)

After discussion with Jonathan Nieder, I think it may be possible to make this new pathspec magic work without the need for thread safety. This is archived by constructing all relevant attr_check stacks before the preload_index functionality (which is threaded) and then use the preconstructed attr_checks to get attr_results just like in this series.

That approach would come with the benefit of not needing mutexes at all inside the attr code.