Re: [PATCHv3] attr: convert to new threadsafe API

Stefan Beller <sbeller@google.com> writes:

This looks stale by a few revisions of the other parts of the patch?

This is exactly what I meant by "can we avoid alloc/free of result in leaf function when we _know_ how many attributes we are interested in already, which is the majority of the case?".

Starting with a simple but unoptimized internal implementation of the attr subsystem is one thing (which is good). Exposing an API that cannot be optimally implemented later without changing the callers is another (which is bad).

By encapsulating each element into "struct git_attr_result", we can extend the API without changing the API user [*1*].

But I do not think of a way to allow an efficient implementation later unless the source of the API user somehow says "this user is only interested in this many attributes", like having this

	struct git_attr_result result[2];

(because this caller only wants "ignore" and "subst") on the API user's side [*2*]. Without such a clue (like the patch above, that only says "there is a structure called 'result'"), I do not think of a way to avoid dynamic allocation on the API implementation side.

All the other callers in the patch (pack-objects, convert, ll-merge, etc.) seem to share the exact same pattern. Each of the leaf functions knows a fixed set of attributes it is interested in, the caller iterates over many paths and makes calls to these leaf functions, and it is a waste to pay alloc/free overhead for each and every iteration when we know how many elements result needs to store.

[Footnote]

*1* Would we need a wrapping struct around the array of results?  If
    that is the case, we may need something ugly like this on the
    API user side:

	GIT_ATTR_RESULT_TYPE(2) result = {2,};

    with something like the following on the API implementation
    side:

        #define GIT_ATTR_RESULT_TYPE(n) \
            struct { \
                    int num_slots; \
                    const char *value[n]; \
            }

        struct git_attr_result {
                int num_slots;
                const char *value[FLEX_ARRAY];
        };
        git_attr_result_init(void *result_, struct git_attr_check *check)
        {
                struct git_attr_result *result = result_;

                assert(result->num_slots, check->num_attrs);
                ...
        }                
        git_check_attr(const char *path,
                       struct git_attr_check *check,
                       void *result_)
        {                       
                struct git_attr_result *result = result_;

                assert(result->num_slots, check->num_attrs);
                for (i = 0; i < check_num_attrs; i++)
                        result->value[i] = ... found value ...;
        }

*2* Or the uglier

	GIT_ATTR_RESULT_TYPE(2) result = {2,};

    I can see why the "check" side would benefit from a structure
    that contains an array, but I do not see why "result" side would
    want to, so I am hoping that we won't have to do this uglier
    variant and just go with the simple "array of resulting values".