From: Erik Faye-Lund <kusmabite@googlemail.com>
Date: Mon, 15 Mar 2010 23:50:07 GMT
Subject: Re: [PATCH RFC] cherry: support --abbrev option
Message-ID: <40aa078e1003151650m549bafdbwdd849e23349ce6a9@mail.gmail.com>
In-Reply-To: <4B9EBFEB.5070108@lsrfire.ath.cx>

On Tue, Mar 16, 2010 at 12:16 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 15.03.2010 23:30, schrieb Erik Faye-Lund:
>> On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe
>>> If I use --no-abbrev, do I get 0 or 40 hash chars?  I didn't actually
>>> test it, but I suspect an "if (!abbrev) abbrev = 40;" is needed somewhere.
>>
>> "abbrev" is initialized to 40 when declared, so you get the same
>> behavior as before by default.
>
> Yes, but --no-abbrev sets it to zero.  Which is OK, though, as I found
> out after actually testing your patch this time.  A closer look at
> find_unique_abbrev() in sha1_name.c reveals that the function returns
> the full hash if len is either 40 or 0.
>

Ah yes, sorry. I misread your initial comment. That's what I get for
answering e-mail after coming home from a pub-quiz ;)

> So you could initialize abbrev to zero and avoid the magic constant 40
> there altogether.  (Is this still nitpicking or already bikeshedding? ;)
>

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.

-- 
Erik "kusma" Faye-Lund