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

[Tom Grennan <tmgrennan@gmail.com>]

On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>
>> +static const unsigned char *match_points_at(const unsigned char *sha1)
>> +{
>> +	int i;
>> +	const unsigned char *tagged_sha1 = (unsigned char*)"";
>> +	struct object *obj = parse_object(sha1);
>> +
>> +	if (obj && obj->type == OBJ_TAG)
>> +		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
>
>This is not safe. A sha1 is not NUL-terminated, but is rather _always_
>20 bytes. So when the object is not a tag, you do the hashcmp against
>your single-byte string literal above, and we end up comparing whatever
>garbage is in the data segment after the string literal.

Yikes! That was dumb.

>What you want instead is the all-zeros sha1, like:
>
>  const unsigned char null_sha1[20] = { 0 };
>
>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.

static const unsigned char *match_points_at(const unsigned char *sha1)
{
	int i;
	const unsigned char *tagged_sha1 = NULL;
	struct object *obj = parse_object(sha1);

	if (obj && obj->type == OBJ_TAG)
		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
	for (i = 0; i < points_at.nr; i++)
		if (!hashcmp(points_at.sha1[i], sha1))
			return sha1;
		else if (tagged_sha1 &&
			 !hashcmp(points_at.sha1[i], tagged_sha1))
			return tagged_sha1;
	return NULL;
}

>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.

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;
	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
		return tagged_sha1;
	return NULL;
}

>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()

int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
			const char *arg, int unset)
{
	unsigned char sha1[20];

	if (unset) {
		sha1_array_clear(&points_at);
		return 0;
	}
	if (!arg)
		return error(_("switch 'points-at' requires an object"));
	if (get_sha1(arg, sha1))
		return error(_("malformed object name '%s'"), arg);
	sha1_array_append(&points_at, sha1);
	return 0;
}

>> +	for (i = 0; i < points_at.nr; i++)
>> +		if (!hashcmp(points_at.sha1[i], sha1))
>> +			return sha1;
>> +		else if (!hashcmp(points_at.sha1[i], tagged_sha1))
>> +			return tagged_sha1;
>> +	return NULL;
>
>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).

>Other than that, the patch looks OK to me.

Thanks, I'll send what I hope to be the final version later today.

-- 
TomG