{"thread":{"id":"44195","subject":"[PATCH 0/3] auto-sizing default abbreviation length","startedAt":"2016-10-01T00:19:47Z","lastAt":"2016-11-02T02:12:13Z","messageCount":13,"participants":["Junio C Hamano","Jeff King","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"303033","messageId":"20161001001937.10884-1-gitster@pobox.com","threadId":"44195","inReplyTo":null,"subject":"[PATCH 0/3] auto-sizing default abbreviation length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-01T00:19:34Z","receivedAt":"2016-10-01T00:19:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is what I queued in 'pu' tonight after back and forth with\nLinus and Peff.  The third step from Linus needs to be signed off\nand also a meaningful log message for it needs to be written, and\nalso may need to be updated to include what Linus did in [*1*], but\notherwise I think these are in good enough shape for people to start\nplaying with them.\n\nThey apply on top of Peff's jk/ambiguous-short-object-names topic\nthat ends at 5b33cb1fd7 (\"get_short_sha1: make default\ndisambiguation configurable\", 2016-09-27).\n\n\n*1*\n\nhttp://public-inbox.org/git/<CA+55aFxyF=xX84AXr8MG14MRHwdrQw00PBM20UfqBdidaeqdMg@mail.gmail.com>\n\n\nJunio C Hamano (2):\n  abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing\n  abbrev: prepare for new world order\n\nLinus Torvalds (1):\n  abbrev: auto size the default abbreviation\n\n builtin/fetch.c     |  3 +++\n builtin/rev-parse.c |  5 +++--\n cache.h             |  4 ++++\n diff.c              | 25 +++++++++++++++++++++++--\n environment.c       |  2 +-\n sha1_name.c         | 28 +++++++++++++++++++++++++++-\n transport.h         |  3 +--\n 7 files changed, 62 insertions(+), 8 deletions(-)\n\n-- \n2.10.0-622-g05f606bbb0\n\n"},{"id":"303034","messageId":"20161001001937.10884-2-gitster@pobox.com","threadId":"44195","inReplyTo":"20161001001937.10884-1-gitster@pobox.com","subject":"[PATCH 1/3] abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-01T00:19:35Z","receivedAt":"2016-10-01T00:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We'll be introducing a new way to decide the default abbreviation\nlength by initialising DEFAULT_ABBREV to -1 to signal the first call\nto \"find unique abbreviation\" codepath to compute a reasonable value\nbased on the number of objects we have to avoid collisions.\n\nWe have long relied on DEFAULT_ABBREV being a positive concrete\nvalue that is used as the abbreviation length when no extra\nconfiguration or command line option has overridden it.  Some\ncodepaths wants to use such a positive concrete default value\neven before making their first request to actually trigger the\ncomputation for the auto sized default.\n\nIntroduce FALLBACK_DEFAULT_ABBREV and use it to the code that\nattempts to align the report from \"git fetch\".  For now, this\nmacro is also used to initialize the default_abbrev variable,\nbut the auto-sizing code will use -1 and then use the value of\nFALLBACK_DEFAULT_ABBREV as the starting point of auto-sizing.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 3 +++\n cache.h         | 3 +++\n environment.c   | 2 +-\n transport.h     | 3 +--\n 4 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 164623bb6f..a9f12cc5cf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -17,6 +17,9 @@\n #include \"argv-array.h\"\n #include \"utf8.h\"\n \n+#define TRANSPORT_SUMMARY(x) \\\n+\t(int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n+\n static const char * const builtin_fetch_usage[] = {\n \tN_(\"git fetch [<options>] [<repository> [<refspec>...]]\"),\n \tN_(\"git fetch [<options>] <group>\"),\ndiff --git a/cache.h b/cache.h\nindex f346c01708..5a651b8435 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1183,6 +1183,9 @@ static inline int hex2chr(const char *s)\n #define MINIMUM_ABBREV minimum_abbrev\n #define DEFAULT_ABBREV default_abbrev\n \n+/* used when the code does not know or care what the default abbrev is */\n+#define FALLBACK_DEFAULT_ABBREV 7\n+\n struct object_context {\n \tunsigned char tree[20];\n \tchar path[PATH_MAX];\ndiff --git a/environment.c b/environment.c\nindex cd5aa57179..44fb107b8a 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -16,7 +16,7 @@ int trust_executable_bit = 1;\n int trust_ctime = 1;\n int check_stat = 1;\n int has_symlinks = 1;\n-int minimum_abbrev = 4, default_abbrev = 7;\n+int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;\n int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\ndiff --git a/transport.h b/transport.h\nindex 6fe3485325..e783377e40 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -142,8 +142,7 @@ struct transport {\n #define TRANSPORT_PUSH_ATOMIC 8192\n #define TRANSPORT_PUSH_OPTIONS 16384\n \n-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n-#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n+#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)\n \n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n-- \n2.10.0-622-g05f606bbb0\n\n"},{"id":"303035","messageId":"20161001001937.10884-3-gitster@pobox.com","threadId":"44195","inReplyTo":"20161001001937.10884-1-gitster@pobox.com","subject":"[PATCH 2/3] abbrev: prepare for new world order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-01T00:19:36Z","receivedAt":"2016-10-01T00:19:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code that sets custom abbreviation length, in response to\ncommand line argument, often does something like this:\n\n\tif (skip_prefix(arg, \"--abbrev=\", &arg))\n\t\tabbrev = atoi(arg);\n\telse if (!strcmp(\"--abbrev\", &arg))\n\t\tabbrev = DEFAULT_ABBREV;\n\t/* make the value sane */\n\tif (abbrev < 0 || 40 < abbrev)\n\t\tabbrev = ... some sane value ...\n\nHowever, it is pointless to sanity-check and tweak the value\nobtained from DEFAULT_ABBREV.  We are going to allow it to be\ninitially set to -1 to signal that the default abbreviation length\nmust be auto sized upon the first request to abbreviate, based on\nthe number of objects in the repository, and when that happens,\nrejecting or tweaking a negative value to a \"saner\" one will\nnegatively interfere with the auto sizing.  The codepaths for\n\n    git rev-parse --short <object>\n    git diff --raw --abbrev\n\ndo exactly that; allow them to pass possibly negative abbrevs\nintact, that will come from DEFAULT_ABBREV in the future.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/rev-parse.c | 5 +++--\n diff.c              | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 76cf05e2ad..17cbfabdde 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -643,8 +643,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tfilter &= ~(DO_FLAGS|DO_NOREV);\n \t\t\t\tverify = 1;\n \t\t\t\tabbrev = DEFAULT_ABBREV;\n-\t\t\t\tif (arg[7] == '=')\n-\t\t\t\t\tabbrev = strtoul(arg + 8, NULL, 10);\n+\t\t\t\tif (!arg[7])\n+\t\t\t\t\tcontinue;\n+\t\t\t\tabbrev = strtoul(arg + 8, NULL, 10);\n \t\t\t\tif (abbrev < MINIMUM_ABBREV)\n \t\t\t\t\tabbrev = MINIMUM_ABBREV;\n \t\t\t\telse if (40 <= abbrev)\ndiff --git a/diff.c b/diff.c\nindex c6da383c56..cefc13eb8e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3399,7 +3399,7 @@ void diff_setup_done(struct diff_options *options)\n \t\t\t */\n \t\t\tread_cache();\n \t}\n-\tif (options->abbrev <= 0 || 40 < options->abbrev)\n+\tif (40 < options->abbrev)\n \t\toptions->abbrev = 40; /* full */\n \n \t/*\n-- \n2.10.0-622-g05f606bbb0\n\n"},{"id":"303036","messageId":"20161001001937.10884-4-gitster@pobox.com","threadId":"44195","inReplyTo":"20161001001937.10884-1-gitster@pobox.com","subject":"[PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-01T00:19:37Z","receivedAt":"2016-10-01T00:20:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Linus Torvalds <torvalds@linux-foundation.org>\n\nIn fairly early days we somehow decided to abbreviate object names\ndown to 7-hexdigits, but as projects grow, it is becoming more and\nmore likely to see such a short object names made in earlier days\nand recorded in the log messages no longer unique.\n\nCurrently the Linux kernel project needs 11 to 12 hexdigits, while\nGit itself needs 10 hexdigits to uniquely identify the objects they\nhave, while many smaller projects may still be fine with the\noriginal 7-hexdigit default.  One-size does not fit all projects.\n\nIntroduce a mechanism, where we estimate the number of objects in\nthe repository upon the first request to abbreviate an object name\nwith the default setting and come up with a sane default for the\nrepository.  Based on the expectation that we would see collision in\na repository with 2^(2N) objects when using object names shortened\nto first N bits, use sufficient number of hexdigits to cover the\nnumber of objects in the repository.  Each hexdigit (4-bits) we add\nto the shortened name allows us to have four times (2-bits) as many\nobjects in the repository.\n\n---\n cache.h       |  1 +\n environment.c |  2 +-\n sha1_name.c   | 28 +++++++++++++++++++++++++++-\n 3 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5a651b8435..0e2a0595e5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1204,6 +1204,7 @@ struct object_context {\n #define GET_SHA1_TREEISH          020\n #define GET_SHA1_BLOB             040\n #define GET_SHA1_FOLLOW_SYMLINKS 0100\n+#define GET_SHA1_AUTOMATIC\t 0200\n #define GET_SHA1_ONLY_TO_DIE    04000\n \n #define GET_SHA1_DISAMBIGUATORS \\\ndiff --git a/environment.c b/environment.c\nindex 44fb107b8a..6f9d290563 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -16,7 +16,7 @@ int trust_executable_bit = 1;\n int trust_ctime = 1;\n int check_stat = 1;\n int has_symlinks = 1;\n-int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;\n+int minimum_abbrev = 4, default_abbrev = -1;\n int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3b647fd7cf..beb7ab588b 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -15,6 +15,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);\n \n struct disambiguate_state {\n \tint len; /* length of prefix in hex chars */\n+\tunsigned int nrobjects;\n \tchar hex_pfx[GIT_SHA1_HEXSZ + 1];\n \tunsigned char bin_pfx[GIT_SHA1_RAWSZ];\n \n@@ -118,6 +119,14 @@ static void find_short_object_filename(struct disambiguate_state *ds)\n \n \t\t\tif (strlen(de->d_name) != 38)\n \t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * We only look at the one subdirectory, and we assume\n+\t\t\t * each subdirectory is roughly similar, so each\n+\t\t\t * object we find probably has 255 other objects in\n+\t\t\t * the other fan-out directories.\n+\t\t\t */\n+\t\t\tds->nrobjects += 256;\n \t\t\tif (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))\n \t\t\t\tcontinue;\n \t\t\tmemcpy(hex + 2, de->d_name, 38);\n@@ -151,6 +160,7 @@ static void unique_in_pack(struct packed_git *p,\n \n \topen_pack_index(p);\n \tnum = p->num_objects;\n+\tds->nrobjects += num;\n \tlast = num;\n \twhile (first < last) {\n \t\tuint32_t mid = (first + last) / 2;\n@@ -380,6 +390,9 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)\n \treturn 0;\n }\n \n+/* start from our historical default before the automatic abbreviation */\n+static int default_automatic_abbrev = FALLBACK_DEFAULT_ABBREV;\n+\n static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \t\t\t  unsigned flags)\n {\n@@ -426,6 +439,14 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \t\tfor_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);\n \t}\n \n+\tif (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {\n+\t\tunsigned int expect_collision = 1 << (len * 2);\n+\t\tif (ds.nrobjects > expect_collision) {\n+\t\t\tdefault_automatic_abbrev = len+1;\n+\t\t\treturn SHORT_NAME_AMBIGUOUS;\n+\t\t}\n+\t}\n+\n \treturn status;\n }\n \n@@ -458,14 +479,19 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)\n int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)\n {\n \tint status, exists;\n+\tint flags = GET_SHA1_QUIETLY;\n \n+\tif (len < 0) {\n+\t\tflags |= GET_SHA1_AUTOMATIC;\n+\t\tlen = default_automatic_abbrev;\n+\t}\n \tsha1_to_hex_r(hex, sha1);\n \tif (len == 40 || !len)\n \t\treturn 40;\n \texists = has_sha1_file(sha1);\n \twhile (len < 40) {\n \t\tunsigned char sha1_ret[20];\n-\t\tstatus = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);\n+\t\tstatus = get_short_sha1(hex, len, sha1_ret, flags);\n \t\tif (exists\n \t\t    ? !status\n \t\t    : status == SHORT_NAME_NOT_FOUND) {\n-- \n2.10.0-622-g05f606bbb0\n\n"},{"id":"303154","messageId":"20161003222701.za5njew33rqc5b6g@sigill.intra.peff.net","threadId":"44195","inReplyTo":"20161001001937.10884-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T22:27:02Z","receivedAt":"2016-10-03T22:27:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote:\n\n> Introduce a mechanism, where we estimate the number of objects in\n> the repository upon the first request to abbreviate an object name\n> with the default setting and come up with a sane default for the\n> repository.  Based on the expectation that we would see collision in\n> a repository with 2^(2N) objects when using object names shortened\n> to first N bits, use sufficient number of hexdigits to cover the\n> number of objects in the repository.  Each hexdigit (4-bits) we add\n> to the shortened name allows us to have four times (2-bits) as many\n> objects in the repository.\n> \n> ---\n>  cache.h       |  1 +\n>  environment.c |  2 +-\n>  sha1_name.c   | 28 +++++++++++++++++++++++++++-\n>  3 files changed, 29 insertions(+), 2 deletions(-)\n\nFor reference, here's a working version that just uses a separate\ncounting function (no commit message, because I would just steal the one\nfrom Linus ;) ).\n\n---\n cache.h       |  6 ++++++\n environment.c |  2 +-\n sha1_file.c   | 27 +++++++++++++++++++++++++++\n sha1_name.c   | 20 ++++++++++++++++++++\n 4 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5a651b8..f22ace5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1455,6 +1455,12 @@ extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\n extern void install_packed_git(struct packed_git *pack);\n \n+/*\n+ * Give a rough count of objects in the repository. This sacrifices accuracy\n+ * for speed.\n+ */\n+unsigned long approximate_object_count(void);\n+\n extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \t\t\t\t\t struct packed_git *packs);\n \ndiff --git a/environment.c b/environment.c\nindex 44fb107..6f9d290 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -16,7 +16,7 @@ int trust_executable_bit = 1;\n int trust_ctime = 1;\n int check_stat = 1;\n int has_symlinks = 1;\n-int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;\n+int minimum_abbrev = 4, default_abbrev = -1;\n int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..4882440 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1381,6 +1381,32 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tstrbuf_release(&path);\n }\n \n+static int approximate_object_count_valid;\n+\n+/*\n+ * Give a fast, rough count of the number of objects in the repository. This\n+ * ignores loose objects completely. If you have a lot of them, then either\n+ * you should repack because your performance will be awful, or they are\n+ * all unreachable objects about to be pruned, in which case they're not really\n+ * interesting as a measure of repo size in the first place.\n+ */\n+unsigned long approximate_object_count(void)\n+{\n+\tstatic unsigned long count;\n+\tif (!approximate_object_count_valid) {\n+\t\tstruct packed_git *p;\n+\n+\t\tprepare_packed_git();\n+\t\tcount = 0;\n+\t\tfor (p = packed_git; p; p = p->next) {\n+\t\t\tif (open_pack_index(p))\n+\t\t\t\tcontinue;\n+\t\t\tcount += p->num_objects;\n+\t\t}\n+\t}\n+\treturn count;\n+}\n+\n static void *get_next_packed_git(const void *p)\n {\n \treturn ((const struct packed_git *)p)->next;\n@@ -1455,6 +1481,7 @@ void prepare_packed_git(void)\n \n void reprepare_packed_git(void)\n {\n+\tapproximate_object_count_valid = 0;\n \tprepare_packed_git_run_once = 0;\n \tprepare_packed_git();\n }\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3b647fd..ecc4b54 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -455,10 +455,30 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)\n \treturn ret;\n }\n \n+/*\n+ * Return the slot of the most-significant bit set in \"val\". There are various\n+ * ways to do this quickly with fls() or __builtin_clzl(), but speed is\n+ * probably not a big deal here.\n+ */\n+unsigned msb(unsigned long val)\n+{\n+\tunsigned r = 0;\n+\twhile (val >>= 1)\n+\t\tr++;\n+\treturn r;\n+}\n+\n int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)\n {\n \tint status, exists;\n \n+\tif (len < 0) {\n+\t\tunsigned long count = approximate_object_count();\n+\t\tlen = (msb(count) + 1) / 2;\n+\t\tif (len < 0)\n+\t\t\tlen = FALLBACK_DEFAULT_ABBREV;\n+\t}\n+\n \tsha1_to_hex_r(hex, sha1);\n \tif (len == 40 || !len)\n \t\treturn 40;\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303158","messageId":"CA+55aFydV+9c3-5C03XUj7v_wGJF5NyJNaP6742zLVgZs410FA@mail.gmail.com","threadId":"44195","inReplyTo":"20161003222701.za5njew33rqc5b6g@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-03T22:34:03Z","receivedAt":"2016-10-03T22:34:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Oct 3, 2016 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n>\n> +       if (len < 0) {\n> +               unsigned long count = approximate_object_count();\n> +               len = (msb(count) + 1) / 2;\n> +               if (len < 0)\n> +                       len = FALLBACK_DEFAULT_ABBREV;\n> +       }\n\nthat second \"if (len < 0)\" should probably be testing against\nFALLBACK_DEFAULT_ABBREV, not zero. Or at the very least\nMINIMUM_ABBREV. Because a two-character abbreviation won't even be\nrecognized, even if the git project is very small indeed.\n\n            Linus\n"},{"id":"303159","messageId":"20161003224028.ksvwaplxe7a3vtwv@sigill.intra.peff.net","threadId":"44195","inReplyTo":"CA+55aFydV+9c3-5C03XUj7v_wGJF5NyJNaP6742zLVgZs410FA@mail.gmail.com","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T22:40:28Z","receivedAt":"2016-10-03T22:40:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 03:34:03PM -0700, Linus Torvalds wrote:\n\n> On Mon, Oct 3, 2016 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n> >\n> > +       if (len < 0) {\n> > +               unsigned long count = approximate_object_count();\n> > +               len = (msb(count) + 1) / 2;\n> > +               if (len < 0)\n> > +                       len = FALLBACK_DEFAULT_ABBREV;\n> > +       }\n> \n> that second \"if (len < 0)\" should probably be testing against\n> FALLBACK_DEFAULT_ABBREV, not zero. Or at the very least\n> MINIMUM_ABBREV. Because a two-character abbreviation won't even be\n> recognized, even if the git project is very small indeed.\n\nOops, yes, clearly it should be FALLBACK_DEFAULT_ABBREV. What is there\nwould not even pass the tests (it _does_ work on linux.git, of course,\nbecause it is much too large for that code to be triggered).\n\n-Peff\n"},{"id":"303160","messageId":"xmqqoa313v0j.fsf@gitster.mtv.corp.google.com","threadId":"44195","inReplyTo":"20161003224028.ksvwaplxe7a3vtwv@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T22:52:44Z","receivedAt":"2016-10-03T22:52:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Oct 03, 2016 at 03:34:03PM -0700, Linus Torvalds wrote:\n>\n>> On Mon, Oct 3, 2016 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n>> >\n>> > +       if (len < 0) {\n>> > +               unsigned long count = approximate_object_count();\n>> > +               len = (msb(count) + 1) / 2;\n>> > +               if (len < 0)\n>> > +                       len = FALLBACK_DEFAULT_ABBREV;\n>> > +       }\n>> \n>> that second \"if (len < 0)\" should probably be testing against\n>> FALLBACK_DEFAULT_ABBREV, not zero. Or at the very least\n>> MINIMUM_ABBREV. Because a two-character abbreviation won't even be\n>> recognized, even if the git project is very small indeed.\n>\n> Oops, yes, clearly it should be FALLBACK_DEFAULT_ABBREV. What is there\n> would not even pass the tests (it _does_ work on linux.git, of course,\n> because it is much too large for that code to be triggered).\n\nOK, as Linus's \"count at the point of use\" is already in 'next',\ncould you make it incremental with a log message?\n\nThanks.\n"},{"id":"303164","messageId":"20161003234728.s5sadekukxoppcmw@sigill.intra.peff.net","threadId":"44195","inReplyTo":"xmqqoa313v0j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T23:47:28Z","receivedAt":"2016-10-03T23:47:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 03:52:44PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, Oct 03, 2016 at 03:34:03PM -0700, Linus Torvalds wrote:\n> >\n> >> On Mon, Oct 3, 2016 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n> >> >\n> >> > +       if (len < 0) {\n> >> > +               unsigned long count = approximate_object_count();\n> >> > +               len = (msb(count) + 1) / 2;\n> >> > +               if (len < 0)\n> >> > +                       len = FALLBACK_DEFAULT_ABBREV;\n> >> > +       }\n> >> \n> >> that second \"if (len < 0)\" should probably be testing against\n> >> FALLBACK_DEFAULT_ABBREV, not zero. Or at the very least\n> >> MINIMUM_ABBREV. Because a two-character abbreviation won't even be\n> >> recognized, even if the git project is very small indeed.\n> >\n> > Oops, yes, clearly it should be FALLBACK_DEFAULT_ABBREV. What is there\n> > would not even pass the tests (it _does_ work on linux.git, of course,\n> > because it is much too large for that code to be triggered).\n> \n> OK, as Linus's \"count at the point of use\" is already in 'next',\n> could you make it incremental with a log message?\n\nSure. I wasn't sure if you actually liked my direction or not, so I was\nmostly just showing off what the completed one would look like.\n\nHere it is as an incremental on top of lt/abbrev-auto. I also tweaked\nthe math a bit to round-up more aggressively, and commented it more (I\ncould also just make the math look exactly like Linus's, counting up\nuntil hitting an expected collision. I dunno if that is more clear).\n\n-- >8 --\nSubject: [PATCH] find_unique_abbrev: move logic out of get_short_sha1()\n\nThe get_short_sha1() is only about reading short sha1s; we\ndo call it in a loop to check \"is this long enough\" for each\nobject, but otherwise it should not need to know about\nthings like our default_abbrev setting.\n\nSo instead of asking it to set default_automatic_abbrev as a\nside-effect, let's just have find_unique_abbrev() pick the\nright place to start its loop.  This requires a separate\napproximate_object_count() function, but that naturally\nbelongs with the rest of sha1_file.c.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     |  7 ++++++-\n sha1_file.c | 27 +++++++++++++++++++++++++++\n sha1_name.c | 60 +++++++++++++++++++++++++++++++++++-------------------------\n 3 files changed, 68 insertions(+), 26 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0e2a059..f22ace5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1204,7 +1204,6 @@ struct object_context {\n #define GET_SHA1_TREEISH          020\n #define GET_SHA1_BLOB             040\n #define GET_SHA1_FOLLOW_SYMLINKS 0100\n-#define GET_SHA1_AUTOMATIC\t 0200\n #define GET_SHA1_ONLY_TO_DIE    04000\n \n #define GET_SHA1_DISAMBIGUATORS \\\n@@ -1456,6 +1455,12 @@ extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\n extern void install_packed_git(struct packed_git *pack);\n \n+/*\n+ * Give a rough count of objects in the repository. This sacrifices accuracy\n+ * for speed.\n+ */\n+unsigned long approximate_object_count(void);\n+\n extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \t\t\t\t\t struct packed_git *packs);\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..4882440 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1381,6 +1381,32 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tstrbuf_release(&path);\n }\n \n+static int approximate_object_count_valid;\n+\n+/*\n+ * Give a fast, rough count of the number of objects in the repository. This\n+ * ignores loose objects completely. If you have a lot of them, then either\n+ * you should repack because your performance will be awful, or they are\n+ * all unreachable objects about to be pruned, in which case they're not really\n+ * interesting as a measure of repo size in the first place.\n+ */\n+unsigned long approximate_object_count(void)\n+{\n+\tstatic unsigned long count;\n+\tif (!approximate_object_count_valid) {\n+\t\tstruct packed_git *p;\n+\n+\t\tprepare_packed_git();\n+\t\tcount = 0;\n+\t\tfor (p = packed_git; p; p = p->next) {\n+\t\t\tif (open_pack_index(p))\n+\t\t\t\tcontinue;\n+\t\t\tcount += p->num_objects;\n+\t\t}\n+\t}\n+\treturn count;\n+}\n+\n static void *get_next_packed_git(const void *p)\n {\n \treturn ((const struct packed_git *)p)->next;\n@@ -1455,6 +1481,7 @@ void prepare_packed_git(void)\n \n void reprepare_packed_git(void)\n {\n+\tapproximate_object_count_valid = 0;\n \tprepare_packed_git_run_once = 0;\n \tprepare_packed_git();\n }\ndiff --git a/sha1_name.c b/sha1_name.c\nindex beb7ab5..76e6885 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -15,7 +15,6 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);\n \n struct disambiguate_state {\n \tint len; /* length of prefix in hex chars */\n-\tunsigned int nrobjects;\n \tchar hex_pfx[GIT_SHA1_HEXSZ + 1];\n \tunsigned char bin_pfx[GIT_SHA1_RAWSZ];\n \n@@ -119,14 +118,6 @@ static void find_short_object_filename(struct disambiguate_state *ds)\n \n \t\t\tif (strlen(de->d_name) != 38)\n \t\t\t\tcontinue;\n-\n-\t\t\t/*\n-\t\t\t * We only look at the one subdirectory, and we assume\n-\t\t\t * each subdirectory is roughly similar, so each\n-\t\t\t * object we find probably has 255 other objects in\n-\t\t\t * the other fan-out directories.\n-\t\t\t */\n-\t\t\tds->nrobjects += 256;\n \t\t\tif (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))\n \t\t\t\tcontinue;\n \t\t\tmemcpy(hex + 2, de->d_name, 38);\n@@ -160,7 +151,6 @@ static void unique_in_pack(struct packed_git *p,\n \n \topen_pack_index(p);\n \tnum = p->num_objects;\n-\tds->nrobjects += num;\n \tlast = num;\n \twhile (first < last) {\n \t\tuint32_t mid = (first + last) / 2;\n@@ -390,9 +380,6 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)\n \treturn 0;\n }\n \n-/* start from our historical default before the automatic abbreviation */\n-static int default_automatic_abbrev = FALLBACK_DEFAULT_ABBREV;\n-\n static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \t\t\t  unsigned flags)\n {\n@@ -439,14 +426,6 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n \t\tfor_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);\n \t}\n \n-\tif (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {\n-\t\tunsigned int expect_collision = 1 << (len * 2);\n-\t\tif (ds.nrobjects > expect_collision) {\n-\t\t\tdefault_automatic_abbrev = len+1;\n-\t\t\treturn SHORT_NAME_AMBIGUOUS;\n-\t\t}\n-\t}\n-\n \treturn status;\n }\n \n@@ -476,22 +455,53 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)\n \treturn ret;\n }\n \n+/*\n+ * Return the slot of the most-significant bit set in \"val\". There are various\n+ * ways to do this quickly with fls() or __builtin_clzl(), but speed is\n+ * probably not a big deal here.\n+ */\n+unsigned msb(unsigned long val)\n+{\n+\tunsigned r = 0;\n+\twhile (val >>= 1)\n+\t\tr++;\n+\treturn r;\n+}\n+\n int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)\n {\n \tint status, exists;\n-\tint flags = GET_SHA1_QUIETLY;\n \n \tif (len < 0) {\n-\t\tflags |= GET_SHA1_AUTOMATIC;\n-\t\tlen = default_automatic_abbrev;\n+\t\tunsigned long count = approximate_object_count();\n+\t\t/*\n+\t\t * Add one because the MSB only tells us the highest bit set,\n+\t\t * not including the value of all the _other_ bits (so \"15\"\n+\t\t * is only one off of 2^4, but the MSB is the 3rd bit.\n+\t\t */\n+\t\tlen = msb(count) + 1;\n+\t\t/*\n+\t\t * We now know we have on the order of 2^len objects, which\n+\t\t * expects a collision at 2^(len/2). But we also care about hex\n+\t\t * chars, not bits, and there are 4 bits per hex. So all\n+\t\t * together we need to divide by 2; but we also want to round\n+\t\t * odd numbers up, hence adding one before dividing.\n+\t\t */\n+\t\tlen = (len + 1) / 2;\n+\t\t/*\n+\t\t * For very small repos, we stick with our regular fallback.\n+\t\t */\n+\t\tif (len < FALLBACK_DEFAULT_ABBREV)\n+\t\t\tlen = FALLBACK_DEFAULT_ABBREV;\n \t}\n+\n \tsha1_to_hex_r(hex, sha1);\n \tif (len == 40 || !len)\n \t\treturn 40;\n \texists = has_sha1_file(sha1);\n \twhile (len < 40) {\n \t\tunsigned char sha1_ret[20];\n-\t\tstatus = get_short_sha1(hex, len, sha1_ret, flags);\n+\t\tstatus = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);\n \t\tif (exists\n \t\t    ? !status\n \t\t    : status == SHORT_NAME_NOT_FOUND) {\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303169","messageId":"xmqqbmz051yp.fsf@gitster.mtv.corp.google.com","threadId":"44195","inReplyTo":"20161003234728.s5sadekukxoppcmw@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T01:37:18Z","receivedAt":"2016-10-04T01:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> OK, as Linus's \"count at the point of use\" is already in 'next',\n>> could you make it incremental with a log message?\n>\n> Sure. I wasn't sure if you actually liked my direction or not, so I was\n> mostly just showing off what the completed one would look like.\n\nTo be quite honest, I am not just unsure if I liked your direction;\nrather I am not sure if I actually understood what you perceived as\na difference that matters between the two approaches.  I wanted to\nhear you explain the difference in terms of \"Linus's does this, but\nit is bad in X and Y way, so let's avoid it and do it like Z\ninstead\".  One effective way to extract that out of you was to force\nyou to justify the \"incremental\" update.\n\nAnd it seems that I succeeded ;-).\n\nI am still not sure if I 100% agree with your first paragraph, but\nat least now I think I see where you are coming from.\n\nYou probably will hear from Ramsay about extern-ness of msb().\n\n> -- >8 --\n> Subject: [PATCH] find_unique_abbrev: move logic out of get_short_sha1()\n>\n> The get_short_sha1() is only about reading short sha1s; we\n> do call it in a loop to check \"is this long enough\" for each\n> object, but otherwise it should not need to know about\n> things like our default_abbrev setting.\n>\n> So instead of asking it to set default_automatic_abbrev as a\n> side-effect, let's just have find_unique_abbrev() pick the\n> right place to start its loop.  This requires a separate\n> approximate_object_count() function, but that naturally\n> belongs with the rest of sha1_file.c.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  cache.h     |  7 ++++++-\n>  sha1_file.c | 27 +++++++++++++++++++++++++++\n>  sha1_name.c | 60 +++++++++++++++++++++++++++++++++++-------------------------\n>  3 files changed, 68 insertions(+), 26 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 0e2a059..f22ace5 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1204,7 +1204,6 @@ struct object_context {\n>  #define GET_SHA1_TREEISH          020\n>  #define GET_SHA1_BLOB             040\n>  #define GET_SHA1_FOLLOW_SYMLINKS 0100\n> -#define GET_SHA1_AUTOMATIC\t 0200\n>  #define GET_SHA1_ONLY_TO_DIE    04000\n>  \n>  #define GET_SHA1_DISAMBIGUATORS \\\n> @@ -1456,6 +1455,12 @@ extern void prepare_packed_git(void);\n>  extern void reprepare_packed_git(void);\n>  extern void install_packed_git(struct packed_git *pack);\n>  \n> +/*\n> + * Give a rough count of objects in the repository. This sacrifices accuracy\n> + * for speed.\n> + */\n> +unsigned long approximate_object_count(void);\n> +\n>  extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n>  \t\t\t\t\t struct packed_git *packs);\n>  \n> diff --git a/sha1_file.c b/sha1_file.c\n> index b9c1fa3..4882440 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1381,6 +1381,32 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \tstrbuf_release(&path);\n>  }\n>  \n> +static int approximate_object_count_valid;\n> +\n> +/*\n> + * Give a fast, rough count of the number of objects in the repository. This\n> + * ignores loose objects completely. If you have a lot of them, then either\n> + * you should repack because your performance will be awful, or they are\n> + * all unreachable objects about to be pruned, in which case they're not really\n> + * interesting as a measure of repo size in the first place.\n> + */\n> +unsigned long approximate_object_count(void)\n> +{\n> +\tstatic unsigned long count;\n> +\tif (!approximate_object_count_valid) {\n> +\t\tstruct packed_git *p;\n> +\n> +\t\tprepare_packed_git();\n> +\t\tcount = 0;\n> +\t\tfor (p = packed_git; p; p = p->next) {\n> +\t\t\tif (open_pack_index(p))\n> +\t\t\t\tcontinue;\n> +\t\t\tcount += p->num_objects;\n> +\t\t}\n> +\t}\n> +\treturn count;\n> +}\n> +\n>  static void *get_next_packed_git(const void *p)\n>  {\n>  \treturn ((const struct packed_git *)p)->next;\n> @@ -1455,6 +1481,7 @@ void prepare_packed_git(void)\n>  \n>  void reprepare_packed_git(void)\n>  {\n> +\tapproximate_object_count_valid = 0;\n>  \tprepare_packed_git_run_once = 0;\n>  \tprepare_packed_git();\n>  }\n> diff --git a/sha1_name.c b/sha1_name.c\n> index beb7ab5..76e6885 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -15,7 +15,6 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);\n>  \n>  struct disambiguate_state {\n>  \tint len; /* length of prefix in hex chars */\n> -\tunsigned int nrobjects;\n>  \tchar hex_pfx[GIT_SHA1_HEXSZ + 1];\n>  \tunsigned char bin_pfx[GIT_SHA1_RAWSZ];\n>  \n> @@ -119,14 +118,6 @@ static void find_short_object_filename(struct disambiguate_state *ds)\n>  \n>  \t\t\tif (strlen(de->d_name) != 38)\n>  \t\t\t\tcontinue;\n> -\n> -\t\t\t/*\n> -\t\t\t * We only look at the one subdirectory, and we assume\n> -\t\t\t * each subdirectory is roughly similar, so each\n> -\t\t\t * object we find probably has 255 other objects in\n> -\t\t\t * the other fan-out directories.\n> -\t\t\t */\n> -\t\t\tds->nrobjects += 256;\n>  \t\t\tif (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))\n>  \t\t\t\tcontinue;\n>  \t\t\tmemcpy(hex + 2, de->d_name, 38);\n> @@ -160,7 +151,6 @@ static void unique_in_pack(struct packed_git *p,\n>  \n>  \topen_pack_index(p);\n>  \tnum = p->num_objects;\n> -\tds->nrobjects += num;\n>  \tlast = num;\n>  \twhile (first < last) {\n>  \t\tuint32_t mid = (first + last) / 2;\n> @@ -390,9 +380,6 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)\n>  \treturn 0;\n>  }\n>  \n> -/* start from our historical default before the automatic abbreviation */\n> -static int default_automatic_abbrev = FALLBACK_DEFAULT_ABBREV;\n> -\n>  static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n>  \t\t\t  unsigned flags)\n>  {\n> @@ -439,14 +426,6 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,\n>  \t\tfor_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);\n>  \t}\n>  \n> -\tif (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {\n> -\t\tunsigned int expect_collision = 1 << (len * 2);\n> -\t\tif (ds.nrobjects > expect_collision) {\n> -\t\t\tdefault_automatic_abbrev = len+1;\n> -\t\t\treturn SHORT_NAME_AMBIGUOUS;\n> -\t\t}\n> -\t}\n> -\n>  \treturn status;\n>  }\n>  \n> @@ -476,22 +455,53 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)\n>  \treturn ret;\n>  }\n>  \n> +/*\n> + * Return the slot of the most-significant bit set in \"val\". There are various\n> + * ways to do this quickly with fls() or __builtin_clzl(), but speed is\n> + * probably not a big deal here.\n> + */\n> +unsigned msb(unsigned long val)\n> +{\n> +\tunsigned r = 0;\n> +\twhile (val >>= 1)\n> +\t\tr++;\n> +\treturn r;\n> +}\n> +\n>  int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)\n>  {\n>  \tint status, exists;\n> -\tint flags = GET_SHA1_QUIETLY;\n>  \n>  \tif (len < 0) {\n> -\t\tflags |= GET_SHA1_AUTOMATIC;\n> -\t\tlen = default_automatic_abbrev;\n> +\t\tunsigned long count = approximate_object_count();\n> +\t\t/*\n> +\t\t * Add one because the MSB only tells us the highest bit set,\n> +\t\t * not including the value of all the _other_ bits (so \"15\"\n> +\t\t * is only one off of 2^4, but the MSB is the 3rd bit.\n> +\t\t */\n> +\t\tlen = msb(count) + 1;\n> +\t\t/*\n> +\t\t * We now know we have on the order of 2^len objects, which\n> +\t\t * expects a collision at 2^(len/2). But we also care about hex\n> +\t\t * chars, not bits, and there are 4 bits per hex. So all\n> +\t\t * together we need to divide by 2; but we also want to round\n> +\t\t * odd numbers up, hence adding one before dividing.\n> +\t\t */\n> +\t\tlen = (len + 1) / 2;\n> +\t\t/*\n> +\t\t * For very small repos, we stick with our regular fallback.\n> +\t\t */\n> +\t\tif (len < FALLBACK_DEFAULT_ABBREV)\n> +\t\t\tlen = FALLBACK_DEFAULT_ABBREV;\n>  \t}\n> +\n>  \tsha1_to_hex_r(hex, sha1);\n>  \tif (len == 40 || !len)\n>  \t\treturn 40;\n>  \texists = has_sha1_file(sha1);\n>  \twhile (len < 40) {\n>  \t\tunsigned char sha1_ret[20];\n> -\t\tstatus = get_short_sha1(hex, len, sha1_ret, flags);\n> +\t\tstatus = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);\n>  \t\tif (exists\n>  \t\t    ? !status\n>  \t\t    : status == SHORT_NAME_NOT_FOUND) {\n"},{"id":"303208","messageId":"20161004121816.yjvxymv46cgmnf5p@sigill.intra.peff.net","threadId":"44195","inReplyTo":"xmqqbmz051yp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T12:18:16Z","receivedAt":"2016-10-04T12:22:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 06:37:18PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> OK, as Linus's \"count at the point of use\" is already in 'next',\n> >> could you make it incremental with a log message?\n> >\n> > Sure. I wasn't sure if you actually liked my direction or not, so I was\n> > mostly just showing off what the completed one would look like.\n> \n> To be quite honest, I am not just unsure if I liked your direction;\n> rather I am not sure if I actually understood what you perceived as\n> a difference that matters between the two approaches.  I wanted to\n> hear you explain the difference in terms of \"Linus's does this, but\n> it is bad in X and Y way, so let's avoid it and do it like Z\n> instead\".  One effective way to extract that out of you was to force\n> you to justify the \"incremental\" update.\n> \n> And it seems that I succeeded ;-).\n> \n> I am still not sure if I 100% agree with your first paragraph, but\n> at least now I think I see where you are coming from.\n\nFor the record, I am OK with Linus's patch as-is. It's mostly \"that's\nnot how I would have done it, and the flow seems confusing to me\". But\nthat's subjective; I don't think there are any functional flaws in it.\n\n> You probably will hear from Ramsay about extern-ness of msb().\n\nHeh. I seem to have a real problem with that lately.\n\n-Peff\n"},{"id":"305279","messageId":"xmqqmvhimzrh.fsf@gitster.mtv.corp.google.com","threadId":"44195","inReplyTo":"20161003222701.za5njew33rqc5b6g@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-02T01:33:38Z","receivedAt":"2016-11-02T01:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote:\n>\n>> Introduce a mechanism, where we estimate the number of objects in\n>> the repository upon the first request to abbreviate an object name\n>> with the default setting and come up with a sane default for the\n>> repository.  Based on the expectation that we would see collision in\n>> a repository with 2^(2N) objects when using object names shortened\n>> to first N bits, use sufficient number of hexdigits to cover the\n>> number of objects in the repository.  Each hexdigit (4-bits) we add\n>> to the shortened name allows us to have four times (2-bits) as many\n>> objects in the repository.\n\nI was idly browsing the draft release notes and then documentation\nand noticed that, even though the new default is to auto-scale,\nthere is no mention of that in the documentation and there is no way\nto explicitly ask for auto-scaling.\n\nI wonder if we want to have something like this.  I actually am\ninclined to drop the change to config.c and remove the new mention\nof \"auto\" in the documentation.\n\n Documentation/config.txt |  9 +++++----\n config.c                 | 14 ++++++++++----\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a0ab66aae7..b02f8a4025 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -783,10 +783,11 @@ core.sparseCheckout::\n \tlinkgit:git-read-tree[1] for more information.\n \n core.abbrev::\n-\tSet the length object names are abbreviated to.  If unspecified,\n-\tmany commands abbreviate to 7 hexdigits, which may not be enough\n-\tfor abbreviated object names to stay unique for sufficiently long\n-\ttime.\n+\tSet the length object names are abbreviated to.  If\n+\tunspecified or set to \"auto\", an appropriate value is\n+\tcomputed based on the approximate number of packed objects\n+\tin your repository, which hopefully is enough for\n+\tabbreviated object names to stay unique for some time.\n \n add.ignoreErrors::\n add.ignore-errors (deprecated)::\ndiff --git a/config.c b/config.c\nindex 83fdecb1bc..c363cca4a9 100644\n--- a/config.c\n+++ b/config.c\n@@ -834,10 +834,16 @@ static int git_default_core_config(const char *var, const char *value)\n \t}\n \n \tif (!strcmp(var, \"core.abbrev\")) {\n-\t\tint abbrev = git_config_int(var, value);\n-\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n-\t\t\treturn -1;\n-\t\tdefault_abbrev = abbrev;\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif (!strcasecmp(value, \"auto\"))\n+\t\t\tdefault_abbrev = -1;\n+\t\telse {\n+\t\t\tint abbrev = git_config_int(var, value);\n+\t\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n+\t\t\t\treturn -1;\n+\t\t\tdefault_abbrev = abbrev;\n+\t\t}\n \t\treturn 0;\n \t}\n \n"},{"id":"305281","messageId":"20161102021205.xyxnlxxx4nufvhor@sigill.intra.peff.net","threadId":"44195","inReplyTo":"xmqqmvhimzrh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] abbrev: auto size the default abbreviation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T02:12:05Z","receivedAt":"2016-11-02T02:12:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2016 at 06:33:38PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote:\n> >\n> >> Introduce a mechanism, where we estimate the number of objects in\n> >> the repository upon the first request to abbreviate an object name\n> >> with the default setting and come up with a sane default for the\n> >> repository.  Based on the expectation that we would see collision in\n> >> a repository with 2^(2N) objects when using object names shortened\n> >> to first N bits, use sufficient number of hexdigits to cover the\n> >> number of objects in the repository.  Each hexdigit (4-bits) we add\n> >> to the shortened name allows us to have four times (2-bits) as many\n> >> objects in the repository.\n> \n> I was idly browsing the draft release notes and then documentation\n> and noticed that, even though the new default is to auto-scale,\n> there is no mention of that in the documentation and there is no way\n> to explicitly ask for auto-scaling.\n> \n> I wonder if we want to have something like this.  I actually am\n> inclined to drop the change to config.c and remove the new mention\n> of \"auto\" in the documentation.\n\nI doubt anybody cares that much either way, but in theory\ncore.abbrev=auto is a way to override core.abbrev=10 in /etc/gitconfig\nor something. Though I'm having trouble envisioning a case where anybody\nwould set it in /etc/gitconfig, or why somebody would then want to\noverride that back to auto.\n\nSo I think it is fine either way (but I do agree that the core.abbrev\nneeds _some_ update to mention the auto-scaling behavior).\n\n> diff --git a/config.c b/config.c\n> index 83fdecb1bc..c363cca4a9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -834,10 +834,16 @@ static int git_default_core_config(const char *var, const char *value)\n>  \t}\n>  \n>  \tif (!strcmp(var, \"core.abbrev\")) {\n> -\t\tint abbrev = git_config_int(var, value);\n> -\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n> -\t\t\treturn -1;\n> -\t\tdefault_abbrev = abbrev;\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tif (!strcasecmp(value, \"auto\"))\n> +\t\t\tdefault_abbrev = -1;\n> +\t\telse {\n> +\t\t\tint abbrev = git_config_int(var, value);\n> +\t\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n> +\t\t\t\treturn -1;\n> +\t\t\tdefault_abbrev = abbrev;\n> +\t\t}\n\nThis isn't a new problem you added, but that \"return -1\" would probably\nleave people confused:\n\n  $ git -c core.abbrev=2 log\n  fatal: unable to parse 'core.abbrev' from command-line config\n\nProbably something like:\n\n  return error(\"abbrev length out of range: %d\", abbrev);\n\nwould be more descriptive. I doubt it's a big deal in practice, though\n(why would you set core.abbrev to something silly in the first place?).\n\n-Peff\n"}]}