Re: [PATCH v6 03/11] ref-filter: implement '--points-at' option

[Karthik Nayak <karthik.188@gmail.com>]

On Mon, Jun 29, 2015 at 11:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +/*
>> + * Given a ref (sha1, refname) see if it points to one of the sha1s
>> + * in a sha1_array.
>> + */
>> +static int match_points_at(struct sha1_array *points_at, const unsigned char *sha1,
>> +                        const char *refname)
>> +{
>> +     struct object *obj;
>> +
>> +     if (!points_at || !points_at->nr)
>> +             return 1;
>> +
>> +     if (sha1_array_lookup(points_at, sha1) >= 0)
>> +             return 1;
>> +
>> +     obj = parse_object_or_die(sha1, refname);
>> +     if (obj->type == OBJ_TAG &&
>> +         sha1_array_lookup(points_at, ((struct tag *)obj)->tagged->sha1) >= 0)
>> +             return 1;
>> +
>> +     return 0;
>> +}
>> +
>
> Interesting.  I think the change done while copying the code does
> not change anything from the original (other than that the helper
> lost its ability to return the peeled object name), and I think you
> shouldn't make any change while copying the code that would change
> the benaviour, but I notice a few things that we might want to keep
> in mind and revisit them later (i.e. might be a good idea to add
> NEEDSWORK comment to record them near the function):

Reverted the change.
OK will do and add this, I will work on this after GSoC is done.
But like you said I'll add a comment so if someone wants to they can
work on it for now.

>
>  - The original only peeled one level of indirection, so does this
>    implementation.  But is that really what we want, I wonder?
>
>    After doing:
>
>    $ git tag -a -m 'annotated' atag $commit
>    $ git tag -a -m 'annotated doubly' dtag atag
>
>    atag^0, dtag^0 and $commit all refer to the same commit object.
>    Do we want to miss dtag with --point-at=$commit?
>
>  - As we are in for-each-ref (or eventually tag -l) that is walking
>    the cached refs, we may know what refname peels to without
>    parsing the object at all.  Could it be more efficient to ask
>    peel_ref() for the pointee without doing parse_object()
>    ourselves?
>

Shouldn't both these scenarios be solved together by using peel_ref()?

After what you said I just tried a hacked up version of using peel_ref()
rather than parsing the object, tried it out on the Linux tree.

$time git tag -l --points-at=HEAD~501
git tag -l --points-at=HEAD~501  0.03s user 0.01s system 97% cpu 0.044 total

*Using the modified version which uses peel_ref() *
$time ../git/git tag -l --points-at=HEAD~501
../git/git tag -l --points-at=HEAD~501  0.01s user 0.02s system 90%
cpu 0.033 total

This was the average of around 5 tests, Might not be the best way to check, but
I'm sure there's an improvement.

Thanks :)

-- 
Regards,
Karthik Nayak