{"thread":{"id":"12390","subject":"Redefine semantics of find_unique_abbrev()","startedAt":"2008-03-01T01:41:53Z","lastAt":"2008-03-02T08:11:10Z","messageCount":7,"participants":["Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"70457","messageId":"7vpruf9q5a.fsf@gitster.siamese.dyndns.org","threadId":"12390","inReplyTo":null,"subject":"Redefine semantics of find_unique_abbrev()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-01T01:41:53Z","receivedAt":"2008-03-01T01:41:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function returned NULL when no object that matches the name\nwas found, but that made the callers more complicated, as nobody\nused that NULL return as an indication that no object with such\na name exists.  They (at least the careful ones) instead took\nthe full 40-hexdigit and used in such a case, and the careless\nones segfaulted.\n\nWith this \"git rev-parse --short 5555555555555555555555555555555555555555\"\nwould stop segfaulting.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-reset.c     |    4 ----\n builtin-send-pack.c |    4 +---\n diff.c              |    2 --\n sha1_name.c         |    8 +++-----\n 4 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex af0037e..be58a8b 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -66,10 +66,6 @@ static void print_new_head_line(struct commit *commit)\n \tconst char *hex, *dots = \"...\", *body;\n \n \thex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n-\tif (!hex) {\n-\t\thex = sha1_to_hex(commit->object.sha1);\n-\t\tdots = \"\";\n-\t}\n \tprintf(\"HEAD is now at %s%s\", hex, dots);\n \tbody = strstr(commit->buffer, \"\\n\\n\");\n \tif (body) {\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex b0cfae8..930e0fb 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -264,9 +264,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str\n \n static const char *status_abbrev(unsigned char sha1[20])\n {\n-\tconst char *abbrev;\n-\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n-\treturn abbrev ? abbrev : sha1_to_hex(sha1);\n+\treturn find_unique_abbrev(sha1, DEFAULT_ABBREV);\n }\n \n static void print_ok_ref_status(struct ref *ref)\ndiff --git a/diff.c b/diff.c\nindex ad16164..b80f656 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2581,8 +2581,6 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)\n \t\treturn sha1_to_hex(sha1);\n \n \tabbrev = find_unique_abbrev(sha1, len);\n-\tif (!abbrev)\n-\t\treturn sha1_to_hex(sha1);\n \tabblen = strlen(abbrev);\n \tif (abblen < 37) {\n \t\tstatic char hex[41];\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 9d088cc..05c2e7a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -202,16 +202,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n \twhile (len < 40) {\n \t\tunsigned char sha1_ret[20];\n \t\tstatus = get_short_sha1(hex, len, sha1_ret, 1);\n-\t\tif (!status ||\n-\t\t    (is_null && status != SHORT_NAME_AMBIGUOUS)) {\n+\t\tif ((!is_null && !status) ||\n+\t\t    (is_null && status == SHORT_NAME_NOT_FOUND)) {\n \t\t\thex[len] = 0;\n \t\t\treturn hex;\n \t\t}\n-\t\tif (status != SHORT_NAME_AMBIGUOUS)\n-\t\t\treturn NULL;\n \t\tlen++;\n \t}\n-\treturn NULL;\n+\treturn hex;\n }\n \n static int ambiguous_path(const char *path, int len)\n"},{"id":"70467","messageId":"20080301050641.GB8969@coredump.intra.peff.net","threadId":"12390","inReplyTo":"7vpruf9q5a.fsf@gitster.siamese.dyndns.org","subject":"Re: Redefine semantics of find_unique_abbrev()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-01T05:06:41Z","receivedAt":"2008-03-01T05:06:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 29, 2008 at 05:41:53PM -0800, Junio C Hamano wrote:\n\n> The function returned NULL when no object that matches the name\n> was found, but that made the callers more complicated, as nobody\n> used that NULL return as an indication that no object with such\n> a name exists.  They (at least the careful ones) instead took\n> the full 40-hexdigit and used in such a case, and the careless\n> ones segfaulted.\n> \n> With this \"git rev-parse --short 5555555555555555555555555555555555555555\"\n> would stop segfaulting.\n\nI have been meaning to clean up and submit a similar patch from the\n1.5.4 freeze period. However, your patch will always print the\n40-hexdigit version, which looks quite ugly in status output. Instead,\nwe can do much better by finding the longest subsequence we _do_ know\nabout, and adding one digit to it.\n\nSo you get:\n\n  # this is the current master, abbreviated\n  $ git rev-parse --short 97b97c58e609a1cd23b3c2514f41cdb0405870ee\n  97b97c5\n\n  # this is an object we don't have, but similar to that\n  $ git rev-parse --short 97b97c58e609a1cd23b3c2514f41cdb0405870ff\n  97b97c58e609a1cd23b3c2514f41cdb0405870f\n\n  # and less similar means we can abbreviate more\n  $ git rev-parse --short 97b97c58e609a1cd23b3c2514fffffffffffffff\n  97b97c58e609a1cd23b3c2514ff\n\nImplementation is below:\n\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -202,16 +202,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n>  \twhile (len < 40) {\n>  \t\tunsigned char sha1_ret[20];\n>  \t\tstatus = get_short_sha1(hex, len, sha1_ret, 1);\n> -\t\tif (!status ||\n> -\t\t    (is_null && status != SHORT_NAME_AMBIGUOUS)) {\n> +\t\tif ((!is_null && !status) ||\n> +\t\t    (is_null && status == SHORT_NAME_NOT_FOUND)) {\n>  \t\t\thex[len] = 0;\n>  \t\t\treturn hex;\n>  \t\t}\n> -\t\tif (status != SHORT_NAME_AMBIGUOUS)\n> -\t\t\treturn NULL;\n>  \t\tlen++;\n>  \t}\n> -\treturn NULL;\n> +\treturn hex;\n>  }\n\nAll we have to do is treat the \"missing\" case like we treat \"is_null\"\n(and in fact, is_null is just a specialized case of something we don't\nhave). So replace this hunk with:\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 05c2e7a..9c71d1b 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -192,18 +192,18 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \n const char *find_unique_abbrev(const unsigned char *sha1, int len)\n {\n-\tint status, is_null;\n+\tint status, missing;\n \tstatic char hex[41];\n \n-\tis_null = is_null_sha1(sha1);\n+\tmissing = !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-\t\tif ((!is_null && !status) ||\n-\t\t    (is_null && status == SHORT_NAME_NOT_FOUND)) {\n+\t\tif ((!missing && !status) ||\n+\t\t    (missing && status == SHORT_NAME_NOT_FOUND)) {\n \t\t\thex[len] = 0;\n \t\t\treturn hex;\n \t\t}\n\n\n-Peff\n"},{"id":"70596","messageId":"7vd4qdtw7k.fsf@gitster.siamese.dyndns.org","threadId":"12390","inReplyTo":"20080301050641.GB8969@coredump.intra.peff.net","subject":"Re: Redefine semantics of find_unique_abbrev()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T07:35:11Z","receivedAt":"2008-03-02T07:35:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I have been meaning to clean up and submit a similar patch from the\n> 1.5.4 freeze period. However, your patch will always print the\n> 40-hexdigit version, which looks quite ugly in status output. Instead,\n> we can do much better by finding the longest subsequence we _do_ know\n> about, and adding one digit to it.\n\nHere is what I ended up with.  Instead of saying \"missing\", I said\n\"exists\", which I think makes the logic easier to read, at least to me.\n\nThat is, \"for objects we have, make sure it uniquely identifies,\notherwise, make sure the phoney name is long enough such that it would not\nname any existing object\".\n\n---\n\n sha1_name.c |   13 ++++++-------\n 1 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 9d088cc..8358ba2 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -192,26 +192,25 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \n const char *find_unique_abbrev(const unsigned char *sha1, int len)\n {\n-\tint status, is_null;\n+\tint status, exists;\n \tstatic char hex[41];\n \n-\tis_null = is_null_sha1(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-\t\tif (!status ||\n-\t\t    (is_null && status != SHORT_NAME_AMBIGUOUS)) {\n+\t\tif (exists\n+\t\t    ? !status\n+\t\t    : status == SHORT_NAME_NOT_FOUND) {\n \t\t\thex[len] = 0;\n \t\t\treturn hex;\n \t\t}\n-\t\tif (status != SHORT_NAME_AMBIGUOUS)\n-\t\t\treturn NULL;\n \t\tlen++;\n \t}\n-\treturn NULL;\n+\treturn hex;\n }\n \n static int ambiguous_path(const char *path, int len)\n"},{"id":"70598","messageId":"20080302074230.GD3935@coredump.intra.peff.net","threadId":"12390","inReplyTo":"7vd4qdtw7k.fsf@gitster.siamese.dyndns.org","subject":"Re: Redefine semantics of find_unique_abbrev()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T07:42:30Z","receivedAt":"2008-03-02T07:42:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 01, 2008 at 11:35:11PM -0800, Junio C Hamano wrote:\n\n> That is, \"for objects we have, make sure it uniquely identifies,\n> otherwise, make sure the phoney name is long enough such that it would not\n> name any existing object\".\n\nI think your logic is correct, and I think naming it 'exists' is more\nreadable (I don't have a tendency not to double-negate).\n\nBut...\n\n> -\t\tif (!status ||\n> -\t\t    (is_null && status != SHORT_NAME_AMBIGUOUS)) {\n> +\t\tif (exists\n> +\t\t    ? !status\n> +\t\t    : status == SHORT_NAME_NOT_FOUND) {\n>  \t\t\thex[len] = 0;\n>  \t\t\treturn hex;\n>  \t\t}\n\nMaybe it is just me, but I find the ternary operator here reduces\nreadability. I would have liked the more verbose:\n\n  if ((exists && !status) ||\n      (!exists && status == SHORT_NAME_NOT_FOUND)) {\n\nBut now I am just painting your bikeshed.\n\n-Peff\n"},{"id":"70600","messageId":"7vy791sgzt.fsf_-_@gitster.siamese.dyndns.org","threadId":"12390","inReplyTo":"20080301050641.GB8969@coredump.intra.peff.net","subject":"[PATCH] Clean up find_unique_abbrev() callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T07:49:10Z","receivedAt":"2008-03-02T07:49:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Now find_unique_abbrev() never returns NULL, there is no need for callers\nto prepare for seeing NULL and fall back to giving the full 40 hexdigits.\n\nWhile we are at it, drop \"...\" in the \"git reset\" output that reports the\nlocation of the new HEAD, between the abbreviated commit object name and\nthe one line commit summary.  Because we are always showing the HEAD\n(which cannot be missing!), we never had a case where we show the full 40\nhexdigits that is not followed by three dots, and these three dots were\nstealing 3 columns from the precious horizontal screen real estate out of\n80 that can better be used for the one line commit summary.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n builtin-reset.c     |    8 ++------\n builtin-send-pack.c |    4 +---\n diff.c              |    2 --\n 3 files changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex af0037e..bb3e192 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -63,14 +63,10 @@ static int reset_index_file(const unsigned char *sha1, int is_hard_reset)\n \n static void print_new_head_line(struct commit *commit)\n {\n-\tconst char *hex, *dots = \"...\", *body;\n+\tconst char *hex, *body;\n \n \thex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n-\tif (!hex) {\n-\t\thex = sha1_to_hex(commit->object.sha1);\n-\t\tdots = \"\";\n-\t}\n-\tprintf(\"HEAD is now at %s%s\", hex, dots);\n+\tprintf(\"HEAD is now at %s\", hex);\n \tbody = strstr(commit->buffer, \"\\n\\n\");\n \tif (body) {\n \t\tconst char *eol;\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex b0cfae8..930e0fb 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -264,9 +264,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str\n \n static const char *status_abbrev(unsigned char sha1[20])\n {\n-\tconst char *abbrev;\n-\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n-\treturn abbrev ? abbrev : sha1_to_hex(sha1);\n+\treturn find_unique_abbrev(sha1, DEFAULT_ABBREV);\n }\n \n static void print_ok_ref_status(struct ref *ref)\ndiff --git a/diff.c b/diff.c\nindex ad16164..b80f656 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2581,8 +2581,6 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)\n \t\treturn sha1_to_hex(sha1);\n \n \tabbrev = find_unique_abbrev(sha1, len);\n-\tif (!abbrev)\n-\t\treturn sha1_to_hex(sha1);\n \tabblen = strlen(abbrev);\n \tif (abblen < 37) {\n \t\tstatic char hex[41];\n"},{"id":"70601","messageId":"20080302075152.GA14843@coredump.intra.peff.net","threadId":"12390","inReplyTo":"7vy791sgzt.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Clean up find_unique_abbrev() callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T07:51:53Z","receivedAt":"2008-03-02T07:51:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 01, 2008 at 11:49:10PM -0800, Junio C Hamano wrote:\n\n>  3 files changed, 3 insertions(+), 11 deletions(-)\n\nI like it. :)\n\n-Peff\n"},{"id":"70602","messageId":"7vtzjpsfz5.fsf@gitster.siamese.dyndns.org","threadId":"12390","inReplyTo":"20080302074230.GD3935@coredump.intra.peff.net","subject":"Re: Redefine semantics of find_unique_abbrev()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-02T08:11:10Z","receivedAt":"2008-03-02T08:11:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Mar 01, 2008 at 11:35:11PM -0800, Junio C Hamano wrote:\n>\n>> That is, \"for objects we have, make sure it uniquely identifies,\n>> otherwise, make sure the phoney name is long enough such that it would not\n>> name any existing object\".\n>\n> I think your logic is correct, and I think naming it 'exists' is more\n> readable (I don't have a tendency not to double-negate).\n>\n> But...\n>\n>> -\t\tif (!status ||\n>> -\t\t    (is_null && status != SHORT_NAME_AMBIGUOUS)) {\n>> +\t\tif (exists\n>> +\t\t    ? !status\n>> +\t\t    : status == SHORT_NAME_NOT_FOUND) {\n>>  \t\t\thex[len] = 0;\n>>  \t\t\treturn hex;\n>>  \t\t}\n>\n> Maybe it is just me, but I find the ternary operator here reduces\n> readability. I would have liked the more verbose:\n>\n>   if ((exists && !status) ||\n>       (!exists && status == SHORT_NAME_NOT_FOUND)) {\n>\n> But now I am just painting your bikeshed.\n\nHeh, the ternary is a mini \"if-then-else\" by itself.  Turn your head\nsideways (just like you do when you meet a smiley) and the parse tree will\njump at you ;-).\n\n\n"}]}