On Tue, Mar 16, 2010 at 1:46 AM, René Scharfe <rene.scharfe@xxxxxxxxxxxxxx> 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 -- 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