Re: [PATCH RFC] cherry: support --abbrev option

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Tue, Mar 16, 2010 at 1:46 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 16.03.2010 00:50, schrieb Erik Faye-Lund:
>> It's still just nitpicking, and I appreciate the feed-back. I'm a
>> little bit hesitant here though, for the following reasons:
>> - All other users of OPT__ABBREV (with the exception of ls-files,
>> ls-tree and show-ref) initialize abbrev to it's default value (but
>> they all use DEFAULT_ABBREV).
>> - ls-files and ls-tree (but not show-ref) both does the following,
>> when using abbrev: "abbrev ? find_unique_abbrev(ce->sha1,abbrev) :
>> sha1_to_hex(ce->sha1)".
>> - ls-files, ls-tree and show-ref, all seems to default "abbrev" to
>> zero (by making it a static global).
>> - I want to be consistent with the existing code.
>>
>> So, basically, ls-files and ls-tree seems to think
>> find_unique_abbrev() does not correctly (for this purpose) handle
>> abbrev=0. However, show-ref does seem to assume so. Looking at the
>> implementation of find_unique_abbrev(), it is clear that it does. But
>> as I said: I want to be consistent, and the variation from show-ref
>> (basically what you're suggesting) is the least common one.
>>
>> So I guess I can either:
>> 1) Change the code to be consistent with show-ref, and submit an
>> additional patch to make ls-files and ls-tree consistent with this.
>> This might have a performance-impact though, since
>> find_unique_abbrev() does some extra work (checking the sha1 for
>> existence and an extra buffer-copy).
>> 2) Change the code as you suggest, and not care so much about consistency.
>> 3) Leave the code to be functionally consistent with those who
>> initialize abbrev to DEFAULT_ABBREV (but with a different default,
>> which in itself is slightly inconsistent).
>>
>> I'm leaning towards 3) for now, but I don't have any strong feelings.
>
> If you do 2) then it stands 2:2 (ls-files, ls-tree vs. show-ref, cherry). :)
>

True

> find_unique_abbrev() could be streamlined to degrade to sha1_to_hex()
> early on if len is 0 or >= 40, without any existence check or copy..
>

True indeed, and as an added bonus, we could get rid of duplicate
logic. I don't think the function-call overhead is big enough to care
about for ls-files and ls-tree.

I might go for 2), with the intention of sending some follow-up
patches addressing this...

Luckily, now is sleepy sleepy time, and I can consider this tomorrow ;)

-- 
Erik "kusma" Faye-Lund