On Sat, May 10, 2008 at 05:30:56PM -0700, Krzysztof Kowalczyk wrote:
> >> - 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.
Yes. Please don't interpret my comment as "this change isn't worth it";
I meant it as "yes, it is good to be simplifying and making this part
less error prone; I just want to nitpick your exact implementation".
> 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.
OK. Ultimately it is up to Junio. I think your version is a bit complex,
but at the very least it contains that complexity neatly in one function.
Actually, the version I posted actually has an optimization, too (it
remembers the strlen calculation to reuse it). I think the simplest
would just be:
struct ref *alloc_ref_from_str(cons char *str)
{
struct ref *ret = alloc_ref(strlen(str) + 1);
strcpy(ret->name, str);
return ret;
}
But really my main worry is that now we have _two_ functions which
allocate refs, so if "struct ref" ever grows a new field that needs
initializing, it has to go in two places (whereas if alloc_ref_from_str
calls alloc_ref, it works automatically).
> 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).
It looks like just slop, and should probably be fixed at the same time.
There are also quite a number of:
ret = alloc_ref(strlen(name) + 6);
sprintf(ret->name, "refs/%s", name);
which are error-prone and owuld be nice to fix. However, I don't think
there is an easy way short of making ref->name a strbuf. And now it
seems we are getting into a lot of code churn for a relatively small
benefit.
-Peff
--
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
