{"thread":{"id":"23111","subject":"[PATCH v2 0/3] cherry-abbrev","startedAt":"2010-03-20T18:55:26Z","lastAt":"2010-03-21T21:09:49Z","messageCount":6,"participants":["Erik Faye-Lund","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"137358","messageId":"1269111329-5896-1-git-send-email-kusmabite@gmail.com","threadId":"23111","inReplyTo":null,"subject":"[PATCH v2 0/3] cherry-abbrev","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-20T18:55:26Z","receivedAt":"2010-03-20T18:55:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This is the second round of ef/cherry-abbrev.\n\n1/3 is the same as last round, but with the corrections suggested\nby René Scharfe.\n\n2/3 isn't strictly needed, but it makes two call-sites for f_u_a\nmore consistent with the others.\n\n3/3 fixes the gains back the performance that was lost in 2/3:\n\nBest of five runs of \"git ls-tree v1.6.5\" in a loop:\n\nwithout 3/3:\nreal        0m0.082s\nuser        0m0.040s\nsys         0m0.036s\n\nwith 3/3:\nreal        0m0.074s\nuser        0m0.040s\nsys         0m0.040s\n\nErik Faye-Lund (3):\n  cherry: support --abbrev option\n  ls: remove redundant logic\n  find_unique_abbrev: early out without a memcpy\n\n builtin/log.c      |   39 +++++++++++++++++++++------------------\n builtin/ls-files.c |    7 ++-----\n builtin/ls-tree.c  |    6 ++----\n sha1_name.c        |    4 ++--\n 4 files changed, 27 insertions(+), 29 deletions(-)\n"},{"id":"137359","messageId":"1269111329-5896-2-git-send-email-kusmabite@gmail.com","threadId":"23111","inReplyTo":"1269111329-5896-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 1/3] cherry: support --abbrev option","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-20T18:55:27Z","receivedAt":"2010-03-20T18:55:27Z","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 builtin/log.c |   39 +++++++++++++++++++++------------------\n 1 files changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b70d0f7..fd8d18c 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,25 @@ 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 = 0;\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, 0);\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 +1329,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 +1382,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.456.g64f24\n"},{"id":"137360","messageId":"1269111329-5896-3-git-send-email-kusmabite@gmail.com","threadId":"23111","inReplyTo":"1269111329-5896-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 2/3] ls: remove redundant logic","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-20T18:55:28Z","receivedAt":"2010-03-20T18:55:28Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"find_unique_abbrev() already returns the full SHA-1 if abbrev = 0,\nso we can remove the logic that avoids the call.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n builtin/ls-files.c |    7 ++-----\n builtin/ls-tree.c  |    6 ++----\n 2 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b065061..c0fbcdc 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -153,8 +153,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t\tprintf(\"%s%06o %s %d\\t\",\n \t\t       tag,\n \t\t       ce->ce_mode,\n-\t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\n-\t\t\t\t: sha1_to_hex(ce->sha1),\n+\t\t       find_unique_abbrev(ce->sha1,abbrev),\n \t\t       ce_stage(ce));\n \t}\n \twrite_name_quoted(ce->name + offset, stdout, line_terminator);\n@@ -176,9 +175,7 @@ static int show_one_ru(struct string_list_item *item, void *cbdata)\n \t\tif (!ui->mode[i])\n \t\t\tcontinue;\n \t\tprintf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n-\t\t       abbrev\n-\t\t       ? find_unique_abbrev(ui->sha1[i], abbrev)\n-\t\t       : sha1_to_hex(ui->sha1[i]),\n+\t\t       find_unique_abbrev(ui->sha1[i], abbrev),\n \t\t       i + 1);\n \t\twrite_name_quoted(path + offset, stdout, line_terminator);\n \t}\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 4484185..dc86b0d 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -103,13 +103,11 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n \t\t\t} else\n \t\t\t\tstrcpy(size_text, \"-\");\n \t\t\tprintf(\"%06o %s %s %7s\\t\", mode, type,\n-\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-\t\t\t\t      : sha1_to_hex(sha1),\n+\t\t\t       find_unique_abbrev(sha1, abbrev),\n \t\t\t       size_text);\n \t\t} else\n \t\t\tprintf(\"%06o %s %s\\t\", mode, type,\n-\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-\t\t\t              : sha1_to_hex(sha1));\n+\t\t\t       find_unique_abbrev(sha1, abbrev));\n \t}\n \twrite_name_quotedpfx(base + chomp_prefix, baselen - chomp_prefix,\n \t\t\t  pathname, stdout, line_termination);\n-- \n1.7.0.2.456.g64f24\n"},{"id":"137361","messageId":"1269111329-5896-4-git-send-email-kusmabite@gmail.com","threadId":"23111","inReplyTo":"1269111329-5896-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 3/3] find_unique_abbrev: early out without a memcpy","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-20T18:55:29Z","receivedAt":"2010-03-20T18:55:29Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n sha1_name.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex bf92417..2b1be58 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n \tint status, exists;\n \tstatic char hex[41];\n \n+\tif (len == 40 || !len)\n+\t\treturn sha1_to_hex(sha1);\n \texists = has_sha1_file(sha1);\n \tmemcpy(hex, sha1_to_hex(sha1), 40);\n-\tif (len == 40 || !len)\n-\t\treturn hex;\n \twhile (len < 40) {\n \t\tunsigned char sha1_ret[20];\n \t\tstatus = get_short_sha1(hex, len, sha1_ret, 1);\n-- \n1.7.0.2.456.g64f24\n"},{"id":"137477","messageId":"7v39ztmnjp.fsf@alter.siamese.dyndns.org","threadId":"23111","inReplyTo":"1269111329-5896-4-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-21T20:23:38Z","receivedAt":"2010-03-21T20:23:38Z","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> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> ---\n>  sha1_name.c |    4 ++--\n>  1 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index bf92417..2b1be58 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n>  \tint status, exists;\n>  \tstatic char hex[41];\n>  \n> +\tif (len == 40 || !len)\n> +\t\treturn sha1_to_hex(sha1);\n>  \texists = has_sha1_file(sha1);\n>  \tmemcpy(hex, sha1_to_hex(sha1), 40);\n> -\tif (len == 40 || !len)\n> -\t\treturn hex;\n\nThis is somewhat iffy.  hex[] being static means there can only be one\noutstanding return value from f-u-a being used, iow\n\n\tprintf(\"%s %s\", f-u-a(a, 0), f-u-a(b, 0))\n\nis a no-no.  But at the same time, it means that you can use one more\nrecycled buffer than sha1_to_hex() gives us, so this may be safe:\n\n\tchar *ua = f-u-a(a, 0);\n        printf(\"%s %s %s %s %s\", ua,\n            sha1_to_hex(b), sha1_to_hex(c),  sha1_to_hex(d), sha1_to_hex(e));\n\nbut with the above it probably is not anymore, no?\n\nAs an optimization patch, I would buy that delaying the \"exists\" check\nuntil the \"no abbreviation\" check returned early would make sense, though.\n"},{"id":"137479","messageId":"40aa078e1003211409q50b9efc3pf5bcfccb48b7eeb6@mail.gmail.com","threadId":"23111","inReplyTo":"7v39ztmnjp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-03-21T21:09:49Z","receivedAt":"2010-03-21T21:09:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sun, Mar 21, 2010 at 9:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Faye-Lund <kusmabite@googlemail.com> writes:\n>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>> ---\n>>  sha1_name.c |    4 ++--\n>>  1 files changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/sha1_name.c b/sha1_name.c\n>> index bf92417..2b1be58 100644\n>> --- a/sha1_name.c\n>> +++ b/sha1_name.c\n>> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n>>       int status, exists;\n>>       static char hex[41];\n>>\n>> +     if (len == 40 || !len)\n>> +             return sha1_to_hex(sha1);\n>>       exists = has_sha1_file(sha1);\n>>       memcpy(hex, sha1_to_hex(sha1), 40);\n>> -     if (len == 40 || !len)\n>> -             return hex;\n>\n> This is somewhat iffy.  hex[] being static means there can only be one\n> outstanding return value from f-u-a being used, iow\n>\n>        printf(\"%s %s\", f-u-a(a, 0), f-u-a(b, 0))\n>\n> is a no-no.  But at the same time, it means that you can use one more\n> recycled buffer than sha1_to_hex() gives us, so this may be safe:\n>\n>        char *ua = f-u-a(a, 0);\n>        printf(\"%s %s %s %s %s\", ua,\n>            sha1_to_hex(b), sha1_to_hex(c),  sha1_to_hex(d), sha1_to_hex(e));\n>\n> but with the above it probably is not anymore, no?\n>\n\nTrue, I didn't think of that.\n\nBut is this really a problem? I mean, we already have 3 guard-buffers\nhere, so it would only trigger when the result of more than four calls\nto sha1_to_hex (directly, or through find_unique_abbrev) has to be\nkept alive at the same time.\n\nThe only documentation I could find of this was the comment \"/* static\nbuffer result! */\" from cache.h, and it doesn't mention that there's\nmultiple buffers. So I'd say that any call-site that required four\nbuffers are, well, perhaps a little bit TOO tightly integrated with\nthat function ;)\n\nThe test-suite didn't catch any failures in it, but I guess I would\nhave to analyze the calling code a bit more to be sure.\n\n> As an optimization patch, I would buy that delaying the \"exists\" check\n> until the \"no abbreviation\" check returned early would make sense, though.\n>\n\n...or we could just do this. To be honest, this is probably where most\nof the change in performance come from. Which in case makes the commit\nmessage somewhat misleading.\n\nThen again, the third option is the easiest one; just drop 2/3 and\n3/3. I'd be perfectly fine with that, I just went a little OCD with\nconsistency here. Those two patches gain us very little. I haven't\nmeasured performance change outside of ls-tree, though.\n\n-- \nErik \"kusma\" Faye-Lund\n"}]}