{"thread":{"id":"23024","subject":"[PATCH RFC] cherry: support --abbrev option","startedAt":"2010-03-15T16:03:19Z","lastAt":"2010-03-16T01:44:15Z","messageCount":10,"participants":["Erik Faye-Lund","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"136840","messageId":"1268668999-4188-1-git-send-email-kusmabite@gmail.com","threadId":"23024","inReplyTo":null,"subject":"[PATCH RFC] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-15T16:03:19Z","receivedAt":"2010-03-15T16:03:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Switch to parse-options API while we're at it.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n\nI sometimes find it useful to look at the commit-subject together with\nthe SHA-1s. Using --abbrev increases the chance that the lines fits on\nan 80 character wide terminal, making the output easier to read.\n\n builtin/log.c |   40 ++++++++++++++++++++++------------------\n 1 files changed, 22 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b70d0f7..020d618 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1286,8 +1286,11 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)\n \treturn -1;\n }\n \n-static const char cherry_usage[] =\n-\"git cherry [-v] [<upstream> [<head> [<limit>]]]\";\n+static const char * const cherry_usage[] = {\n+\t\"git cherry [-v] [<upstream> [<head> [<limit>]]]\",\n+\tNULL\n+};\n+\n int cmd_cherry(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -1298,26 +1301,26 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \tconst char *upstream;\n \tconst char *head = \"HEAD\";\n \tconst char *limit = NULL;\n-\tint verbose = 0;\n+\tint verbose = 0, abbrev = 40;\n \n-\tif (argc > 1 && !strcmp(argv[1], \"-v\")) {\n-\t\tverbose = 1;\n-\t\targc--;\n-\t\targv++;\n-\t}\n+\tstruct option options[] = {\n+\t\tOPT__ABBREV(&abbrev),\n+\t\tOPT__VERBOSE(&verbose),\n+\t\tOPT_END()\n+\t};\n \n-\tif (argc > 1 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(cherry_usage);\n+\targc = parse_options(argc, argv, prefix, options, cherry_usage,\n+\t    PARSE_OPT_KEEP_UNKNOWN);\n \n \tswitch (argc) {\n-\tcase 4:\n-\t\tlimit = argv[3];\n-\t\t/* FALLTHROUGH */\n \tcase 3:\n-\t\thead = argv[2];\n+\t\tlimit = argv[2];\n \t\t/* FALLTHROUGH */\n \tcase 2:\n-\t\tupstream = argv[1];\n+\t\thead = argv[1];\n+\t\t/* FALLTHROUGH */\n+\tcase 1:\n+\t\tupstream = argv[0];\n \t\tbreak;\n \tdefault:\n \t\tcurrent_branch = branch_get(NULL);\n@@ -1327,7 +1330,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, \"Could not find a tracked\"\n \t\t\t\t\t\" remote branch, please\"\n \t\t\t\t\t\" specify <upstream> manually.\\n\");\n-\t\t\tusage(cherry_usage);\n+\t\t\tusage_with_options(cherry_usage, options);\n \t\t}\n \n \t\tupstream = current_branch->merge[0]->dst;\n@@ -1380,12 +1383,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\t\tpretty_print_commit(CMIT_FMT_ONELINE, commit,\n \t\t\t\t\t    &buf, &ctx);\n \t\t\tprintf(\"%c %s %s\\n\", sign,\n-\t\t\t       sha1_to_hex(commit->object.sha1), buf.buf);\n+\t\t\t       find_unique_abbrev(commit->object.sha1, abbrev),\n+\t\t\t       buf.buf);\n \t\t\tstrbuf_release(&buf);\n \t\t}\n \t\telse {\n \t\t\tprintf(\"%c %s\\n\", sign,\n-\t\t\t       sha1_to_hex(commit->object.sha1));\n+\t\t\t       find_unique_abbrev(commit->object.sha1, abbrev));\n \t\t}\n \n \t\tlist = list->next;\n-- \n1.7.0.2.416.g84492\n"},{"id":"136844","messageId":"4B9E69A4.2080304@lsrfire.ath.cx","threadId":"23024","inReplyTo":"1268668999-4188-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-15T17:08:52Z","receivedAt":"2010-03-15T17:08:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.03.2010 17:03, schrieb Erik Faye-Lund:\n> Switch to parse-options API while we're at it.\n> \n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> ---\n> \n> I sometimes find it useful to look at the commit-subject together with\n> the SHA-1s. Using --abbrev increases the chance that the lines fits on\n> an 80 character wide terminal, making the output easier to read.\n> \n>  builtin/log.c |   40 ++++++++++++++++++++++------------------\n>  1 files changed, 22 insertions(+), 18 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index b70d0f7..020d618 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1286,8 +1286,11 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)\n>  \treturn -1;\n>  }\n>  \n> -static const char cherry_usage[] =\n> -\"git cherry [-v] [<upstream> [<head> [<limit>]]]\";\n> +static const char * const cherry_usage[] = {\n> +\t\"git cherry [-v] [<upstream> [<head> [<limit>]]]\",\n> +\tNULL\n> +};\n> +\n>  int cmd_cherry(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct rev_info revs;\n> @@ -1298,26 +1301,26 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n>  \tconst char *upstream;\n>  \tconst char *head = \"HEAD\";\n>  \tconst char *limit = NULL;\n> -\tint verbose = 0;\n> +\tint verbose = 0, abbrev = 40;\n>  \n> -\tif (argc > 1 && !strcmp(argv[1], \"-v\")) {\n> -\t\tverbose = 1;\n> -\t\targc--;\n> -\t\targv++;\n> -\t}\n> +\tstruct option options[] = {\n> +\t\tOPT__ABBREV(&abbrev),\n> +\t\tOPT__VERBOSE(&verbose),\n> +\t\tOPT_END()\n> +\t};\n\nIf I use --no-abbrev, do I get 0 or 40 hash chars?  I didn't actually\ntest it, but I suspect an \"if (!abbrev) abbrev = 40;\" is needed somewhere.\n\n> -\tif (argc > 1 && !strcmp(argv[1], \"-h\"))\n> -\t\tusage(cherry_usage);\n> +\targc = parse_options(argc, argv, prefix, options, cherry_usage,\n> +\t    PARSE_OPT_KEEP_UNKNOWN);\n\nWhy do you use PARSE_OPT_KEEP_UNKNOWN here?  I think the old option\nparsing code lazily relied on invalid options being found out by\nadd_pending_commit() later.  We can be lazy again by leaving invalid\noption handling to parse_options(); sure, it changes the error message,\nbut for the better.\n\nRené\n"},{"id":"136846","messageId":"7vwrxdse24.fsf@alter.siamese.dyndns.org","threadId":"23024","inReplyTo":"1268668999-4188-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-15T17:13:55Z","receivedAt":"2010-03-15T17:13:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@googlemail.com> writes:\n\n> Switch to parse-options API while we're at it.\n>\n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> ---\n>\n> I sometimes find it useful to look at the commit-subject together with\n> the SHA-1s. Using --abbrev increases the chance that the lines fits on\n> an 80 character wide terminal, making the output easier to read.\n\nEven though \"cherry\" historically was meant to be a low-level helper for\nPorcelain scripts, I've seen people use it as the top-level UI, and I\nthink what the patch tries to do makes sense.\n"},{"id":"136887","messageId":"40aa078e1003151530y19f10bf1l852a0e5230b11cb6@mail.gmail.com","threadId":"23024","inReplyTo":"4B9E69A4.2080304@lsrfire.ath.cx","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-15T22:30:45Z","receivedAt":"2010-03-15T22:30:45Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> Am 15.03.2010 17:03, schrieb Erik Faye-Lund:\n>> Switch to parse-options API while we're at it.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>> ---\n>>\n>> I sometimes find it useful to look at the commit-subject together with\n>> the SHA-1s. Using --abbrev increases the chance that the lines fits on\n>> an 80 character wide terminal, making the output easier to read.\n>>\n>>  builtin/log.c |   40 ++++++++++++++++++++++------------------\n>>  1 files changed, 22 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index b70d0f7..020d618 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -1286,8 +1286,11 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)\n>>       return -1;\n>>  }\n>>\n>> -static const char cherry_usage[] =\n>> -\"git cherry [-v] [<upstream> [<head> [<limit>]]]\";\n>> +static const char * const cherry_usage[] = {\n>> +     \"git cherry [-v] [<upstream> [<head> [<limit>]]]\",\n>> +     NULL\n>> +};\n>> +\n>>  int cmd_cherry(int argc, const char **argv, const char *prefix)\n>>  {\n>>       struct rev_info revs;\n>> @@ -1298,26 +1301,26 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n>>       const char *upstream;\n>>       const char *head = \"HEAD\";\n>>       const char *limit = NULL;\n>> -     int verbose = 0;\n>> +     int verbose = 0, abbrev = 40;\n>>\n>> -     if (argc > 1 && !strcmp(argv[1], \"-v\")) {\n>> -             verbose = 1;\n>> -             argc--;\n>> -             argv++;\n>> -     }\n>> +     struct option options[] = {\n>> +             OPT__ABBREV(&abbrev),\n>> +             OPT__VERBOSE(&verbose),\n>> +             OPT_END()\n>> +     };\n>\n> If I use --no-abbrev, do I get 0 or 40 hash chars?  I didn't actually\n> test it, but I suspect an \"if (!abbrev) abbrev = 40;\" is needed somewhere.\n>\n\n\"abbrev\" is initialized to 40 when declared, so you get the same\nbehavior as before by default.\n\n>> -     if (argc > 1 && !strcmp(argv[1], \"-h\"))\n>> -             usage(cherry_usage);\n>> +     argc = parse_options(argc, argv, prefix, options, cherry_usage,\n>> +         PARSE_OPT_KEEP_UNKNOWN);\n>\n> Why do you use PARSE_OPT_KEEP_UNKNOWN here? I think the old option\n> parsing code lazily relied on invalid options being found out by\n> add_pending_commit() later. We can be lazy again by leaving invalid\n> option handling to parse_options(); sure, it changes the error message,\n> but for the better.\n\nActually, It's a mistake, thanks for pointing it out :)\n\nYeah, I think your suggestion is better. Corrected.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"136890","messageId":"40aa078e1003151539u2d1873bcy394063466c8e404b@mail.gmail.com","threadId":"23024","inReplyTo":"7vwrxdse24.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-15T22:39:10Z","receivedAt":"2010-03-15T22:39:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Mar 15, 2010 at 6:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Faye-Lund <kusmabite@googlemail.com> writes:\n>\n>> Switch to parse-options API while we're at it.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>> ---\n>>\n>> I sometimes find it useful to look at the commit-subject together with\n>> the SHA-1s. Using --abbrev increases the chance that the lines fits on\n>> an 80 character wide terminal, making the output easier to read.\n>\n> Even though \"cherry\" historically was meant to be a low-level helper for\n> Porcelain scripts, I've seen people use it as the top-level UI, and I\n> think what the patch tries to do makes sense.\n>\n\nI find it very useful. In fact, even the documentation seems to\nsuggest to use it to find out if a patch is merged upstream, which\ndoesn't sound very scriptish to me. So I think it has somehow exceeded\nit's original goal :)\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"136894","messageId":"4B9EBFEB.5070108@lsrfire.ath.cx","threadId":"23024","inReplyTo":"40aa078e1003151530y19f10bf1l852a0e5230b11cb6@mail.gmail.com","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-15T23:16:59Z","receivedAt":"2010-03-15T23:16:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.03.2010 23:30, schrieb Erik Faye-Lund:\n> On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe\n>> If I use --no-abbrev, do I get 0 or 40 hash chars?  I didn't actually\n>> test it, but I suspect an \"if (!abbrev) abbrev = 40;\" is needed somewhere.\n> \n> \"abbrev\" is initialized to 40 when declared, so you get the same\n> behavior as before by default.\n\nYes, but --no-abbrev sets it to zero.  Which is OK, though, as I found\nout after actually testing your patch this time.  A closer look at\nfind_unique_abbrev() in sha1_name.c reveals that the function returns\nthe full hash if len is either 40 or 0.\n\nSo you could initialize abbrev to zero and avoid the magic constant 40\nthere altogether.  (Is this still nitpicking or already bikeshedding? ;)\n\nRené\n"},{"id":"136904","messageId":"40aa078e1003151650m549bafdbwdd849e23349ce6a9@mail.gmail.com","threadId":"23024","inReplyTo":"4B9EBFEB.5070108@lsrfire.ath.cx","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-15T23:50:07Z","receivedAt":"2010-03-15T23:50:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Mar 16, 2010 at 12:16 AM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> Am 15.03.2010 23:30, schrieb Erik Faye-Lund:\n>> On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe\n>>> If I use --no-abbrev, do I get 0 or 40 hash chars?  I didn't actually\n>>> test it, but I suspect an \"if (!abbrev) abbrev = 40;\" is needed somewhere.\n>>\n>> \"abbrev\" is initialized to 40 when declared, so you get the same\n>> behavior as before by default.\n>\n> Yes, but --no-abbrev sets it to zero.  Which is OK, though, as I found\n> out after actually testing your patch this time.  A closer look at\n> find_unique_abbrev() in sha1_name.c reveals that the function returns\n> the full hash if len is either 40 or 0.\n>\n\nAh yes, sorry. I misread your initial comment. That's what I get for\nanswering e-mail after coming home from a pub-quiz ;)\n\n> So you could initialize abbrev to zero and avoid the magic constant 40\n> there altogether.  (Is this still nitpicking or already bikeshedding? ;)\n>\n\nIt's still just nitpicking, and I appreciate the feed-back. I'm a\nlittle bit hesitant here though, for the following reasons:\n- All other users of OPT__ABBREV (with the exception of ls-files,\nls-tree and show-ref) initialize abbrev to it's default value (but\nthey all use DEFAULT_ABBREV).\n- ls-files and ls-tree (but not show-ref) both does the following,\nwhen using abbrev: \"abbrev ? find_unique_abbrev(ce->sha1,abbrev) :\nsha1_to_hex(ce->sha1)\".\n- ls-files, ls-tree and show-ref, all seems to default \"abbrev\" to\nzero (by making it a static global).\n- I want to be consistent with the existing code.\n\nSo, basically, ls-files and ls-tree seems to think\nfind_unique_abbrev() does not correctly (for this purpose) handle\nabbrev=0. However, show-ref does seem to assume so. Looking at the\nimplementation of find_unique_abbrev(), it is clear that it does. But\nas I said: I want to be consistent, and the variation from show-ref\n(basically what you're suggesting) is the least common one.\n\nSo I guess I can either:\n1) Change the code to be consistent with show-ref, and submit an\nadditional patch to make ls-files and ls-tree consistent with this.\nThis might have a performance-impact though, since\nfind_unique_abbrev() does some extra work (checking the sha1 for\nexistence and an extra buffer-copy).\n2) Change the code as you suggest, and not care so much about consistency.\n3) Leave the code to be functionally consistent with those who\ninitialize abbrev to DEFAULT_ABBREV (but with a different default,\nwhich in itself is slightly inconsistent).\n\nI'm leaning towards 3) for now, but I don't have any strong feelings.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"136907","messageId":"4B9ED500.2040705@lsrfire.ath.cx","threadId":"23024","inReplyTo":"40aa078e1003151650m549bafdbwdd849e23349ce6a9@mail.gmail.com","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-03-16T00:46:56Z","receivedAt":"2010-03-16T00:46:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.03.2010 00:50, schrieb Erik Faye-Lund:\n> It's still just nitpicking, and I appreciate the feed-back. I'm a\n> little bit hesitant here though, for the following reasons:\n> - All other users of OPT__ABBREV (with the exception of ls-files,\n> ls-tree and show-ref) initialize abbrev to it's default value (but\n> they all use DEFAULT_ABBREV).\n> - ls-files and ls-tree (but not show-ref) both does the following,\n> when using abbrev: \"abbrev ? find_unique_abbrev(ce->sha1,abbrev) :\n> sha1_to_hex(ce->sha1)\".\n> - ls-files, ls-tree and show-ref, all seems to default \"abbrev\" to\n> zero (by making it a static global).\n> - I want to be consistent with the existing code.\n> \n> So, basically, ls-files and ls-tree seems to think\n> find_unique_abbrev() does not correctly (for this purpose) handle\n> abbrev=0. However, show-ref does seem to assume so. Looking at the\n> implementation of find_unique_abbrev(), it is clear that it does. But\n> as I said: I want to be consistent, and the variation from show-ref\n> (basically what you're suggesting) is the least common one.\n> \n> So I guess I can either:\n> 1) Change the code to be consistent with show-ref, and submit an\n> additional patch to make ls-files and ls-tree consistent with this.\n> This might have a performance-impact though, since\n> find_unique_abbrev() does some extra work (checking the sha1 for\n> existence and an extra buffer-copy).\n> 2) Change the code as you suggest, and not care so much about consistency.\n> 3) Leave the code to be functionally consistent with those who\n> initialize abbrev to DEFAULT_ABBREV (but with a different default,\n> which in itself is slightly inconsistent).\n> \n> I'm leaning towards 3) for now, but I don't have any strong feelings.\n\nIf you do 2) then it stands 2:2 (ls-files, ls-tree vs. show-ref, cherry). :)\n\nfind_unique_abbrev() could be streamlined to degrade to sha1_to_hex()\nearly on if len is 0 or >= 40, without any existence check or copy..\n\nRené\n"},{"id":"136909","messageId":"40aa078e1003151759v1825c4fbo62d0d77af81bf66@mail.gmail.com","threadId":"23024","inReplyTo":"4B9ED500.2040705@lsrfire.ath.cx","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-16T00:59:33Z","receivedAt":"2010-03-16T00:59:33Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Mar 16, 2010 at 1:46 AM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> Am 16.03.2010 00:50, schrieb Erik Faye-Lund:\n>> It's still just nitpicking, and I appreciate the feed-back. I'm a\n>> little bit hesitant here though, for the following reasons:\n>> - All other users of OPT__ABBREV (with the exception of ls-files,\n>> ls-tree and show-ref) initialize abbrev to it's default value (but\n>> they all use DEFAULT_ABBREV).\n>> - ls-files and ls-tree (but not show-ref) both does the following,\n>> when using abbrev: \"abbrev ? find_unique_abbrev(ce->sha1,abbrev) :\n>> sha1_to_hex(ce->sha1)\".\n>> - ls-files, ls-tree and show-ref, all seems to default \"abbrev\" to\n>> zero (by making it a static global).\n>> - I want to be consistent with the existing code.\n>>\n>> So, basically, ls-files and ls-tree seems to think\n>> find_unique_abbrev() does not correctly (for this purpose) handle\n>> abbrev=0. However, show-ref does seem to assume so. Looking at the\n>> implementation of find_unique_abbrev(), it is clear that it does. But\n>> as I said: I want to be consistent, and the variation from show-ref\n>> (basically what you're suggesting) is the least common one.\n>>\n>> So I guess I can either:\n>> 1) Change the code to be consistent with show-ref, and submit an\n>> additional patch to make ls-files and ls-tree consistent with this.\n>> This might have a performance-impact though, since\n>> find_unique_abbrev() does some extra work (checking the sha1 for\n>> existence and an extra buffer-copy).\n>> 2) Change the code as you suggest, and not care so much about consistency.\n>> 3) Leave the code to be functionally consistent with those who\n>> initialize abbrev to DEFAULT_ABBREV (but with a different default,\n>> which in itself is slightly inconsistent).\n>>\n>> I'm leaning towards 3) for now, but I don't have any strong feelings.\n>\n> If you do 2) then it stands 2:2 (ls-files, ls-tree vs. show-ref, cherry). :)\n>\n\nTrue\n\n> find_unique_abbrev() could be streamlined to degrade to sha1_to_hex()\n> early on if len is 0 or >= 40, without any existence check or copy..\n>\n\nTrue indeed, and as an added bonus, we could get rid of duplicate\nlogic. I don't think the function-call overhead is big enough to care\nabout for ls-files and ls-tree.\n\nI might go for 2), with the intention of sending some follow-up\npatches addressing this...\n\nLuckily, now is sleepy sleepy time, and I can consider this tomorrow ;)\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"136912","messageId":"7v634xyr9s.fsf@alter.siamese.dyndns.org","threadId":"23024","inReplyTo":"40aa078e1003151650m549bafdbwdd849e23349ce6a9@mail.gmail.com","subject":"Re: [PATCH RFC] cherry: support --abbrev option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-16T01:44:15Z","receivedAt":"2010-03-16T01:44:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@googlemail.com> writes:\n\n> So, basically, ls-files and ls-tree seems to think\n> find_unique_abbrev() does not correctly (for this purpose) handle\n> abbrev=0. However, show-ref does seem to assume so.\n\nI think this is merely historical; the former two were written way back\nbefore corner case behaviour, namely length=0 case, was cast in stone, and\nhence they call f-u-a unnecessarily defefnsively.\n\n.\n"}]}