Stefan Beller <sbel...@google.com> writes:
> I think this patch is the most interesting patch, so I'll refrain from
> resending the other 27 patches, though I have adressed the review comments
> locally. I'll resend everything once we are in agreement for this one.
What is the primary purpose of this patch? Is it to prepare callers
so that the way they interact with the attr subsystem will not have to
change when they become threaded and the attr subsystem becomes
thread ready?
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?
> const char *path_without_prefix;
> int err;
>
> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char
> *sha1, const char *base,
> strbuf_addch(&path, '/');
> path_without_prefix = path.buf + args->baselen;
>
> - if (!check)
> - check = git_attr_check_initl("export-ignore", "export-subst",
> NULL);
> - if (!git_check_attr(path_without_prefix, check)) {
> - if (ATTR_TRUE(check->check[0].value))
> + if (!check) {
> + git_attr_check_initl(&check, "export-ignore", "export-subst",
> NULL);
> + git_attr_result_init(&result, check);
> + }
Are we assuming that storing and checking of a single pointer is
atomic? I would not expose that assumption to the callers. On a
platform where that assumption holds, "if check is not NULL,
somebody must have done it already, so return without doing nothing"
can be the first thing git_attr_check_initl()'s implementation does,
though. Or it may not hold anywhere without some barriers. All
that implementation details should be hidden inside _initl()'s
implementation. So this caller should instead just do an
unconditional:
git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
Also, as "result" should be per running thread, hence non-static,
and because we do not want repeated heap allocations and releases
but luckily most callers _know_ not just how many but what exact
attributes they are interested in (I think there are only two
callers that do not know it; check-all-attrs one, and your pathspec
magic one that does not exist at this point in the series), I would
think it is much more preferrable to allow the caller to prepare an
on-stack array and call it "initialized already".
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.
