On Sun, Mar 21, 2010 at 9:23 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Erik Faye-Lund <kusmabite@xxxxxxxxxxxxxx> writes:
>
>> Signed-off-by: Erik Faye-Lund <kusmabite@xxxxxxxxx>
>> ---
>> sha1_name.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index bf92417..2b1be58 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>> int status, exists;
>> static char hex[41];
>>
>> + if (len == 40 || !len)
>> + return sha1_to_hex(sha1);
>> exists = has_sha1_file(sha1);
>> memcpy(hex, sha1_to_hex(sha1), 40);
>> - if (len == 40 || !len)
>> - return hex;
>
> This is somewhat iffy. hex[] being static means there can only be one
> outstanding return value from f-u-a being used, iow
>
> printf("%s %s", f-u-a(a, 0), f-u-a(b, 0))
>
> is a no-no. But at the same time, it means that you can use one more
> recycled buffer than sha1_to_hex() gives us, so this may be safe:
>
> char *ua = f-u-a(a, 0);
> printf("%s %s %s %s %s", ua,
> sha1_to_hex(b), sha1_to_hex(c), sha1_to_hex(d), sha1_to_hex(e));
>
> but with the above it probably is not anymore, no?
>
True, I didn't think of that.
But is this really a problem? I mean, we already have 3 guard-buffers
here, so it would only trigger when the result of more than four calls
to sha1_to_hex (directly, or through find_unique_abbrev) has to be
kept alive at the same time.
The only documentation I could find of this was the comment "/* static
buffer result! */" from cache.h, and it doesn't mention that there's
multiple buffers. So I'd say that any call-site that required four
buffers are, well, perhaps a little bit TOO tightly integrated with
that function ;)
The test-suite didn't catch any failures in it, but I guess I would
have to analyze the calling code a bit more to be sure.
> As an optimization patch, I would buy that delaying the "exists" check
> until the "no abbreviation" check returned early would make sense, though.
>
...or we could just do this. To be honest, this is probably where most
of the change in performance come from. Which in case makes the commit
message somewhat misleading.
Then again, the third option is the easiest one; just drop 2/3 and
3/3. I'd be perfectly fine with that, I just went a little OCD with
consistency here. Those two patches gain us very little. I haven't
measured performance change outside of ls-tree, though.
--
Erik "kusma" Faye-Lund
--
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