On Mon, Nov 28, 2016 at 2:11 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> 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.