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

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:

quoted

+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;
}

quoted

+	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