{"thread":{"id":"48679","subject":"[PATCH 00/20] unconditional O(1) SHA-1 abbreviation","startedAt":"2018-06-08T22:41:53Z","lastAt":"2018-06-14T19:08:23Z","messageCount":37,"participants":["Ævar Arnfjörð Bjarmason","Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"349747","messageId":"20180608224136.20220-1-avarab@gmail.com","threadId":"48679","inReplyTo":null,"subject":"[PATCH 00/20] unconditional O(1) SHA-1 abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:16Z","receivedAt":"2018-06-08T22:41:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This patch series implements an entirely alternate method of achieving\nsome of the same ends as the MIDX, using the approach suggested by\nJeff King from the RFC thread back in January[1]. You can now do:\n\n core.abbrev = 20\n core.validateAbbrev = false\n\nOr:\n\n core.abbrev = +2\n core.validateAbbrev = false\n\nOn linux.git `git log --oneline --raw --parents` with 64MB packs gives\nthis improvement with core.abbrev=15 & core.validateAbbrev=false\n(v.s. true):\n\n Test HEAD~ HEAD\n ----------------------------------------------------------------------------------------\n 0014.1: git log --oneline --raw --parents 95.68(95.07+0.53) 42.74(42.33+0.39) -55.3%\n\nWhile cleaning up the RFC version of this which I sent in [2] I\ndiscovered that almost none of the existing functionality was tested\nfor, and was very inconsistent since we have 4 different places where\nthe abbrev config is parsed.\n\nSee 19/20 and 20/20 for what this whole thing is building towards, the\nrest is all tests, cleanup, and preparatory work.\n\n(There's still other long-standing issues with the existing behavior\nwhich this doesn't change, but I had to stop somewhere to make this\ndigestible).\n\n1. https://public-inbox.org/git/20180108102029.GA21232@sigill.intra.peff.net/\n2. https://public-inbox.org/git/20180606102719.27145-1-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (20):\n t/README: clarify the description of test_line_count\n test library: add a test_byte_count\n blame doc: explicitly note how --abbrev=40 gives 39 chars\n abbrev tests: add tests for core.abbrev and --abbrev\n abbrev tests: test \"git-blame\" behavior\n blame: fix a bug, core.abbrev should work like --abbrev\n abbrev tests: test \"git branch\" behavior\n abbrev tests: test for \"git-describe\" behavior\n abbrev tests: test for \"git-log\" behavior\n abbrev tests: test for \"git-diff\" behavior\n abbrev tests: test for plumbing behavior\n abbrev tests: test for --abbrev and core.abbrev=[+-]N\n parse-options-cb.c: convert uses of 40 to GIT_SHA1_HEXSZ\n config.c: use braces on multiple conditional arms\n parse-options-cb.c: use braces on multiple conditional arms\n abbrev: unify the handling of non-numeric values\n abbrev: unify the handling of empty values\n abbrev parsing: use braces on multiple conditional arms\n abbrev: support relative abbrev values\n abbrev: add a core.validateAbbrev setting\n\n Documentation/config.txt | 49 +++\n Documentation/diff-options.txt | 3 +\n Documentation/git-blame.txt | 14 +\n Documentation/git-branch.txt | 3 +\n Documentation/git-describe.txt | 3 +\n Documentation/git-ls-files.txt | 3 +\n Documentation/git-ls-tree.txt | 3 +\n Documentation/git-show-ref.txt | 3 +\n builtin/blame.c | 2 +\n cache.h | 2 +\n config.c | 22 +-\n diff.c | 24 +-\n environment.c | 2 +\n parse-options-cb.c | 19 +-\n revision.c | 24 +-\n sha1-name.c | 15 +\n t/README | 6 +-\n t/perf/p0014-abbrev.sh | 13 +\n t/t0014-abbrev.sh | 452 ++++++++++++++++++++++++++++\n t/t1512-rev-parse-disambiguation.sh | 47 +++\n t/t6006-rev-list-format.sh | 6 +-\n t/test-lib-functions.sh | 23 ++\n 22 files changed, 722 insertions(+), 16 deletions(-)\n create mode 100755 t/perf/p0014-abbrev.sh\n create mode 100755 t/t0014-abbrev.sh\n\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349748","messageId":"20180608224136.20220-2-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 01/20] t/README: clarify the description of test_line_count","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:17Z","receivedAt":"2018-06-08T22:41:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Referring to the \"length\" of a file is ambiguous. We mean the number\nof lines, so let's say that.\n\nChanges the description added in fb3340a6a7 (\"test-lib: introduce\ntest_line_count to measure files\", 2010-10-31).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/README b/t/README\nindex 8373a27fea..eede11d649 100644\n--- a/t/README\n+++ b/t/README\n@@ -730,7 +730,7 @@ library for your script to use.\n \n - test_line_count (= | -lt | -ge | ...) \n \n- Check whether a file has the length it is expected to.\n+ Check whether a file has the number of lines it is expected to.\n \n - test_path_is_file []\n test_path_is_dir []\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349749","messageId":"20180608224136.20220-3-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 02/20] test library: add a test_byte_count","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:18Z","receivedAt":"2018-06-08T22:42:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This new function is like test_line_count except it uses \"wc -c\"\ninstead of \"wc -l\". Perhaps this should be a parameter, but I don't\nsee us needing \"wc -m\" (chars), \"wc -w\" (words) etc.\n\nChange a couple of existing tests that use this, I expect to use this\nin later patches when adding more tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 4 ++++\n t/t6006-rev-list-format.sh | 6 ++----\n t/test-lib-functions.sh | 23 +++++++++++++++++++++++\n 3 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex eede11d649..3139c27e4e 100644\n--- a/t/README\n+++ b/t/README\n@@ -728,6 +728,10 @@ library for your script to use.\n Check whether the rev points to the same commit as the\n rev.\n \n+ - test_byte_count (= | -lt | -ge | ...) \n+\n+ Check whether a file has the number of bytes it is expected to.\n+\n - test_line_count (= | -lt | -ge | ...) \n \n Check whether a file has the number of lines it is expected to.\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex ec42c2f779..ec068c55ab 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -456,14 +456,12 @@ test_expect_success '--abbrev' '\n \n test_expect_success '%H is not affected by --abbrev-commit' '\n \tgit log -1 --format=%H --abbrev-commit --abbrev=20 HEAD >actual &&\n-\tlen=$(wc -c actual &&\n-\tlen=$(wc -c output &&\n+#\t\ttest_byte_count = 1 output\n+#\t'\n+#\n+# is like \"test $(wc -c \n---\n t/t0014-abbrev.sh | 118 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 118 insertions(+)\n create mode 100755 t/t0014-abbrev.sh\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nnew file mode 100755\nindex 0000000000..1c60f5ff93\n--- /dev/null\n+++ b/t/t0014-abbrev.sh\n@@ -0,0 +1,118 @@\n+#!/bin/sh\n+\n+test_description='test core.abbrev and related features'\n+\n+. ./test-lib.sh\n+\n+tr_d_n() {\n+\ttr -d '\\n'\n+}\n+\n+cut_tr_d_n_field_n() {\n+\tcut -d \" \" -f $1 | tr_d_n\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit A &&\n+\tgit tag -a -mannotated A.annotated &&\n+\ttest_commit B &&\n+\ttest_commit C &&\n+\tmkdir X Y &&\n+\ttouch X/file1 Y/file2\n+'\n+\n+test_expect_success 'the FALLBACK_DEFAULT_ABBREV is 7' '\n+\tgit log -1 --pretty=format:%h >log &&\n+\ttest_byte_count = 7 log\n+'\n+\n+test_expect_success 'abbrev empty value handling differs ' '\n+\ttest_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&\n+\ttest_i18ngrep \"bad numeric config value.*invalid unit\" stderr &&\n+\n+\tgit branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 40 branch &&\n+\n+\tgit log --abbrev= -1 --pretty=format:%h >log &&\n+\ttest_byte_count = 4 log &&\n+\n+\tgit diff --raw --abbrev= HEAD~ >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 4 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 4 diff.4 &&\n+\n+\ttest_must_fail git diff --raw --abbrev= --no-index X Y >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 4 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 4 diff.4\n+'\n+\n+test_expect_success 'abbrev non-integer value handling differs ' '\n+\ttest_must_fail git -c core.abbrev=XYZ log -1 --pretty=format:%h 2>stderr &&\n+\ttest_i18ngrep \"bad numeric config value.*invalid unit\" stderr &&\n+\n+\ttest_must_fail git branch -v --abbrev=XYZ 2>stderr &&\n+\ttest_i18ngrep \"expects a numerical value\" stderr &&\n+\n+\tgit log --abbrev=XYZ -1 --pretty=format:%h 2>stderr &&\n+\t! test -s stderr &&\n+\n+\tgit diff --raw --abbrev=XYZ HEAD~ 2>stderr &&\n+\t! test -s stderr &&\n+\n+\ttest_must_fail git diff --raw --abbrev=XYZ --no-index X Y 2>stderr &&\n+\t! test -s stderr\n+'\n+\n+for i in -41 -20 -10 -1 0 1 2 3 41\n+do\n+\ttest_expect_success \"core.abbrev value $i out of range errors out\" \"\n+\t\ttest_must_fail git -c core.abbrev=$i log -1 --pretty=format:%h 2>stderr &&\n+\t\ttest_i18ngrep 'abbrev length out of range' stderr\n+\t\"\n+done\n+\n+for i in -41 -20 -10 -1\n+do\n+\ttest_expect_success \"negative --abbrev=$i value out of range means --abbrev=40\" \"\n+\t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = 40 log\n+\t\"\n+done\n+\n+for i in 0 1 2 3 4\n+do\n+\ttest_expect_success \"non-negative --abbrev=$i value log &&\n+\t\ttest_byte_count = 4 log\n+\t\"\n+done\n+\n+for i in 41 9001\n+do\n+\ttest_expect_success \"non-negative --abbrev=$i value >MINIMUM_ABBREV falls back on 40\" \"\n+\t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = 40 log\n+\t\"\n+done\n+\n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"core.abbrev=$i and --abbrev=$i in combination within the valid range\" \"\n+\t\t# Both core.abbrev=X and --abbrev=X do the same thing\n+\t\t# in isolation\n+\t\tgit -c core.abbrev=$i log -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = $i log &&\n+\t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = $i log &&\n+\n+\t\t# The --abbrev option should take priority over\n+\t\t# core.abbrev\n+\t\tgit -c core.abbrev=20 log --abbrev=$i -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = $i log\n+\t\"\n+done\n+\n+test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349751","messageId":"20180608224136.20220-8-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 07/20] abbrev tests: test \"git branch\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:23Z","receivedAt":"2018-06-08T22:42:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Signed-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 934c54a96b..d8b060d922 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -168,4 +168,14 @@ do\n \t\"\n done\n \n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"branch core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\t\ttest_byte_count = $i branch &&\n+\t\tgit branch --abbrev=$i -v | cut_tr_d_n_field_n 3 >branch &&\n+\t\ttest_byte_count = $i branch\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349752","messageId":"20180608224136.20220-11-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 10/20] abbrev tests: test for \"git-diff\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:26Z","receivedAt":"2018-06-08T22:42:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"diff\" family of commands does its own parsing for --abbrev in\ndiff.c, so having dedicated tests for it makes sense.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex a66051c040..783807475f 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -213,4 +213,35 @@ do\n \t\"\n done\n \n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"diff --no-index --raw core.abbrev=$i and --abbrev=$i\" \"\n+\t\ttest_must_fail git -c core.abbrev=$i diff --no-index --raw X Y >diff &&\n+\t\tcut_tr_d_n_field_n 3 diff.3 &&\n+\t\ttest_byte_count = $i diff.3 &&\n+\t\tcut_tr_d_n_field_n 4 diff.4 &&\n+\t\ttest_byte_count = $i diff.4 &&\n+\n+\t\ttest_must_fail git diff --no-index --raw --abbrev=$i X Y >diff &&\n+\t\tcut_tr_d_n_field_n 3 diff.3 &&\n+\t\ttest_byte_count = $i diff.3 &&\n+\t\tcut_tr_d_n_field_n 4 diff.4 &&\n+\t\ttest_byte_count = $i diff.4\n+\t\"\n+\n+\ttest_expect_success \"diff --raw core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i diff --raw HEAD~ >diff &&\n+\t\tcut_tr_d_n_field_n 3 diff.3 &&\n+\t\ttest_byte_count = $i diff.3 &&\n+\t\tcut_tr_d_n_field_n 4 diff.4 &&\n+\t\ttest_byte_count = $i diff.4 &&\n+\n+\t\tgit diff --raw --abbrev=$i HEAD~ >diff &&\n+\t\tcut_tr_d_n_field_n 3 diff.3 &&\n+\t\ttest_byte_count = $i diff.3 &&\n+\t\tcut_tr_d_n_field_n 4 diff.4 &&\n+\t\ttest_byte_count = $i diff.4\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349753","messageId":"20180608224136.20220-15-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 14/20] config.c: use braces on multiple conditional arms","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:30Z","receivedAt":"2018-06-08T22:42:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust this code that'll be modified in a subsequent change to have\nmore than one line per branch to use braces per the CodingGuidelines,\nthis makes the later change easier to understand.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex fbbf0f8e9f..12f762ad92 100644\n--- a/config.c\n+++ b/config.c\n@@ -1149,9 +1149,9 @@ static int git_default_core_config(const char *var, const char *value)\n \tif (!strcmp(var, \"core.abbrev\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tif (!strcasecmp(value, \"auto\"))\n+\t\tif (!strcasecmp(value, \"auto\")) {\n \t\t\tdefault_abbrev = -1;\n-\t\telse {\n+\t\t} else {\n \t\t\tint abbrev = git_config_int(var, value);\n \t\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n \t\t\t\treturn error(\"abbrev length out of range: %d\", abbrev);\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349754","messageId":"20180608224136.20220-17-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 16/20] abbrev: unify the handling of non-numeric values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:32Z","receivedAt":"2018-06-08T22:42:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Unify how --abbrev=XYZ and core.abbrev=XYZ is handled. 2/4 parsers for\nthese values would just let these invalid values pass, treating them\nas though \"0\" was provided, which due to other inconsistent fallback\nlogic (soon to be fixed) would be treated as providing MINIMUM_ABBREV.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 5 ++++-\n revision.c | 5 ++++-\n t/t0014-abbrev.sh | 10 +++++-----\n 3 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 136d44b455..75935322f1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4801,7 +4801,10 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n-\t\toptions->abbrev = strtoul(arg, NULL, 10);\n+\t\tchar *end;\n+\t\toptions->abbrev = strtoul(arg, &end, 10);\n+\t\tif (*end)\n+\t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", arg);\n \t\tif (options->abbrev < MINIMUM_ABBREV)\n \t\t\toptions->abbrev = MINIMUM_ABBREV;\n \t\telse if (the_hash_algo->hexsz < options->abbrev)\ndiff --git a/revision.c b/revision.c\nindex 40fd91ff2b..aa87afa77f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2047,7 +2047,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n \t} else if (skip_prefix(arg, \"--abbrev=\", &optarg)) {\n-\t\trevs->abbrev = strtoul(optarg, NULL, 10);\n+\t\tchar *end;\n+\t\trevs->abbrev = strtoul(optarg, &end, 10);\n+\t\tif (*end)\n+\t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", optarg);\n \t\tif (revs->abbrev < MINIMUM_ABBREV)\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n \t\telse if (revs->abbrev > hexsz)\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 6dee92f35e..203fe316b9 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -64,14 +64,14 @@ test_expect_success 'abbrev non-integer value handling differs ' '\n \ttest_must_fail git branch -v --abbrev=XYZ 2>stderr &&\n \ttest_i18ngrep \"expects a numerical value\" stderr &&\n \n-\tgit log --abbrev=XYZ -1 --pretty=format:%h 2>stderr &&\n-\t! test -s stderr &&\n+\ttest_must_fail git log --abbrev=XYZ -1 --pretty=format:%h 2>stderr &&\n+\ttest_i18ngrep \"expects a numerical value\" stderr &&\n \n-\tgit diff --raw --abbrev=XYZ HEAD~ 2>stderr &&\n-\t! test -s stderr &&\n+\ttest_must_fail git diff --raw --abbrev=XYZ HEAD~ 2>stderr &&\n+\ttest_i18ngrep \"expects a numerical value\" stderr &&\n \n \ttest_must_fail git diff --raw --abbrev=XYZ --no-index X Y 2>stderr &&\n-\t! test -s stderr\n+\ttest_i18ngrep \"expects a numerical value\" stderr\n '\n \n for i in -41 -20 -10 -1 -0 +0 0 1 2 3 41\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349755","messageId":"20180608224136.20220-18-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 17/20] abbrev: unify the handling of empty values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:33Z","receivedAt":"2018-06-08T22:42:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"For no good reason the --abbrev= command-line option was less strict\nthan the core.abbrev config option, which came down to the latter\nusing git_config_int() which rejects an empty string, but the rest of\nthe parsing using strtoul() which will convert it to 0.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 2 ++\n parse-options-cb.c | 2 ++\n revision.c | 2 ++\n t/t0014-abbrev.sh | 22 ++++++++--------------\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 75935322f1..cab79d24ab 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4802,6 +4802,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n \t\tchar *end;\n+\t\tif (!strcmp(arg, \"\"))\n+\t\t\tdie(\"--abbrev expects a value, got '%s'\", arg);\n \t\toptions->abbrev = strtoul(arg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", arg);\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex e3cd87fbd6..aa9984f164 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -16,6 +16,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \tif (!arg) {\n \t\tv = unset ? 0 : DEFAULT_ABBREV;\n \t} else {\n+\t\tif (!strcmp(arg, \"\"))\n+\t\t\treturn opterror(opt, \"expects a value\", 0);\n \t\tv = strtol(arg, (char **)&arg, 10);\n \t\tif (*arg)\n \t\t\treturn opterror(opt, \"expects a numerical value\", 0);\ndiff --git a/revision.c b/revision.c\nindex aa87afa77f..d39a292895 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2048,6 +2048,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->abbrev = DEFAULT_ABBREV;\n \t} else if (skip_prefix(arg, \"--abbrev=\", &optarg)) {\n \t\tchar *end;\n+\t\tif (!strcmp(optarg, \"\"))\n+\t\t\tdie(\"--abbrev expects a value, got '%s'\", optarg);\n \t\trevs->abbrev = strtoul(optarg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", optarg);\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 203fe316b9..8448f78560 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -38,23 +38,17 @@ test_expect_success 'abbrev empty value handling differs ' '\n \ttest_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&\n \ttest_i18ngrep \"bad numeric config value.*invalid unit\" stderr &&\n \n-\tgit branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&\n-\ttest_byte_count = 40 branch &&\n+\ttest_must_fail git branch -v --abbrev= 2>stderr &&\n+\ttest_i18ngrep \"expects a value\" stderr &&\n \n-\tgit log --abbrev= -1 --pretty=format:%h >log &&\n-\ttest_byte_count = 4 log &&\n+\ttest_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&\n+\ttest_i18ngrep \"expects a value\" stderr &&\n \n-\tgit diff --raw --abbrev= HEAD~ >diff &&\n-\tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 4 diff.3 &&\n-\tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 4 diff.4 &&\n+\ttest_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&\n+\ttest_i18ngrep \"expects a value\" stderr &&\n \n-\ttest_must_fail git diff --raw --abbrev= --no-index X Y >diff &&\n-\tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 4 diff.3 &&\n-\tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 4 diff.4\n+\ttest_must_fail git diff --raw --abbrev= --no-index X Y 2>stderr &&\n+\ttest_i18ngrep \"expects a value\" stderr\n '\n \n test_expect_success 'abbrev non-integer value handling differs ' '\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349756","messageId":"20180608224136.20220-20-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 19/20] abbrev: support relative abbrev values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:35Z","receivedAt":"2018-06-08T22:42:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the core.abbrev config variable and the corresponding --abbrev\ncommand-line option to support relative values such as +1 or -1.\n\nBefore Linus's e6c587c733 (\"abbrev: auto size the default\nabbreviation\", 2016-09-30) git would default to abbreviating object\nnames to 7-hexdigits, and only picking longer SHA-1s as needed if that\nwas ambiguous.\n\nThat change instead set the default length as a function of the\nestimated current count of objects:\n\n Based on the expectation that we would see collision in a\n 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\n add to the shortened name allows us to have four times (2-bits) as\n many objects in the repository.\n\nBy supporting relative values for core.abbrev we can allow users to\nconsistently opt-in for either a higher or lower probability of\ncollision, without needing to hardcode a given numeric value like\n\"10\", which would be overkill on some repositories, and far to small\non others.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 6 ++\n Documentation/diff-options.txt | 3 +\n Documentation/git-blame.txt | 3 +\n Documentation/git-branch.txt | 3 +\n Documentation/git-describe.txt | 3 +\n Documentation/git-ls-files.txt | 3 +\n Documentation/git-ls-tree.txt | 3 +\n Documentation/git-show-ref.txt | 3 +\n cache.h | 1 +\n config.c | 11 +++\n diff.c | 18 +++-\n environment.c | 1 +\n parse-options-cb.c | 12 ++-\n revision.c | 18 +++-\n sha1-name.c | 11 +++\n t/t0014-abbrev.sh | 170 ++++++++++++++++++++++-----------\n 16 files changed, 204 insertions(+), 65 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ab641bf5a9..abf07be7b6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -919,6 +919,12 @@ core.abbrev::\n \tin your repository, which hopefully is enough for\n \tabbreviated object names to stay unique for some time.\n \tThe minimum length is 4.\n++\n+This can also be set to relative values such as `+2` or `-2`, which\n+means to add or subtract N characters from the SHA-1 that Git would\n+otherwise print, this allows for producing more future-proof SHA-1s\n+for use within a given project, while adjusting the value for the\n+current approximate number of objects.\n \n add.ignoreErrors::\n add.ignore-errors (deprecated)::\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex f466600972..f1114a7b8d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -384,6 +384,9 @@ endif::git-format-patch[]\n \tindependent of the `--full-index` option above, which controls\n \tthe diff-patch output format. Non default number of\n \tdigits can be specified with `--abbrev=`.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n -B[][/]::\n --break-rewrites[=[][/]]::\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex d6cddbcb2e..8559d3b0c7 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -99,6 +99,9 @@ requested, resulting in unaligned output.\n Before 2.19, setting `core.abbrev=40` wouldn't apply the above rule\n and would cause blame to emit output that was unaligned. This bug has\n since been fixed.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n \n THE PORCELAIN FORMAT\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 02eccbb931..0f8032cec6 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -182,6 +182,9 @@ See `--create-reflog` above for details.\n \tAlter the sha1's minimum display length in the output listing.\n \tThe default value is 7 and can be overridden by the `core.abbrev`\n \tconfig option.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n --no-abbrev::\n \tDisplay the full sha1s in the output listing rather than abbreviating them.\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex e027fb8c4b..a3d5c7e817 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -65,6 +65,9 @@ OPTIONS\n \tabbreviated object name, use digits, or as many digits\n \tas needed to form a unique object name. An of 0\n \twill suppress long format, only showing the closest tag.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n --candidates=::\n \tInstead of considering only the 10 most recent tags as\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 5298f1bc30..f9af2b23bf 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -153,6 +153,9 @@ a space) at the start of each line:\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\n \tNon default number of digits can be specified with --abbrev=.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n --debug::\n \tAfter each line that describes a file, add more data about its\ndiff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\nindex 9dee7bef35..bcba39eed8 100644\n--- a/Documentation/git-ls-tree.txt\n+++ b/Documentation/git-ls-tree.txt\n@@ -64,6 +64,9 @@ OPTIONS\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\n \tNon default number of digits can be specified with --abbrev=.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n --full-name::\n \tInstead of showing the path names relative to the current working\ndiff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt\nindex d28e6154c6..05c5579075 100644\n--- a/Documentation/git-show-ref.txt\n+++ b/Documentation/git-show-ref.txt\n@@ -66,6 +66,9 @@ OPTIONS\n \n \tAbbreviate the object name. When using `--hash`, you do\n \tnot have to say `--hash --abbrev`; `--hash=n` would do.\n++\n+Can also be set to a relative value, see `core.abbrev` in\n+linkgit:git-diff[1].\n \n -q::\n --quiet::\ndiff --git a/cache.h b/cache.h\nindex 89a107a7f7..0fb4211804 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -772,6 +772,7 @@ extern int check_stat;\n extern int quote_path_fully;\n extern int has_symlinks;\n extern int minimum_abbrev, default_abbrev;\n+extern int default_abbrev_relative;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\ndiff --git a/config.c b/config.c\nindex 12f762ad92..cd95c6bdfb 100644\n--- a/config.c\n+++ b/config.c\n@@ -1151,6 +1151,17 @@ static int git_default_core_config(const char *var, const char *value)\n \t\t\treturn config_error_nonbool(var);\n \t\tif (!strcasecmp(value, \"auto\")) {\n \t\t\tdefault_abbrev = -1;\n+\t\t} else if (*value == '+' || *value == '-') {\n+\t\t\tint relative = git_config_int(var, value);\n+\t\t\tif (relative == 0)\n+\t\t\t\tdie(_(\"bad core.abbrev value %s. \"\n+\t\t\t\t \"relative values must be non-zero\"),\n+\t\t\t\t value);\n+\t\t\tif (abs(relative) > GIT_SHA1_HEXSZ)\n+\t\t\t\tdie(_(\"bad core.abbrev value %s. \"\n+\t\t\t\t \"impossibly out of range\"),\n+\t\t\t\t value);\n+\t\t\tdefault_abbrev_relative = relative;\n \t\t} else {\n \t\t\tint abbrev = git_config_int(var, value);\n \t\t\tif (abbrev < minimum_abbrev || abbrev > 40)\ndiff --git a/diff.c b/diff.c\nindex e0141cfbc0..f7861b8472 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4801,16 +4801,28 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n+\t\tint v;\n \t\tchar *end;\n \t\tif (!strcmp(arg, \"\"))\n \t\t\tdie(\"--abbrev expects a value, got '%s'\", arg);\n-\t\toptions->abbrev = strtoul(arg, &end, 10);\n+\t\tv = strtoul(arg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", arg);\n-\t\tif (options->abbrev < MINIMUM_ABBREV) {\n+\t\tif (*arg == '+' || *arg == '-') {\n+\t\t\tif (v == 0) {\n+\t\t\t\tdie(\"relative abbrev must be non-zero\");\n+\t\t\t} else if (abs(v) > the_hash_algo->hexsz) {\n+\t\t\t\tdie(\"relative abbrev impossibly out of range\");\n+\t\t\t} else {\n+\t\t\t\tdefault_abbrev_relative = v;\n+\t\t\t\toptions->abbrev = DEFAULT_ABBREV;\n+\t\t\t}\n+\t\t} else if (v < MINIMUM_ABBREV) {\n \t\t\toptions->abbrev = MINIMUM_ABBREV;\n-\t\t} else if (the_hash_algo->hexsz < options->abbrev) {\n+\t\t} else if (the_hash_algo->hexsz < v) {\n \t\t\toptions->abbrev = the_hash_algo->hexsz;\n+\t\t} else {\n+\t\t\toptions->abbrev = v;\n \t\t}\n \t}\n \telse if ((argcount = parse_long_opt(\"src-prefix\", av, &optarg))) {\ndiff --git a/environment.c b/environment.c\nindex 2a6de2330b..0d48d52fba 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -22,6 +22,7 @@ int trust_ctime = 1;\n int check_stat = 1;\n int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = -1;\n+int default_abbrev_relative = 0;\n int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex aa9984f164..2a1ab449bf 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -16,12 +16,22 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \tif (!arg) {\n \t\tv = unset ? 0 : DEFAULT_ABBREV;\n \t} else {\n+\t\tconst char *origarg = arg;\n \t\tif (!strcmp(arg, \"\"))\n \t\t\treturn opterror(opt, \"expects a value\", 0);\n \t\tv = strtol(arg, (char **)&arg, 10);\n \t\tif (*arg)\n \t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n-\t\tif (v && v < MINIMUM_ABBREV) {\n+\t\tif (*origarg == '+' || *origarg == '-') {\n+\t\t\tif (v == 0) {\n+\t\t\t\treturn opterror(opt, \"relative abbrev must be non-zero\", 0);\n+\t\t\t} else if (abs(v) > GIT_SHA1_HEXSZ) {\n+\t\t\t\treturn opterror(opt, \"relative abbrev impossibly out of range\", 0);\n+\t\t\t} else {\n+\t\t\t\tdefault_abbrev_relative = v;\n+\t\t\t\tv = -1;\n+\t\t\t}\n+\t\t} else if (v && v < MINIMUM_ABBREV) {\n \t\t\tv = MINIMUM_ABBREV;\n \t\t} else if (v > GIT_SHA1_HEXSZ) {\n \t\t\tv = GIT_SHA1_HEXSZ;\ndiff --git a/revision.c b/revision.c\nindex 2a75fef22d..c858d32da7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2047,16 +2047,28 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n \t} else if (skip_prefix(arg, \"--abbrev=\", &optarg)) {\n+\t\tint v;\n \t\tchar *end;\n \t\tif (!strcmp(optarg, \"\"))\n \t\t\tdie(\"--abbrev expects a value, got '%s'\", optarg);\n-\t\trevs->abbrev = strtoul(optarg, &end, 10);\n+\t\tv = strtoul(optarg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", optarg);\n-\t\tif (revs->abbrev < MINIMUM_ABBREV) {\n+\t\tif (*optarg == '+' || *optarg == '-') {\n+\t\t\tif (v == 0) {\n+\t\t\t\tdie(\"relative abbrev must be non-zero\");\n+\t\t\t} else if (abs(v) > hexsz) {\n+\t\t\t\tdie(\"relative abbrev impossibly out of range\");\n+\t\t\t} else {\n+\t\t\t\tdefault_abbrev_relative = v;\n+\t\t\t\trevs->abbrev = DEFAULT_ABBREV;\n+\t\t\t}\n+\t\t} else if (v < MINIMUM_ABBREV) {\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n-\t\t} else if (revs->abbrev > hexsz) {\n+\t\t} else if (v > hexsz) {\n \t\t\trevs->abbrev = hexsz;\n+\t\t} else {\n+\t\t\trevs->abbrev = v;\n \t\t}\n \t} else if (!strcmp(arg, \"--abbrev-commit\")) {\n \t\trevs->abbrev_commit = 1;\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 60d9ef3c7e..75f1bef7d1 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -602,6 +602,17 @@ int find_unique_abbrev_r(char *hex, const struct object_id *oid, int len)\n \tif (len == GIT_SHA1_HEXSZ || !len)\n \t\treturn GIT_SHA1_HEXSZ;\n \n+\tif (default_abbrev_relative) {\n+\t\tint dar = default_abbrev_relative;\n+\t\tif (len + dar > GIT_SHA1_HEXSZ) {\n+\t\t\treturn GIT_SHA1_HEXSZ;\n+\t\t} else if (len + dar < MINIMUM_ABBREV) {\n+\t\t\tlen = MINIMUM_ABBREV;\n+\t\t\tdar = 0;\n+\t\t}\n+\t\tlen += dar;\n+\t}\n+\n \tmad.init_len = len;\n \tmad.cur_len = len;\n \tmad.hex = hex;\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 8448f78560..73d990ecc1 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -68,23 +68,33 @@ test_expect_success 'abbrev non-integer value handling differs ' '\n \ttest_i18ngrep \"expects a numerical value\" stderr\n '\n \n-for i in -41 -20 -10 -1 -0 +0 0 1 2 3 41\n+for i in -41 +41\n do\n \ttest_expect_success \"core.abbrev value $i out of range errors out\" \"\n \t\ttest_must_fail git -c core.abbrev=$i log -1 --pretty=format:%h 2>stderr &&\n-\t\ttest_i18ngrep 'abbrev length out of range' stderr\n+\t\ttest_i18ngrep 'impossibly out of range' stderr\n \t\"\n done\n \n-for i in -41 -20 -10 -1\n+for i in -39 -20 -10 -3\n do\n-\ttest_expect_success \"negative --abbrev=$i value out of range means --abbrev=40\" \"\n+\ttest_expect_success \"negative -31..-3 --abbrev=$i mean --abbrev=4\" \"\n \t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n-\t\ttest_byte_count = 40 log\n+\t\ttest_byte_count = 4 log\n+\t\"\n+done\n+\n+for i in -9001 -41 +41 +9001\n+do\n+\ttest_expect_success \"core.abbrev=$i and --abbrev=$i values out of range error out\" \"\n+\t\ttest_must_fail git -c core.abbrev=$i branch -v 2>stderr &&\n+\t\ttest_i18ngrep 'impossibly out of range' stderr &&\n+\t\ttest_must_fail git branch -v --abbrev=$i 2>stderr &&\n+\t\ttest_i18ngrep 'impossibly out of range' stderr\n \t\"\n done\n \n-for i in 0 1 2 3 4 -0 +0 +1 +2 +3 +4\n+for i in 0 1 2 3 4\n do\n \ttest_expect_success \"non-negative --abbrev=$i value log &&\n@@ -92,7 +102,7 @@ do\n \t\"\n done\n \n-for i in 41 9001 +41 +9001\n+for i in 41 9001\n do\n \ttest_expect_success \"non-negative --abbrev=$i value >MINIMUM_ABBREV falls back on 40\" \"\n \t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n@@ -110,9 +120,23 @@ do\n \t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n \t\ttest_byte_count = $i log &&\n \n-\t\t# core.abbrev=+N is the same as core.abbrev=N\n+\t\t# core.abbrev=+N is the same as core.abbrev=7+N\n \t\tgit -c core.abbrev=+$i log -1 --pretty=format:%h >log &&\n-\t\ttest_byte_count = $i log &&\n+\t\tif test \\$((7 + $i)) -gt 40\n+\t\tthen\n+\t\t\ttest_byte_count = 40 log\n+\t\telse\n+\t\t\ttest_byte_count = \\$((7 + $i)) log\n+\t\tfi &&\n+\n+\t\t# --abbrev=+N is the same as --abbrev=7+N\n+\t\tgit log --abbrev=+$i -1 --pretty=format:%h >log &&\n+\t\tif test \\$((7 + $i)) -gt 40\n+\t\tthen\n+\t\t\ttest_byte_count = 40 log\n+\t\telse\n+\t\t\ttest_byte_count = \\$((7 + $i)) log\n+\t\tfi &&\n \n \t\t# The --abbrev option should take priority over\n \t\t# core.abbrev\n@@ -171,14 +195,17 @@ do\n done\n \n test_expect_success 'blame core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n-\ttest_must_fail git -c core.abbrev=-1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n+\tgit -c core.abbrev=+1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_byte_count = 9 blame &&\n \n-\tgit blame --abbrev=-1 A.t | cut_tr_d_n_field_n 1 >blame &&\n-\ttest_byte_count = 5 blame &&\n+\tgit -c core.abbrev=-1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_byte_count = 7 blame &&\n \n \tgit blame --abbrev=+1 A.t | cut_tr_d_n_field_n 1 >blame &&\n-\ttest_byte_count = 5 blame\n+\ttest_byte_count = 9 blame &&\n+\n+\tgit blame --abbrev=-1 A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_byte_count = 7 blame\n '\n \n for i in $(test_seq 4 40)\n@@ -193,14 +220,17 @@ do\n done\n \n test_expect_success 'branch core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n-\ttest_must_fail git -c core.abbrev=-1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\tgit -c core.abbrev=+1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 8 branch &&\n \n-\tgit branch --abbrev=-1 -v | cut_tr_d_n_field_n 3 >branch &&\n-\ttest_byte_count = 4 branch &&\n+\tgit -c core.abbrev=-1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 6 branch &&\n \n-\tgit branch --abbrev=+1 -v | cut_tr_d_n_field_n 3 >branch &&\n-\ttest_byte_count = 4 branch\n+\tgit branch -v --abbrev=+1 | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 8 branch &&\n+\n+\tgit branch -v --abbrev=-1 | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 6 branch\n '\n \n test_expect_success 'describe core.abbrev and --abbrev special cases' '\n@@ -225,14 +255,17 @@ do\n done\n \n test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n-\ttest_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n+\tgit -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n+\ttest_byte_count = 6 describe &&\n+\n+\tgit -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n+\ttest_byte_count = 8 describe &&\n \n \tgit describe --abbrev=-1 | sed_g_tr_d_n >describe &&\n-\ttest_byte_count = 4 describe &&\n+\ttest_byte_count = 6 describe &&\n \n \tgit describe --abbrev=+1 | sed_g_tr_d_n >describe &&\n-\ttest_byte_count = 4 describe\n+\ttest_byte_count = 8 describe\n '\n \n for i in $(test_seq 4 40)\n@@ -246,17 +279,17 @@ do\n done\n \n test_expect_success 'log core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 log --pretty=format:%h -1 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\tgit -c core.abbrev=+1 log --pretty=format:%h -1 >log &&\n+\ttest_byte_count = 8 log &&\n \n-\ttest_must_fail git -c core.abbrev=-1 log --pretty=format:%h -1 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\tgit -c core.abbrev=-1 log --pretty=format:%h -1 >log &&\n+\ttest_byte_count = 6 log &&\n \n \tgit log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&\n-\ttest_byte_count = 4 log &&\n+\ttest_byte_count = 8 log &&\n \n \tgit log --abbrev=-1 --pretty=format:%h -1 | tr_d_n >log &&\n-\ttest_byte_count = 40 log\n+\ttest_byte_count = 6 log\n '\n \n for i in $(test_seq 4 40)\n@@ -291,43 +324,55 @@ do\n done\n \n test_expect_success 'diff --no-index --raw core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 diff --no-index --raw X Y 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\ttest_must_fail git -c core.abbrev=+1 diff --no-index --raw X Y >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 8 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 8 diff.4 &&\n \n-\ttest_must_fail git -c core.abbrev=-1 diff --no-index --raw X Y 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\ttest_must_fail git -c core.abbrev=-1 diff --no-index --raw X Y >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 6 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 6 diff.4 &&\n \n \ttest_must_fail git diff --no-index --raw --abbrev=+1 X Y >diff &&\n \tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 4 diff.3 &&\n+\ttest_byte_count = 8 diff.3 &&\n \tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 4 diff.4 &&\n+\ttest_byte_count = 8 diff.4 &&\n \n \ttest_must_fail git diff --no-index --raw --abbrev=-1 X Y >diff &&\n \tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 4 diff.3 &&\n+\ttest_byte_count = 6 diff.3 &&\n \tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 4 diff.4\n+\ttest_byte_count = 6 diff.4\n '\n \n test_expect_success 'diff --raw core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 diff HEAD~ 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\tgit -c core.abbrev=+1 diff --raw HEAD~ >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 8 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 8 diff.4 &&\n \n-\ttest_must_fail git -c core.abbrev=-1 diff HEAD~ 2>stderr &&\n-\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\tgit -c core.abbrev=-1 diff --raw HEAD~ >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 6 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 6 diff.4 &&\n \n-\tgit diff --raw --abbrev=+1 HEAD~ >diff &&\n+\tgit diff --raw --abbrev=+1 --raw HEAD~ >diff &&\n \tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 4 diff.3 &&\n+\ttest_byte_count = 8 diff.3 &&\n \tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 4 diff.4 &&\n+\ttest_byte_count = 8 diff.4 &&\n \n-\tgit diff --raw --abbrev=-1 HEAD~ >diff &&\n+\tgit diff --raw --abbrev=-1 --raw HEAD~ >diff &&\n \tcut_tr_d_n_field_n 3 diff.3 &&\n-\ttest_byte_count = 40 diff.3 &&\n+\ttest_byte_count = 6 diff.3 &&\n \tcut_tr_d_n_field_n 4 diff.4 &&\n-\ttest_byte_count = 40 diff.4\n+\ttest_byte_count = 6 diff.4\n '\n \n for i in $(test_seq 4 40)\n@@ -342,13 +387,16 @@ done\n \n test_expect_success 'ls-files core.abbrev=[-+]1 and --abbrev=[-+]1' '\n \ttest_must_fail git -c core.abbrev=+1 ls-files --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\ttest_byte_count = 40 ls-files &&\n+\n \ttest_must_fail git -c core.abbrev=-1 ls-files --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\ttest_byte_count = 40 ls-files &&\n \n \tgit ls-files --abbrev=-1 --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n-\ttest_byte_count = 4 ls-files &&\n+\ttest_byte_count = 6 ls-files &&\n \n \tgit ls-files --abbrev=+1 --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n-\ttest_byte_count = 4 ls-files\n+\ttest_byte_count = 8 ls-files\n '\n \n for i in $(test_seq 4 40)\n@@ -362,14 +410,17 @@ do\n done\n \n test_expect_success 'ls-tree core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n-\ttest_must_fail git -c core.abbrev=-1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\tgit -c core.abbrev=+1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\ttest_byte_count = 40 ls-tree &&\n+\n+\tgit -c core.abbrev=-1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\ttest_byte_count = 40 ls-tree &&\n \n \tgit ls-tree --abbrev=-1 HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n-\ttest_byte_count = 4 ls-tree &&\n+\ttest_byte_count = 6 ls-tree &&\n \n \tgit ls-tree --abbrev=+1 HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n-\ttest_byte_count = 4 ls-tree\n+\ttest_byte_count = 8 ls-tree\n '\n \n for i in $(test_seq 4 40)\n@@ -385,14 +436,17 @@ do\n done\n \n test_expect_success 'show-ref core.abbrev=[-+]1 and --abbrev=[-+]1' '\n-\ttest_must_fail git -c core.abbrev=+1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n-\ttest_must_fail git -c core.abbrev=-1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\tgit -c core.abbrev=+1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\ttest_byte_count = 40 show-ref &&\n+\n+\tgit -c core.abbrev=-1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\ttest_byte_count = 40 show-ref &&\n \n \tgit show-ref --abbrev=-1 --hash refs/heads/master | tr_d_n >show-ref &&\n-\ttest_byte_count = 4 show-ref &&\n+\ttest_byte_count = 6 show-ref &&\n \n \tgit show-ref --abbrev=+1 --hash refs/heads/master | tr_d_n >show-ref &&\n-\ttest_byte_count = 4 show-ref\n+\ttest_byte_count = 8 show-ref\n '\n \n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349757","messageId":"20180608224136.20220-21-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 20/20] abbrev: add a core.validateAbbrev setting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:36Z","receivedAt":"2018-06-08T22:42:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Operations that need to abbreviate a lot of SHA-1s such as 'git log\n--oneline' experience degraded performance when traversing a lot of\npacks. See [1] and [2] for some relevant performance numbers.\n\nOne way to address this is something like the MIDX to make looking up\nthe SHA-1s cheaper.\n\nThis change adds an alternate method of achieving some of the same\nends (but possibly not all, see [3] and replies to the original thread\nat [1]).\n\nInstead of trying really hard to find an unambiguous SHA-1 we can with\ncore.validateAbbrev=false, and preferably combined with the new\nsupport for relative core.abbrev values (such as +4) unconditionally\nprint a short SHA-1 without doing any disambiguation check. I.e. it\nallows for picking a trade-off between performance, and the odds that\nfuture or remote (or current and local) short SHA-1 will be ambiguous.\n\nWith the included performance test my git.git repacked with with `git\nrepack -A -d --max-pack-size=X` gives the following results against\ngit.git itself with X=64M:\n\n Test HEAD~ HEAD\n ------------------------------------------------------------------------------------\n 0014.1: git log --oneline --raw --parents 2.53(2.48+0.05) 2.20(2.14+0.05) -13.0%\n\nWith one big pack -7.6%, and with 16M packs -23.8%.\n\n1. https://public-inbox.org/git/20180107181459.222909-1-dstolee@microsoft.com/\n2. https://public-inbox.org/git/20180607140338.32440-1-dstolee@microsoft.com/\n3. https://public-inbox.org/git/87lgbsz61p.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 43 ++++++++++++++++++++++++++\n cache.h | 1 +\n config.c | 7 +++++\n environment.c | 1 +\n sha1-name.c | 4 +++\n t/perf/p0014-abbrev.sh | 13 ++++++++\n t/t1512-rev-parse-disambiguation.sh | 47 +++++++++++++++++++++++++++++\n 7 files changed, 116 insertions(+)\n create mode 100755 t/perf/p0014-abbrev.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex abf07be7b6..df31d1351f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -925,6 +925,49 @@ means to add or subtract N characters from the SHA-1 that Git would\n otherwise print, this allows for producing more future-proof SHA-1s\n for use within a given project, while adjusting the value for the\n current approximate number of objects.\n++\n+This is especially useful in combination with the\n+`core.validateAbbrev` setting, or to get more future-proof hashes to\n+reference in the future in a repository whose number of objects is\n+expected to grow.\n+\n+core.validateAbbrev::\n+\tIf set to false (true by default) don't do any validation when\n+\tprinting abbreviated object names to see if they're really\n+\tunique. This makes printing objects more performant at the\n+\tcost of potentially printing object names that aren't unique\n+\twithin the current repository.\n++\n+When printing abbreviated object names Git needs to look through the\n+local object store. This is an `O(log N)` operation assuming all the\n+objects are in a single pack file, but `X * O(log N)` given `X` pack\n+files, which can get expensive on some larger repositories.\n++\n+This setting changes that to `O(1)`, but with the trade-off that\n+depending on the value of `core.abbrev` we may be printing abbreviated\n+hashes that collide. Too see how likely this is, try running:\n++\n+-----------------------------------------------------------------------------------------------------------\n+git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' | sort | uniq -c | sort -nr\n+-----------------------------------------------------------------------------------------------------------\n++\n+This shows how many commits were found at each abbreviation length. On\n+linux.git in June 2018 this shows a bit more than 750,000 commits,\n+with just 4 needing 11 characters to be fully abbreviated, and the\n+default heuristic picks a length of 12.\n++\n+Even without `core.validateAbbrev=false` the results abbreviation\n+already a bit of a probability game. They're guaranteed at the moment\n+of generation, but as more objects are added, ambiguities may be\n+introduced. Likewise, what's unambiguous for you may not be for\n+somebody else you're communicating with, if they have their own clone.\n++\n+Therefore the default of `core.validateAbbrev=true` may not save you\n+in practice if you're sharing the SHA-1 or noting it now to use after\n+a `git fetch`. You may be better off setting `core.abbrev` to\n+e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine\n+that with `core.validateAbbrev=false` to get a reasonable trade-off\n+between safety and performance.\n \n add.ignoreErrors::\n add.ignore-errors (deprecated)::\ndiff --git a/cache.h b/cache.h\nindex 0fb4211804..6dc5af9482 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -773,6 +773,7 @@ extern int quote_path_fully;\n extern int has_symlinks;\n extern int minimum_abbrev, default_abbrev;\n extern int default_abbrev_relative;\n+extern int validate_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\ndiff --git a/config.c b/config.c\nindex cd95c6bdfb..fd11e1047d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1146,6 +1146,13 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.validateabbrev\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tvalidate_abbrev = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.abbrev\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\ndiff --git a/environment.c b/environment.c\nindex 0d48d52fba..4a24d8126b 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -23,6 +23,7 @@ int check_stat = 1;\n int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = -1;\n int default_abbrev_relative = 0;\n+int validate_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 75f1bef7d1..57dc782782 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -612,6 +612,10 @@ int find_unique_abbrev_r(char *hex, const struct object_id *oid, int len)\n \t\t}\n \t\tlen += dar;\n \t}\n+\tif (!validate_abbrev) {\n+\t\thex[len] = 0;\n+\t\treturn len;\n+\t}\n \n \tmad.init_len = len;\n \tmad.cur_len = len;\ndiff --git a/t/perf/p0014-abbrev.sh b/t/perf/p0014-abbrev.sh\nnew file mode 100755\nindex 0000000000..15c3764265\n--- /dev/null\n+++ b/t/perf/p0014-abbrev.sh\n@@ -0,0 +1,13 @@\n+#!/bin/sh\n+\n+test_description=\"Tests core.validateAbbrev performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+\n+test_perf 'git log --oneline --raw --parents' '\n+\tgit -c core.abbrev=15 -c core.validateAbbrev=false log --oneline --raw --parents >/dev/null\n+'\n+\n+test_done\ndiff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh\nindex 96fe3754c8..89565af508 100755\n--- a/t/t1512-rev-parse-disambiguation.sh\n+++ b/t/t1512-rev-parse-disambiguation.sh\n@@ -232,6 +232,53 @@ test_expect_success 'more history' '\n \n '\n \n+test_expect_success 'ambiguous objects and core.abbrev' '\n+\tcat >expected <<-\\EOF &&\n+\t00000000006\n+\t0000000005\n+\t00000000008\n+\t000000000004\n+\t0000000000e\n+\tEOF\n+\tgit -c core.abbrev=4 log --pretty=tformat:%h >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'ambiguous objects and core.validateAbbrev' '\n+\tgit -c core.abbrev=4 -c core.validateabbrev=true log --pretty=tformat:%h >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tcat >expected <<-\\EOF &&\n+\t0000\n+\t0000\n+\t0000\n+\t0000\n+\t0000\n+\tEOF\n+\tgit -c core.abbrev=4 -c core.validateabbrev=false log --pretty=tformat:%h >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tcat >expected <<-\\EOF &&\n+\t00000000006\n+\t0000000005b\n+\t00000000008\n+\t000000000004\n+\t0000000000e\n+\tEOF\n+\tgit -c core.abbrev=+4 log --pretty=tformat:%h >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tcat >expected <<-\\EOF &&\n+\t00000000006\n+\t0000000005b\n+\t00000000008\n+\t00000000000\n+\t0000000000e\n+\tEOF\n+\tgit -c core.abbrev=+4 -c core.validateabbrev=false log --pretty=tformat:%h >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_failure 'parse describe name taking advantage of generation' '\n \t# ambiguous at the object name level, but there is only one\n \t# such commit at generation 0\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349758","messageId":"20180608224136.20220-19-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 18/20] abbrev parsing: use braces on multiple conditional arms","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:34Z","receivedAt":"2018-06-08T22:42:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust this code that'll be modified in a subsequent change to have\nmore than one line per branch to use braces per the CodingGuidelines,\nthis makes the later change easier to understand.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 5 +++--\n revision.c | 5 +++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex cab79d24ab..e0141cfbc0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4807,10 +4807,11 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->abbrev = strtoul(arg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", arg);\n-\t\tif (options->abbrev < MINIMUM_ABBREV)\n+\t\tif (options->abbrev < MINIMUM_ABBREV) {\n \t\t\toptions->abbrev = MINIMUM_ABBREV;\n-\t\telse if (the_hash_algo->hexsz < options->abbrev)\n+\t\t} else if (the_hash_algo->hexsz < options->abbrev) {\n \t\t\toptions->abbrev = the_hash_algo->hexsz;\n+\t\t}\n \t}\n \telse if ((argcount = parse_long_opt(\"src-prefix\", av, &optarg))) {\n \t\toptions->a_prefix = optarg;\ndiff --git a/revision.c b/revision.c\nindex d39a292895..2a75fef22d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2053,10 +2053,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->abbrev = strtoul(optarg, &end, 10);\n \t\tif (*end)\n \t\t\tdie(\"--abbrev expects a numerical value, got '%s'\", optarg);\n-\t\tif (revs->abbrev < MINIMUM_ABBREV)\n+\t\tif (revs->abbrev < MINIMUM_ABBREV) {\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n-\t\telse if (revs->abbrev > hexsz)\n+\t\t} else if (revs->abbrev > hexsz) {\n \t\t\trevs->abbrev = hexsz;\n+\t\t}\n \t} else if (!strcmp(arg, \"--abbrev-commit\")) {\n \t\trevs->abbrev_commit = 1;\n \t\trevs->abbrev_commit_given = 1;\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349759","messageId":"20180608224136.20220-10-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 09/20] abbrev tests: test for \"git-log\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:25Z","receivedAt":"2018-06-08T22:42:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"log\" family of commands does its own parsing for --abbrev in\nrevision.c, so having dedicated tests for it makes sense.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 645bcca1d1..a66051c040 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -203,4 +203,14 @@ do\n \t\"\n done\n \n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"log core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i log --pretty=format:%h -1 | tr_d_n >log &&\n+\t\ttest_byte_count = $i log &&\n+\t\tgit log --abbrev=$i --pretty=format:%h -1 | tr_d_n >log &&\n+\t\ttest_byte_count = $i log\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349760","messageId":"20180608224136.20220-16-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 15/20] parse-options-cb.c: use braces on multiple conditional arms","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:31Z","receivedAt":"2018-06-08T22:42:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust this code that'll be modified in a subsequent change to have\nmore than one line per branch to use braces per the CodingGuidelines,\nthis makes the later change easier to understand.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options-cb.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 298b5735c8..e3cd87fbd6 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -19,10 +19,11 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\tv = strtol(arg, (char **)&arg, 10);\n \t\tif (*arg)\n \t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n-\t\tif (v && v < MINIMUM_ABBREV)\n+\t\tif (v && v < MINIMUM_ABBREV) {\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > GIT_SHA1_HEXSZ)\n+\t\t} else if (v > GIT_SHA1_HEXSZ) {\n \t\t\tv = GIT_SHA1_HEXSZ;\n+\t\t}\n \t}\n \t*(int *)(opt->value) = v;\n \treturn 0;\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349761","messageId":"20180608224136.20220-13-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 12/20] abbrev tests: test for --abbrev and core.abbrev=[+-]N","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:28Z","receivedAt":"2018-06-08T22:42:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In a later change I mean to make values like -1 and +1 mean something\ndifferent, but right now they're implicitly parsed. Let's test for the\ncurrent behavior before changing it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 131 ++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 128 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 5a99cbe434..6dee92f35e 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -74,7 +74,7 @@ test_expect_success 'abbrev non-integer value handling differs ' '\n \t! test -s stderr\n '\n \n-for i in -41 -20 -10 -1 0 1 2 3 41\n+for i in -41 -20 -10 -1 -0 +0 0 1 2 3 41\n do\n \ttest_expect_success \"core.abbrev value $i out of range errors out\" \"\n \t\ttest_must_fail git -c core.abbrev=$i log -1 --pretty=format:%h 2>stderr &&\n@@ -90,7 +90,7 @@ do\n \t\"\n done\n \n-for i in 0 1 2 3 4\n+for i in 0 1 2 3 4 -0 +0 +1 +2 +3 +4\n do\n \ttest_expect_success \"non-negative --abbrev=$i value log &&\n@@ -98,7 +98,7 @@ do\n \t\"\n done\n \n-for i in 41 9001\n+for i in 41 9001 +41 +9001\n do\n \ttest_expect_success \"non-negative --abbrev=$i value >MINIMUM_ABBREV falls back on 40\" \"\n \t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n@@ -116,6 +116,10 @@ do\n \t\tgit log --abbrev=$i -1 --pretty=format:%h >log &&\n \t\ttest_byte_count = $i log &&\n \n+\t\t# core.abbrev=+N is the same as core.abbrev=N\n+\t\tgit -c core.abbrev=+$i log -1 --pretty=format:%h >log &&\n+\t\ttest_byte_count = $i log &&\n+\n \t\t# The --abbrev option should take priority over\n \t\t# core.abbrev\n \t\tgit -c core.abbrev=20 log --abbrev=$i -1 --pretty=format:%h >log &&\n@@ -172,16 +176,39 @@ do\n \t\"\n done\n \n+test_expect_success 'blame core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_must_fail git -c core.abbrev=-1 blame A.t | cut_tr_d_n_field_n 1 >blame &&\n+\n+\tgit blame --abbrev=-1 A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_byte_count = 5 blame &&\n+\n+\tgit blame --abbrev=+1 A.t | cut_tr_d_n_field_n 1 >blame &&\n+\ttest_byte_count = 5 blame\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"branch core.abbrev=$i and --abbrev=$i\" \"\n \t\tgit -c core.abbrev=$i branch -v | cut_tr_d_n_field_n 3 >branch &&\n \t\ttest_byte_count = $i branch &&\n+\n \t\tgit branch --abbrev=$i -v | cut_tr_d_n_field_n 3 >branch &&\n \t\ttest_byte_count = $i branch\n \t\"\n done\n \n+test_expect_success 'branch core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_must_fail git -c core.abbrev=-1 branch -v | cut_tr_d_n_field_n 3 >branch &&\n+\n+\tgit branch --abbrev=-1 -v | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 4 branch &&\n+\n+\tgit branch --abbrev=+1 -v | cut_tr_d_n_field_n 3 >branch &&\n+\ttest_byte_count = 4 branch\n+'\n+\n test_expect_success 'describe core.abbrev and --abbrev special cases' '\n \t# core.abbrev=0 behaves as usual...\n \ttest_must_fail git -c core.abbrev=0 describe &&\n@@ -203,6 +230,17 @@ do\n \t\"\n done\n \n+test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n+\ttest_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n+\n+\tgit describe --abbrev=-1 | sed_g_tr_d_n >describe &&\n+\ttest_byte_count = 4 describe &&\n+\n+\tgit describe --abbrev=+1 | sed_g_tr_d_n >describe &&\n+\ttest_byte_count = 4 describe\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"log core.abbrev=$i and --abbrev=$i\" \"\n@@ -213,6 +251,20 @@ do\n \t\"\n done\n \n+test_expect_success 'log core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 log --pretty=format:%h -1 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\ttest_must_fail git -c core.abbrev=-1 log --pretty=format:%h -1 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\tgit log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&\n+\ttest_byte_count = 4 log &&\n+\n+\tgit log --abbrev=-1 --pretty=format:%h -1 | tr_d_n >log &&\n+\ttest_byte_count = 40 log\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"diff --no-index --raw core.abbrev=$i and --abbrev=$i\" \"\n@@ -244,6 +296,46 @@ do\n \t\"\n done\n \n+test_expect_success 'diff --no-index --raw core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 diff --no-index --raw X Y 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\ttest_must_fail git -c core.abbrev=-1 diff --no-index --raw X Y 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\ttest_must_fail git diff --no-index --raw --abbrev=+1 X Y >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 4 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 4 diff.4 &&\n+\n+\ttest_must_fail git diff --no-index --raw --abbrev=-1 X Y >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 4 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 4 diff.4\n+'\n+\n+test_expect_success 'diff --raw core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 diff HEAD~ 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\ttest_must_fail git -c core.abbrev=-1 diff HEAD~ 2>stderr &&\n+\ttest_i18ngrep \"abbrev length out of range\" stderr &&\n+\n+\tgit diff --raw --abbrev=+1 HEAD~ >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 4 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 4 diff.4 &&\n+\n+\tgit diff --raw --abbrev=-1 HEAD~ >diff &&\n+\tcut_tr_d_n_field_n 3 diff.3 &&\n+\ttest_byte_count = 40 diff.3 &&\n+\tcut_tr_d_n_field_n 4 diff.4 &&\n+\ttest_byte_count = 40 diff.4\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"ls-files core.abbrev=$i and --abbrev=$i\" \"\n@@ -254,6 +346,17 @@ do\n \t\"\n done\n \n+test_expect_success 'ls-files core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 ls-files --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\ttest_must_fail git -c core.abbrev=-1 ls-files --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\n+\tgit ls-files --abbrev=-1 --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\ttest_byte_count = 4 ls-files &&\n+\n+\tgit ls-files --abbrev=+1 --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\ttest_byte_count = 4 ls-files\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"ls-tree core.abbrev=$i and --abbrev=$i\" \"\n@@ -264,6 +367,17 @@ do\n \t\"\n done\n \n+test_expect_success 'ls-tree core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\ttest_must_fail git -c core.abbrev=-1 ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\n+\tgit ls-tree --abbrev=-1 HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\ttest_byte_count = 4 ls-tree &&\n+\n+\tgit ls-tree --abbrev=+1 HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\ttest_byte_count = 4 ls-tree\n+'\n+\n for i in $(test_seq 4 40)\n do\n \ttest_expect_success \"show-ref core.abbrev=$i and --abbrev=$i\" \"\n@@ -276,4 +390,15 @@ do\n \t\"\n done\n \n+test_expect_success 'show-ref core.abbrev=[-+]1 and --abbrev=[-+]1' '\n+\ttest_must_fail git -c core.abbrev=+1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\ttest_must_fail git -c core.abbrev=-1 show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\n+\tgit show-ref --abbrev=-1 --hash refs/heads/master | tr_d_n >show-ref &&\n+\ttest_byte_count = 4 show-ref &&\n+\n+\tgit show-ref --abbrev=+1 --hash refs/heads/master | tr_d_n >show-ref &&\n+\ttest_byte_count = 4 show-ref\n+'\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349762","messageId":"20180608224136.20220-14-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 13/20] parse-options-cb.c: convert uses of 40 to GIT_SHA1_HEXSZ","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:29Z","receivedAt":"2018-06-08T22:42:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In ac53fe8601 (\"sha1_name: convert uses of 40 to GIT_SHA1_HEXSZ\",\n2017-07-13) the code this is validating user input for in\nfind_unique_abbrev_r() was converted to GIT_SHA1_HEXSZ, but the\ncorresponding validation codepath wasn't change. Let's do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options-cb.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex 0f9f311a7a..298b5735c8 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -21,8 +21,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > 40)\n-\t\t\tv = 40;\n+\t\telse if (v > GIT_SHA1_HEXSZ)\n+\t\t\tv = GIT_SHA1_HEXSZ;\n \t}\n \t*(int *)(opt->value) = v;\n \treturn 0;\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349763","messageId":"20180608224136.20220-7-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 06/20] blame: fix a bug, core.abbrev should work like --abbrev","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:22Z","receivedAt":"2018-06-08T22:42:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 84393bfd73 (\"blame: add --abbrev command line option and make it\nhonor core.abbrev\", 2011-04-06) first released with v1.7.6 \"git blame\"\nhas supported both core.abbrev and --abbrev to change the abbreviation\nlength.\n\nInitially output wouldn't alter the abbreviation length to account for\nthe boundary commit. This was changed in 91229834c2 (\"blame: fix\nalignment with --abbrev=40\", 2017-01-05) first released with v2.11.1.\n\nThat change had a bug which I'm fixing here. It didn't account for the\nabbreviation length also being set via core.abbrev, not just via the\n--abbrev command-line option.\n\nSo let's handle that. The easiest way to do that is to check if the\nglobal default_abbrev variable (-1 by default) has been set by\ngit_default_core_config(), and if so pretend we had the --abbrev\noption is set, in lieu of making everything that uses the \"abbrev\"\nvariable now read that OR default_abbrev).\n\nThe reason I'm documenting these past behaviors is that whatever our\ndesires with --porcelain people do parse the human-readable \"git\nblame\" output, and for any machine use are likely to use\ncore.abbrev=40 or --abbrev=40. Documenting how the format has changed\nover time will help those users avoid nasty surprises.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-blame.txt | 8 +++++++-\n builtin/blame.c | 2 ++\n t/t0014-abbrev.sh | 7 ++++++-\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 7b562494ac..d6cddbcb2e 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -92,7 +92,13 @@ include::blame-options.txt[]\n Because of this UI design, the only way to get the full SHA-1 of the\n boundary commit is to use the `--porcelain` format. With `--abbrev=40`\n only 39 characters of the boundary SHA-1 will be emitted, since one\n-will be used for the caret to mark the boundary.\n+will be used for the caret to mark the boundary. This behavior was\n+different before 2.11.1, git would then emit the 40 character if\n+requested, resulting in unaligned output.\n++\n+Before 2.19, setting `core.abbrev=40` wouldn't apply the above rule\n+and would cause blame to emit output that was unaligned. This bug has\n+since been fixed.\n \n \n THE PORCELAIN FORMAT\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4202584f97..6ab08561d1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -868,6 +868,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else if (show_progress < 0)\n \t\tshow_progress = isatty(2);\n \n+\tif (default_abbrev >= 0)\n+\t\tabbrev = default_abbrev;\n \tif (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)\n \t\t/* one more abbrev length is needed for the boundary commit */\n \t\tabbrev++;\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 77f15d5b0b..934c54a96b 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -135,7 +135,12 @@ do\n \ttest_expect_success \"blame core.abbrev=$i and --abbrev=$i with boundary\" \"\n \t\t# See the blame documentation for why this is off-by-one\n \t\tgit -c core.abbrev=$i blame A.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n-\t\ttest_byte_count = $i blame &&\n+\t\tif test $i -eq 40\n+\t\tthen\n+\t\t\ttest_byte_count = 39 blame\n+\t\telse\n+\t\t\ttest_byte_count = $i blame\n+\t\tfi &&\n \t\tgit blame --abbrev=$i A.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n \t\tif test $i -eq 40\n \t\tthen\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349764","messageId":"20180608224136.20220-9-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 08/20] abbrev tests: test for \"git-describe\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:24Z","receivedAt":"2018-06-08T22:42:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The only thing out of the ordinary with git-describe is the --abbrev=0\nspecial-case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex d8b060d922..645bcca1d1 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -16,6 +16,10 @@ nocaret() {\n \tsed 's/\\^//'\n }\n \n+sed_g_tr_d_n() {\n+\tsed 's/.*g//' | tr_d_n\n+}\n+\n test_expect_success 'setup' '\n \ttest_commit A &&\n \tgit tag -a -mannotated A.annotated &&\n@@ -178,4 +182,25 @@ do\n \t\"\n done\n \n+test_expect_success 'describe core.abbrev and --abbrev special cases' '\n+\t# core.abbrev=0 behaves as usual...\n+\ttest_must_fail git -c core.abbrev=0 describe &&\n+\n+\t# ...but --abbrev=0 is special-cased to print the nearest tag,\n+\t# not fall back on \"4\" like git-log.\n+\techo A.annotated >expected &&\n+\tgit describe --abbrev=0 >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"describe core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i describe | sed_g_tr_d_n >describe &&\n+\t\ttest_byte_count = $i describe &&\n+\t\tgit describe --abbrev=$i | sed_g_tr_d_n >describe &&\n+\t\ttest_byte_count = $i describe\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349765","messageId":"20180608224136.20220-12-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 11/20] abbrev tests: test for plumbing behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:27Z","receivedAt":"2018-06-08T22:42:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"git-{ls-files,ls-tree,show-ref}\" commands all have in common that\nthe core.abbrev variable is ignored, and only --abbrev works. This is\nintentional since these are all plumbing.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 783807475f..5a99cbe434 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -244,4 +244,36 @@ do\n \t\"\n done\n \n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"ls-files core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i ls-files --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\t\ttest_byte_count = 40 ls-files &&\n+\t\tgit ls-files --abbrev=$i --stage A.t | cut_tr_d_n_field_n 2 >ls-files &&\n+\t\ttest_byte_count = $i ls-files\n+\t\"\n+done\n+\n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"ls-tree core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i ls-tree HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\t\ttest_byte_count = 40 ls-tree &&\n+\t\tgit ls-tree --abbrev=$i HEAD A.t | cut -f 1 | cut_tr_d_n_field_n 3 >ls-tree &&\n+\t\ttest_byte_count = $i ls-tree\n+\t\"\n+done\n+\n+for i in $(test_seq 4 40)\n+do\n+\ttest_expect_success \"show-ref core.abbrev=$i and --abbrev=$i\" \"\n+\t\tgit -c core.abbrev=$i show-ref --hash refs/heads/master | tr_d_n >show-ref &&\n+\t\ttest_byte_count = 40 show-ref &&\n+\t\tgit show-ref --hash --abbrev=$i refs/heads/master | tr_d_n >show-ref &&\n+\t\ttest_byte_count = $i show-ref &&\n+\t\tgit show-ref --hash=$i refs/heads/master | tr_d_n >show-ref &&\n+\t\ttest_byte_count = $i show-ref\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349767","messageId":"20180608224136.20220-4-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 03/20] blame doc: explicitly note how --abbrev=40 gives 39 chars","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:19Z","receivedAt":"2018-06-08T22:42:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In a later change I'm adding stress testing of the commit abbreviation\nas it relates to git-blame and others, and initially thought that the\ninability to extract full SHA-1s from the non-\"--porcelain\" output was\na bug.\n\nIn hindsight I could have read the existing paragraph more carefully,\nbut let's make this clearer by explicitly stating this limitation of\n--abbrev as it relates to git-blame, it is not shared by any other\ncommand that supports core.abbrev or --abbrev.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-blame.txt | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 16323eb80e..7b562494ac 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -88,6 +88,11 @@ include::blame-options.txt[]\n \tInstead of using the default 7+1 hexadecimal digits as the\n \tabbreviated object name, use +1 digits. Note that 1 column\n \tis used for a caret to mark the boundary commit.\n++\n+Because of this UI design, the only way to get the full SHA-1 of the\n+boundary commit is to use the `--porcelain` format. With `--abbrev=40`\n+only 39 characters of the boundary SHA-1 will be emitted, since one\n+will be used for the caret to mark the boundary.\n \n \n THE PORCELAIN FORMAT\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349768","messageId":"20180608224136.20220-6-avarab@gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-1-avarab@gmail.com","subject":"[PATCH 05/20] abbrev tests: test \"git-blame\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:41:21Z","receivedAt":"2018-06-08T22:42:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests showing how \"git-blame\" behaves. As noted in an earlier\nchange there's a behavior difference between core.abbrev=40 and\n--abbrev=40.\n\nLet's also assert that neither way of changing the abbreviation length\nmodifies the porcelain output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0014-abbrev.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\nindex 1c60f5ff93..77f15d5b0b 100755\n--- a/t/t0014-abbrev.sh\n+++ b/t/t0014-abbrev.sh\n@@ -12,6 +12,10 @@ cut_tr_d_n_field_n() {\n \tcut -d \" \" -f $1 | tr_d_n\n }\n \n+nocaret() {\n+\tsed 's/\\^//'\n+}\n+\n test_expect_success 'setup' '\n \ttest_commit A &&\n \tgit tag -a -mannotated A.annotated &&\n@@ -115,4 +119,48 @@ do\n \t\"\n done\n \n+for i in $(test_seq 4 40)\n+do\n+\tfor opt in --porcelain --line-porcelain\n+\tdo\n+\t\ttest_expect_success \"blame $opt ignores core.abbrev=$i and --abbrev=$i\" \"\n+\t\t\tgit -c core.abbrev=$i blame $opt A.t | head -n 1 | cut_tr_d_n_field_n 1 >blame &&\n+\t\t\ttest_byte_count = 40 blame &&\n+\t\t\tgit blame $opt --abbrev=$i A.t | head -n 1 | cut_tr_d_n_field_n 1 >blame &&\n+\t\t\ttest_byte_count = 40 blame\n+\t\t\"\n+\tdone\n+\n+\n+\ttest_expect_success \"blame core.abbrev=$i and --abbrev=$i with boundary\" \"\n+\t\t# See the blame documentation for why this is off-by-one\n+\t\tgit -c core.abbrev=$i blame A.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n+\t\ttest_byte_count = $i blame &&\n+\t\tgit blame --abbrev=$i A.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n+\t\tif test $i -eq 40\n+\t\tthen\n+\t\t\ttest_byte_count = 39 blame\n+\t\telse\n+\t\t\ttest_byte_count = $i blame\n+\t\tfi\n+\t\"\n+\n+\ttest_expect_success \"blame core.abbrev=$i and --abbrev=$i without boundary\" \"\n+\t\tgit -c core.abbrev=$i blame B.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n+\t\tif test $i -eq 40\n+\t\tthen\n+\t\t\ttest_byte_count = $i blame\n+\t\telse\n+\t\t\ttest_byte_count = \\$(($i + 1)) blame\n+\t\tfi &&\n+\t\tgit blame --abbrev=$i B.t | cut_tr_d_n_field_n 1 | nocaret >blame &&\n+\t\tif test $i -eq 40\n+\t\tthen\n+\t\t\ttest_byte_count = $i blame\n+\t\telse\n+\t\t\ttest_byte_count = \\$(($i + 1)) blame\n+\t\tfi\n+\t\"\n+done\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"349784","messageId":"CAN0heSoaNotgsvbLv03tqRxC75rXzS6LvvnYBrS=f6pk-Y_x3Q@mail.gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-10-avarab@gmail.com","subject":"Re: [PATCH 09/20] abbrev tests: test for \"git-log\" behavior","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T08:43:22Z","receivedAt":"2018-06-09T08:43:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n> The \"log\" family of commands does its own parsing for --abbrev in\n> revision.c, so having dedicated tests for it makes sense.\n\n> +for i in $(test_seq 4 40)\n\nI've just been skimming so might have missed something, but I see\nseveral instances of this construct, and I wonder what this brute-force\napproach really buys us. An alternative would be, e.g., \"for i in 4 23\n40\". That is, min/max and some arbitrary number in between (odd because\nthe others are even).\n\nOf course, we might have a bug which magically happens for the number 9,\nbut I'd expect us to test for that only if we have some reason to\nbelieve that number 9 is indeed magical.\n\nAlso, 40 is of course tied to SHA-1. You could perhaps define a variable\nat the top of this file to simplify a future generalization. (Same for\n39/41 which are related to 40.)\n\nMartin\n"},{"id":"349795","messageId":"87a7s4471y.fsf@evledraar.gmail.com","threadId":"48679","inReplyTo":"CAN0heSoaNotgsvbLv03tqRxC75rXzS6LvvnYBrS=f6pk-Y_x3Q@mail.gmail.com","subject":"Re: [PATCH 09/20] abbrev tests: test for \"git-log\" behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-09T09:56:25Z","receivedAt":"2018-06-09T09:56:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 09 2018, Martin Ågren wrote:\n\n> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n>> The \"log\" family of commands does its own parsing for --abbrev in\n>> revision.c, so having dedicated tests for it makes sense.\n>\n>> +for i in $(test_seq 4 40)\n>\n> I've just been skimming so might have missed something, but I see\n> several instances of this construct, and I wonder what this brute-force\n> approach really buys us. An alternative would be, e.g., \"for i in 4 23\n> 40\". That is, min/max and some arbitrary number in between (odd because\n> the others are even).\n>\n> Of course, we might have a bug which magically happens for the number 9,\n> but I'd expect us to test for that only if we have some reason to\n> believe that number 9 is indeed magical.\n\nGood point, I'll change this in v2, or at least guard it with\nEXPENSIVE. I hacked it up like this while exhaustively testing things\nduring development, and discovered some edge cases (e.g. \"0\" is special\nsometimes).\n\n> Also, 40 is of course tied to SHA-1. You could perhaps define a variable\n> at the top of this file to simplify a future generalization. (Same for\n> 39/41 which are related to 40.)\n\nI forgot to note this in the commit message, but I intentionally didn't\nguard this test with the SHA1 prereq, there's nothing per-se specific to\nSHA-1 here, it's not a given that whatever our NewHash is that we won't\nuse 40 characters, and the rest of the magic constants like 4 and 7 is\nsomething we're likely to retain with NewHash.\n\nAlthough maybe we should expose GIT_SHA1_HEXSZ to the test suite.\n"},{"id":"349799","messageId":"CAN0heSrh6kJyiAATc377sEg46O=ofEnczCrBubrSM5pxT72W7Q@mail.gmail.com","threadId":"48679","inReplyTo":"87a7s4471y.fsf@evledraar.gmail.com","subject":"Re: [PATCH 09/20] abbrev tests: test for \"git-log\" behavior","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T13:56:23Z","receivedAt":"2018-06-09T13:56:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 11:56, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Sat, Jun 09 2018, Martin Ågren wrote:\n>\n>> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n>>> The \"log\" family of commands does its own parsing for --abbrev in\n>>> revision.c, so having dedicated tests for it makes sense.\n>>\n>>> +for i in $(test_seq 4 40)\n>>\n>> I've just been skimming so might have missed something, but I see\n>> several instances of this construct, and I wonder what this brute-force\n>> approach really buys us. An alternative would be, e.g., \"for i in 4 23\n>> 40\". That is, min/max and some arbitrary number in between (odd because\n>> the others are even).\n>>\n>> Of course, we might have a bug which magically happens for the number 9,\n>> but I'd expect us to test for that only if we have some reason to\n>> believe that number 9 is indeed magical.\n>\n> Good point, I'll change this in v2, or at least guard it with\n> EXPENSIVE. I hacked it up like this while exhaustively testing things\n> during development, and discovered some edge cases (e.g. \"0\" is special\n> sometimes).\n\nAh, \"useful during hacking\" explains why you did it like this. Of your\ntwo approaches, I'd probably favour \"make it cheaper\" over \"mark it as\nEXPENSIVE\". Nothing I feel strongly about.\n\n>> Also, 40 is of course tied to SHA-1. You could perhaps define a variable\n>> at the top of this file to simplify a future generalization. (Same for\n>> 39/41 which are related to 40.)\n>\n> I forgot to note this in the commit message, but I intentionally didn't\n> guard this test with the SHA1 prereq, there's nothing per-se specific to\n> SHA-1 here, it's not a given that whatever our NewHash is that we won't\n> use 40 characters, and the rest of the magic constants like 4 and 7 is\n> something we're likely to retain with NewHash.\n\nI'd tend to agree about not marking this SHA1.\n\n> Although maybe we should expose GIT_SHA1_HEXSZ to the test suite.\n\nIt seems like brian's \"test_translate\"-approach [1] would be a good\nchoice of tool for this. That is, you'd just define something at the top\nof this file for now, then once that tool is in place, a one-line change\ncould get \"hexsz\" from `test_translate` instead.\n\n[1] https://public-inbox.org/git/20180604235229.279814-2-sandals@crustytoothpaste.net/\n\nMartin\n"},{"id":"349800","messageId":"CAN0heSqrgUw1u8Ykk-9eGosWwFS0NFM7sLBe5Loo4cN=tkrXaA@mail.gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-18-avarab@gmail.com","subject":"Re: [PATCH 17/20] abbrev: unify the handling of empty values","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T14:24:30Z","receivedAt":"2018-06-09T14:24:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n> For no good reason the --abbrev= command-line option was less strict\n> than the core.abbrev config option, which came down to the latter\n> using git_config_int() which rejects an empty string, but the rest of\n> the parsing using strtoul() which will convert it to 0.\n\nIt will still be less strict in that it accepts trailing garbage, e.g.,\n`--abbrev=7a`. Probably ok to leave it at that in this series, but\npossibly useful to mention here that this only makes these options \"less\ndifferently strict\".\n\nI also notice that the documentation of `--abbrev` starts with \"Instead\nof showing the full 40-byte hexadecimal object name\" which doesn't seem\nright. I get much shorter IDs and I can't see that I'd have any\nconfiguration causing this. Anyway, that might not be anything this\nseries needs to do anything about.\n\n> + if (!strcmp(arg, \"\"))\n> + die(\"--abbrev expects a value, got '%s'\", arg);\n\n> + if (!strcmp(arg, \"\"))\n> + return opterror(opt, \"expects a value\", 0);\n\n> + if (!strcmp(optarg, \"\"))\n> + die(\"--abbrev expects a value, got '%s'\", optarg);\n\n> + test_must_fail git branch -v --abbrev= 2>stderr &&\n> + test_i18ngrep \"expects a value\" stderr &&\n\n> + test_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&\n> + test_i18ngrep \"expects a value\" stderr &&\n\n> + test_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&\n> + test_i18ngrep \"expects a value\" stderr &&\n\nMismatch re i18n-ed or not between implementations and tests?\n\nMartin\n"},{"id":"349801","messageId":"CAN0heSpMSmn=34ze4GmuETzR=VZ2TO4ooNC=PS+OM5yAawB3og@mail.gmail.com","threadId":"48679","inReplyTo":"CAN0heSqrgUw1u8Ykk-9eGosWwFS0NFM7sLBe5Loo4cN=tkrXaA@mail.gmail.com","subject":"Re: [PATCH 17/20] abbrev: unify the handling of empty values","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T14:31:57Z","receivedAt":"2018-06-09T14:32:01Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 16:24, Martin Ågren wrote:\n> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n>> For no good reason the --abbrev= command-line option was less strict\n>> than the core.abbrev config option, which came down to the latter\n>> using git_config_int() which rejects an empty string, but the rest of\n>> the parsing using strtoul() which will convert it to 0.\n>\n> It will still be less strict in that it accepts trailing garbage, e.g.,\n> `--abbrev=7a`. Probably ok to leave it at that in this series, but\n> possibly useful to mention here that this only makes these options \"less\n> differently strict\".\n\nHmpf, please ignore. That's what I get for looking at a few patches,\ntaking a break, picking it up again and completely forgetting what's\ngoing on...\n"},{"id":"349803","messageId":"CAN0heSr=4GmPqOgtdbSUH97FcS5owRVd__qJzKA5-WDvRqwmjQ@mail.gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-20-avarab@gmail.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T15:38:44Z","receivedAt":"2018-06-09T15:38:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ab641bf5a9..abf07be7b6 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -919,6 +919,12 @@ core.abbrev::\n> in your repository, which hopefully is enough for\n> abbreviated object names to stay unique for some time.\n> The minimum length is 4.\n> ++\n> +This can also be set to relative values such as `+2` or `-2`, which\n> +means to add or subtract N characters from the SHA-1 that Git would\n> +otherwise print, this allows for producing more future-proof SHA-1s\n> +for use within a given project, while adjusting the value for the\n> +current approximate number of objects.\n\nHow about s/, this/. This/ to break it up a little? Also, you write \"+2\"\nand \"-2\" but then \"N\". Unify it?\n\nAlso, I'd suggest s/SHA-1/object ID/ to be future-proof.\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index f466600972..f1114a7b8d 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -384,6 +384,9 @@ endif::git-format-patch[]\n> independent of the `--full-index` option above, which controls\n> the diff-patch output format. Non default number of\n> digits can be specified with `--abbrev=`.\n> ++\n> +Can also be set to a relative value, see `core.abbrev` in\n> +linkgit:git-diff[1].\n\nGood. You then add this paragraph to lots of other places...\n\n> diff --git a/config.c b/config.c\n> index 12f762ad92..cd95c6bdfb 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1151,6 +1151,17 @@ static int git_default_core_config(const char *var, const char *value)\n> return config_error_nonbool(var);\n> if (!strcasecmp(value, \"auto\")) {\n> default_abbrev = -1;\n> + } else if (*value == '+' || *value == '-') {\n> + int relative = git_config_int(var, value);\n> + if (relative == 0)\n> + die(_(\"bad core.abbrev value %s. \"\n\nTrailing period? Same below.\n\n> + \"relative values must be non-zero\"),\n> + value);\n> + if (abs(relative) > GIT_SHA1_HEXSZ)\n> + die(_(\"bad core.abbrev value %s. \"\n> + \"impossibly out of range\"),\n> + value);\n> + default_abbrev_relative = relative;\n> } else {\n> int abbrev = git_config_int(var, value);\n> if (abbrev < minimum_abbrev || abbrev > 40)\n> diff --git a/diff.c b/diff.c\n> index e0141cfbc0..f7861b8472 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4801,16 +4801,28 @@ int diff_opt_parse(struct diff_options *options,\n> else if (!strcmp(arg, \"--abbrev\"))\n> options->abbrev = DEFAULT_ABBREV;\n> else if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n> + int v;\n> char *end;\n> if (!strcmp(arg, \"\"))\n> die(\"--abbrev expects a value, got '%s'\", arg);\n> - options->abbrev = strtoul(arg, &end, 10);\n> + v = strtoul(arg, &end, 10);\n> if (*end)\n> die(\"--abbrev expects a numerical value, got '%s'\", arg);\n> - if (options->abbrev < MINIMUM_ABBREV) {\n> + if (*arg == '+' || *arg == '-') {\n> + if (v == 0) {\n> + die(\"relative abbrev must be non-zero\");\n> + } else if (abs(v) > the_hash_algo->hexsz) {\n> + die(\"relative abbrev impossibly out of range\");\n> + } else {\n> + default_abbrev_relative = v;\n> + options->abbrev = DEFAULT_ABBREV;\n> + }\n> + } else if (v < MINIMUM_ABBREV) {\n> options->abbrev = MINIMUM_ABBREV;\n> - } else if (the_hash_algo->hexsz < options->abbrev) {\n> + } else if (the_hash_algo->hexsz < v) {\n> options->abbrev = the_hash_algo->hexsz;\n> + } else {\n> + options->abbrev = v;\n\nI've cut out a few instances of more-or-less repeated code. Any\npossibility of extracting this into a helper? Maybe after you've done\nthe preparatory work of unifying these sites. Or as part of it, i.e.,\n\"let's switch this spot to use the helper; that makes it stricter in\nthis-and-that sense\".\n\nThese can't all be entirely unified, I guess, but maybe \"mostly\"?\n\nMartin\n"},{"id":"349804","messageId":"CAN0heSqo1WSVkYNiFAv4A7x8hG0wEx-iSz7ssLmYYVuen7b-LQ@mail.gmail.com","threadId":"48679","inReplyTo":"20180608224136.20220-21-avarab@gmail.com","subject":"Re: [PATCH 20/20] abbrev: add a core.validateAbbrev setting","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-06-09T15:47:18Z","receivedAt":"2018-06-09T15:47:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason wrote:\n\n> Instead of trying really hard to find an unambiguous SHA-1 we can with\n> core.validateAbbrev=false, and preferably combined with the new\n> support for relative core.abbrev values (such as +4) unconditionally\n> print a short SHA-1 without doing any disambiguation check. I.e. it\n\nThis first paragraph read weirdly the first time. On the second attempt\nI knew how to structure it and got it right. It might be easier to read\nif the part about core.appreb=+4 were in a separate second sentence.\n\nThat last \"it\" is \"the combination of these two configs\", right?\n\n> allows for picking a trade-off between performance, and the odds that\n> future or remote (or current and local) short SHA-1 will be ambiguous.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index abf07be7b6..df31d1351f 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -925,6 +925,49 @@ means to add or subtract N characters from the SHA-1 that Git would\n> otherwise print, this allows for producing more future-proof SHA-1s\n> for use within a given project, while adjusting the value for the\n> current approximate number of objects.\n> ++\n> +This is especially useful in combination with the\n> +`core.validateAbbrev` setting, or to get more future-proof hashes to\n> +reference in the future in a repository whose number of objects is\n> +expected to grow.\n\nMaybe s/validateAbbrev/validateAbbrev = false/?\n\n> +\n> +core.validateAbbrev::\n> + If set to false (true by default) don't do any validation when\n> + printing abbreviated object names to see if they're really\n> + unique. This makes printing objects more performant at the\n> + cost of potentially printing object names that aren't unique\n> + within the current repository.\n\nGood. I understand why I'd want to use it, and why not.\n\n> ++\n> +When printing abbreviated object names Git needs to look through the\n> +local object store. This is an `O(log N)` operation assuming all the\n> +objects are in a single pack file, but `X * O(log N)` given `X` pack\n> +files, which can get expensive on some larger repositories.\n\nThis might be very close to too much information.\n\n> ++\n> +This setting changes that to `O(1)`, but with the trade-off that\n> +depending on the value of `core.abbrev` we may be printing abbreviated\n> +hashes that collide. Too see how likely this is, try running:\n> ++\n> +-----------------------------------------------------------------------------------------------------------\n> +git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' | sort | uniq -c | sort -nr\n> +-----------------------------------------------------------------------------------------------------------\n> ++\n> +This shows how many commits were found at each abbreviation length. On\n> +linux.git in June 2018 this shows a bit more than 750,000 commits,\n> +with just 4 needing 11 characters to be fully abbreviated, and the\n> +default heuristic picks a length of 12.\n\nThese last few paragraphs seem like too much to me.\n\n> ++\n> +Even without `core.validateAbbrev=false` the results abbreviation\n> +already a bit of a probability game. They're guaranteed at the moment\n> +of generation, but as more objects are added, ambiguities may be\n> +introduced. Likewise, what's unambiguous for you may not be for\n> +somebody else you're communicating with, if they have their own clone.\n\nThis seems more useful.\n\n> ++\n> +Therefore the default of `core.validateAbbrev=true` may not save you\n> +in practice if you're sharing the SHA-1 or noting it now to use after\n> +a `git fetch`. You may be better off setting `core.abbrev` to\n> +e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine\n> +that with `core.validateAbbrev=false` to get a reasonable trade-off\n> +between safety and performance.\n\nMakes sense. As before, I'd suggest s/SHA-1/object ID/.\n\nI do wonder if this documentation could be a bit less verbose without\nsacrificing too much correctness and clarity. No brilliant suggestions\non how to do that, sorry.\n\nMartin\n"},{"id":"349999","messageId":"xmqqbmcfanb6.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"20180608224136.20220-4-avarab@gmail.com","subject":"Re: [PATCH 03/20] blame doc: explicitly note how --abbrev=40 gives 39 chars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T18:10:05Z","receivedAt":"2018-06-12T18:10:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In a later change I'm adding stress testing of the commit abbreviation\n> as it relates to git-blame and others, and initially thought that the\n> inability to extract full SHA-1s from the non-\"--porcelain\" output was\n> a bug.\n\n... meaning that it is not actually a bug, as the output format\nother than porcelain is for human consumption?\n\n> --- a/Documentation/git-blame.txt\n> +++ b/Documentation/git-blame.txt\n> @@ -88,6 +88,11 @@ include::blame-options.txt[]\n> \tInstead of using the default 7+1 hexadecimal digits as the\n> \tabbreviated object name, use +1 digits. Note that 1 column\n> \tis used for a caret to mark the boundary commit.\n\nThis is outside the scope of this patch, but is the above 7+1 still\ncurrent or do we need updating it for the (not so) recent change to\nauto-scale the default abbreviation width?\n\n> ++\n> +Because of this UI design, the only way to get the full SHA-1 of the\n> +boundary commit is to use the `--porcelain` format. With `--abbrev=40`\n> +only 39 characters of the boundary SHA-1 will be emitted, since one\n> +will be used for the caret to mark the boundary.\n\nOK.\n"},{"id":"350003","messageId":"xmqq4li7ambf.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"20180608224136.20220-5-avarab@gmail.com","subject":"Re: [PATCH 04/20] abbrev tests: add tests for core.abbrev and --abbrev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T18:31:32Z","receivedAt":"2018-06-12T18:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh\n> new file mode 100755\n> index 0000000000..1c60f5ff93\n> --- /dev/null\n> +++ b/t/t0014-abbrev.sh\n> @@ -0,0 +1,118 @@\n> +#!/bin/sh\n> +\n> +test_description='test core.abbrev and related features'\n> +\n> +. ./test-lib.sh\n> +\n> +tr_d_n() {\n> +\ttr -d '\\n'\n> +}\n\n- I personally would prefer to see the reason for having a helper\n function like this to be \"make it easier to reason about\", rather\n \"make it shorter to type\". tr_d_n feels more about the latter; if\n we aimed for the former, this would be called strip_LF or\n something.\n\n- In the existing stcipts, it seems that we prefer to spell tr args\n with 3-octal, e.g. \\012 for LF, \\015 for CR.\n\n- Also let's have SP on both sides of (), i.e.\n\n\tfuncname () {\n\n for shell function definitions.\n\n> +cut_tr_d_n_field_n() {\n> +\tcut -d \" \" -f $1 | tr_d_n\n> +}\n\nLikewise. Name this not after how it does what it does, but after\nwhat it does and why it does so. In other words, if your answer to\nthe question: \"what does the caller want?\" is \"it wants to pick the\nnth field\", then name it \"pick_nth_field\" or something?\n\n> +test_expect_success 'abbrev empty value handling differs ' '\n> +\ttest_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&\n> +\ttest_i18ngrep \"bad numeric config value.*invalid unit\" stderr &&\n> +\n> +\tgit branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&\n> +\ttest_byte_count = 40 branch &&\n\nSounds like a good thing to unify. If anything, --options=value\nshould be stricter than vari.able=value but it is the other way\naround.\n\n> +\tgit log --abbrev= -1 --pretty=format:%h >log &&\n> +\ttest_byte_count = 4 log &&\n\nMakes readers wonder if 4 is about 3 hex plus terminating LF. The\nreason why this works is because --pretty=format:%h (not --format=%h\nor --pretty=tformat:%h) uses delimiter semantics and we won't need\nany LF to show a single record.\n\nIf we use the helper to measure the length of hexadecimal digits, it\nmay make sense to add a wrapper around test_byte_count that strips\nLF; that way, the caller can use --format=%h instead and there will\nbe one less thing for the reader to worry about.\n\n> +\tgit diff --raw --abbrev= HEAD~ >diff &&\n> +\tcut_tr_d_n_field_n 3 diff.3 &&\n> +\ttest_byte_count = 4 diff.3 &&\n> +\tcut_tr_d_n_field_n 4 diff.4 &&\n> +\ttest_byte_count = 4 diff.4 &&\n\nThese all depend on the fact that we do not have excessive number of\nirrelevant objects to force us to abbreviate using more hexdigits\nthan the minimum 4, right? I _think_ that is a reasonable\nassumption we can depend on, even across the hash function\ntransition. We may want to leave in-code comment, though.\n\n> +\ttest_must_fail git diff --raw --abbrev= --no-index X Y >diff &&\n> +\tcut_tr_d_n_field_n 3 diff.3 &&\n> +\ttest_byte_count = 4 diff.3 &&\n"},{"id":"350005","messageId":"xmqqzhzz96hu.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"CAN0heSqo1WSVkYNiFAv4A7x8hG0wEx-iSz7ssLmYYVuen7b-LQ@mail.gmail.com","subject":"Re: [PATCH 20/20] abbrev: add a core.validateAbbrev setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T18:58:37Z","receivedAt":"2018-06-12T18:58:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> +This is especially useful in combination with the\n>> +`core.validateAbbrev` setting, or to get more future-proof hashes to\n>> +reference in the future in a repository whose number of objects is\n>> +expected to grow.\n>\n> Maybe s/validateAbbrev/validateAbbrev = false/?\n\nPerhaps, but even with =true it would equally be useful, as the\npoint of this setting is to future-proofing.\n\n>> ++\n>> +When printing abbreviated object names Git needs to look through the\n>> +local object store. This is an `O(log N)` operation assuming all the\n>> +objects are in a single pack file, but `X * O(log N)` given `X` pack\n>> +files, which can get expensive on some larger repositories.\n>\n> This might be very close to too much information.\n\nNot very close, but just too much information without crucial detail\n(i.e. log N times what constant???). I'd drop it.\n\n>> ++\n>> +This setting changes that to `O(1)`, but with the trade-off that\n>> +depending on the value of `core.abbrev` we may be printing abbreviated\n>> +hashes that collide.\n\nIt may not be technically wrong to say \"This changes it to O(1)\",\nbut I think to most people it is more understandable to say \"This\nchanges it to zero\" ;-)\n\n Setting this variable to false makes Git not to validate the\n abbreviation it produces uniquely identifies an object among the\n current set of objects in the repository. Depending on the\n value of `core.abbrev`, we may be printing abbreviated hashes\n that collide. Note that setting this variable to true (or\n leaving it unset) does not guarantee that an abbreviated hash\n will never collide with future objects in the repository (you\n need to set core.abbrevLength to a larger value for that).\n\nwould be sufficient to clarify, and also nuke the following\noverly-detailed paragraph.\n\n>> ... Too see how likely this is, try running:\n>> ++\n>> +-----------------------------------------------------------------------------------------------------------\n>> +git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' | sort | uniq -c | sort -nr\n>> +-----------------------------------------------------------------------------------------------------------\n>> ++\n>> +This shows how many commits were found at each abbreviation length. On\n>> +linux.git in June 2018 this shows a bit more than 750,000 commits,\n>> +with just 4 needing 11 characters to be fully abbreviated, and the\n>> +default heuristic picks a length of 12.\n>\n> These last few paragraphs seem like too much to me.\n\nYeah, it goes to too low level a detail, especially with the \"try\nrunning\" part. I'd remove everything but \"On linux.git in June ...\"\nif I were writing it from the above.\n\n>> ++\n>> +Even without `core.validateAbbrev=false` the results abbreviation\n>> +already a bit of a probability game. They're guaranteed at the moment\n>> +of generation, but as more objects are added, ambiguities may be\n>> +introduced. Likewise, what's unambiguous for you may not be for\n>> +somebody else you're communicating with, if they have their own clone.\n>\n> This seems more useful.\n\nYes, but still overly verbose; I think rolling it in the single\nparagraph description like I showed above would be sufficient.\n\n>> ++\n>> +Therefore the default of `core.validateAbbrev=true` may not save you\n>> +in practice if you're sharing the SHA-1 or noting it now to use after\n>> +a `git fetch`. You may be better off setting `core.abbrev` to\n>> +e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine\n>> +that with `core.validateAbbrev=false` to get a reasonable trade-off\n>> +between safety and performance.\n>\n> Makes sense. As before, I'd suggest s/SHA-1/object ID/.\n\nLikewise. If we were to keep it, then s/object ID/object name/.\n"},{"id":"350008","messageId":"xmqqvaan95os.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"20180608224136.20220-20-avarab@gmail.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T19:16:03Z","receivedAt":"2018-06-12T19:16:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the core.abbrev config variable and the corresponding --abbrev\n> command-line option to support relative values such as +1 or -1.\n>\n> Before Linus's e6c587c733 (\"abbrev: auto size the default\n> abbreviation\", 2016-09-30) git would default to abbreviating object\n> names to 7-hexdigits, and only picking longer SHA-1s as needed if that\n> was ambiguous.\n>\n> That change instead set the default length as a function of the\n> estimated current count of objects:\n>\n> Based on the expectation that we would see collision in a\n> 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\n> add to the shortened name allows us to have four times (2-bits) as\n> many objects in the repository.\n>\n> By supporting relative values for core.abbrev we can allow users to\n> consistently opt-in for either a higher or lower probability of\n> collision, without needing to hardcode a given numeric value like\n> \"10\", which would be overkill on some repositories, and far to small\n> on others.\n\nNicely explained and calculated ;-)\n\n> test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '\n> -\ttest_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n> -\ttest_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n> +\tgit -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n> +\ttest_byte_count = 6 describe &&\n> +\n> +\tgit -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n> +\ttest_byte_count = 8 describe &&\n\nEven though I see the point of supporting absurdly small absolute\nvalues like 4, I do not quite see the point of supporting negative\nrelative values here. What's the expected use case?\n\n> \tgit log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&\n> -\ttest_byte_count = 4 log &&\n> +\ttest_byte_count = 8 log &&\n\nThis, together with many many others in the rest of the patch, is\ncute but confusing in that the diff shows change from 4 to 8 due to\nthe redefinition of what abbrev=+1 means. I have a feeling that it\nmay not be worth doing it \"right\", but if we were doing it \"right\",\nwe would probably have done it in multiple steps:\n\n - the earlier patches in this series that demonstrates\n --abbrev=+1 is --abbrev=1 and core.abbrev=+1 is an error.\n\n - ensure --abbrev=+1 is rejected as syntax error just like\n core.abbrev=+1 was, without introducing relative values\n\n - introduce relative value.\n\nThat way, the last step (which corresponds to this patch) would show\nchange from \"log --abbrev=+1\" failing due to syntax error to showing\nabbreviated value that is slightly longer than the default.\n\nBut a I said, it may not be worth doing so. \"Is it worth supporting\nnegative relative length?\" still stands, though.\n"},{"id":"350098","messageId":"871sdawcmh.fsf@evledraar.gmail.com","threadId":"48679","inReplyTo":"xmqqvaan95os.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-13T22:22:14Z","receivedAt":"2018-06-13T22:22:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 12 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the core.abbrev config variable and the corresponding --abbrev\n>> command-line option to support relative values such as +1 or -1.\n>>\n>> Before Linus's e6c587c733 (\"abbrev: auto size the default\n>> abbreviation\", 2016-09-30) git would default to abbreviating object\n>> names to 7-hexdigits, and only picking longer SHA-1s as needed if that\n>> was ambiguous.\n>>\n>> That change instead set the default length as a function of the\n>> estimated current count of objects:\n>>\n>> Based on the expectation that we would see collision in a\n>> 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\n>> add to the shortened name allows us to have four times (2-bits) as\n>> many objects in the repository.\n>>\n>> By supporting relative values for core.abbrev we can allow users to\n>> consistently opt-in for either a higher or lower probability of\n>> collision, without needing to hardcode a given numeric value like\n>> \"10\", which would be overkill on some repositories, and far to small\n>> on others.\n>\n> Nicely explained and calculated ;-)\n>\n>> test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '\n>> -\ttest_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n>> -\ttest_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n>> +\tgit -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&\n>> +\ttest_byte_count = 6 describe &&\n>> +\n>> +\tgit -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&\n>> +\ttest_byte_count = 8 describe &&\n>\n> Even though I see the point of supporting absurdly small absolute\n> values like 4, I do not quite see the point of supporting negative\n> relative values here. What's the expected use case?\n\nI'll add a better explanation for this to the commit message.\n\nInitially I did this for consistency, since it was easy to implement,\nand there's no reason to have that arbitrary limitation, but thinking\nabout it again I think I'll use this for some of my projects.\n\nE.g. here's a breakdown of my dotfiles repo:\n\n $ git -c core.abbrev=4 log --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr\n 784 4\n 59 5\n 7 6\n\nI don't have a single commit that needs 7 characters, yet that's our\ndefault. This is a sane trade-off for the kernel, but for something\nthat's just a toy or something you're playing around with something\nshorter can make sense.\n\nSHA-1s aren't just written down, but also e.g. remembered in wetware\nshort-term memory.\n\n>> \tgit log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&\n>> -\ttest_byte_count = 4 log &&\n>> +\ttest_byte_count = 8 log &&\n>\n> This, together with many many others in the rest of the patch, is\n> cute but confusing in that the diff shows change from 4 to 8 due to\n> the redefinition of what abbrev=+1 means. I have a feeling that it\n> may not be worth doing it \"right\", but if we were doing it \"right\",\n> we would probably have done it in multiple steps:\n>\n> - the earlier patches in this series that demonstrates\n> --abbrev=+1 is --abbrev=1 and core.abbrev=+1 is an error.\n>\n> - ensure --abbrev=+1 is rejected as syntax error just like\n> core.abbrev=+1 was, without introducing relative values\n>\n> - introduce relative value.\n>\n> That way, the last step (which corresponds to this patch) would show\n> change from \"log --abbrev=+1\" failing due to syntax error to showing\n> abbreviated value that is slightly longer than the default.\n>\n> But a I said, it may not be worth doing so. \"Is it worth supporting\n> negative relative length?\" still stands, though.\n\nI'll see what I can do about this value churn.\n"},{"id":"350100","messageId":"xmqqa7ry5n9h.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"871sdawcmh.fsf@evledraar.gmail.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T22:34:34Z","receivedAt":"2018-06-13T22:34:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> E.g. here's a breakdown of my dotfiles repo:\n>\n> $ git -c core.abbrev=4 log --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr\n> 784 4\n> 59 5\n> 7 6\n>\n> I don't have a single commit that needs 7 characters, yet that's our\n> default. This is a sane trade-off for the kernel, but for something\n> that's just a toy or something you're playing around with something\n> shorter can make sense.\n>\n> SHA-1s aren't just written down, but also e.g. remembered in wetware\n> short-term memory.\n\nThat's a fine example of what I called \"supporting absurdly small\nabsolute values like 4\"; I still do not see why you want \"negative\nrelative values\" from that example.\n"},{"id":"350142","messageId":"87zhzxvmyv.fsf@evledraar.gmail.com","threadId":"48679","inReplyTo":"xmqqa7ry5n9h.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-14T07:36:24Z","receivedAt":"2018-06-14T07:36:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 13 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> E.g. here's a breakdown of my dotfiles repo:\n>>\n>> $ git -c core.abbrev=4 log --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr\n>> 784 4\n>> 59 5\n>> 7 6\n>>\n>> I don't have a single commit that needs 7 characters, yet that's our\n>> default. This is a sane trade-off for the kernel, but for something\n>> that's just a toy or something you're playing around with something\n>> shorter can make sense.\n>>\n>> SHA-1s aren't just written down, but also e.g. remembered in wetware\n>> short-term memory.\n>\n> That's a fine example of what I called \"supporting absurdly small\n> absolute values like 4\"; I still do not see why you want \"negative\n> relative values\" from that example.\n\nBecause hardcoding -2 is very different than setting it to 5, because\nthe -2 will scale to the size of the repository, but 5 is just 7-2 where\n7 is our default value.\n\nSo, in general if you want less future proof hashes by some\nprobabilistic metric (whether you use core.validateAbbrev or not) you'd\nuse -2 or -3, just like you might use +2 or +3 if you'd like to have\nmore future-proof hashes (especially with core.validateAbbrev=true).\n"},{"id":"350157","messageId":"xmqq602l5pvs.fsf@gitster-ct.c.googlers.com","threadId":"48679","inReplyTo":"87zhzxvmyv.fsf@evledraar.gmail.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-14T15:50:15Z","receivedAt":"2018-06-14T15:50:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Jun 13 2018, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> E.g. here's a breakdown of my dotfiles repo:\n>>>\n>>> $ git -c core.abbrev=4 log --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr\n>>> 784 4\n>>> 59 5\n>>> 7 6\n>>>\n>>> I don't have a single commit that needs 7 characters, yet that's our\n>>> default. This is a sane trade-off for the kernel, but for something\n>>> that's just a toy or something you're playing around with something\n>>> shorter can make sense.\n>>>\n>>> SHA-1s aren't just written down, but also e.g. remembered in wetware\n>>> short-term memory.\n>>\n>> That's a fine example of what I called \"supporting absurdly small\n>> absolute values like 4\"; I still do not see why you want \"negative\n>> relative values\" from that example.\n>\n> Because hardcoding -2 is very different than setting it to 5, because\n> the -2 will scale to the size of the repository, but 5 is just 7-2 where\n> 7 is our default value.\n>\n> So, in general if you want less future proof hashes by some\n> probabilistic metric (whether you use core.validateAbbrev or not) you'd\n> use -2 or -3, just like you might use +2 or +3 if you'd like to have\n> more future-proof hashes (especially with core.validateAbbrev=true).\n\nThat still does not make much sense to me at all.\n\nI do agree that something shorter than the default 7 may be more\nappropriate for our wetware short-term memory, and it would make\nsense to grow the \"riskier to collide than the default heuristics\nbut more memorable\" variant as the project grows, _ONLY_ _IF_ our\nwetware short-term memory scales with the project we happen to be\nworking on. But our wetware does not scale with the project we work\non; at least mine does not.\n\nSo...\n\n"},{"id":"350187","messageId":"87y3fhuqye.fsf@evledraar.gmail.com","threadId":"48679","inReplyTo":"xmqq602l5pvs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 19/20] abbrev: support relative abbrev values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-14T19:07:53Z","receivedAt":"2018-06-14T19:08:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 14 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Wed, Jun 13 2018, Junio C Hamano wrote:\n>>\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>>> E.g. here's a breakdown of my dotfiles repo:\n>>>>\n>>>> $ git -c core.abbrev=4 log --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr\n>>>> 784 4\n>>>> 59 5\n>>>> 7 6\n>>>>\n>>>> I don't have a single commit that needs 7 characters, yet that's our\n>>>> default. This is a sane trade-off for the kernel, but for something\n>>>> that's just a toy or something you're playing around with something\n>>>> shorter can make sense.\n>>>>\n>>>> SHA-1s aren't just written down, but also e.g. remembered in wetware\n>>>> short-term memory.\n>>>\n>>> That's a fine example of what I called \"supporting absurdly small\n>>> absolute values like 4\"; I still do not see why you want \"negative\n>>> relative values\" from that example.\n>>\n>> Because hardcoding -2 is very different than setting it to 5, because\n>> the -2 will scale to the size of the repository, but 5 is just 7-2 where\n>> 7 is our default value.\n>>\n>> So, in general if you want less future proof hashes by some\n>> probabilistic metric (whether you use core.validateAbbrev or not) you'd\n>> use -2 or -3, just like you might use +2 or +3 if you'd like to have\n>> more future-proof hashes (especially with core.validateAbbrev=true).\n>\n> That still does not make much sense to me at all.\n>\n> I do agree that something shorter than the default 7 may be more\n> appropriate for our wetware short-term memory, and it would make\n> sense to grow the \"riskier to collide than the default heuristics\n> but more memorable\" variant as the project grows, _ONLY_ _IF_ our\n> wetware short-term memory scales with the project we happen to be\n> working on. But our wetware does not scale with the project we work\n> on; at least mine does not.\n\nYes, it's a trade-off, but just because something is a trade-off doesn't\nmake it useless.\n\nAside from the feature I'm proposing, the same thing applies to the\nshort SHA-1 currently. My ~1k commit dotfiles has 7 characters, but\nlinux.git has 12. Does that make printing the short SHA-1 at all useless\nand we should just fall back to 40 characters if it's say >= 12? No.\n\n12 is still better than 40, but if we could get away with it 10 would be\nbetter than 12. Right now the \"get away with it\" calculation is a\nhardcoded constant, this makes it configurable.\n\nThe reason to make it configurable is because you may want more future\nproof *or* less future-proof SHA-1s depending on the use-case. Printing\na longer SHA-1 has a cost, including but not limited to:\n\n * Remembering it for a short time\n * Seeing it on one screen and typing it into another computer manually\n * `git log --oneline` output in a terminal where horizontal space is at\n a premium\n"}]}