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