Re: [PATCHv3] tag: add --points-at list option

[Jeff King <peff@peff.net>]

On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:

> >Though we provide a null_sha1 global already. So doing:
> >
> >  const unsigned char *tagged_sha1 = null_sha1;
> >
> >would be sufficient.
> 
> Or just initialize at test tagged_sha1 with NULL.

Oh yeah, that is even better.

> >That being said, I don't know why you want to do both lookups in the
> >same loop of the points_at. If it's a lightweight tag and the tag
> >matches, you can get away with not parsing the object at all (although
> >to be fair, that is the minority case, so it is unlikely to matter).
> 
> Yes, I think your saying that the lightweight search could go before the
> tag object search like this.

Exactly, though:

> static const unsigned char *match_points_at(const unsigned char *sha1)
> {
> 	const unsigned char *tagged_sha1 = NULL;
> 	struct object *obj = parse_object(sha1);
> 
> 	if (sha1_array_lookup(&points_at, sha1) >= 0)
> 		return sha1;
> 	if (obj && obj->type == OBJ_TAG)
> 		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;

You can delay the relatively expensive parse_object until you find the
results of the first lookup (though like I said earlier, it is unlikely to
matter, as it only helps in the positive-match case. Out of N tags, you
will likely end up parsing N-1 of them anyway).

> >Also, should we be producing an error if !obj? It would indicate a tag
> >that points to a bogus object.
> 
> I think the test of (obj) is redundant as this should be caught
> by get_sha1() in parse_opt_points_at()

No, it's not redundant. get_sha1 is purely about looking up the name and
finding a sha1. parse_object is about looking up the object represented
by that sha1 in the object db. get_sha1 can sometimes involve parsing
objects (e.g., looking for "foo^1" will need to parse the commit object
at "foo"),  but does not have to.

Besides which, you are not calling parse_object on the sha1 from
--points-at, but rather the sha1 for each tag ref given to us by
for_each_tag_ref.

> >Why write your own linear search? sha1_array_lookup will do a binary
> >search for you.
> 
> Well, it's only a linear search of the points_at command arguments.
> But by that reasoning, might as well do two sha1_array_lookups like
> above and save some code b/c "less code is always better"(TM).

Right. I expect the N to be small in this case, so I doubt it matters.
But two sha1_array_lookups is still asymptotically smaller, because the
expensive operation is hashcmp(). So two binary searches is O(2*lg n),
whereas a linear walk with 2 hashcmps per item is O(2*n).

> >Other than that, the patch looks OK to me.
> 
> Thanks, I'll send what I hope to be the final version later today.

Thanks for working on this and being so responsive to review.

-Peff