From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Tue, 16 Mar 2010 00:46:56 GMT
Subject: Re: [PATCH RFC] cherry: support --abbrev option
Message-ID: <4B9ED500.2040705@lsrfire.ath.cx>
In-Reply-To: <40aa078e1003151650m549bafdbwdd849e23349ce6a9@mail.gmail.com>

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). :)

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..

René