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