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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html