On Sat, May 10, 2008 at 4:39 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Sat, May 10, 2008 at 04:26:58PM -0700, kkowalczyk@xxxxxxxxx wrote:
>
>> As a byproduct, fixes one place where string wasn't properly terminated.
>
> Great. Does this fix a user-visible bug? It would be nice to mention in
> the commit log _which_ place (though after reading the patch carefully,
> it looks like the one interpret_target) so that people looking at the
> commit later can understand exactly what was fixed.
It was a subtle memory corruption that wouldn't cause problems in
99.99% cases, but valgrind would probably catch it. And yes, it's the
interp_target().
>> - ref = alloc_ref(strlen(refname) + 1);
>> - strcpy(ref->name, refname);
>> + ref = alloc_ref_from_str(refname);
>
> So this turns a 2-line construct into a 1-line construct...
And avoids future prossible mistakes with not terminating the string,
like the one just commited.
>> +struct ref *alloc_ref_from_str(const char* str)
>> +{
>> + struct ref *ret;
>> + unsigned len = strlen(str) + 1;
>> + char *tmp = xmalloc(sizeof(struct ref) + len);
>> + ret = (struct ref*)tmp;
>> + memset(tmp, 0, sizeof(struct ref));
>> + tmp += sizeof(struct ref);
>> + memcpy(tmp, str, len);
>> + return ret;
>> +}
>
> But why do we need an 8-line function to do it?
>
> The only difference I can see over
>
> struct ref *alloc_ref_from_str(const char *str)
> {
> unsigned len = strlen(str) + 1;
> struct ref *ret = alloc_ref(len);
> memcpy(ret->name, str, len);
> return ret;
> }
>
> is that we avoid memsetting the name portion of the struct to 0 before
> copying to it. It seems like an unproven micro-optimization that makes
> it a bit harder to read.
You're absolutely right - it's a micro-optimization and your version
might be preferred for clarity. This is the first time I submit a
patch to git so I don't have a good feel for what kind of treadoffs
people find acceptable.
I should also mention that
static struct ref *try_explicit_object_name(const char *name)
{
unsigned char sha1[20];
struct ref *ref;
if (!*name) {
ref = alloc_ref(20);
strcpy(ref->name, "(delete)");
hashclr(ref->new_sha1);
return ref;
}
...
could also be replaced with alloc_ref_str() - I just wasn't 100% sure
if overallocating 10 bytes (20 - strlen("(delete)")) was just sloppy
code or does other code relies on that (which is unlikely and if true
then it wouldn't be good).
Regards,
-- kjk
--
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
