{"thread":{"id":"44205","subject":"[PATCH 0/18] alternate object database cleanups","startedAt":"2016-10-03T20:33:29Z","lastAt":"2016-11-08T19:27:48Z","messageCount":84,"participants":["Jeff King","Jacob Keller","Stefan Beller","Junio C Hamano","Aaron Schrab","Jakub Narębski","René Scharfe","Bryan Turner"],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"303111","messageId":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","threadId":"44205","inReplyTo":null,"subject":"[PATCH 0/18] alternate object database cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:33:22Z","receivedAt":"2016-10-03T20:33:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series is the result of René nerd-sniping me with the claim that we\ncould \"easily\" teach count-objects to print out the list of alternates\nin:\n\n  http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/\n\nMy real goal is just patch 17, which is needed for the quarantine series\nin that thread. But along the way there were quite a few opportunities\nfor cleanups along with a few minor bugfixes (in patches 7 and 18), and\nI think the count-objects change in patch 16 is a nice general debugging\ntool.\n\nThe rest of it is \"just\" cleanup, but I'll note that it clears up some\nhairy allocation code. These were bits that I noticed in my big\nallocation-cleanup series last year, but were too nasty to fit any of\nthe more general fixes. I think the end result is much better.\n\nThe number of patches is a little intimidating, but I tried hard to\nbreak the refactoring down into a sequence of obviously-correct steps.\nYou can be the judge of my success.\n\n  [01/18]: t5613: drop reachable_via function\n  [02/18]: t5613: drop test_valid_repo function\n  [03/18]: t5613: use test_must_fail\n  [04/18]: t5613: whitespace/style cleanups\n  [05/18]: t5613: do not chdir in main process\n  [06/18]: t5613: clarify \"too deep\" recursion tests\n  [07/18]: link_alt_odb_entry: handle normalize_path errors\n  [08/18]: link_alt_odb_entry: refactor string handling\n  [09/18]: alternates: provide helper for adding to alternates list\n  [10/18]: alternates: provide helper for allocating alternate\n  [11/18]: alternates: encapsulate alt->base munging\n  [12/18]: alternates: use a separate scratch space\n  [13/18]: fill_sha1_file: write \"boring\" characters\n  [14/18]: alternates: store scratch buffer as strbuf\n  [15/18]: fill_sha1_file: write into a strbuf\n  [16/18]: count-objects: report alternates via verbose mode\n  [17/18]: sha1_file: always allow relative paths to alternates\n  [18/18]: alternates: use fspathcmp to detect duplicates\n\n Documentation/git-count-objects.txt |   5 +\n builtin/count-objects.c             |  12 +++\n builtin/fsck.c                      |  10 +-\n builtin/submodule--helper.c         |  11 +-\n cache.h                             |  36 ++++++-\n sha1_file.c                         | 179 ++++++++++++++++++--------------\n sha1_name.c                         |  17 +--\n strbuf.c                            |  20 ++++\n strbuf.h                            |   8 ++\n submodule.c                         |  23 +---\n t/t5613-info-alternate.sh           | 202 ++++++++++++++++++++----------------\n transport.c                         |   4 +-\n 12 files changed, 305 insertions(+), 222 deletions(-)\n\n"},{"id":"303112","messageId":"20161003203350.3u6ddr6ndr3jwr74@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 01/18] t5613: drop reachable_via function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:33:51Z","receivedAt":"2016-10-03T20:33:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function was never used since its inception in dd05ea1\n(test case for transitive info/alternates, 2006-05-07).\nWhich is just as well, since it mutates the repo state in a\nway that would invalidate further tests, without cleaning up\nafter itself. Let's get rid of it so that nobody is tempted\nto use it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 9cd2626..e13f57d 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -6,16 +6,6 @@\n test_description='test transitive info/alternate entries'\n . ./test-lib.sh\n \n-# test that a file is not reachable in the current repository\n-# but that it is after creating a info/alternate entry\n-reachable_via() {\n-\talternate=\"$1\"\n-\tfile=\"$2\"\n-\tif git cat-file -e \"HEAD:$file\"; then return 1; fi\n-\techo \"$alternate\" >> .git/objects/info/alternate\n-\tgit cat-file -e \"HEAD:$file\"\n-}\n-\n test_valid_repo() {\n \tgit fsck --full > fsck.log &&\n \ttest_line_count = 0 fsck.log\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303113","messageId":"20161003203357.3cpeg2jyalzykm65@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 02/18] t5613: drop test_valid_repo function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:33:58Z","receivedAt":"2016-10-03T20:34:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function makes sure that \"git fsck\" does not report any\nerrors. But \"--full\" has been the default since f29cd39\n(fsck: default to \"git fsck --full\", 2009-10-20), and we can\nuse the exit code (instead of counting the lines) since\ne2b4f63 (fsck: exit with non-zero status upon errors,\n2007-03-05).\n\nSo we can just use \"git fsck\", which is shorter and more\nflexible (e.g., we can use \"git -C\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex e13f57d..4548fb0 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -6,11 +6,6 @@\n test_description='test transitive info/alternate entries'\n . ./test-lib.sh\n \n-test_valid_repo() {\n-\tgit fsck --full > fsck.log &&\n-\ttest_line_count = 0 fsck.log\n-}\n-\n base_dir=$(pwd)\n \n test_expect_success 'preparing first repository' \\\n@@ -52,7 +47,7 @@ git clone --bare -l -s G H'\n \n test_expect_success 'invalidity of deepest repository' \\\n 'cd H && {\n-\ttest_valid_repo\n+\tgit fsck\n \ttest $? -ne 0\n }'\n \n@@ -60,41 +55,41 @@ cd \"$base_dir\"\n \n test_expect_success 'validity of third repository' \\\n 'cd C &&\n-test_valid_repo'\n+git fsck'\n \n cd \"$base_dir\"\n \n test_expect_success 'validity of fourth repository' \\\n 'cd D &&\n-test_valid_repo'\n+git fsck'\n \n cd \"$base_dir\"\n \n test_expect_success 'breaking of loops' \\\n 'echo \"$base_dir\"/B/.git/objects >> \"$base_dir\"/A/.git/objects/info/alternates&&\n cd C &&\n-test_valid_repo'\n+git fsck'\n \n cd \"$base_dir\"\n \n test_expect_success 'that info/alternates is necessary' \\\n 'cd C &&\n rm -f .git/objects/info/alternates &&\n-! (test_valid_repo)'\n+! (git fsck)'\n \n cd \"$base_dir\"\n \n test_expect_success 'that relative alternate is possible for current dir' \\\n 'cd C &&\n echo \"../../../B/.git/objects\" > .git/objects/info/alternates &&\n-test_valid_repo'\n+git fsck'\n \n cd \"$base_dir\"\n \n test_expect_success \\\n     'that relative alternate is only possible for current dir' '\n     cd D &&\n-    ! (test_valid_repo)\n+    ! (git fsck)\n '\n \n cd \"$base_dir\"\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303114","messageId":"20161003203401.d4awnljukgqbku2n@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 03/18] t5613: use test_must_fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:01Z","receivedAt":"2016-10-03T20:34:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Besides being our normal style, this correctly checks for an\nerror exit() versus signal death.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 4548fb0..65074dd 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -46,10 +46,9 @@ git clone -l -s F G &&\n git clone --bare -l -s G H'\n \n test_expect_success 'invalidity of deepest repository' \\\n-'cd H && {\n-\tgit fsck\n-\ttest $? -ne 0\n-}'\n+'cd H &&\n+test_must_fail git fsck\n+'\n \n cd \"$base_dir\"\n \n@@ -75,7 +74,8 @@ cd \"$base_dir\"\n test_expect_success 'that info/alternates is necessary' \\\n 'cd C &&\n rm -f .git/objects/info/alternates &&\n-! (git fsck)'\n+test_must_fail git fsck\n+'\n \n cd \"$base_dir\"\n \n@@ -89,7 +89,7 @@ cd \"$base_dir\"\n test_expect_success \\\n     'that relative alternate is only possible for current dir' '\n     cd D &&\n-    ! (git fsck)\n+    test_must_fail git fsck\n '\n \n cd \"$base_dir\"\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303115","messageId":"20161003203408.qnakqgcninzty3sr@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 05/18] t5613: do not chdir in main process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:08Z","receivedAt":"2016-10-03T20:34:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our usual style when working with subdirectories is to chdir\ninside a subshell or to use \"git -C\", which means we do not\nhave to constantly return to the main test directory. Let's\nconvert this old test, which does not follow that style.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 92 +++++++++++++++++------------------------------\n 1 file changed, 33 insertions(+), 59 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 1f283a5..7bc1c3c 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -6,44 +6,39 @@\n test_description='test transitive info/alternate entries'\n . ./test-lib.sh\n \n-base_dir=$(pwd)\n-\n test_expect_success 'preparing first repository' '\n-\ttest_create_repo A &&\n-\tcd A &&\n-\techo \"Hello World\" > file1 &&\n-\tgit add file1 &&\n-\tgit commit -m \"Initial commit\" file1 &&\n-\tgit repack -a -d &&\n-\tgit prune\n+\ttest_create_repo A && (\n+\t\tcd A &&\n+\t\techo \"Hello World\" > file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"Initial commit\" file1 &&\n+\t\tgit repack -a -d &&\n+\t\tgit prune\n+\t)\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'preparing second repository' '\n-\tgit clone -l -s A B &&\n-\tcd B &&\n-\techo \"foo bar\" > file2 &&\n-\tgit add file2 &&\n-\tgit commit -m \"next commit\" file2 &&\n-\tgit repack -a -d -l &&\n-\tgit prune\n+\tgit clone -l -s A B && (\n+\t\tcd B &&\n+\t\techo \"foo bar\" > file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"next commit\" file2 &&\n+\t\tgit repack -a -d -l &&\n+\t\tgit prune\n+\t)\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'preparing third repository' '\n-\tgit clone -l -s B C &&\n-\tcd C &&\n-\techo \"Goodbye, cruel world\" > file3 &&\n-\tgit add file3 &&\n-\tgit commit -m \"one more\" file3 &&\n-\tgit repack -a -d -l &&\n-\tgit prune\n+\tgit clone -l -s B C && (\n+\t\tcd C &&\n+\t\techo \"Goodbye, cruel world\" > file3 &&\n+\t\tgit add file3 &&\n+\t\tgit commit -m \"one more\" file3 &&\n+\t\tgit repack -a -d -l &&\n+\t\tgit prune\n+\t)\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'creating too deep nesting' '\n \tgit clone -l -s C D &&\n \tgit clone -l -s D E &&\n@@ -53,55 +48,34 @@ test_expect_success 'creating too deep nesting' '\n '\n \n test_expect_success 'invalidity of deepest repository' '\n-\tcd H &&\n-\ttest_must_fail git fsck\n+\ttest_must_fail git -C H fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'validity of third repository' '\n-\tcd C &&\n-\tgit fsck\n+\tgit -C C fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'validity of fourth repository' '\n-\tcd D &&\n-\tgit fsck\n+\tgit -C D fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'breaking of loops' '\n-\techo \"$base_dir\"/B/.git/objects >>\"$base_dir\"/A/.git/objects/info/alternatesi &&\n-\tcd C &&\n-\tgit fsck\n+\techo \"$(pwd)\"/B/.git/objects >>A/.git/objects/info/alternates &&\n+\tgit -C C fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'that info/alternates is necessary' '\n-\tcd C &&\n-\trm -f .git/objects/info/alternates &&\n-\ttest_must_fail git fsck\n+\trm -f C/.git/objects/info/alternates &&\n+\ttest_must_fail git -C C fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'that relative alternate is possible for current dir' '\n-\tcd C &&\n-\techo \"../../../B/.git/objects\" > .git/objects/info/alternates &&\n+\techo \"../../../B/.git/objects\" >C/.git/objects/info/alternates &&\n \tgit fsck\n '\n \n-cd \"$base_dir\"\n-\n test_expect_success 'that relative alternate is only possible for current dir' '\n-\tcd D &&\n-\ttest_must_fail git fsck\n+\ttest_must_fail git -C D fsck\n '\n \n-cd \"$base_dir\"\n-\n test_done\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303116","messageId":"20161003203405.nzijl552nlqg63ab@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 04/18] t5613: whitespace/style cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:05Z","receivedAt":"2016-10-03T20:34:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our normal test style these days puts the opening quote of\nthe body on the description line, and indents the body with\na single tab. This ancient test did not follow this.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 114 +++++++++++++++++++++++++---------------------\n 1 file changed, 62 insertions(+), 52 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 65074dd..1f283a5 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -8,88 +8,98 @@ test_description='test transitive info/alternate entries'\n \n base_dir=$(pwd)\n \n-test_expect_success 'preparing first repository' \\\n-'test_create_repo A && cd A &&\n-echo \"Hello World\" > file1 &&\n-git add file1 &&\n-git commit -m \"Initial commit\" file1 &&\n-git repack -a -d &&\n-git prune'\n+test_expect_success 'preparing first repository' '\n+\ttest_create_repo A &&\n+\tcd A &&\n+\techo \"Hello World\" > file1 &&\n+\tgit add file1 &&\n+\tgit commit -m \"Initial commit\" file1 &&\n+\tgit repack -a -d &&\n+\tgit prune\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'preparing second repository' \\\n-'git clone -l -s A B && cd B &&\n-echo \"foo bar\" > file2 &&\n-git add file2 &&\n-git commit -m \"next commit\" file2 &&\n-git repack -a -d -l &&\n-git prune'\n+test_expect_success 'preparing second repository' '\n+\tgit clone -l -s A B &&\n+\tcd B &&\n+\techo \"foo bar\" > file2 &&\n+\tgit add file2 &&\n+\tgit commit -m \"next commit\" file2 &&\n+\tgit repack -a -d -l &&\n+\tgit prune\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'preparing third repository' \\\n-'git clone -l -s B C && cd C &&\n-echo \"Goodbye, cruel world\" > file3 &&\n-git add file3 &&\n-git commit -m \"one more\" file3 &&\n-git repack -a -d -l &&\n-git prune'\n+test_expect_success 'preparing third repository' '\n+\tgit clone -l -s B C &&\n+\tcd C &&\n+\techo \"Goodbye, cruel world\" > file3 &&\n+\tgit add file3 &&\n+\tgit commit -m \"one more\" file3 &&\n+\tgit repack -a -d -l &&\n+\tgit prune\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'creating too deep nesting' \\\n-'git clone -l -s C D &&\n-git clone -l -s D E &&\n-git clone -l -s E F &&\n-git clone -l -s F G &&\n-git clone --bare -l -s G H'\n+test_expect_success 'creating too deep nesting' '\n+\tgit clone -l -s C D &&\n+\tgit clone -l -s D E &&\n+\tgit clone -l -s E F &&\n+\tgit clone -l -s F G &&\n+\tgit clone --bare -l -s G H\n+'\n \n-test_expect_success 'invalidity of deepest repository' \\\n-'cd H &&\n-test_must_fail git fsck\n+test_expect_success 'invalidity of deepest repository' '\n+\tcd H &&\n+\ttest_must_fail git fsck\n '\n \n cd \"$base_dir\"\n \n-test_expect_success 'validity of third repository' \\\n-'cd C &&\n-git fsck'\n+test_expect_success 'validity of third repository' '\n+\tcd C &&\n+\tgit fsck\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'validity of fourth repository' \\\n-'cd D &&\n-git fsck'\n+test_expect_success 'validity of fourth repository' '\n+\tcd D &&\n+\tgit fsck\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'breaking of loops' \\\n-'echo \"$base_dir\"/B/.git/objects >> \"$base_dir\"/A/.git/objects/info/alternates&&\n-cd C &&\n-git fsck'\n+test_expect_success 'breaking of loops' '\n+\techo \"$base_dir\"/B/.git/objects >>\"$base_dir\"/A/.git/objects/info/alternatesi &&\n+\tcd C &&\n+\tgit fsck\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success 'that info/alternates is necessary' \\\n-'cd C &&\n-rm -f .git/objects/info/alternates &&\n-test_must_fail git fsck\n+test_expect_success 'that info/alternates is necessary' '\n+\tcd C &&\n+\trm -f .git/objects/info/alternates &&\n+\ttest_must_fail git fsck\n '\n \n cd \"$base_dir\"\n \n-test_expect_success 'that relative alternate is possible for current dir' \\\n-'cd C &&\n-echo \"../../../B/.git/objects\" > .git/objects/info/alternates &&\n-git fsck'\n+test_expect_success 'that relative alternate is possible for current dir' '\n+\tcd C &&\n+\techo \"../../../B/.git/objects\" > .git/objects/info/alternates &&\n+\tgit fsck\n+'\n \n cd \"$base_dir\"\n \n-test_expect_success \\\n-    'that relative alternate is only possible for current dir' '\n-    cd D &&\n-    test_must_fail git fsck\n+test_expect_success 'that relative alternate is only possible for current dir' '\n+\tcd D &&\n+\ttest_must_fail git fsck\n '\n \n cd \"$base_dir\"\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303117","messageId":"20161003203412.bekizvlqtg4ls5fb@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:12Z","receivedAt":"2016-10-03T20:34:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These tests are just trying to show that we allow recursion\nup to a certain depth, but not past it. But the counting is\na bit non-intuitive, and rather than test at the edge of the\nbreakage, we test \"OK\" cases in the middle of the chain.\nLet's explain what's going on, and explicitly test the\nswitch between \"OK\" and \"too deep\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5613-info-alternate.sh | 24 ++++++++++++++++--------\n 1 file changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 7bc1c3c..b393613 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n \t)\n '\n \n+# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n+# the depth at 0 and count links, not repositories, so in a chain like:\n+#\n+#   A -> B -> C -> D -> E -> F -> G -> H\n+#      0    1    2    3    4    5    6\n+#\n+# we are OK at \"G\", but break at \"H\".\n+#\n+# Note also that we must use \"--bare -l\" to make the link to H. The \"-l\"\n+# ensures we do not do a connectivity check, and the \"--bare\" makes sure\n+# we do not try to checkout the result (which needs objects), either of\n+# which would cause the clone to fail.\n test_expect_success 'creating too deep nesting' '\n \tgit clone -l -s C D &&\n \tgit clone -l -s D E &&\n@@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '\n \tgit clone --bare -l -s G H\n '\n \n-test_expect_success 'invalidity of deepest repository' '\n-\ttest_must_fail git -C H fsck\n-'\n-\n-test_expect_success 'validity of third repository' '\n-\tgit -C C fsck\n+test_expect_success 'validity of fifth-deep repository' '\n+\tgit -C G fsck\n '\n \n-test_expect_success 'validity of fourth repository' '\n-\tgit -C D fsck\n+test_expect_success 'invalidity of sixth-deep repository' '\n+\ttest_must_fail git -C H fsck\n '\n \n test_expect_success 'breaking of loops' '\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303118","messageId":"20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:17Z","receivedAt":"2016-10-03T20:34:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we add a new alternate to the list, we try to normalize\nout any redundant \"..\", etc. However, we do not look at the\nreturn value of normalize_path_copy(), and will happily\ncontinue with a path that could not be normalized. Worse,\nthe normalizing process is done in-place, so we are left\nwith whatever half-finished working state the normalizing\nfunction was in.\n\nFortunately, this cannot cause us to read past the end of\nour buffer, as that working state will always leave the\nNUL from the original path in place. And we do tend to\nnotice problems when we check is_directory() on the path.\nBut you can see the nonsense that we feed to is_directory\nwith an entry like:\n\n  this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n\nin your objects/info/alternates, which yields:\n\n  error: object directory\n  /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n  does not exist; check .git/objects/info/alternates.\n\nWe can easily fix this just by checking the return value.\nBut that makes it hard to generate a good error message,\nsince we're normalizing in-place and our input value has\nbeen overwritten by cruft.\n\nInstead, let's provide a strbuf helper that does an in-place\nnormalize, but restores the original contents on error. This\nuses a second buffer under the hood, which is slightly less\nefficient, but this is not a performance-critical code path.\n\nThe strbuf helper can also properly set the \"len\" parameter\nof the strbuf before returning. Just doing:\n\n  normalize_path_copy(buf.buf, buf.buf);\n\nwill shorten the string, but leave buf.len at the original\nlength. That may be confusing to later code which uses the\nstrbuf.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 11 +++++++++--\n strbuf.c    | 20 ++++++++++++++++++++\n strbuf.h    |  8 ++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..68571bd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \t}\n \tstrbuf_addstr(&pathbuf, entry);\n \n-\tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n+\tif (strbuf_normalize_path(&pathbuf) < 0) {\n+\t\terror(\"unable to normalize alternate object path: %s\",\n+\t\t      pathbuf.buf);\n+\t\tstrbuf_release(&pathbuf);\n+\t\treturn -1;\n+\t}\n \n \tpfxlen = strlen(pathbuf.buf);\n \n@@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t}\n \n \tstrbuf_add_absolute_path(&objdirbuf, get_object_directory());\n-\tnormalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n+\tif (strbuf_normalize_path(&objdirbuf) < 0)\n+\t\tdie(\"unable to normalize object directory: %s\",\n+\t\t    objdirbuf.buf);\n \n \talt_copy = xmemdupz(alt, len);\n \tstring_list_split_in_place(&entries, alt_copy, sep, -1);\ndiff --git a/strbuf.c b/strbuf.c\nindex b839be4..8fec657 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n \n \tstrbuf_setlen(sb, j);\n }\n+\n+int strbuf_normalize_path(struct strbuf *src)\n+{\n+\tstruct strbuf dst = STRBUF_INIT;\n+\n+\tstrbuf_grow(&dst, src->len);\n+\tif (normalize_path_copy(dst.buf, src->buf) < 0) {\n+\t\tstrbuf_release(&dst);\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * normalize_path does not tell us the new length, so we have to\n+\t * compute it by looking for the new NUL it placed\n+\t */\n+\tstrbuf_setlen(&dst, strlen(dst.buf));\n+\tstrbuf_swap(src, &dst);\n+\tstrbuf_release(&dst);\n+\treturn 0;\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex ba8d5f1..2262b12 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);\n  */\n extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n+\n+/**\n+ * Normalize in-place the path contained in the strbuf. See\n+ * normalize_path_copy() for details. If an error occurs, the contents of \"sb\"\n+ * are left untouched, and -1 is returned.\n+ */\n+extern int strbuf_normalize_path(struct strbuf *sb);\n+\n /**\n  * Strip whitespace from a buffer. The second parameter controls if\n  * comments are considered contents to be removed or not.\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303119","messageId":"20161003203448.cdfbitl5jmhlpb5o@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 08/18] link_alt_odb_entry: refactor string handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:34:48Z","receivedAt":"2016-10-03T20:34:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The string handling in link_alt_odb_entry() is mostly an\nartifact of the original version, which took the path as a\nptr/len combo, and did not have a NUL-terminated string\nuntil we created one in the alternate_object_database\nstruct.  But since 5bdf0a8 (sha1_file: normalize alt_odb\npath before comparing and storing, 2011-09-07), the first\nthing we do is put the path into a strbuf, which gives us\nsome easy opportunities for cleanup.\n\nIn particular:\n\n  - we call strlen(pathbuf.buf), which is silly; we can look\n    at pathbuf.len.\n\n  - even though we have a strbuf, we don't maintain its\n    \"len\" field when chomping extra slashes from the\n    end, and instead keep a separate \"pfxlen\" variable. We\n    can fix this and then drop \"pfxlen\" entirely.\n\n  - we don't check whether the path is usable until after we\n    allocate the new struct, making extra cleanup work for\n    ourselves. Since we have a NUL-terminated string, we can\n    bump the \"is it usable\" checks higher in the function.\n    While we're at it, we can move that logic to its own\n    helper, which makes the flow of link_alt_odb_entry()\n    easier to follow.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnd you can probably guess now how I found the issue in the last patch\nwhere pathbuf.len is totally bogus after calling normalize_path_copy. :)\n\n sha1_file.c | 83 +++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 45 insertions(+), 38 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 68571bd..f396823 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -234,6 +234,36 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n struct alternate_object_database *alt_odb_list;\n static struct alternate_object_database **alt_odb_tail;\n \n+/*\n+ * Return non-zero iff the path is usable as an alternate object database.\n+ */\n+static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)\n+{\n+\tstruct alternate_object_database *alt;\n+\n+\t/* Detect cases where alternate disappeared */\n+\tif (!is_directory(path->buf)) {\n+\t\terror(\"object directory %s does not exist; \"\n+\t\t      \"check .git/objects/info/alternates.\",\n+\t\t      path->buf);\n+\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Prevent the common mistake of listing the same\n+\t * thing twice, or object directory itself.\n+\t */\n+\tfor (alt = alt_odb_list; alt; alt = alt->next) {\n+\t\tif (path->len == alt->name - alt->base - 1 &&\n+\t\t    !memcmp(path->buf, alt->base, path->len))\n+\t\t\treturn 0;\n+\t}\n+\tif (!fspathcmp(path->buf, normalized_objdir))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n /*\n  * Prepare alternate object database registry.\n  *\n@@ -253,8 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \tint depth, const char *normalized_objdir)\n {\n \tstruct alternate_object_database *ent;\n-\tstruct alternate_object_database *alt;\n-\tsize_t pfxlen, entlen;\n+\tsize_t entlen;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (!is_absolute_path(entry) && relative_base) {\n@@ -270,47 +299,26 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \t\treturn -1;\n \t}\n \n-\tpfxlen = strlen(pathbuf.buf);\n-\n \t/*\n \t * The trailing slash after the directory name is given by\n \t * this function at the end. Remove duplicates.\n \t */\n-\twhile (pfxlen && pathbuf.buf[pfxlen-1] == '/')\n-\t\tpfxlen -= 1;\n-\n-\tentlen = st_add(pfxlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n-\tent = xmalloc(st_add(sizeof(*ent), entlen));\n-\tmemcpy(ent->base, pathbuf.buf, pfxlen);\n-\tstrbuf_release(&pathbuf);\n-\n-\tent->name = ent->base + pfxlen + 1;\n-\tent->base[pfxlen + 3] = '/';\n-\tent->base[pfxlen] = ent->base[entlen-1] = 0;\n+\twhile (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n+\t\tstrbuf_setlen(&pathbuf, pathbuf.len - 1);\n \n-\t/* Detect cases where alternate disappeared */\n-\tif (!is_directory(ent->base)) {\n-\t\terror(\"object directory %s does not exist; \"\n-\t\t      \"check .git/objects/info/alternates.\",\n-\t\t      ent->base);\n-\t\tfree(ent);\n+\tif (!alt_odb_usable(&pathbuf, normalized_objdir)) {\n+\t\tstrbuf_release(&pathbuf);\n \t\treturn -1;\n \t}\n \n-\t/* Prevent the common mistake of listing the same\n-\t * thing twice, or object directory itself.\n-\t */\n-\tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tif (pfxlen == alt->name - alt->base - 1 &&\n-\t\t    !memcmp(ent->base, alt->base, pfxlen)) {\n-\t\t\tfree(ent);\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n-\tif (!fspathcmp(ent->base, normalized_objdir)) {\n-\t\tfree(ent);\n-\t\treturn -1;\n-\t}\n+\tentlen = st_add(pathbuf.len, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n+\tent = xmalloc(st_add(sizeof(*ent), entlen));\n+\tmemcpy(ent->base, pathbuf.buf, pathbuf.len);\n+\n+\tent->name = ent->base + pathbuf.len + 1;\n+\tent->base[pathbuf.len] = '/';\n+\tent->base[pathbuf.len + 3] = '/';\n+\tent->base[entlen-1] = 0;\n \n \t/* add the alternate entry */\n \t*alt_odb_tail = ent;\n@@ -318,10 +326,9 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \tent->next = NULL;\n \n \t/* recursively add alternates */\n-\tread_info_alternates(ent->base, depth + 1);\n-\n-\tent->base[pfxlen] = '/';\n+\tread_info_alternates(pathbuf.buf, depth + 1);\n \n+\tstrbuf_release(&pathbuf);\n \treturn 0;\n }\n \n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303120","messageId":"20161003203503.omjwvg4ocz7pjyzt@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 09/18] alternates: provide helper for adding to alternates list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:35:03Z","receivedAt":"2016-10-03T20:35:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The submodule code wants to temporarily add an alternate\nobject store to our in-memory alt_odb list, but does it\nmanually. Let's provide a helper so it can reuse the code in\nlink_alt_odb_entry().\n\nWhile we're adding our new add_to_alternates_memory(), let's\ndocument add_to_alternates_file(), as the two are related.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     | 14 +++++++++++++-\n sha1_file.c | 11 +++++++++++\n submodule.c | 23 +----------------------\n 3 files changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ed3d5df..9a91378 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1388,10 +1388,22 @@ extern struct alternate_object_database {\n extern void prepare_alt_odb(void);\n extern void read_info_alternates(const char * relative_base, int depth);\n extern char *compute_alternate_path(const char *path, struct strbuf *err);\n-extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern int foreach_alt_odb(alt_odb_fn, void*);\n \n+/*\n+ * Add the directory to the on-disk alternates file; the new entry will also\n+ * take effect in the current process.\n+ */\n+extern void add_to_alternates_file(const char *dir);\n+\n+/*\n+ * Add the directory to the in-memory list of alternates (along with any\n+ * recursive alternates it points to), but do not modify the on-disk alternates\n+ * file.\n+ */\n+extern void add_to_alternates_memory(const char *dir);\n+\n struct pack_window {\n \tstruct pack_window *next;\n \tunsigned char *base;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f396823..2e41b26 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -440,6 +440,17 @@ void add_to_alternates_file(const char *reference)\n \tfree(alts);\n }\n \n+void add_to_alternates_memory(const char *reference)\n+{\n+\t/*\n+\t * Make sure alternates are initialized, or else our entry may be\n+\t * overwritten when they are.\n+\t */\n+\tprepare_alt_odb();\n+\n+\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+}\n+\n /*\n  * Compute the exact path an alternate is at and returns it. In case of\n  * error NULL is returned and the human readable error is added to `err`\ndiff --git a/submodule.c b/submodule.c\nindex 0ef2ff4..8b3274a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -123,9 +123,7 @@ void stage_updated_gitmodules(void)\n static int add_submodule_odb(const char *path)\n {\n \tstruct strbuf objects_directory = STRBUF_INIT;\n-\tstruct alternate_object_database *alt_odb;\n \tint ret = 0;\n-\tsize_t alloc;\n \n \tret = strbuf_git_path_submodule(&objects_directory, path, \"objects/\");\n \tif (ret)\n@@ -134,26 +132,7 @@ static int add_submodule_odb(const char *path)\n \t\tret = -1;\n \t\tgoto done;\n \t}\n-\t/* avoid adding it twice */\n-\tprepare_alt_odb();\n-\tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n-\t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n-\t\t\t\t!strncmp(alt_odb->base, objects_directory.buf,\n-\t\t\t\t\tobjects_directory.len))\n-\t\t\tgoto done;\n-\n-\talloc = st_add(objects_directory.len, 42); /* for \"12/345...\" sha1 */\n-\talt_odb = xmalloc(st_add(sizeof(*alt_odb), alloc));\n-\talt_odb->next = alt_odb_list;\n-\txsnprintf(alt_odb->base, alloc, \"%s\", objects_directory.buf);\n-\talt_odb->name = alt_odb->base + objects_directory.len;\n-\talt_odb->name[2] = '/';\n-\talt_odb->name[40] = '\\0';\n-\talt_odb->name[41] = '\\0';\n-\talt_odb_list = alt_odb;\n-\n-\t/* add possible alternates from the submodule */\n-\tread_info_alternates(objects_directory.buf, 0);\n+\tadd_to_alternates_memory(objects_directory.buf);\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303121","messageId":"20161003203531.bppczvzmdfumtnb2@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 10/18] alternates: provide helper for allocating alternate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:35:31Z","receivedAt":"2016-10-03T20:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Allocating a struct alternate_object_database is tricky, as\nwe must over-allocate the buffer to provide scratch space,\nand then put in particular '/' and NUL markers.\n\nLet's encapsulate this in a function so that the complexity\ndoesn't leak into callers (and so that we can modify it\nlater).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     |  6 ++++++\n sha1_file.c | 28 +++++++++++++++++++---------\n sha1_name.c |  7 +------\n 3 files changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9a91378..d36b2ad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1391,6 +1391,12 @@ extern char *compute_alternate_path(const char *path, struct strbuf *err);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern int foreach_alt_odb(alt_odb_fn, void*);\n \n+/*\n+ * Allocate a \"struct alternate_object_database\" but do _not_ actually\n+ * add it to the list of alternates.\n+ */\n+struct alternate_object_database *alloc_alt_odb(const char *dir);\n+\n /*\n  * Add the directory to the on-disk alternates file; the new entry will also\n  * take effect in the current process.\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2e41b26..549cf1e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -283,7 +283,6 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \tint depth, const char *normalized_objdir)\n {\n \tstruct alternate_object_database *ent;\n-\tsize_t entlen;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (!is_absolute_path(entry) && relative_base) {\n@@ -311,14 +310,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \t\treturn -1;\n \t}\n \n-\tentlen = st_add(pathbuf.len, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n-\tent = xmalloc(st_add(sizeof(*ent), entlen));\n-\tmemcpy(ent->base, pathbuf.buf, pathbuf.len);\n-\n-\tent->name = ent->base + pathbuf.len + 1;\n-\tent->base[pathbuf.len] = '/';\n-\tent->base[pathbuf.len + 3] = '/';\n-\tent->base[entlen-1] = 0;\n+\tent = alloc_alt_odb(pathbuf.buf);\n \n \t/* add the alternate entry */\n \t*alt_odb_tail = ent;\n@@ -395,6 +387,24 @@ void read_info_alternates(const char * relative_base, int depth)\n \tmunmap(map, mapsz);\n }\n \n+struct alternate_object_database *alloc_alt_odb(const char *dir)\n+{\n+\tstruct alternate_object_database *ent;\n+\tsize_t dirlen = strlen(dir);\n+\tsize_t entlen;\n+\n+\tentlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n+\tent = xmalloc(st_add(sizeof(*ent), entlen));\n+\tmemcpy(ent->base, dir, dirlen);\n+\n+\tent->name = ent->base + dirlen + 1;\n+\tent->base[dirlen] = '/';\n+\tent->base[dirlen + 3] = '/';\n+\tent->base[entlen-1] = 0;\n+\n+\treturn ent;\n+}\n+\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\ndiff --git a/sha1_name.c b/sha1_name.c\nindex faf873c..98152a6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -86,12 +86,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa\n \t\t * alt->name/alt->base while iterating over the\n \t\t * object databases including our own.\n \t\t */\n-\t\tconst char *objdir = get_object_directory();\n-\t\tsize_t objdir_len = strlen(objdir);\n-\t\tfakeent = xmalloc(st_add3(sizeof(*fakeent), objdir_len, 43));\n-\t\tmemcpy(fakeent->base, objdir, objdir_len);\n-\t\tfakeent->name = fakeent->base + objdir_len + 1;\n-\t\tfakeent->name[-1] = '/';\n+\t\tfakeent = alloc_alt_odb(get_object_directory());\n \t}\n \tfakeent->next = alt_odb_list;\n \n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303122","messageId":"20161003203543.qkylezpwuruwpjsa@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 11/18] alternates: encapsulate alt->base munging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:35:43Z","receivedAt":"2016-10-03T20:35:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The alternate_object_database struct holds a path to the\nalternate objects, but we also use that buffer as scratch\nspace for forming loose object filenames. Let's pull that\nlogic into a helper function so that we can more easily\nmodify it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 549cf1e..ccf59ba 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -204,6 +204,13 @@ const char *sha1_file_name(const unsigned char *sha1)\n \treturn buf;\n }\n \n+static const char *alt_sha1_path(struct alternate_object_database *alt,\n+\t\t\t\t const unsigned char *sha1)\n+{\n+\tfill_sha1_path(alt->name, sha1);\n+\treturn alt->base;\n+}\n+\n /*\n  * Return the name of the pack or index file with the specified sha1\n  * in its filename.  *base and *name are scratch space that must be\n@@ -601,8 +608,8 @@ static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n \tstruct alternate_object_database *alt;\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tfill_sha1_path(alt->name, sha1);\n-\t\tif (check_and_freshen_file(alt->base, freshen))\n+\t\tconst char *path = alt_sha1_path(alt, sha1);\n+\t\tif (check_and_freshen_file(path, freshen))\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -1600,8 +1607,8 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n \tprepare_alt_odb();\n \terrno = ENOENT;\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tfill_sha1_path(alt->name, sha1);\n-\t\tif (!lstat(alt->base, st))\n+\t\tconst char *path = alt_sha1_path(alt, sha1);\n+\t\tif (!lstat(path, st))\n \t\t\treturn 0;\n \t}\n \n@@ -1621,8 +1628,8 @@ static int open_sha1_file(const unsigned char *sha1)\n \n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tfill_sha1_path(alt->name, sha1);\n-\t\tfd = git_open_noatime(alt->base);\n+\t\tconst char *path = alt_sha1_path(alt, sha1);\n+\t\tfd = git_open_noatime(path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303123","messageId":"20161003203551.tmqp5rll6nqkewxz@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 12/18] alternates: use a separate scratch space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:35:51Z","receivedAt":"2016-10-03T20:35:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The alternate_object_database struct uses a single buffer\nboth for storing the path to the alternate, and as a scratch\nbuffer for forming object names. This is efficient (since\notherwise we'd end up storing the path twice), but it makes\nlife hard for callers who just want to know the path to the\nalternate. They have to remember to stop reading after\n\"alt->name - alt->base\" bytes, and to subtract one for the\ntrailing '/'.\n\nIt would be much simpler if they could simply access a\nNUL-terminated path string. We could encapsulate this in a\nfunction which puts a NUL in the scratch buffer and returns\nthe string, but that opens up questions about the lifetime\nof the result. The first time another caller uses the\nalternate, the scratch buffer may get other data tacked onto\nit.\n\nLet's instead just store the root path separately from the\nscratch buffer. There aren't enough alternates being stored\nfor the duplicated data to matter for performance, and this\nkeeps things simple and safe for the callers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c              | 10 ++--------\n builtin/submodule--helper.c | 11 +++--------\n cache.h                     |  5 ++++-\n sha1_file.c                 | 28 ++++++++++++----------------\n sha1_name.c                 |  3 ++-\n transport.c                 |  4 +---\n 6 files changed, 24 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 055dfdc..f01b81e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -644,14 +644,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_object_dir(get_object_directory());\n \n \t\tprepare_alt_odb();\n-\t\tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\t\t/* directory name, minus trailing slash */\n-\t\t\tsize_t namelen = alt->name - alt->base - 1;\n-\t\t\tstruct strbuf name = STRBUF_INIT;\n-\t\t\tstrbuf_add(&name, alt->base, namelen);\n-\t\t\tfsck_object_dir(name.buf);\n-\t\t\tstrbuf_release(&name);\n-\t\t}\n+\t\tfor (alt = alt_odb_list; alt; alt = alt->next)\n+\t\t\tfsck_object_dir(alt->path);\n \t}\n \n \tif (check_full) {\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e3fdc0a..fd72c90 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -492,20 +492,16 @@ static int add_possible_reference_from_superproject(\n {\n \tstruct submodule_alternate_setup *sas = sas_cb;\n \n-\t/* directory name, minus trailing slash */\n-\tsize_t namelen = alt->name - alt->base - 1;\n-\tstruct strbuf name = STRBUF_INIT;\n-\tstrbuf_add(&name, alt->base, namelen);\n-\n \t/*\n \t * If the alternate object store is another repository, try the\n \t * standard layout with .git/modules/<name>/objects\n \t */\n-\tif (ends_with(name.buf, \".git/objects\")) {\n+\tif (ends_with(alt->path, \".git/objects\")) {\n \t\tchar *sm_alternate;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstrbuf_add(&sb, name.buf, name.len - strlen(\"objects\"));\n+\t\tstrbuf_add(&sb, alt->path, strlen(alt->path) - strlen(\"objects\"));\n+\n \t\t/*\n \t\t * We need to end the new path with '/' to mark it as a dir,\n \t\t * otherwise a submodule name containing '/' will be broken\n@@ -533,7 +529,6 @@ static int add_possible_reference_from_superproject(\n \t\tstrbuf_release(&sb);\n \t}\n \n-\tstrbuf_release(&name);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex d36b2ad..e1996c5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1382,8 +1382,11 @@ extern void remove_scheduled_dirs(void);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\n+\n \tchar *name;\n-\tchar base[FLEX_ARRAY]; /* more */\n+\tchar *scratch;\n+\n+\tchar path[FLEX_ARRAY];\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n extern void read_info_alternates(const char * relative_base, int depth);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ccf59ba..70c3e2f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -208,7 +208,7 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,\n \t\t\t\t const unsigned char *sha1)\n {\n \tfill_sha1_path(alt->name, sha1);\n-\treturn alt->base;\n+\treturn alt->scratch;\n }\n \n /*\n@@ -261,8 +261,7 @@ static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)\n \t * thing twice, or object directory itself.\n \t */\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tif (path->len == alt->name - alt->base - 1 &&\n-\t\t    !memcmp(path->buf, alt->base, path->len))\n+\t\tif (!strcmp(path->buf, alt->path))\n \t\t\treturn 0;\n \t}\n \tif (!fspathcmp(path->buf, normalized_objdir))\n@@ -401,13 +400,14 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n \tsize_t entlen;\n \n \tentlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n-\tent = xmalloc(st_add(sizeof(*ent), entlen));\n-\tmemcpy(ent->base, dir, dirlen);\n+\tFLEX_ALLOC_STR(ent, path, dir);\n+\tent->scratch = xmalloc(entlen);\n+\txsnprintf(ent->scratch, entlen, \"%s/\", dir);\n \n-\tent->name = ent->base + dirlen + 1;\n-\tent->base[dirlen] = '/';\n-\tent->base[dirlen + 3] = '/';\n-\tent->base[entlen-1] = 0;\n+\tent->name = ent->scratch + dirlen + 1;\n+\tent->scratch[dirlen] = '/';\n+\tent->scratch[dirlen + 3] = '/';\n+\tent->scratch[entlen-1] = 0;\n \n \treturn ent;\n }\n@@ -1485,11 +1485,8 @@ void prepare_packed_git(void)\n \t\treturn;\n \tprepare_packed_git_one(get_object_directory(), 1);\n \tprepare_alt_odb();\n-\tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\talt->name[-1] = 0;\n-\t\tprepare_packed_git_one(alt->base, 0);\n-\t\talt->name[-1] = '/';\n-\t}\n+\tfor (alt = alt_odb_list; alt; alt = alt->next)\n+\t\tprepare_packed_git_one(alt->path, 0);\n \trearrange_packed_git();\n \tprepare_packed_git_mru();\n \tprepare_packed_git_run_once = 1;\n@@ -3670,8 +3667,7 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,\n \tstruct strbuf buf = STRBUF_INIT;\n \tint r;\n \n-\t/* copy base not including trailing '/' */\n-\tstrbuf_add(&buf, alt->base, alt->name - alt->base - 1);\n+\tstrbuf_addstr(&buf, alt->path);\n \tr = for_each_loose_file_in_objdir_buf(&buf,\n \t\t\t\t\t      data->cb, NULL, NULL,\n \t\t\t\t\t      data->data);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 98152a6..770ea4f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -94,12 +94,13 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa\n \tfor (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {\n \t\tstruct dirent *de;\n \t\tDIR *dir;\n+\n \t\t/*\n \t\t * every alt_odb struct has 42 extra bytes after the base\n \t\t * for exactly this purpose\n \t\t */\n \t\txsnprintf(alt->name, 42, \"%.2s/\", hex_pfx);\n-\t\tdir = opendir(alt->base);\n+\t\tdir = opendir(alt->scratch);\n \t\tif (!dir)\n \t\t\tcontinue;\n \ndiff --git a/transport.c b/transport.c\nindex 94d6dc3..4bc4eea 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1084,9 +1084,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n \tconst struct ref *extra;\n \tstruct alternate_refs_data *cb = data;\n \n-\te->name[-1] = '\\0';\n-\tother = xstrdup(real_path(e->base));\n-\te->name[-1] = '/';\n+\tother = xstrdup(real_path(e->path));\n \tlen = strlen(other);\n \n \twhile (other[len-1] == '/')\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303124","messageId":"20161003203555.6xadycotmmkuf34h@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:35:55Z","receivedAt":"2016-10-03T20:36:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function forms a sha1 as \"xx/yyyy...\", but skips over\nthe slot for the slash rather than writing it, leaving it to\nthe caller to do so. It also does not bother to put in a\ntrailing NUL, even though every caller would want it (we're\nforming a path which by definition is not a directory, so\nthe only thing to do with it is feed it to a system call).\n\nLet's make the lives of our callers easier by just writing\nout the internal \"/\" and the NUL.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 70c3e2f..c6308c1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)\n \tfor (i = 0; i < 20; i++) {\n \t\tstatic char hex[] = \"0123456789abcdef\";\n \t\tunsigned int val = sha1[i];\n-\t\tchar *pos = pathbuf + i*2 + (i > 0);\n-\t\t*pos++ = hex[val >> 4];\n-\t\t*pos = hex[val & 0xf];\n+\t\t*pathbuf++ = hex[val >> 4];\n+\t\t*pathbuf++ = hex[val & 0xf];\n+\t\tif (!i)\n+\t\t\t*pathbuf++ = '/';\n \t}\n+\t*pathbuf = '\\0';\n }\n \n const char *sha1_file_name(const unsigned char *sha1)\n@@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1)\n \t\tdie(\"insanely long object directory %s\", objdir);\n \tmemcpy(buf, objdir, len);\n \tbuf[len] = '/';\n-\tbuf[len+3] = '/';\n-\tbuf[len+42] = '\\0';\n \tfill_sha1_path(buf + len + 1, sha1);\n \treturn buf;\n }\n@@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n \n \tent->name = ent->scratch + dirlen + 1;\n \tent->scratch[dirlen] = '/';\n-\tent->scratch[dirlen + 3] = '/';\n-\tent->scratch[entlen-1] = 0;\n \n \treturn ent;\n }\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303125","messageId":"20161003203604.6qwvc226nkgw44d2@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 14/18] alternates: store scratch buffer as strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:36:04Z","receivedAt":"2016-10-03T20:36:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pre-size the scratch buffer to hold a loose object\nfilename of the form \"xx/yyyy...\", which leads to allocation\ncode that is hard to verify. We have to use some magic\nnumbers during the initial allocation, and then writers must\nblindly assume that the buffer is big enough. Using a strbuf\nmakes it more clear that we cannot overflow.\n\nUnfortunately, we do still need some magic numbers to grow\nour strbuf before calling fill_sha1_path(), but the strbuf\ngrowth is much closer to the point of use. This makes it\neasier to see that it's correct, and opens the possibility\nof pushing it even further down if fill_sha1_path() learns\nto work on strbufs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h     | 13 +++++++++++--\n sha1_file.c | 28 ++++++++++++++++++----------\n sha1_name.c |  9 +++------\n 3 files changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e1996c5..9866e46 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1383,8 +1383,9 @@ extern void remove_scheduled_dirs(void);\n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\n \n-\tchar *name;\n-\tchar *scratch;\n+\t/* see alt_scratch_buf() */\n+\tstruct strbuf scratch;\n+\tsize_t base_len;\n \n \tchar path[FLEX_ARRAY];\n } *alt_odb_list;\n@@ -1413,6 +1414,14 @@ extern void add_to_alternates_file(const char *dir);\n  */\n extern void add_to_alternates_memory(const char *dir);\n \n+/*\n+ * Returns a scratch strbuf pre-filled with the alternate object directory,\n+ * including a trailing slash, which can be used to access paths in the\n+ * alternate. Always use this over direct access to alt->scratch, as it\n+ * cleans up any previous use of the scratch buffer.\n+ */\n+extern struct strbuf *alt_scratch_buf(struct alternate_object_database *alt);\n+\n struct pack_window {\n \tstruct pack_window *next;\n \tunsigned char *base;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c6308c1..efc8cee 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -204,11 +204,24 @@ const char *sha1_file_name(const unsigned char *sha1)\n \treturn buf;\n }\n \n+struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n+{\n+\tstrbuf_setlen(&alt->scratch, alt->base_len);\n+\treturn &alt->scratch;\n+}\n+\n static const char *alt_sha1_path(struct alternate_object_database *alt,\n \t\t\t\t const unsigned char *sha1)\n {\n-\tfill_sha1_path(alt->name, sha1);\n-\treturn alt->scratch;\n+\t/* hex sha1 plus internal \"/\" */\n+\tsize_t len = GIT_SHA1_HEXSZ + 1;\n+\tstruct strbuf *buf = alt_scratch_buf(alt);\n+\n+\tstrbuf_grow(buf, len);\n+\tfill_sha1_path(buf->buf + buf->len, sha1);\n+\tstrbuf_setlen(buf, buf->len + len);\n+\n+\treturn buf->buf;\n }\n \n /*\n@@ -396,16 +409,11 @@ void read_info_alternates(const char * relative_base, int depth)\n struct alternate_object_database *alloc_alt_odb(const char *dir)\n {\n \tstruct alternate_object_database *ent;\n-\tsize_t dirlen = strlen(dir);\n-\tsize_t entlen;\n \n-\tentlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n \tFLEX_ALLOC_STR(ent, path, dir);\n-\tent->scratch = xmalloc(entlen);\n-\txsnprintf(ent->scratch, entlen, \"%s/\", dir);\n-\n-\tent->name = ent->scratch + dirlen + 1;\n-\tent->scratch[dirlen] = '/';\n+\tstrbuf_init(&ent->scratch, 0);\n+\tstrbuf_addf(&ent->scratch, \"%s/\", dir);\n+\tent->base_len = ent->scratch.len;\n \n \treturn ent;\n }\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 770ea4f..defbb3e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -92,15 +92,12 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa\n \n \txsnprintf(hex, sizeof(hex), \"%.2s\", hex_pfx);\n \tfor (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {\n+\t\tstruct strbuf *buf = alt_scratch_buf(alt);\n \t\tstruct dirent *de;\n \t\tDIR *dir;\n \n-\t\t/*\n-\t\t * every alt_odb struct has 42 extra bytes after the base\n-\t\t * for exactly this purpose\n-\t\t */\n-\t\txsnprintf(alt->name, 42, \"%.2s/\", hex_pfx);\n-\t\tdir = opendir(alt->scratch);\n+\t\tstrbuf_addf(buf, \"%.2s/\", hex_pfx);\n+\t\tdir = opendir(buf->buf);\n \t\tif (!dir)\n \t\t\tcontinue;\n \n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303126","messageId":"20161003203609.4hig3e24lyvswdcf@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 15/18] fill_sha1_file: write into a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:36:09Z","receivedAt":"2016-10-03T20:36:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's currently the responsibility of the caller to give\nfill_sha1_file() enough bytes to write into, leading them to\nmanually compute the required lengths. Instead, let's just\nwrite into a strbuf so that it's impossible to get this\nwrong.\n\nThe alt_odb caller already has a strbuf, so this makes\nthings strictly simpler. The other caller, sha1_file_name(),\nuses a static PATH_MAX buffer and dies when it would\noverflow. We can convert this to a static strbuf, which\nmeans our allocation cost is amortized (and as a bonus, we\nno longer have to worry about PATH_MAX being too short for\nnormal use).\n\nThis does introduce some small overhead in fill_sha1_file(),\nas each strbuf_addchar() will check whether it needs to\ngrow. However, between the optimization in fec501d\n(strbuf_addch: avoid calling strbuf_grow, 2015-04-16) and\nthe fact that this is not generally called in a tight loop\n(after all, the next step is typically to access the file!)\nthis probably doesn't matter. And even if it did, the right\nplace to micro-optimize is inside fill_sha1_file(), by\ncalling a single strbuf_grow() there.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 34 ++++++++++------------------------\n 1 file changed, 10 insertions(+), 24 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex efc8cee..80a3333 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -172,36 +172,28 @@ enum scld_error safe_create_leading_directories_const(const char *path)\n \treturn result;\n }\n \n-static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)\n+static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n {\n \tint i;\n \tfor (i = 0; i < 20; i++) {\n \t\tstatic char hex[] = \"0123456789abcdef\";\n \t\tunsigned int val = sha1[i];\n-\t\t*pathbuf++ = hex[val >> 4];\n-\t\t*pathbuf++ = hex[val & 0xf];\n+\t\tstrbuf_addch(buf, hex[val >> 4]);\n+\t\tstrbuf_addch(buf, hex[val & 0xf]);\n \t\tif (!i)\n-\t\t\t*pathbuf++ = '/';\n+\t\t\tstrbuf_addch(buf, '/');\n \t}\n-\t*pathbuf = '\\0';\n }\n \n const char *sha1_file_name(const unsigned char *sha1)\n {\n-\tstatic char buf[PATH_MAX];\n-\tconst char *objdir;\n-\tint len;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n \n-\tobjdir = get_object_directory();\n-\tlen = strlen(objdir);\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, \"%s/\", get_object_directory());\n \n-\t/* '/' + sha1(2) + '/' + sha1(38) + '\\0' */\n-\tif (len + 43 > PATH_MAX)\n-\t\tdie(\"insanely long object directory %s\", objdir);\n-\tmemcpy(buf, objdir, len);\n-\tbuf[len] = '/';\n-\tfill_sha1_path(buf + len + 1, sha1);\n-\treturn buf;\n+\tfill_sha1_path(&buf, sha1);\n+\treturn buf.buf;\n }\n \n struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n@@ -213,14 +205,8 @@ struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n static const char *alt_sha1_path(struct alternate_object_database *alt,\n \t\t\t\t const unsigned char *sha1)\n {\n-\t/* hex sha1 plus internal \"/\" */\n-\tsize_t len = GIT_SHA1_HEXSZ + 1;\n \tstruct strbuf *buf = alt_scratch_buf(alt);\n-\n-\tstrbuf_grow(buf, len);\n-\tfill_sha1_path(buf->buf + buf->len, sha1);\n-\tstrbuf_setlen(buf, buf->len + len);\n-\n+\tfill_sha1_path(buf, sha1);\n \treturn buf->buf;\n }\n \n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303127","messageId":"20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 16/18] count-objects: report alternates via verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:36:18Z","receivedAt":"2016-10-03T20:36:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no way to get the list of alternates that git\ncomputes internally; our tests only infer it based on which\nobjects are available. In addition to testing, knowing this\nlist may be helpful for somebody debugging their alternates\nsetup.\n\nLet's add it to the \"count-objects -v\" output. We could give\nit a separate flag, but there's not really any need.\n\"count-objects -v\" is already a debugging catch-all for the\nobject database, its output is easily extensible to new data\nitems, and printing the alternates is not expensive (we\nalready had to find them to count the objects).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-count-objects.txt |  5 +++++\n builtin/count-objects.c             | 10 ++++++++++\n t/t5613-info-alternate.sh           | 10 ++++++++++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex 2ff3568..cb9b4d2 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -38,6 +38,11 @@ objects nor valid packs\n +\n size-garbage: disk space consumed by garbage files, in KiB (unless -H is\n specified)\n++\n+alternate: absolute path of alternate object databases; may appear\n+multiple times, one line per path. Note that if the path contains\n+non-printable characters, it may be surrounded by double-quotes and\n+contain C-style backslashed escape sequences.\n \n -H::\n --human-readable::\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..a700409 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -8,6 +8,7 @@\n #include \"dir.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n+#include \"quote.h\"\n \n static unsigned long garbage;\n static off_t size_garbage;\n@@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char *path, void *data)\n \treturn 0;\n }\n \n+static int print_alternate(struct alternate_object_database *alt, void *data)\n+{\n+\tprintf(\"alternate: \");\n+\tquote_c_style(alt->path, NULL, stdout, 0);\n+\tputchar('\\n');\n+\treturn 0;\n+}\n+\n static char const * const count_objects_usage[] = {\n \tN_(\"git count-objects [-v] [-H | --human-readable]\"),\n \tNULL\n@@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n \t\tprintf(\"garbage: %lu\\n\", garbage);\n \t\tprintf(\"size-garbage: %s\\n\", garbage_buf.buf);\n+\t\tforeach_alt_odb(print_alternate, NULL);\n \t\tstrbuf_release(&loose_buf);\n \t\tstrbuf_release(&pack_buf);\n \t\tstrbuf_release(&garbage_buf);\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex b393613..74f6770 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '\n \t)\n '\n \n+test_expect_success 'count-objects shows the alternates' '\n+\tcat >expect <<-EOF &&\n+\talternate: $(pwd)/B/.git/objects\n+\talternate: $(pwd)/A/.git/objects\n+\tEOF\n+\tgit -C C count-objects -v >actual &&\n+\tgrep ^alternate: actual >actual.alternates &&\n+\ttest_cmp expect actual.alternates\n+'\n+\n # Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n # the depth at 0 and count links, not repositories, so in a chain like:\n #\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303128","messageId":"20161003203622.7uz76ay5f7bqqpfm@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 17/18] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:36:22Z","receivedAt":"2016-10-03T20:36:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We recursively expand alternates repositories, so that if A\nborrows from B which borrows from C, A can see all objects.\n\nFor the root object database, we allow relative paths, so A\ncan point to B as \"../B/objects\". However, we currently do\nnot allow relative paths when recursing, so B must use an\nabsolute path to reach C.\n\nThat is an ancient protection from c2f493a (Transitively\nread alternatives, 2006-05-07) that tries to avoid adding\nthe same alternate through two different paths. Since\n5bdf0a8 (sha1_file: normalize alt_odb path before comparing\nand storing, 2011-09-07), we use a normalized absolute path\nfor each alt_odb entry.\n\nThis means that in most cases the protection is no longer\nnecessary; we will detect the duplicate no matter how we got\nthere (but see below).  And it's a good idea to get rid of\nit, as it creates an unnecessary complication when setting\nup recursive alternates (B has to know that A is going to\nborrow from it and make sure to use an absolute path).\n\nNote that our normalization doesn't actually look at the\nfilesystem, so it can still be fooled by crossing symbolic\nlinks. But that's also true of absolute paths, so it's not a\ngood reason to disallow only relative paths (it's\npotentially a reason to switch to real_path(), but that's a\nseparate and non-trivial change).\n\nWe adjust the test script here to demonstrate that this now\nworks, and add new tests to show that the normalization does\nindeed suppress duplicates.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c               |  7 +------\n t/t5613-info-alternate.sh | 24 ++++++++++++++++++++++--\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 80a3333..b514167 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -354,12 +354,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\tconst char *entry = entries.items[i].string;\n \t\tif (entry[0] == '\\0' || entry[0] == '#')\n \t\t\tcontinue;\n-\t\tif (!is_absolute_path(entry) && depth) {\n-\t\t\terror(\"%s: ignoring relative alternate object store %s\",\n-\t\t\t\t\trelative_base, entry);\n-\t\t} else {\n-\t\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n-\t\t}\n+\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n \t}\n \tstring_list_clear(&entries, 0);\n \tfree(alt_copy);\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 74f6770..76525a0 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -92,8 +92,28 @@ test_expect_success 'that relative alternate is possible for current dir' '\n \tgit fsck\n '\n \n-test_expect_success 'that relative alternate is only possible for current dir' '\n-\ttest_must_fail git -C D fsck\n+test_expect_success 'that relative alternate is recursive' '\n+\tgit -C D fsck\n+'\n+\n+# we can reach \"A\" from our new repo both directly, and via \"C\".\n+# The deep/subdir is there to make sure we are not doing a stupid\n+# pure-text comparison of the alternate names.\n+test_expect_success 'relative duplicates are eliminated' '\n+\tmkdir -p deep/subdir &&\n+\tgit init --bare deep/subdir/duplicate.git &&\n+\tcat >deep/subdir/duplicate.git/objects/info/alternates <<-\\EOF &&\n+\t../../../../C/.git/objects\n+\t../../../../A/.git/objects\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\talternate: $(pwd)/C/.git/objects\n+\talternate: $(pwd)/B/.git/objects\n+\talternate: $(pwd)/A/.git/objects\n+\tEOF\n+\tgit -C deep/subdir/duplicate.git count-objects -v >actual &&\n+\tgrep ^alternate: actual >actual.alternates &&\n+\ttest_cmp expect actual.alternates\n '\n \n test_done\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303129","messageId":"20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"[PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:36:26Z","receivedAt":"2016-10-03T20:36:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On a case-insensitive filesystem, we should realize that\n\"a/objects\" and \"A/objects\" are the same path. We already\nuse fspathcmp() to check against the main object directory,\nbut until recently we couldn't use it for comparing against\nother alternates (because their paths were not\nNUL-terminated strings). But now we can, so let's do so.\n\nNote that we also need to adjust count-objects to load the\nconfig, so that it can see the setting of core.ignorecase\n(this is required by the test, but is also a general bugfix\nfor users of count-objects).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/count-objects.c   |  2 ++\n sha1_file.c               |  2 +-\n t/t5613-info-alternate.sh | 17 +++++++++++++++++\n 3 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex a700409..a04b4f2 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -97,6 +97,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tgit_config(git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0);\n \t/* we do not take arguments other than flags for now */\n \tif (argc)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b514167..b05ec9c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -260,7 +260,7 @@ static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)\n \t * thing twice, or object directory itself.\n \t */\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tif (!strcmp(path->buf, alt->path))\n+\t\tif (!fspathcmp(path->buf, alt->path))\n \t\t\treturn 0;\n \t}\n \tif (!fspathcmp(path->buf, normalized_objdir))\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 76525a0..926fe14 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -116,4 +116,21 @@ test_expect_success 'relative duplicates are eliminated' '\n \ttest_cmp expect actual.alternates\n '\n \n+test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n+\tgit init --bare insensitive.git &&\n+\t# the previous entry for \"A\" will have used uppercase\n+\tcat >insensitive.git/objects/info/alternates <<-\\EOF &&\n+\t../../C/.git/objects\n+\t../../a/.git/objects\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\talternate: $(pwd)/C/.git/objects\n+\talternate: $(pwd)/B/.git/objects\n+\talternate: $(pwd)/A/.git/objects\n+\tEOF\n+\tgit -C insensitive.git count-objects -v >actual &&\n+\tgrep ^alternate: actual >actual.alternates &&\n+\ttest_cmp expect actual.alternates\n+'\n+\n test_done\n-- \n2.10.0.618.g82cc264\n"},{"id":"303174","messageId":"CA+P7+xob7ohj1MxuLGGLwwJyi4RfqUTeLkbw86u+VvbU=uEyAw@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] alternate object database cleanups","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:47:31Z","receivedAt":"2016-10-04T05:47:57Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:\n> This series is the result of René nerd-sniping me with the claim that we\n> could \"easily\" teach count-objects to print out the list of alternates\n> in:\n>\n>   http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/\n>\n\nHah. Nerd snipes are fun.\n\n> My real goal is just patch 17, which is needed for the quarantine series\n> in that thread. But along the way there were quite a few opportunities\n> for cleanups along with a few minor bugfixes (in patches 7 and 18), and\n> I think the count-objects change in patch 16 is a nice general debugging\n> tool.\n\nYea there are a *lot* of cleanups here.\n\n>\n> The rest of it is \"just\" cleanup, but I'll note that it clears up some\n> hairy allocation code. These were bits that I noticed in my big\n> allocation-cleanup series last year, but were too nasty to fit any of\n> the more general fixes. I think the end result is much better.\n>\n\nDefinitely agreed. I read through all the patches, and each one seemed\nreasonable.\n\n> The number of patches is a little intimidating, but I tried hard to\n> break the refactoring down into a sequence of obviously-correct steps.\n> You can be the judge of my success.\n>\n\nI read through them once. I'm going to re-read through them again and\nleave any comments I had.\n\nRegards,\nJake\n\n>   [01/18]: t5613: drop reachable_via function\n>   [02/18]: t5613: drop test_valid_repo function\n>   [03/18]: t5613: use test_must_fail\n>   [04/18]: t5613: whitespace/style cleanups\n>   [05/18]: t5613: do not chdir in main process\n>   [06/18]: t5613: clarify \"too deep\" recursion tests\n>   [07/18]: link_alt_odb_entry: handle normalize_path errors\n>   [08/18]: link_alt_odb_entry: refactor string handling\n>   [09/18]: alternates: provide helper for adding to alternates list\n>   [10/18]: alternates: provide helper for allocating alternate\n>   [11/18]: alternates: encapsulate alt->base munging\n>   [12/18]: alternates: use a separate scratch space\n>   [13/18]: fill_sha1_file: write \"boring\" characters\n>   [14/18]: alternates: store scratch buffer as strbuf\n>   [15/18]: fill_sha1_file: write into a strbuf\n>   [16/18]: count-objects: report alternates via verbose mode\n>   [17/18]: sha1_file: always allow relative paths to alternates\n>   [18/18]: alternates: use fspathcmp to detect duplicates\n>\n>  Documentation/git-count-objects.txt |   5 +\n>  builtin/count-objects.c             |  12 +++\n>  builtin/fsck.c                      |  10 +-\n>  builtin/submodule--helper.c         |  11 +-\n>  cache.h                             |  36 ++++++-\n>  sha1_file.c                         | 179 ++++++++++++++++++--------------\n>  sha1_name.c                         |  17 +--\n>  strbuf.c                            |  20 ++++\n>  strbuf.h                            |   8 ++\n>  submodule.c                         |  23 +---\n>  t/t5613-info-alternate.sh           | 202 ++++++++++++++++++++----------------\n>  transport.c                         |   4 +-\n>  12 files changed, 305 insertions(+), 222 deletions(-)\n>\n"},{"id":"303175","messageId":"CA+P7+xq3G=CNHNNQ5YpkHkud_5SUrTBOwZ3y7d8DvM9nKyXV9g@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203350.3u6ddr6ndr3jwr74@sigill.intra.peff.net","subject":"Re: [PATCH 01/18] t5613: drop reachable_via function","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:48:31Z","receivedAt":"2016-10-04T05:48:56Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:\n> This function was never used since its inception in dd05ea1\n> (test case for transitive info/alternates, 2006-05-07).\n> Which is just as well, since it mutates the repo state in a\n> way that would invalidate further tests, without cleaning up\n> after itself. Let's get rid of it so that nobody is tempted\n> to use it.\n>\n\nMakes sense. It wouldn't be a good idea to leave this around since it\ndidn't clean up after itself. Curious why no test actually used it\nthough..\n\nThanks,\nJake\n"},{"id":"303176","messageId":"CA+P7+xoUbOCRh0C6CAFHgw2NKLstUs_jGHbMQwvqTidfOoEHqQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203357.3cpeg2jyalzykm65@sigill.intra.peff.net","subject":"Re: [PATCH 02/18] t5613: drop test_valid_repo function","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:50:29Z","receivedAt":"2016-10-04T05:50:53Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:\n> This function makes sure that \"git fsck\" does not report any\n> errors. But \"--full\" has been the default since f29cd39\n> (fsck: default to \"git fsck --full\", 2009-10-20), and we can\n> use the exit code (instead of counting the lines) since\n> e2b4f63 (fsck: exit with non-zero status upon errors,\n> 2007-03-05).\n>\n> So we can just use \"git fsck\", which is shorter and more\n> flexible (e.g., we can use \"git -C\").\n\nThis seems obviously correct. I didn't understand your comment about\nthe use of \"git -C\" at first, because I was confused about why \"git\n-C\" doesn't work with \"git --full\", but then I realized you can't use\n\"git -C\" with the shell test_valid_repo function.\n\nThanks,\nJake\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5613-info-alternate.sh | 19 +++++++------------\n>  1 file changed, 7 insertions(+), 12 deletions(-)\n>\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index e13f57d..4548fb0 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -6,11 +6,6 @@\n>  test_description='test transitive info/alternate entries'\n>  . ./test-lib.sh\n>\n> -test_valid_repo() {\n> -       git fsck --full > fsck.log &&\n> -       test_line_count = 0 fsck.log\n> -}\n> -\n>  base_dir=$(pwd)\n"},{"id":"303177","messageId":"CA+P7+xqnSt5qe4OLi2kDO9+y_keyFtVK2=qx4Q5skusBWXD31Q@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203401.d4awnljukgqbku2n@sigill.intra.peff.net","subject":"Re: [PATCH 03/18] t5613: use test_must_fail","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:51:15Z","receivedAt":"2016-10-04T05:51:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> Besides being our normal style, this correctly checks for an\n> error exit() versus signal death.\n>\n\nAnother very simple but obvious improvement.\n\nRegards,\nJake\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5613-info-alternate.sh | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index 4548fb0..65074dd 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -46,10 +46,9 @@ git clone -l -s F G &&\n>  git clone --bare -l -s G H'\n>\n>  test_expect_success 'invalidity of deepest repository' \\\n> -'cd H && {\n> -       git fsck\n> -       test $? -ne 0\n> -}'\n> +'cd H &&\n> +test_must_fail git fsck\n> +'\n>\n>  cd \"$base_dir\"\n>\n> @@ -75,7 +74,8 @@ cd \"$base_dir\"\n>  test_expect_success 'that info/alternates is necessary' \\\n>  'cd C &&\n>  rm -f .git/objects/info/alternates &&\n> -! (git fsck)'\n> +test_must_fail git fsck\n> +'\n>\n>  cd \"$base_dir\"\n>\n> @@ -89,7 +89,7 @@ cd \"$base_dir\"\n>  test_expect_success \\\n>      'that relative alternate is only possible for current dir' '\n>      cd D &&\n> -    ! (git fsck)\n> +    test_must_fail git fsck\n>  '\n>\n>  cd \"$base_dir\"\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303178","messageId":"CA+P7+xoX6XsKRFUCx+vhUo7ARYMEXktzcbFS=zh1NUTgRdhdUA@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203405.nzijl552nlqg63ab@sigill.intra.peff.net","subject":"Re: [PATCH 04/18] t5613: whitespace/style cleanups","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:52:39Z","receivedAt":"2016-10-04T05:53:04Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> Our normal test style these days puts the opening quote of\n> the body on the description line, and indents the body with\n> a single tab. This ancient test did not follow this.\n>\n\nI was surprised you didn't do this first, but it doesn't really make a\ndifference either way. This is also a pretty straight forward\nimprovement, and I can see why you'd want to split this out to review\nseparately.\n\nRegards,\nJake\n"},{"id":"303179","messageId":"CA+P7+xr5bhsSLwXAD0G3PFwFdojWjHAhMMfn6kcpyVFVMUJN=A@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203408.qnakqgcninzty3sr@sigill.intra.peff.net","subject":"Re: [PATCH 05/18] t5613: do not chdir in main process","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:54:52Z","receivedAt":"2016-10-04T05:55:17Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> Our usual style when working with subdirectories is to chdir\n> inside a subshell or to use \"git -C\", which means we do not\n> have to constantly return to the main test directory. Let's\n> convert this old test, which does not follow that style.\n>\n\nMore obvious cleanup for this test file. Seems like a lot of\nintermediate steps to get this test file into a clean state.\n\nThanks,\nJake\n"},{"id":"303180","messageId":"CA+P7+xok5PoNKO+8R6zF9SXYfDq6BboDTDz9WZYEczs0pFK+pw@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203412.bekizvlqtg4ls5fb@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T05:57:48Z","receivedAt":"2016-10-04T05:58:13Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> These tests are just trying to show that we allow recursion\n> up to a certain depth, but not past it. But the counting is\n> a bit non-intuitive, and rather than test at the edge of the\n> breakage, we test \"OK\" cases in the middle of the chain.\n> Let's explain what's going on, and explicitly test the\n> switch between \"OK\" and \"too deep\".\n>\n\nMakes sense to actually test the edge case here instead of just in the middle.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5613-info-alternate.sh | 24 ++++++++++++++++--------\n>  1 file changed, 16 insertions(+), 8 deletions(-)\n>\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index 7bc1c3c..b393613 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n>         )\n>  '\n>\n> +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> +# the depth at 0 and count links, not repositories, so in a chain like:\n> +#\n> +#   A -> B -> C -> D -> E -> F -> G -> H\n> +#      0    1    2    3    4    5    6\n> +#\n\nOk so we count links, but wouldn't we have 5 links when we hit F, and\nnot G? Or am I missing something here?\n\n> +# we are OK at \"G\", but break at \"H\".\n> +#\n\nSeems like from the wording of this comment that we'd break at G and\nnot H..? Obviously the test below shows G is ok. Aren't the numbers\nhere off by 1?\n\nRegards,\nJake\n\n> +# Note also that we must use \"--bare -l\" to make the link to H. The \"-l\"\n> +# ensures we do not do a connectivity check, and the \"--bare\" makes sure\n> +# we do not try to checkout the result (which needs objects), either of\n> +# which would cause the clone to fail.\n\n\n\n>  test_expect_success 'creating too deep nesting' '\n>         git clone -l -s C D &&\n>         git clone -l -s D E &&\n> @@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '\n>         git clone --bare -l -s G H\n>  '\n>\n> -test_expect_success 'invalidity of deepest repository' '\n> -       test_must_fail git -C H fsck\n> -'\n> -\n> -test_expect_success 'validity of third repository' '\n> -       git -C C fsck\n> +test_expect_success 'validity of fifth-deep repository' '\n> +       git -C G fsck\n>  '\n>\n> -test_expect_success 'validity of fourth repository' '\n> -       git -C D fsck\n> +test_expect_success 'invalidity of sixth-deep repository' '\n> +       test_must_fail git -C H fsck\n>  '\n>\n>  test_expect_success 'breaking of loops' '\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303181","messageId":"CA+P7+xrJZatmKDJR=JY9U_JH5BOpQGZ_G2jBM9qi1utUmAM8YQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:01:57Z","receivedAt":"2016-10-04T06:02:27Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> When we add a new alternate to the list, we try to normalize\n> out any redundant \"..\", etc. However, we do not look at the\n> return value of normalize_path_copy(), and will happily\n> continue with a path that could not be normalized. Worse,\n> the normalizing process is done in-place, so we are left\n> with whatever half-finished working state the normalizing\n> function was in.\n>\n> Fortunately, this cannot cause us to read past the end of\n> our buffer, as that working state will always leave the\n> NUL from the original path in place. And we do tend to\n> notice problems when we check is_directory() on the path.\n> But you can see the nonsense that we feed to is_directory\n> with an entry like:\n>\n>   this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n>\n> in your objects/info/alternates, which yields:\n>\n>   error: object directory\n>   /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n>   does not exist; check .git/objects/info/alternates.\n>\n\nYikes, that doesn't seem helpful.\n\n> We can easily fix this just by checking the return value.\n> But that makes it hard to generate a good error message,\n> since we're normalizing in-place and our input value has\n> been overwritten by cruft.\n\nRight. Definitely want to check the return value here...\n\n>\n> Instead, let's provide a strbuf helper that does an in-place\n> normalize, but restores the original contents on error. This\n> uses a second buffer under the hood, which is slightly less\n> efficient, but this is not a performance-critical code path.\n>\n\nI agree, I don't think this duplication is really a big deal, since it\nhelps ensure that the function doesn't modify its arguments on error.\n\n> The strbuf helper can also properly set the \"len\" parameter\n> of the strbuf before returning. Just doing:\n>\n>   normalize_path_copy(buf.buf, buf.buf);\n>\n> will shorten the string, but leave buf.len at the original\n> length. That may be confusing to later code which uses the\n> strbuf.\n>\n\nMakes sense here. Properly setting the length will help prevent future issues.\n\nThanks,\nJake\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  sha1_file.c | 11 +++++++++--\n>  strbuf.c    | 20 ++++++++++++++++++++\n>  strbuf.h    |  8 ++++++++\n>  3 files changed, 37 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b9c1fa3..68571bd 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n>         }\n>         strbuf_addstr(&pathbuf, entry);\n>\n> -       normalize_path_copy(pathbuf.buf, pathbuf.buf);\n> +       if (strbuf_normalize_path(&pathbuf) < 0) {\n> +               error(\"unable to normalize alternate object path: %s\",\n> +                     pathbuf.buf);\n> +               strbuf_release(&pathbuf);\n> +               return -1;\n> +       }\n>\n>         pfxlen = strlen(pathbuf.buf);\n>\n> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>         }\n>\n>         strbuf_add_absolute_path(&objdirbuf, get_object_directory());\n> -       normalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n> +       if (strbuf_normalize_path(&objdirbuf) < 0)\n> +               die(\"unable to normalize object directory: %s\",\n> +                   objdirbuf.buf);\n>\n>         alt_copy = xmemdupz(alt, len);\n>         string_list_split_in_place(&entries, alt_copy, sep, -1);\n> diff --git a/strbuf.c b/strbuf.c\n> index b839be4..8fec657 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n>\n>         strbuf_setlen(sb, j);\n>  }\n> +\n> +int strbuf_normalize_path(struct strbuf *src)\n> +{\n> +       struct strbuf dst = STRBUF_INIT;\n> +\n> +       strbuf_grow(&dst, src->len);\n> +       if (normalize_path_copy(dst.buf, src->buf) < 0) {\n> +               strbuf_release(&dst);\n> +               return -1;\n> +       }\n> +\n> +       /*\n> +        * normalize_path does not tell us the new length, so we have to\n> +        * compute it by looking for the new NUL it placed\n> +        */\n\nAnd we can't correctly set the length inside normalize_path_copy\nbecause it just takes C strings directly and not actually a strbuf. Ok\nso it makes sense that we have to set it here.\n\nThanks,\nJake\n\n> +       strbuf_setlen(&dst, strlen(dst.buf));\n> +       strbuf_swap(src, &dst);\n> +       strbuf_release(&dst);\n> +       return 0;\n> +}\n> diff --git a/strbuf.h b/strbuf.h\n> index ba8d5f1..2262b12 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);\n>   */\n>  extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n>\n"},{"id":"303182","messageId":"CA+P7+xrUOnDebwZnfu-xv-GuTJka4-eNUAfBudQf5ZhnkczU6w@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203448.cdfbitl5jmhlpb5o@sigill.intra.peff.net","subject":"Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:05:42Z","receivedAt":"2016-10-04T06:06:09Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> The string handling in link_alt_odb_entry() is mostly an\n> artifact of the original version, which took the path as a\n> ptr/len combo, and did not have a NUL-terminated string\n> until we created one in the alternate_object_database\n> struct.  But since 5bdf0a8 (sha1_file: normalize alt_odb\n> path before comparing and storing, 2011-09-07), the first\n> thing we do is put the path into a strbuf, which gives us\n> some easy opportunities for cleanup.\n>\n> In particular:\n>\n>   - we call strlen(pathbuf.buf), which is silly; we can look\n>     at pathbuf.len.\n\nRight. This makes obvious sense.\n\n>\n>   - even though we have a strbuf, we don't maintain its\n>     \"len\" field when chomping extra slashes from the\n>     end, and instead keep a separate \"pfxlen\" variable. We\n>     can fix this and then drop \"pfxlen\" entirely.\n>\n\nMakes sense.\n\n>   - we don't check whether the path is usable until after we\n>     allocate the new struct, making extra cleanup work for\n>     ourselves. Since we have a NUL-terminated string, we can\n>     bump the \"is it usable\" checks higher in the function.\n>     While we're at it, we can move that logic to its own\n>     helper, which makes the flow of link_alt_odb_entry()\n>     easier to follow.\n>\n\nAlso makes sense.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> And you can probably guess now how I found the issue in the last patch\n> where pathbuf.len is totally bogus after calling normalize_path_copy. :)\n>\n>  sha1_file.c | 83 +++++++++++++++++++++++++++++++++----------------------------\n>  1 file changed, 45 insertions(+), 38 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 68571bd..f396823 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -234,6 +234,36 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n>  struct alternate_object_database *alt_odb_list;\n>  static struct alternate_object_database **alt_odb_tail;\n>\n> +/*\n> + * Return non-zero iff the path is usable as an alternate object database.\n> + */\n> +static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)\n> +{\n> +       struct alternate_object_database *alt;\n> +\n> +       /* Detect cases where alternate disappeared */\n> +       if (!is_directory(path->buf)) {\n> +               error(\"object directory %s does not exist; \"\n> +                     \"check .git/objects/info/alternates.\",\n> +                     path->buf);\n> +               return 0;\n> +       }\n> +\n> +       /*\n> +        * Prevent the common mistake of listing the same\n> +        * thing twice, or object directory itself.\n> +        */\n> +       for (alt = alt_odb_list; alt; alt = alt->next) {\n> +               if (path->len == alt->name - alt->base - 1 &&\n> +                   !memcmp(path->buf, alt->base, path->len))\n> +                       return 0;\n> +       }\n> +       if (!fspathcmp(path->buf, normalized_objdir))\n> +               return 0;\n> +\n> +       return 1;\n> +}\n> +\n\nThis definitely makes reading the following function much easier,\nthough the diff is a bit funky. I think the end result is much\nclearer.\n\nThanks,\nJake\n\n>  /*\n>   * Prepare alternate object database registry.\n>   *\n> @@ -253,8 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n>         int depth, const char *normalized_objdir)\n>  {\n>         struct alternate_object_database *ent;\n> -       struct alternate_object_database *alt;\n> -       size_t pfxlen, entlen;\n> +       size_t entlen;\n>         struct strbuf pathbuf = STRBUF_INIT;\n>\n>         if (!is_absolute_path(entry) && relative_base) {\n> @@ -270,47 +299,26 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n>                 return -1;\n>         }\n>\n> -       pfxlen = strlen(pathbuf.buf);\n> -\n>         /*\n>          * The trailing slash after the directory name is given by\n>          * this function at the end. Remove duplicates.\n>          */\n> -       while (pfxlen && pathbuf.buf[pfxlen-1] == '/')\n> -               pfxlen -= 1;\n> -\n> -       entlen = st_add(pfxlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n> -       ent = xmalloc(st_add(sizeof(*ent), entlen));\n> -       memcpy(ent->base, pathbuf.buf, pfxlen);\n> -       strbuf_release(&pathbuf);\n> -\n> -       ent->name = ent->base + pfxlen + 1;\n> -       ent->base[pfxlen + 3] = '/';\n> -       ent->base[pfxlen] = ent->base[entlen-1] = 0;\n> +       while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')\n> +               strbuf_setlen(&pathbuf, pathbuf.len - 1);\n>\n> -       /* Detect cases where alternate disappeared */\n> -       if (!is_directory(ent->base)) {\n> -               error(\"object directory %s does not exist; \"\n> -                     \"check .git/objects/info/alternates.\",\n> -                     ent->base);\n> -               free(ent);\n> +       if (!alt_odb_usable(&pathbuf, normalized_objdir)) {\n> +               strbuf_release(&pathbuf);\n>                 return -1;\n>         }\n>\n> -       /* Prevent the common mistake of listing the same\n> -        * thing twice, or object directory itself.\n> -        */\n> -       for (alt = alt_odb_list; alt; alt = alt->next) {\n> -               if (pfxlen == alt->name - alt->base - 1 &&\n> -                   !memcmp(ent->base, alt->base, pfxlen)) {\n> -                       free(ent);\n> -                       return -1;\n> -               }\n> -       }\n> -       if (!fspathcmp(ent->base, normalized_objdir)) {\n> -               free(ent);\n> -               return -1;\n> -       }\n> +       entlen = st_add(pathbuf.len, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */\n> +       ent = xmalloc(st_add(sizeof(*ent), entlen));\n> +       memcpy(ent->base, pathbuf.buf, pathbuf.len);\n> +\n> +       ent->name = ent->base + pathbuf.len + 1;\n> +       ent->base[pathbuf.len] = '/';\n> +       ent->base[pathbuf.len + 3] = '/';\n> +       ent->base[entlen-1] = 0;\n>\n>         /* add the alternate entry */\n>         *alt_odb_tail = ent;\n> @@ -318,10 +326,9 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n>         ent->next = NULL;\n>\n>         /* recursively add alternates */\n> -       read_info_alternates(ent->base, depth + 1);\n> -\n> -       ent->base[pfxlen] = '/';\n> +       read_info_alternates(pathbuf.buf, depth + 1);\n>\n> +       strbuf_release(&pathbuf);\n>         return 0;\n>  }\n>\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303183","messageId":"CA+P7+xq+NiCn53r+NCnPUysKW_V0semX4_1dGSN3OgMrJS2cJA@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203503.omjwvg4ocz7pjyzt@sigill.intra.peff.net","subject":"Re: [PATCH 09/18] alternates: provide helper for adding to alternates list","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:07:42Z","receivedAt":"2016-10-04T06:08:07Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n> The submodule code wants to temporarily add an alternate\n> object store to our in-memory alt_odb list, but does it\n> manually. Let's provide a helper so it can reuse the code in\n> link_alt_odb_entry().\n>\n> While we're adding our new add_to_alternates_memory(), let's\n> document add_to_alternates_file(), as the two are related.\n>\n\nYa the code used in the submodule area always felt a bit wrong to me.\nIt took me a bit to realize why we can just replace this all with a\ncall to link_alt_odb_entry, but the resulting code reduction is\ndefinitely nice.\n\n\n> -       /* avoid adding it twice */\n> -       prepare_alt_odb();\n> -       for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n> -               if (alt_odb->name - alt_odb->base == objects_directory.len &&\n> -                               !strncmp(alt_odb->base, objects_directory.buf,\n> -                                       objects_directory.len))\n> -                       goto done;\n> -\n> -       alloc = st_add(objects_directory.len, 42); /* for \"12/345...\" sha1 */\n> -       alt_odb = xmalloc(st_add(sizeof(*alt_odb), alloc));\n> -       alt_odb->next = alt_odb_list;\n> -       xsnprintf(alt_odb->base, alloc, \"%s\", objects_directory.buf);\n> -       alt_odb->name = alt_odb->base + objects_directory.len;\n> -       alt_odb->name[2] = '/';\n> -       alt_odb->name[40] = '\\0';\n> -       alt_odb->name[41] = '\\0';\n> -       alt_odb_list = alt_odb;\n> -\n\nGetting rid of multiple places for this funky extra allocation is a\nnice improvement.\n\nThanks,\nJake\n\n> -       /* add possible alternates from the submodule */\n> -       read_info_alternates(objects_directory.buf, 0);\n> +       add_to_alternates_memory(objects_directory.buf);\n>  done:\n>         strbuf_release(&objects_directory);\n>         return ret;\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303184","messageId":"CA+P7+xpgNo1d_6h6XZneEei6zbV02GcC8o+e+51wpnAeenJz=g@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203531.bppczvzmdfumtnb2@sigill.intra.peff.net","subject":"Re: [PATCH 10/18] alternates: provide helper for allocating alternate","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:09:46Z","receivedAt":"2016-10-04T06:10:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n> Allocating a struct alternate_object_database is tricky, as\n> we must over-allocate the buffer to provide scratch space,\n> and then put in particular '/' and NUL markers.\n>\n> Let's encapsulate this in a function so that the complexity\n> doesn't leak into callers (and so that we can modify it\n> later).\n\nThe overall way this was broken up is definitely a lot of patches to\nfollow but understanding the end goal this is a huge improvement in\ncode maintainability here. This original allocation is indeed very\ntricky.\n\nThanks,\nJake\n"},{"id":"303185","messageId":"CA+P7+xqyuHHcMaUPajgEjt3Uzkcx6tEPEykuRaTPdvKSzt3jzg@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203551.tmqp5rll6nqkewxz@sigill.intra.peff.net","subject":"Re: [PATCH 12/18] alternates: use a separate scratch space","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:12:19Z","receivedAt":"2016-10-04T06:12:44Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n> The alternate_object_database struct uses a single buffer\n> both for storing the path to the alternate, and as a scratch\n> buffer for forming object names. This is efficient (since\n> otherwise we'd end up storing the path twice), but it makes\n> life hard for callers who just want to know the path to the\n> alternate. They have to remember to stop reading after\n> \"alt->name - alt->base\" bytes, and to subtract one for the\n> trailing '/'.\n>\n> It would be much simpler if they could simply access a\n> NUL-terminated path string. We could encapsulate this in a\n> function which puts a NUL in the scratch buffer and returns\n> the string, but that opens up questions about the lifetime\n> of the result. The first time another caller uses the\n> alternate, the scratch buffer may get other data tacked onto\n> it.\n>\n> Let's instead just store the root path separately from the\n> scratch buffer. There aren't enough alternates being stored\n> for the duplicated data to matter for performance, and this\n> keeps things simple and safe for the callers.\n>\n\nDefinitely agree here. The resulting code seems a lot easier to\nfollow, and making the callers simpler here is a very goo thing.\n\nThanks,\nJake\n"},{"id":"303186","messageId":"CA+P7+xpOxoRBDZGF_CU1Q-SYiQZtMx2vuwQKS0og864awZod5g@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203555.6xadycotmmkuf34h@sigill.intra.peff.net","subject":"Re: [PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:13:59Z","receivedAt":"2016-10-04T06:14:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n> This function forms a sha1 as \"xx/yyyy...\", but skips over\n> the slot for the slash rather than writing it, leaving it to\n> the caller to do so. It also does not bother to put in a\n> trailing NUL, even though every caller would want it (we're\n> forming a path which by definition is not a directory, so\n> the only thing to do with it is feed it to a system call).\n>\n> Let's make the lives of our callers easier by just writing\n> out the internal \"/\" and the NUL.\n>\n\nYa this makes sense.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  sha1_file.c | 12 +++++-------\n>  1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 70c3e2f..c6308c1 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)\n>         for (i = 0; i < 20; i++) {\n>                 static char hex[] = \"0123456789abcdef\";\n>                 unsigned int val = sha1[i];\n> -               char *pos = pathbuf + i*2 + (i > 0);\n> -               *pos++ = hex[val >> 4];\n> -               *pos = hex[val & 0xf];\n> +               *pathbuf++ = hex[val >> 4];\n> +               *pathbuf++ = hex[val & 0xf];\n> +               if (!i)\n> +                       *pathbuf++ = '/';\n>         }\n> +       *pathbuf = '\\0';\n\nI think this makes a lot more sense than making the callers have to do this.\n\nThanks,\nJake\n\n>  }\n>\n>  const char *sha1_file_name(const unsigned char *sha1)\n> @@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1)\n>                 die(\"insanely long object directory %s\", objdir);\n>         memcpy(buf, objdir, len);\n>         buf[len] = '/';\n> -       buf[len+3] = '/';\n> -       buf[len+42] = '\\0';\n>         fill_sha1_path(buf + len + 1, sha1);\n>         return buf;\n>  }\n> @@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)\n>\n>         ent->name = ent->scratch + dirlen + 1;\n>         ent->scratch[dirlen] = '/';\n> -       ent->scratch[dirlen + 3] = '/';\n> -       ent->scratch[entlen-1] = 0;\n>\n>         return ent;\n>  }\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303187","messageId":"CA+P7+xrVSsNfHo_+tT2+tmXkzAiETVRDVwud-2ADGX8G42W+GQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203609.4hig3e24lyvswdcf@sigill.intra.peff.net","subject":"Re: [PATCH 15/18] fill_sha1_file: write into a strbuf","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:44:37Z","receivedAt":"2016-10-04T06:45:03Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> It's currently the responsibility of the caller to give\n> fill_sha1_file() enough bytes to write into, leading them to\n> manually compute the required lengths. Instead, let's just\n> write into a strbuf so that it's impossible to get this\n> wrong.\n\nYea this makes sense.\n\n>\n> The alt_odb caller already has a strbuf, so this makes\n> things strictly simpler. The other caller, sha1_file_name(),\n> uses a static PATH_MAX buffer and dies when it would\n> overflow. We can convert this to a static strbuf, which\n> means our allocation cost is amortized (and as a bonus, we\n> no longer have to worry about PATH_MAX being too short for\n> normal use).\n>\n> This does introduce some small overhead in fill_sha1_file(),\n> as each strbuf_addchar() will check whether it needs to\n> grow. However, between the optimization in fec501d\n> (strbuf_addch: avoid calling strbuf_grow, 2015-04-16) and\n> the fact that this is not generally called in a tight loop\n> (after all, the next step is typically to access the file!)\n> this probably doesn't matter. And even if it did, the right\n> place to micro-optimize is inside fill_sha1_file(), by\n> calling a single strbuf_grow() there.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  sha1_file.c | 34 ++++++++++------------------------\n>  1 file changed, 10 insertions(+), 24 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index efc8cee..80a3333 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -172,36 +172,28 @@ enum scld_error safe_create_leading_directories_const(const char *path)\n>         return result;\n>  }\n>\n> -static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)\n> +static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n>  {\n>         int i;\n>         for (i = 0; i < 20; i++) {\n>                 static char hex[] = \"0123456789abcdef\";\n>                 unsigned int val = sha1[i];\n> -               *pathbuf++ = hex[val >> 4];\n> -               *pathbuf++ = hex[val & 0xf];\n> +               strbuf_addch(buf, hex[val >> 4]);\n> +               strbuf_addch(buf, hex[val & 0xf]);\n>                 if (!i)\n> -                       *pathbuf++ = '/';\n> +                       strbuf_addch(buf, '/');\n>         }\n> -       *pathbuf = '\\0';\n>  }\n>\n>  const char *sha1_file_name(const unsigned char *sha1)\n>  {\n> -       static char buf[PATH_MAX];\n> -       const char *objdir;\n> -       int len;\n> +       static struct strbuf buf = STRBUF_INIT;\n>\n> -       objdir = get_object_directory();\n> -       len = strlen(objdir);\n> +       strbuf_reset(&buf);\n> +       strbuf_addf(&buf, \"%s/\", get_object_directory());\n>\n> -       /* '/' + sha1(2) + '/' + sha1(38) + '\\0' */\n> -       if (len + 43 > PATH_MAX)\n> -               die(\"insanely long object directory %s\", objdir);\n> -       memcpy(buf, objdir, len);\n> -       buf[len] = '/';\n> -       fill_sha1_path(buf + len + 1, sha1);\n> -       return buf;\n\nI'm definitely a fan of seeing the magic number here go away.\n\n> +       fill_sha1_path(&buf, sha1);\n> +       return buf.buf;\n>  }\n>\n>  struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n> @@ -213,14 +205,8 @@ struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n>  static const char *alt_sha1_path(struct alternate_object_database *alt,\n>                                  const unsigned char *sha1)\n>  {\n> -       /* hex sha1 plus internal \"/\" */\n> -       size_t len = GIT_SHA1_HEXSZ + 1;\n>         struct strbuf *buf = alt_scratch_buf(alt);\n\nFunny story.. While reviewing this code on my screen, my monitor has a\nnice little bit of gunk just between the lines that made this one look\nlike it was being deleted. So I was really confused as to what strbuf\nyou were using and why you removed a call to alt_scratch_buf()..\nObviously this line just isn't being removed.\n\n> -\n> -       strbuf_grow(buf, len);\n> -       fill_sha1_path(buf->buf + buf->len, sha1);\n> -       strbuf_setlen(buf, buf->len + len);\n> -\n> +       fill_sha1_path(buf, sha1);\n>         return buf->buf;\n>  }\n>\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303189","messageId":"CA+P7+xoR-14fmzrYU_sGoTRgcfP8fe5y-+kSkoM-2=E3Jb56FQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net","subject":"Re: [PATCH 16/18] count-objects: report alternates via verbose mode","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:46:32Z","receivedAt":"2016-10-04T06:46:58Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\\On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> There's no way to get the list of alternates that git\n> computes internally; our tests only infer it based on which\n> objects are available. In addition to testing, knowing this\n> list may be helpful for somebody debugging their alternates\n> setup.\n>\n> Let's add it to the \"count-objects -v\" output. We could give\n> it a separate flag, but there's not really any need.\n> \"count-objects -v\" is already a debugging catch-all for the\n> object database, its output is easily extensible to new data\n> items, and printing the alternates is not expensive (we\n> already had to find them to count the objects).\n>\n\nMakes sense. Unless there's a compelling reason you'd want to print\nout these alternates *without* anything else from -v, but you can just\nuse grep like the test does so this seems fine to me.\n\nThanks,\nJake\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Documentation/git-count-objects.txt |  5 +++++\n>  builtin/count-objects.c             | 10 ++++++++++\n>  t/t5613-info-alternate.sh           | 10 ++++++++++\n>  3 files changed, 25 insertions(+)\n>\n> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\n> index 2ff3568..cb9b4d2 100644\n> --- a/Documentation/git-count-objects.txt\n> +++ b/Documentation/git-count-objects.txt\n> @@ -38,6 +38,11 @@ objects nor valid packs\n>  +\n>  size-garbage: disk space consumed by garbage files, in KiB (unless -H is\n>  specified)\n> ++\n> +alternate: absolute path of alternate object databases; may appear\n> +multiple times, one line per path. Note that if the path contains\n> +non-printable characters, it may be surrounded by double-quotes and\n> +contain C-style backslashed escape sequences.\n>\n>  -H::\n>  --human-readable::\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index ba92919..a700409 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -8,6 +8,7 @@\n>  #include \"dir.h\"\n>  #include \"builtin.h\"\n>  #include \"parse-options.h\"\n> +#include \"quote.h\"\n>\n>  static unsigned long garbage;\n>  static off_t size_garbage;\n> @@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char *path, void *data)\n>         return 0;\n>  }\n>\n> +static int print_alternate(struct alternate_object_database *alt, void *data)\n> +{\n> +       printf(\"alternate: \");\n> +       quote_c_style(alt->path, NULL, stdout, 0);\n> +       putchar('\\n');\n> +       return 0;\n> +}\n> +\n>  static char const * const count_objects_usage[] = {\n>         N_(\"git count-objects [-v] [-H | --human-readable]\"),\n>         NULL\n> @@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>                 printf(\"prune-packable: %lu\\n\", packed_loose);\n>                 printf(\"garbage: %lu\\n\", garbage);\n>                 printf(\"size-garbage: %s\\n\", garbage_buf.buf);\n> +               foreach_alt_odb(print_alternate, NULL);\n>                 strbuf_release(&loose_buf);\n>                 strbuf_release(&pack_buf);\n>                 strbuf_release(&garbage_buf);\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index b393613..74f6770 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '\n>         )\n>  '\n>\n> +test_expect_success 'count-objects shows the alternates' '\n> +       cat >expect <<-EOF &&\n> +       alternate: $(pwd)/B/.git/objects\n> +       alternate: $(pwd)/A/.git/objects\n> +       EOF\n> +       git -C C count-objects -v >actual &&\n> +       grep ^alternate: actual >actual.alternates &&\n> +       test_cmp expect actual.alternates\n> +'\n> +\n>  # Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>  # the depth at 0 and count links, not repositories, so in a chain like:\n>  #\n> --\n> 2.10.0.618.g82cc264\n>\n"},{"id":"303190","messageId":"CA+P7+xr9ugWWcoyQ6dToFacwff8rGJhYJNxy+E5_iGjubONLPQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203622.7uz76ay5f7bqqpfm@sigill.intra.peff.net","subject":"Re: [PATCH 17/18] sha1_file: always allow relative paths to alternates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:50:30Z","receivedAt":"2016-10-04T06:50:55Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> We recursively expand alternates repositories, so that if A\n> borrows from B which borrows from C, A can see all objects.\n>\n> For the root object database, we allow relative paths, so A\n> can point to B as \"../B/objects\". However, we currently do\n> not allow relative paths when recursing, so B must use an\n> absolute path to reach C.\n>\n> That is an ancient protection from c2f493a (Transitively\n> read alternatives, 2006-05-07) that tries to avoid adding\n> the same alternate through two different paths. Since\n> 5bdf0a8 (sha1_file: normalize alt_odb path before comparing\n> and storing, 2011-09-07), we use a normalized absolute path\n> for each alt_odb entry.\n>\n> This means that in most cases the protection is no longer\n> necessary; we will detect the duplicate no matter how we got\n> there (but see below).  And it's a good idea to get rid of\n> it, as it creates an unnecessary complication when setting\n> up recursive alternates (B has to know that A is going to\n> borrow from it and make sure to use an absolute path).\n>\n\nI think this makes sense. We already normalize a path, and if the\nnormalization is too complicated, then we (now) fail nicely so we\nshould always have an absolute path to the store.\n\n> Note that our normalization doesn't actually look at the\n> filesystem, so it can still be fooled by crossing symbolic\n> links. But that's also true of absolute paths, so it's not a\n> good reason to disallow only relative paths (it's\n> potentially a reason to switch to real_path(), but that's a\n> separate and non-trivial change).\n\nHmm, ya using real_path would fix that but I definitely agree that's\nnot trivial and can be done in the future if we think it is or becomes\nnecessary.\n"},{"id":"303191","messageId":"CA+P7+xqhuYmp-H=b-SrNdZjN5urWGHPuNkWbeVgCBF1UuhQZKQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net","subject":"Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T06:51:59Z","receivedAt":"2016-10-04T06:52:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> On a case-insensitive filesystem, we should realize that\n> \"a/objects\" and \"A/objects\" are the same path. We already\n> use fspathcmp() to check against the main object directory,\n> but until recently we couldn't use it for comparing against\n> other alternates (because their paths were not\n> NUL-terminated strings). But now we can, so let's do so.\n>\n\nYep, makes sense.\n\n> Note that we also need to adjust count-objects to load the\n> config, so that it can see the setting of core.ignorecase\n> (this is required by the test, but is also a general bugfix\n> for users of count-objects).\n\nAlso makes sense.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/count-objects.c   |  2 ++\n>  sha1_file.c               |  2 +-\n>  t/t5613-info-alternate.sh | 17 +++++++++++++++++\n>  3 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index a700409..a04b4f2 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -97,6 +97,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>                 OPT_END(),\n>         };\n>\n> +       git_config(git_default_config, NULL);\n> +\n>         argc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0);\n>         /* we do not take arguments other than flags for now */\n>         if (argc)\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b514167..b05ec9c 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -260,7 +260,7 @@ static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)\n>          * thing twice, or object directory itself.\n>          */\n>         for (alt = alt_odb_list; alt; alt = alt->next) {\n> -               if (!strcmp(path->buf, alt->path))\n> +               if (!fspathcmp(path->buf, alt->path))\n>                         return 0;\n>         }\n>         if (!fspathcmp(path->buf, normalized_objdir))\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index 76525a0..926fe14 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -116,4 +116,21 @@ test_expect_success 'relative duplicates are eliminated' '\n>         test_cmp expect actual.alternates\n>  '\n>\n> +test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n> +       git init --bare insensitive.git &&\n> +       # the previous entry for \"A\" will have used uppercase\n> +       cat >insensitive.git/objects/info/alternates <<-\\EOF &&\n> +       ../../C/.git/objects\n> +       ../../a/.git/objects\n> +       EOF\n> +       cat >expect <<-EOF &&\n> +       alternate: $(pwd)/C/.git/objects\n> +       alternate: $(pwd)/B/.git/objects\n> +       alternate: $(pwd)/A/.git/objects\n> +       EOF\n> +       git -C insensitive.git count-objects -v >actual &&\n> +       grep ^alternate: actual >actual.alternates &&\n> +       test_cmp expect actual.alternates\n> +'\n> +\n>  test_done\n> --\n> 2.10.0.618.g82cc264\n"},{"id":"303232","messageId":"20161004134120.aj6oywkiy4li7aeh@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xob7ohj1MxuLGGLwwJyi4RfqUTeLkbw86u+VvbU=uEyAw@mail.gmail.com","subject":"Re: [PATCH 0/18] alternate object database cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:41:20Z","receivedAt":"2016-10-04T13:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 10:47:31PM -0700, Jacob Keller wrote:\n\n> > The number of patches is a little intimidating, but I tried hard to\n> > break the refactoring down into a sequence of obviously-correct steps.\n> > You can be the judge of my success.\n> \n> I read through them once. I'm going to re-read through them again and\n> leave any comments I had.\n\nThanks for having the fortitude to read them all. :) After looking at\nyour comments, I don't _think_ there's anything that necessitates a\nre-roll, but I'll respond to a few of them individually.\n\n-Peff\n"},{"id":"303233","messageId":"20161004134339.pn4elyuixslsaya4@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xq3G=CNHNNQ5YpkHkud_5SUrTBOwZ3y7d8DvM9nKyXV9g@mail.gmail.com","subject":"Re: [PATCH 01/18] t5613: drop reachable_via function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:43:40Z","receivedAt":"2016-10-04T13:43:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 10:48:31PM -0700, Jacob Keller wrote:\n\n> On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:\n> > This function was never used since its inception in dd05ea1\n> > (test case for transitive info/alternates, 2006-05-07).\n> > Which is just as well, since it mutates the repo state in a\n> > way that would invalidate further tests, without cleaning up\n> > after itself. Let's get rid of it so that nobody is tempted\n> > to use it.\n> >\n> \n> Makes sense. It wouldn't be a good idea to leave this around since it\n> didn't clean up after itself. Curious why no test actually used it\n> though..\n\nI wondered that, too. Sometimes in cases like this a call got dropped\nduring a re-roll on the list. But the original email:\n\n  http://public-inbox.org/git/20060507181947.GE23738@admingilde.org/\n\nhas the problem, too. So probably it was just leftover cruft that was\nmade obsolete even before it hit the list.\n\n-Peff\n"},{"id":"303234","messageId":"20161004134702.evnm6xea7y6mbppo@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xoX6XsKRFUCx+vhUo7ARYMEXktzcbFS=zh1NUTgRdhdUA@mail.gmail.com","subject":"Re: [PATCH 04/18] t5613: whitespace/style cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:47:02Z","receivedAt":"2016-10-04T13:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 10:52:39PM -0700, Jacob Keller wrote:\n\n> On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> > Our normal test style these days puts the opening quote of\n> > the body on the description line, and indents the body with\n> > a single tab. This ancient test did not follow this.\n> >\n> \n> I was surprised you didn't do this first, but it doesn't really make a\n> difference either way. This is also a pretty straight forward\n> improvement, and I can see why you'd want to split this out to review\n> separately.\n\nI was trying to leave it to the end, to move the substantive changes up\nfront (and because there _isn't_ a correct style for some of the things\nit was doing). But it just got too painful to do the \"don't chdir\" patch\nwithout updating the style. I agree it might have made more sense at the\nvery beginning, but I didn't think it mattered enough to go through the\ntrouble of rebasing the earlier patches (which would essentially be\nrewriting them).\n\n-Peff\n"},{"id":"303235","messageId":"20161004134853.x3zq33ywyyzgbwsy@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xok5PoNKO+8R6zF9SXYfDq6BboDTDz9WZYEczs0pFK+pw@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:48:53Z","receivedAt":"2016-10-04T13:49:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:\n\n> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> > index 7bc1c3c..b393613 100755\n> > --- a/t/t5613-info-alternate.sh\n> > +++ b/t/t5613-info-alternate.sh\n> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n> >         )\n> >  '\n> >\n> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> > +# the depth at 0 and count links, not repositories, so in a chain like:\n> > +#\n> > +#   A -> B -> C -> D -> E -> F -> G -> H\n> > +#      0    1    2    3    4    5    6\n> > +#\n> \n> Ok so we count links, but wouldn't we have 5 links when we hit F, and\n> not G? Or am I missing something here?\n\nThis is what I was trying to get at with the \"start the depth at 0\". We\ndisallow a depth greater than 5, but because we start at 0-counting,\nit's really six links. I guess saying \"5 as too deep\" is really the\nmisleading part. It should be \"5 as the maximum depth\".\n\n-Peff\n"},{"id":"303236","messageId":"20161004135353.6ywgoxutjcbaali5@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xrUOnDebwZnfu-xv-GuTJka4-eNUAfBudQf5ZhnkczU6w@mail.gmail.com","subject":"Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:53:53Z","receivedAt":"2016-10-04T13:54:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 11:05:42PM -0700, Jacob Keller wrote:\n\n> This definitely makes reading the following function much easier,\n> though the diff is a bit funky. I think the end result is much\n> clearer.\n\nYeah, it's really hard to see that all of the \"ent\" setup is kept,\nbecause it moves _and_ changes its content (from pfxlen to pathbuf.len).\n\nI actually tried to split this into two patches to make the diff easier\nto read, but there are two mutually dependent changes: moving to\npathbuf.len everywhere requires not-freeing pathbuf in the early code\npath. But if you do that and don't move all of \"is it usable\" checks up,\nthen you have to add a bunch of new error-handling code that would just\nget ripped out in the next patch.\n\nThere's definitely _some_ of that in this series already (e.g., the\ncounting logic in alt_sha1_path() added by patch 14 that just gets\nripped out in patch 15 when fill_sha1_path() learns to use a strbuf). I\ntried to balance \"show each individual obvious step\" with \"don't make\npeople review a bunch of scaffolding that's not going to be in the final\nproduct\".\n\n-Peff\n"},{"id":"303237","messageId":"20161004135600.5bahz7q75mdtdstn@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xoR-14fmzrYU_sGoTRgcfP8fe5y-+kSkoM-2=E3Jb56FQ@mail.gmail.com","subject":"Re: [PATCH 16/18] count-objects: report alternates via verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T13:56:00Z","receivedAt":"2016-10-04T13:56:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 11:46:32PM -0700, Jacob Keller wrote:\n\n> \\On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> > There's no way to get the list of alternates that git\n> > computes internally; our tests only infer it based on which\n> > objects are available. In addition to testing, knowing this\n> > list may be helpful for somebody debugging their alternates\n> > setup.\n> >\n> > Let's add it to the \"count-objects -v\" output. We could give\n> > it a separate flag, but there's not really any need.\n> > \"count-objects -v\" is already a debugging catch-all for the\n> > object database, its output is easily extensible to new data\n> > items, and printing the alternates is not expensive (we\n> > already had to find them to count the objects).\n> >\n> \n> Makes sense. Unless there's a compelling reason you'd want to print\n> out these alternates *without* anything else from -v, but you can just\n> use grep like the test does so this seems fine to me.\n\nYeah. I could definitely be persuaded otherwise, but I just couldn't see\nthese being used for anything useful beyond debugging.\n\n-Peff\n"},{"id":"303238","messageId":"20161004140011.ueptwobup3n6indy@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xr9ugWWcoyQ6dToFacwff8rGJhYJNxy+E5_iGjubONLPQ@mail.gmail.com","subject":"Re: [PATCH 17/18] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T14:00:11Z","receivedAt":"2016-10-04T14:00:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 11:50:30PM -0700, Jacob Keller wrote:\n\n> > Note that our normalization doesn't actually look at the\n> > filesystem, so it can still be fooled by crossing symbolic\n> > links. But that's also true of absolute paths, so it's not a\n> > good reason to disallow only relative paths (it's\n> > potentially a reason to switch to real_path(), but that's a\n> > separate and non-trivial change).\n> \n> Hmm, ya using real_path would fix that but I definitely agree that's\n> not trivial and can be done in the future if we think it is or becomes\n> necessary.\n\nI did look into this briefly. The trick is that real_path() assumes\nrelative paths are relative from the current directory (and does chdir()\ntrickery to get the filesystem to resolve things for us). So you'd\nreally need a \"real_path_from\" that chdirs to the relative base, issues\nthe real_path() from there, and then chdirs back to the original cwd.\n\nWhich I guess is no less gross than what real_path() is doing itself\ninternally, but it's definitely something for another patch. Given the\nfact that we don't check it now and nobody has complained leads me to\nbelieve that nobody really cares.\n\nActually, given the fact that we didn't allow relative bases in\nrecursive alternates, I suspect that very few people are using\ncomplicated alternate setups in the first place.\n\n-Peff\n"},{"id":"303239","messageId":"20161004141029.medczy5vusmam23h@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xqhuYmp-H=b-SrNdZjN5urWGHPuNkWbeVgCBF1UuhQZKQ@mail.gmail.com","subject":"Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T14:10:30Z","receivedAt":"2016-10-04T14:10:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 11:51:59PM -0700, Jacob Keller wrote:\n\n> On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:\n> > On a case-insensitive filesystem, we should realize that\n> > \"a/objects\" and \"A/objects\" are the same path. We already\n> > use fspathcmp() to check against the main object directory,\n> > but until recently we couldn't use it for comparing against\n> > other alternates (because their paths were not\n> > NUL-terminated strings). But now we can, so let's do so.\n> >\n> \n> Yep, makes sense.\n> \n> > Note that we also need to adjust count-objects to load the\n> > config, so that it can see the setting of core.ignorecase\n> > (this is required by the test, but is also a general bugfix\n> > for users of count-objects).\n> \n> Also makes sense.\n\nBTW, I tested this on a vfat loopback device, but I was surprised to see\nthat quite a few other tests failed on that device.\n\nAt least one of the problems is that symlinks are not supported, but\nlib-httpd.sh wants to use them for its Apache setup. I guess people on\nWindows just don't run the httpd tests at all, which is not too\nsurprising.\n\nLikewise, credential-cache fails because it cannot create a Unix socket\n(and the flag for that is in the build, not a run-time filesystem\ncheck).\n\nSome of the other failures seemed to be due to lack of an executable bit\non the filesystem. I'm not sure if we could or should do better run-time\ndetection of that sort of thing. I think some of the checks are tied to\nthe build, and that's generally good enough in practice because people\ndon't use vfat on their Linux machines. So tracking down each of them\nmay just be pedantic make-work that nobody cares about.\n\nI did wonder if there was another good filesystem to use for\ncase-insensitive experiments on Linux. At the time I didn't think there\nwas good support for making HFS+ filesystems, but it looks Debian cares\nmkfs.hfs. That's probably a better choice for such experiments.\n\n-Peff\n"},{"id":"303295","messageId":"CA+P7+xrRBCjkmWGKYbA9b1=pNHcckphF-Ko8i7OoPAcs9y-bNQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161004134120.aj6oywkiy4li7aeh@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] alternate object database cleanups","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T20:40:06Z","receivedAt":"2016-10-04T20:40:33Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 6:41 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 03, 2016 at 10:47:31PM -0700, Jacob Keller wrote:\n>\n>> > The number of patches is a little intimidating, but I tried hard to\n>> > break the refactoring down into a sequence of obviously-correct steps.\n>> > You can be the judge of my success.\n>>\n>> I read through them once. I'm going to re-read through them again and\n>> leave any comments I had.\n>\n> Thanks for having the fortitude to read them all. :) After looking at\n> your comments, I don't _think_ there's anything that necessitates a\n> re-roll, but I'll respond to a few of them individually.\n>\n> -Peff\n\nYa, I don't either. Most of my comments were just me trying to make\nsure I understood what you were doing.\n\nThanks,\nJake\n"},{"id":"303296","messageId":"CA+P7+xoPhbhaGSLtDRg7myjFcBdAVpmSFLSVFHRncTtFkCq21Q@mail.gmail.com","threadId":"44205","inReplyTo":"20161004134702.evnm6xea7y6mbppo@sigill.intra.peff.net","subject":"Re: [PATCH 04/18] t5613: whitespace/style cleanups","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T20:41:01Z","receivedAt":"2016-10-04T20:41:27Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 6:47 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 03, 2016 at 10:52:39PM -0700, Jacob Keller wrote:\n>\n>> On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n>> > Our normal test style these days puts the opening quote of\n>> > the body on the description line, and indents the body with\n>> > a single tab. This ancient test did not follow this.\n>> >\n>>\n>> I was surprised you didn't do this first, but it doesn't really make a\n>> difference either way. This is also a pretty straight forward\n>> improvement, and I can see why you'd want to split this out to review\n>> separately.\n>\n> I was trying to leave it to the end, to move the substantive changes up\n> front (and because there _isn't_ a correct style for some of the things\n> it was doing). But it just got too painful to do the \"don't chdir\" patch\n> without updating the style. I agree it might have made more sense at the\n> very beginning, but I didn't think it mattered enough to go through the\n> trouble of rebasing the earlier patches (which would essentially be\n> rewriting them).\n>\n> -Peff\n\nRight.\n\nThanks,\nJake\n"},{"id":"303297","messageId":"CA+P7+xok-8vhikxkp+t8pu53YJAyUjZ0NiAwejEW2j3+eP_2Xw@mail.gmail.com","threadId":"44205","inReplyTo":"20161004134853.x3zq33ywyyzgbwsy@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T20:44:23Z","receivedAt":"2016-10-04T20:44:48Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:\n>\n>> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n>> > index 7bc1c3c..b393613 100755\n>> > --- a/t/t5613-info-alternate.sh\n>> > +++ b/t/t5613-info-alternate.sh\n>> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n>> >         )\n>> >  '\n>> >\n>> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>> > +# the depth at 0 and count links, not repositories, so in a chain like:\n>> > +#\n>> > +#   A -> B -> C -> D -> E -> F -> G -> H\n>> > +#      0    1    2    3    4    5    6\n>> > +#\n>>\n>> Ok so we count links, but wouldn't we have 5 links when we hit F, and\n>> not G? Or am I missing something here?\n>\n> This is what I was trying to get at with the \"start the depth at 0\". We\n> disallow a depth greater than 5, but because we start at 0-counting,\n> it's really six links. I guess saying \"5 as too deep\" is really the\n> misleading part. It should be \"5 as the maximum depth\".\n>\n> -Peff\n\nRight, but if A is 0, then:\n\nB = 1\nC = 2\nD = 3\nE = 4\nF = 5\nG = 6  (UhOh??)\nH = 7\n\nSo do you mean that *B* = 0, and C = 1??? That is not clear from this commment.\n\nSo either way it still feels like \"6\" links is what is allowed? Or the\nfirst link has to not count? That's really confusing.\n\nBasically I G is the 7th letter, not the 6th, so even if we're\nsubtractnig 1 it's still 6 which is 1 too deep? That means we not only\ndiscard 0 (the first repository) but we discount the 2nd one as well?\n\nThanks,\nJake\n"},{"id":"303298","messageId":"CA+P7+xqSzm7S4-Mc+keJ1JWioYUmF76es0A3xN+Hwq+EJ6dJSA@mail.gmail.com","threadId":"44205","inReplyTo":"20161004135353.6ywgoxutjcbaali5@sigill.intra.peff.net","subject":"Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T20:46:08Z","receivedAt":"2016-10-04T20:46:33Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 6:53 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 03, 2016 at 11:05:42PM -0700, Jacob Keller wrote:\n>\n>> This definitely makes reading the following function much easier,\n>> though the diff is a bit funky. I think the end result is much\n>> clearer.\n>\n> Yeah, it's really hard to see that all of the \"ent\" setup is kept,\n> because it moves _and_ changes its content (from pfxlen to pathbuf.len).\n>\n> I actually tried to split this into two patches to make the diff easier\n> to read, but there are two mutually dependent changes: moving to\n> pathbuf.len everywhere requires not-freeing pathbuf in the early code\n> path. But if you do that and don't move all of \"is it usable\" checks up,\n> then you have to add a bunch of new error-handling code that would just\n> get ripped out in the next patch.\n>\n> There's definitely _some_ of that in this series already (e.g., the\n> counting logic in alt_sha1_path() added by patch 14 that just gets\n> ripped out in patch 15 when fill_sha1_path() learns to use a strbuf). I\n> tried to balance \"show each individual obvious step\" with \"don't make\n> people review a bunch of scaffolding that's not going to be in the final\n> product\".\n>\n> -Peff\n\nMostly the diff is funky because of how the diff selected which chunks\nmoved vs how your patch described what chunks moved.\n\nThanks,\nJake\n"},{"id":"303299","messageId":"20161004204933.ygfhoy24g6psyf6h@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xok-8vhikxkp+t8pu53YJAyUjZ0NiAwejEW2j3+eP_2Xw@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T20:49:34Z","receivedAt":"2016-10-04T20:49:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:\n\n> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:\n> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:\n> >\n> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> >> > index 7bc1c3c..b393613 100755\n> >> > --- a/t/t5613-info-alternate.sh\n> >> > +++ b/t/t5613-info-alternate.sh\n> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n> >> >         )\n> >> >  '\n> >> >\n> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n> >> > +#\n> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n> >> > +#      0    1    2    3    4    5    6\n> >> > +#\n> >>\n> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and\n> >> not G? Or am I missing something here?\n> >\n> > This is what I was trying to get at with the \"start the depth at 0\". We\n> > disallow a depth greater than 5, but because we start at 0-counting,\n> > it's really six links. I guess saying \"5 as too deep\" is really the\n> > misleading part. It should be \"5 as the maximum depth\".\n> >\n> > -Peff\n> \n> Right, but if A is 0, then:\n> \n> B = 1\n> C = 2\n> D = 3\n> E = 4\n> F = 5\n> G = 6  (UhOh??)\n> H = 7\n> \n> So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.\n\nNo, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\nis \"1\", and so on.\n\n> So either way it still feels like \"6\" links is what is allowed? Or the\n> first link has to not count? That's really confusing.\n\nRight, 6 links _are_ allowed. Because we count links, and because we\nstart the link-counting at \"0\" and allow through \"5\". The link labeled\n\"6\" (which is really the seventh link!) is the one that is forbidden.\n\n> Basically I G is the 7th letter, not the 6th, so even if we're\n> subtractnig 1 it's still 6 which is 1 too deep? That means we not only\n> discard 0 (the first repository) but we discount the 2nd one as well?\n\nIt's basically two off-by-ones from what you might think is correct.  I\nagree it's unintuitive, but I'm just documenting what's there. We could\nchange it; it's not like anybody cares about the exact value except\n\"deep enough\", but _since_ nobody cares, I preferred not to modify the\ncode.\n\n-Peff\n"},{"id":"303301","messageId":"CA+P7+xo3nxy1EOjDqHvKQuK128c=b73XN=6qqn6g6oRGh2VdFg@mail.gmail.com","threadId":"44205","inReplyTo":"20161004204933.ygfhoy24g6psyf6h@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T20:52:19Z","receivedAt":"2016-10-04T20:52:44Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 1:49 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Oct 04, 2016 at 01:44:23PM -0700, Jacob Keller wrote:\n>\n>> On Tue, Oct 4, 2016 at 6:48 AM, Jeff King <peff@peff.net> wrote:\n>> > On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:\n>> >\n>> >> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n>> >> > index 7bc1c3c..b393613 100755\n>> >> > --- a/t/t5613-info-alternate.sh\n>> >> > +++ b/t/t5613-info-alternate.sh\n>> >> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '\n>> >> >         )\n>> >> >  '\n>> >> >\n>> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n>> >> > +#\n>> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n>> >> > +#      0    1    2    3    4    5    6\n>> >> > +#\n>> >>\n>> >> Ok so we count links, but wouldn't we have 5 links when we hit F, and\n>> >> not G? Or am I missing something here?\n>> >\n>> > This is what I was trying to get at with the \"start the depth at 0\". We\n>> > disallow a depth greater than 5, but because we start at 0-counting,\n>> > it's really six links. I guess saying \"5 as too deep\" is really the\n>> > misleading part. It should be \"5 as the maximum depth\".\n>> >\n>> > -Peff\n>>\n>> Right, but if A is 0, then:\n>>\n>> B = 1\n>> C = 2\n>> D = 3\n>> E = 4\n>> F = 5\n>> G = 6  (UhOh??)\n>> H = 7\n>>\n>> So do you mean that *B* = 0, and C = 1??? That is not clear from this commment.\n>\n> No, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\n> is \"1\", and so on.\n>\n\nIf you need to re-roll for some other reason I would add some spaces\naround the numbers so they line up better with the links so that this\nbecomes more clear.\n\n>> So either way it still feels like \"6\" links is what is allowed? Or the\n>> first link has to not count? That's really confusing.\n>\n> Right, 6 links _are_ allowed. Because we count links, and because we\n> start the link-counting at \"0\" and allow through \"5\". The link labeled\n> \"6\" (which is really the seventh link!) is the one that is forbidden.\n\nRight. Ok this makes more sense now.\n\n>\n>> Basically I G is the 7th letter, not the 6th, so even if we're\n>> subtractnig 1 it's still 6 which is 1 too deep? That means we not only\n>> discard 0 (the first repository) but we discount the 2nd one as well?\n>\n> It's basically two off-by-ones from what you might think is correct.  I\n> agree it's unintuitive, but I'm just documenting what's there. We could\n> change it; it's not like anybody cares about the exact value except\n> \"deep enough\", but _since_ nobody cares, I preferred not to modify the\n> code.\n>\n\nI agree I don't think changing code is necessary, I was just confused\nby the comment that tried to make it clear.\n\nThanks,\nJake\n\n> -Peff\n"},{"id":"303302","messageId":"20161004205510.6bhisw7ixbgcvvwn@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xo3nxy1EOjDqHvKQuK128c=b73XN=6qqn6g6oRGh2VdFg@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T20:55:11Z","receivedAt":"2016-10-04T20:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:\n\n> >> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n> >> >> > +#\n> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n> >> >> > +#      0    1    2    3    4    5    6\n> >> >> > +#\n> [...]\n> > No, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\n> > is \"1\", and so on.\n> \n> If you need to re-roll for some other reason I would add some spaces\n> around the numbers so they line up better with the links so that this\n> becomes more clear.\n\nHmm. Now I am puzzled, because I _did_ line up them specifically to make\nthis clear. I put the numbers under the \">\" of the arrow. Did I screw up\nthe spacing somehow so that isn't how they look to you? Or are you just\nsaying you would prefer them under the \"-\" of the arrow?\n\n-Peff\n"},{"id":"303303","messageId":"CAGZ79kap2ndp=FK4YdqrL4tJ8_VDuuAcSCk1dtX5X2H3aaj6kQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161004205510.6bhisw7ixbgcvvwn@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T20:58:54Z","receivedAt":"2016-10-04T20:59:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:\n>\n>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n>> >> >> > +#\n>> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n>> >> >> > +#      0    1    2    3    4    5    6\n>> >> >> > +#\n>> [...]\n>> > No, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\n>> > is \"1\", and so on.\n>>\n>> If you need to re-roll for some other reason I would add some spaces\n>> around the numbers so they line up better with the links so that this\n>> becomes more clear.\n>\n> Hmm. Now I am puzzled, because I _did_ line up them specifically to make\n> this clear. I put the numbers under the \">\" of the arrow. Did I screw up\n> the spacing somehow so that isn't how they look to you? Or are you just\n> saying you would prefer them under the \"-\" of the arrow?\n>\n> -Peff\n\nInput from a self-claimed design expert for ASCII art. ;)\nWhat about this?\n\n#   A  -0->  B  -1->  C  -2->  ...\n\n(Double space between letter and arrow, number included in the arrow)\n"},{"id":"303304","messageId":"20161004210015.myvb6pjs6gmlv4jr@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CAGZ79kap2ndp=FK4YdqrL4tJ8_VDuuAcSCk1dtX5X2H3aaj6kQ@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T21:00:15Z","receivedAt":"2016-10-04T21:00:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 01:58:54PM -0700, Stefan Beller wrote:\n\n> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:\n> > On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:\n> >\n> >> >> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> >> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n> >> >> >> > +#\n> >> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n> >> >> >> > +#      0    1    2    3    4    5    6\n> >> >> >> > +#\n> >> [...]\n> >> > No, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\n> >> > is \"1\", and so on.\n> >>\n> >> If you need to re-roll for some other reason I would add some spaces\n> >> around the numbers so they line up better with the links so that this\n> >> becomes more clear.\n> >\n> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make\n> > this clear. I put the numbers under the \">\" of the arrow. Did I screw up\n> > the spacing somehow so that isn't how they look to you? Or are you just\n> > saying you would prefer them under the \"-\" of the arrow?\n> >\n> > -Peff\n> \n> Input from a self-claimed design expert for ASCII art. ;)\n> What about this?\n> \n> #   A  -0->  B  -1->  C  -2->  ...\n> \n> (Double space between letter and arrow, number included in the arrow)\n\nI actually find that quite confusing, as it looks like \"-1\", \"-2\", etc.\n\nThis has got to be my favorite bikeshed discussion of all time, though. :)\n\n-Peff\n"},{"id":"303306","messageId":"xmqqwphnsub9.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161003203408.qnakqgcninzty3sr@sigill.intra.peff.net","subject":"Re: [PATCH 05/18] t5613: do not chdir in main process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:00:58Z","receivedAt":"2016-10-04T21:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Our usual style when working with subdirectories is to chdir\n> inside a subshell or to use \"git -C\", which means we do not\n> have to constantly return to the main test directory. Let's\n> convert this old test, which does not follow that style.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5613-info-alternate.sh | 92 +++++++++++++++++------------------------------\n>  1 file changed, 33 insertions(+), 59 deletions(-)\n\nWhew.  Quite a lot of cleanups on this ancient script.  Thanks.\n"},{"id":"303308","messageId":"xmqqshsbstys.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:08:27Z","receivedAt":"2016-10-04T21:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +int strbuf_normalize_path(struct strbuf *src)\n> +{\n> +\tstruct strbuf dst = STRBUF_INIT;\n> +\n> +\tstrbuf_grow(&dst, src->len);\n> +\tif (normalize_path_copy(dst.buf, src->buf) < 0) {\n> +\t\tstrbuf_release(&dst);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/*\n> +\t * normalize_path does not tell us the new length, so we have to\n> +\t * compute it by looking for the new NUL it placed\n> +\t */\n> +\tstrbuf_setlen(&dst, strlen(dst.buf));\n> +\tstrbuf_swap(src, &dst);\n> +\tstrbuf_release(&dst);\n> +\treturn 0;\n> +}\n\nMakes sense.\n\n"},{"id":"303309","messageId":"xmqqoa2zstiu.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161003203448.cdfbitl5jmhlpb5o@sigill.intra.peff.net","subject":"Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:18:01Z","receivedAt":"2016-10-04T21:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The string handling in link_alt_odb_entry() is mostly an\n> artifact of the original version, which took the path as a\n> ptr/len combo, and did not have a NUL-terminated string\n> until we created one in the alternate_object_database\n> struct.  But since 5bdf0a8 (sha1_file: normalize alt_odb\n> path before comparing and storing, 2011-09-07), the first\n> thing we do is put the path into a strbuf, which gives us\n> some easy opportunities for cleanup.\n>\n> In particular:\n>\n>   - we call strlen(pathbuf.buf), which is silly; we can look\n>     at pathbuf.len.\n>\n>   - even though we have a strbuf, we don't maintain its\n>     \"len\" field when chomping extra slashes from the\n>     end, and instead keep a separate \"pfxlen\" variable. We\n>     can fix this and then drop \"pfxlen\" entirely.\n>\n>   - we don't check whether the path is usable until after we\n>     allocate the new struct, making extra cleanup work for\n>     ourselves. Since we have a NUL-terminated string, we can\n>     bump the \"is it usable\" checks higher in the function.\n>     While we're at it, we can move that logic to its own\n>     helper, which makes the flow of link_alt_odb_entry()\n>     easier to follow.\n\nAlso I find that this bit is a nice touch:\n\n>  \t/* recursively add alternates */\n> -\tread_info_alternates(ent->base, depth + 1);\n> -\n> -\tent->base[pfxlen] = '/';\n> +\tread_info_alternates(pathbuf.buf, depth + 1);\n\nWe used to leave ent->base[] string terminated with NUL while\ncalling read_info_alternates() and then added '/' after that, but\nbecause the new code uses a separate pathbuf for the call, ent->base[]\ncan be prepared into the desired shape upfront.\n\nMuch easier to follow.\n\n"},{"id":"303310","messageId":"xmqqk2dnssz9.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161003203551.tmqp5rll6nqkewxz@sigill.intra.peff.net","subject":"Re: [PATCH 12/18] alternates: use a separate scratch space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:29:46Z","receivedAt":"2016-10-04T21:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  extern struct alternate_object_database {\n>  \tstruct alternate_object_database *next;\n> +\n>  \tchar *name;\n> -\tchar base[FLEX_ARRAY]; /* more */\n> +\tchar *scratch;\n> +\n> +\tchar path[FLEX_ARRAY];\n>  } *alt_odb_list;\n\nIt is not wrong per-se, but I am a bit surprised to see that the\ncode keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed\nat by the scratch pointer.\n\nLoss of \"compare only up to the location 'name' points at\" makes the\nusers of the struct that want only the directory path certainly a\nlot simpler and easier to follow.\n\nThanks.\n"},{"id":"303311","messageId":"20161004213241.ihzkl7cohliavydg@sigill.intra.peff.net","threadId":"44205","inReplyTo":"xmqqk2dnssz9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 12/18] alternates: use a separate scratch space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T21:32:41Z","receivedAt":"2016-10-04T21:32:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >  extern struct alternate_object_database {\n> >  \tstruct alternate_object_database *next;\n> > +\n> >  \tchar *name;\n> > -\tchar base[FLEX_ARRAY]; /* more */\n> > +\tchar *scratch;\n> > +\n> > +\tchar path[FLEX_ARRAY];\n> >  } *alt_odb_list;\n> \n> It is not wrong per-se, but I am a bit surprised to see that the\n> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed\n> at by the scratch pointer.\n\nYeah, there's really no reason \"path\" could not become a non-flex\nbuffer. I mostly left it there out of inertia. If you have a preference,\nI'm happy to change it.\n\n-Peff\n"},{"id":"303313","messageId":"xmqqfuobssdg.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net","subject":"Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:42:51Z","receivedAt":"2016-10-04T21:42:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... but until recently we couldn't use it for comparing against\n> other alternates (because their paths were not\n> NUL-terminated strings).\n\n;-)  \n\nI should have expected this when reading the \"let's have\na separate .path field\" conversion.  Nice job.\n\n"},{"id":"303314","messageId":"CA+P7+xoDz2sOPrDrJhAhrqDQsRR8NVU-8kh6+G=8FJeXEJ1dtg@mail.gmail.com","threadId":"44205","inReplyTo":"20161004205510.6bhisw7ixbgcvvwn@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T21:43:24Z","receivedAt":"2016-10-04T21:43:50Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:\n>\n>> >> >> > +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>> >> >> > +# the depth at 0 and count links, not repositories, so in a chain like:\n>> >> >> > +#\n>> >> >> > +#   A -> B -> C -> D -> E -> F -> G -> H\n>> >> >> > +#      0    1    2    3    4    5    6\n>> >> >> > +#\n>> [...]\n>> > No, we count links, not repositories. So the \"A->B\" link is \"0\", \"B->C\"\n>> > is \"1\", and so on.\n>>\n>> If you need to re-roll for some other reason I would add some spaces\n>> around the numbers so they line up better with the links so that this\n>> becomes more clear.\n>\n> Hmm. Now I am puzzled, because I _did_ line up them specifically to make\n> this clear. I put the numbers under the \">\" of the arrow. Did I screw up\n> the spacing somehow so that isn't how they look to you? Or are you just\n> saying you would prefer them under the \"-\" of the arrow?\n>\n> -Peff\n\nI bet they line up in a monospace font and I just happened to be\nviewing this from GMail which isn't showing it in monospace and so it\ndoesn't line up. Ignore me and carry on\n\nThanks,\nJake\n"},{"id":"303315","messageId":"xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"CA+P7+xpOxoRBDZGF_CU1Q-SYiQZtMx2vuwQKS0og864awZod5g@mail.gmail.com","subject":"Re: [PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:46:44Z","receivedAt":"2016-10-04T21:46:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n>> This function forms a sha1 as \"xx/yyyy...\", but skips over\n>> the slot for the slash rather than writing it, leaving it to\n>> the caller to do so. It also does not bother to put in a\n>> trailing NUL, even though every caller would want it (we're\n>> forming a path which by definition is not a directory, so\n>> the only thing to do with it is feed it to a system call).\n>>\n>> Let's make the lives of our callers easier by just writing\n>> out the internal \"/\" and the NUL.\n>> ...\n>\n> I think this makes a lot more sense than making the callers have to do this.\n\nThe cost of fill function having to do the same thing repeatedly is\nnegligible, so I am OK with the result, but for fairness, this was\nnot \"make the callers do this extra thing\", but was \"the caller can\nprepare these unchanging parts just once, and the fill function that\nis repeatedly run does not have to.\"\n\n"},{"id":"303316","messageId":"20161004214851.n2ycxotg6wcdkxch@sigill.intra.peff.net","threadId":"44205","inReplyTo":"xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T21:48:51Z","receivedAt":"2016-10-04T21:48:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:46:44PM -0700, Junio C Hamano wrote:\n\n> Jacob Keller <jacob.keller@gmail.com> writes:\n> \n> > On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n> >> This function forms a sha1 as \"xx/yyyy...\", but skips over\n> >> the slot for the slash rather than writing it, leaving it to\n> >> the caller to do so. It also does not bother to put in a\n> >> trailing NUL, even though every caller would want it (we're\n> >> forming a path which by definition is not a directory, so\n> >> the only thing to do with it is feed it to a system call).\n> >>\n> >> Let's make the lives of our callers easier by just writing\n> >> out the internal \"/\" and the NUL.\n> >> ...\n> >\n> > I think this makes a lot more sense than making the callers have to do this.\n> \n> The cost of fill function having to do the same thing repeatedly is\n> negligible, so I am OK with the result, but for fairness, this was\n> not \"make the callers do this extra thing\", but was \"the caller can\n> prepare these unchanging parts just once, and the fill function that\n> is repeatedly run does not have to.\"\n\nYeah, perhaps \"does not bother\" in the commit message is not entirely\nfair. But it really does feel like quite a premature optimization to\nskip the writing of one \"/\" in the middle of the string, especially as\nit impacts the interface.\n\n-Peff\n"},{"id":"303317","messageId":"20161004214914.kgkot337awszhojs@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CA+P7+xoDz2sOPrDrJhAhrqDQsRR8NVU-8kh6+G=8FJeXEJ1dtg@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T21:49:14Z","receivedAt":"2016-10-04T21:49:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:\n\n> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make\n> > this clear. I put the numbers under the \">\" of the arrow. Did I screw up\n> > the spacing somehow so that isn't how they look to you? Or are you just\n> > saying you would prefer them under the \"-\" of the arrow?\n>\n> I bet they line up in a monospace font and I just happened to be\n> viewing this from GMail which isn't showing it in monospace and so it\n> doesn't line up. Ignore me and carry on\n\nOh, good. I was wondering if I was going crazy. :)\n\n-Peff\n"},{"id":"303318","messageId":"CA+P7+xrBX684an5EzUUk+_Dtu6Ep_F+nB1JyWDWsZjUANWcFoA@mail.gmail.com","threadId":"44205","inReplyTo":"xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T21:49:10Z","receivedAt":"2016-10-04T21:49:35Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.keller@gmail.com> writes:\n>\n>> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:\n>>> This function forms a sha1 as \"xx/yyyy...\", but skips over\n>>> the slot for the slash rather than writing it, leaving it to\n>>> the caller to do so. It also does not bother to put in a\n>>> trailing NUL, even though every caller would want it (we're\n>>> forming a path which by definition is not a directory, so\n>>> the only thing to do with it is feed it to a system call).\n>>>\n>>> Let's make the lives of our callers easier by just writing\n>>> out the internal \"/\" and the NUL.\n>>> ...\n>>\n>> I think this makes a lot more sense than making the callers have to do this.\n>\n> The cost of fill function having to do the same thing repeatedly is\n> negligible, so I am OK with the result, but for fairness, this was\n> not \"make the callers do this extra thing\", but was \"the caller can\n> prepare these unchanging parts just once, and the fill function that\n> is repeatedly run does not have to.\"\n>\n\nSure, but it's a pretty minor optimization and I think the result is\neasier to understand.\n\nThanks,\nJake\n"},{"id":"303319","messageId":"xmqq7f9nss1y.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161004213241.ihzkl7cohliavydg@sigill.intra.peff.net","subject":"Re: [PATCH 12/18] alternates: use a separate scratch space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T21:49:45Z","receivedAt":"2016-10-04T21:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >  extern struct alternate_object_database {\n>> >  \tstruct alternate_object_database *next;\n>> > +\n>> >  \tchar *name;\n>> > -\tchar base[FLEX_ARRAY]; /* more */\n>> > +\tchar *scratch;\n>> > +\n>> > +\tchar path[FLEX_ARRAY];\n>> >  } *alt_odb_list;\n>> \n>> It is not wrong per-se, but I am a bit surprised to see that the\n>> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed\n>> at by the scratch pointer.\n>\n> Yeah, there's really no reason \"path\" could not become a non-flex\n> buffer. I mostly left it there out of inertia. If you have a preference,\n> I'm happy to change it.\n\nMy preference, before reaching the end of the series, actually was\nto overallocate just once and point with *scratch into path[] beyond\nthe end of the fixed \"where is the object directory?\" string.\n\nOf course, that would not mesh very well with the plan this series\nhad after this step to use strbuf for keeping scratch ;-)  And the\nend result looks fine to me.\n\nThanks.\n\n"},{"id":"303320","messageId":"CA+P7+xqzqWk5ddZLjSmmfOMJ+QOW=J6S=8WSiCPYK_VE_pHLiQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161004214914.kgkot337awszhojs@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-04T21:50:20Z","receivedAt":"2016-10-04T21:50:44Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Oct 4, 2016 at 2:49 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:\n>\n>> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make\n>> > this clear. I put the numbers under the \">\" of the arrow. Did I screw up\n>> > the spacing somehow so that isn't how they look to you? Or are you just\n>> > saying you would prefer them under the \"-\" of the arrow?\n>>\n>> I bet they line up in a monospace font and I just happened to be\n>> viewing this from GMail which isn't showing it in monospace and so it\n>> doesn't line up. Ignore me and carry on\n>\n> Oh, good. I was wondering if I was going crazy. :)\n>\n> -Peff\n\nOnly one of us is going crazy, but I'm not sure who ;)\n\n-Jake\n"},{"id":"303321","messageId":"20161004215139.pi4xomnxzctb46vc@sigill.intra.peff.net","threadId":"44205","inReplyTo":"xmqq7f9nss1y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 12/18] alternates: use a separate scratch space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T21:51:39Z","receivedAt":"2016-10-04T21:51:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:49:45PM -0700, Junio C Hamano wrote:\n\n> >> It is not wrong per-se, but I am a bit surprised to see that the\n> >> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed\n> >> at by the scratch pointer.\n> >\n> > Yeah, there's really no reason \"path\" could not become a non-flex\n> > buffer. I mostly left it there out of inertia. If you have a preference,\n> > I'm happy to change it.\n> \n> My preference, before reaching the end of the series, actually was\n> to overallocate just once and point with *scratch into path[] beyond\n> the end of the fixed \"where is the object directory?\" string.\n> \n> Of course, that would not mesh very well with the plan this series\n> had after this step to use strbuf for keeping scratch ;-)  And the\n> end result looks fine to me.\n\nHeh, yeah, I did not think of that (because I had the strbuf end-game in\nmind the whole time). I agree that would be nicer if we were keeping the\nraw buffer, if only because one could free the whole thing in one shot\n(OTOH, we do not ever free these structs at all :) ).\n\n-Peff\n"},{"id":"303337","messageId":"20161005023455.GA6215@pug.qqx.org","threadId":"44205","inReplyTo":"20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net","subject":"Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2016-10-05T02:34:55Z","receivedAt":"2016-10-05T02:42:41Z","isPatch":true,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:\n>On a case-insensitive filesystem, we should realize that\n>\"a/objects\" and \"A/objects\" are the same path.\n\nThe current repository being on a case-insensitive filesystem doesn't \nguarantee that the alternates are as well.\n\nOn the other hand, I suspect that people who use a case-insensitive \nfilesystem would be less likely to use names which differ only by case.\n"},{"id":"303339","messageId":"20161005035447.xdnmilecg6p2uqrp@sigill.intra.peff.net","threadId":"44205","inReplyTo":"20161005023455.GA6215@pug.qqx.org","subject":"Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T03:54:47Z","receivedAt":"2016-10-05T03:54:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 10:34:55PM -0400, Aaron Schrab wrote:\n\n> At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:\n> > On a case-insensitive filesystem, we should realize that\n> > \"a/objects\" and \"A/objects\" are the same path.\n> \n> The current repository being on a case-insensitive filesystem doesn't\n> guarantee that the alternates are as well.\n> \n> On the other hand, I suspect that people who use a case-insensitive\n> filesystem would be less likely to use names which differ only by case.\n\nTrue. I don't think we actually have enough information to make the\ncorrect comparison (not only that, but I think that fspathcmp() can\nsometimes be fooled by a path which is only partially case-insensitive\ndue to a case-insensitive filesystem mounted on a case-sensitive one).\n\nStill, I think in practice this is likely to do more good than harm, as\nI'd guess that being on a single filesystem is the common case.\n\n-Peff\n"},{"id":"303364","messageId":"2ea2f077-ab02-2631-4ce9-93cdd22c3c6b@gmail.com","threadId":"44205","inReplyTo":"CAGZ79kap2ndp=FK4YdqrL4tJ8_VDuuAcSCk1dtX5X2H3aaj6kQ@mail.gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-05T13:58:53Z","receivedAt":"2016-10-05T13:59:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 04.10.2016 o 22:58, Stefan Beller pisze:\n> On Tue, Oct 4, 2016 at 1:55 PM, Jeff King <peff@peff.net> wrote:\n>> On Tue, Oct 04, 2016 at 01:52:19PM -0700, Jacob Keller wrote:\n>>\n>>>>>>>> +# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>>>>>>>> +# the depth at 0 and count links, not repositories, so in a chain like:\n>>>>>>>> +#\n>>>>>>>> +#   A -> B -> C -> D -> E -> F -> G -> H\n>>>>>>>> +#      0    1    2    3    4    5    6\n>>>>>>>> +#\n\n> \n> Input from a self-claimed design expert for ASCII art. ;)\n> What about this?\n> \n> #   A  -0->  B  -1->  C  -2->  ...\n\nI would prefer the following:\n\n#   A --> B --> C --> D --> E --> F --> G --> H\n#      0     1     2     3     4     5     6\n\nthat is, the number below the middle of the arrow\n(which could have been even longer)\n\n#   A ---> B ---> C ---> D ---> E ---> F ---> G ---> H\n#      0      1      2      3      4      5      6\n\n\nLet's paint this bikeshed _plaid_ ;-))))\n-- \nJakub Narębski\n\n"},{"id":"303366","messageId":"f664a03d-838c-4062-c31b-5bd8fe7f2328@gmail.com","threadId":"44205","inReplyTo":"20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net","subject":"Re: [PATCH 16/18] count-objects: report alternates via verbose mode","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-05T14:23:45Z","receivedAt":"2016-10-05T14:24:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 03.10.2016 o 22:36, Jeff King pisze:\n\n> +test_expect_success 'count-objects shows the alternates' '\n> +\tcat >expect <<-EOF &&\n> +\talternate: $(pwd)/B/.git/objects\n> +\talternate: $(pwd)/A/.git/objects\n> +\tEOF\n> +\tgit -C C count-objects -v >actual &&\n> +\tgrep ^alternate: actual >actual.alternates &&\n> +\ttest_cmp expect actual.alternates\n> +'\n\nThis was bit hard to grok for me without quotes around\nregular expression in grep (should it be sane_grep, BTW?):\n\n  +\tgrep \"^alternate:\" actual >actual.alternates &&\n\nBut it might be just me... It's certainly not necessary.\n\n-- \nJakub Narębski\n\n"},{"id":"303368","messageId":"20161005144028.tjuvk3hkoqm3qjfd@sigill.intra.peff.net","threadId":"44205","inReplyTo":"2ea2f077-ab02-2631-4ce9-93cdd22c3c6b@gmail.com","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T14:40:28Z","receivedAt":"2016-10-05T14:40:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:\n\n> I would prefer the following:\n> \n> #   A --> B --> C --> D --> E --> F --> G --> H\n> #      0     1     2     3     4     5     6\n\nYeah, that is also more visually pleasing.\n\nHere's a squashable update that uses that and clarifies the points in\nthe discussion with Jacob.\n\nJunio, do you mind squashing this in to jk/alt-odb-cleanup?\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex b393613..62170b7 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '\n \t)\n '\n \n-# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n-# the depth at 0 and count links, not repositories, so in a chain like:\n+# Note: These tests depend on the hard-coded value of 5 as the maximum depth\n+# we will follow recursion. We start the depth at 0 and count links, not\n+# repositories. This means that in a chain like:\n #\n-#   A -> B -> C -> D -> E -> F -> G -> H\n-#      0    1    2    3    4    5    6\n+#   A --> B --> C --> D --> E --> F --> G --> H\n+#      0     1     2     3     4     5     6\n #\n-# we are OK at \"G\", but break at \"H\".\n+# we are OK at \"G\", but break at \"H\", even though \"H\" is actually the 8th\n+# repository, not the 6th, which you might expect. Counting the links allows\n+# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.\n #\n # Note also that we must use \"--bare -l\" to make the link to H. The \"-l\"\n # ensures we do not do a connectivity check, and the \"--bare\" makes sure\n@@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '\n \tgit clone --bare -l -s G H\n '\n \n-test_expect_success 'validity of fifth-deep repository' '\n+test_expect_success 'validity of seventh repository' '\n \tgit -C G fsck\n '\n \n-test_expect_success 'invalidity of sixth-deep repository' '\n+test_expect_success 'invalidity of eighth repository' '\n \ttest_must_fail git -C H fsck\n '\n \n"},{"id":"303381","messageId":"xmqqh98qrcwh.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"20161005144028.tjuvk3hkoqm3qjfd@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T16:14:38Z","receivedAt":"2016-10-05T16:14:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:\n>\n>> I would prefer the following:\n>> \n>> #   A --> B --> C --> D --> E --> F --> G --> H\n>> #      0     1     2     3     4     5     6\n>\n> Yeah, that is also more visually pleasing.\n>\n> Here's a squashable update that uses that and clarifies the points in\n> the discussion with Jacob.\n>\n> Junio, do you mind squashing this in to jk/alt-odb-cleanup?\n\nNo, I don't.\n\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index b393613..62170b7 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '\n>  \t)\n>  '\n>  \n> -# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> -# the depth at 0 and count links, not repositories, so in a chain like:\n> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth\n> +# we will follow recursion. We start the depth at 0 and count links, not\n> +# repositories. This means that in a chain like:\n>  #\n> -#   A -> B -> C -> D -> E -> F -> G -> H\n> -#      0    1    2    3    4    5    6\n> +#   A --> B --> C --> D --> E --> F --> G --> H\n> +#      0     1     2     3     4     5     6\n>  #\n> -# we are OK at \"G\", but break at \"H\".\n> +# we are OK at \"G\", but break at \"H\", even though \"H\" is actually the 8th\n> +# repository, not the 6th, which you might expect. Counting the links allows\n> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.\n>  #\n>  # Note also that we must use \"--bare -l\" to make the link to H. The \"-l\"\n>  # ensures we do not do a connectivity check, and the \"--bare\" makes sure\n> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '\n>  \tgit clone --bare -l -s G H\n>  '\n>  \n> -test_expect_success 'validity of fifth-deep repository' '\n> +test_expect_success 'validity of seventh repository' '\n>  \tgit -C G fsck\n>  '\n>  \n> -test_expect_success 'invalidity of sixth-deep repository' '\n> +test_expect_success 'invalidity of eighth repository' '\n>  \ttest_must_fail git -C H fsck\n>  '\n>  \n"},{"id":"303388","messageId":"CA+P7+xrhs0j+9sK22+5qR9bM6bwkua1rDZDkCg6HrtL5BDZBNA@mail.gmail.com","threadId":"44205","inReplyTo":"20161005144028.tjuvk3hkoqm3qjfd@sigill.intra.peff.net","subject":"Re: [PATCH 06/18] t5613: clarify \"too deep\" recursion tests","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-05T16:47:00Z","receivedAt":"2016-10-05T16:47:25Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 5, 2016 at 7:40 AM, Jeff King <peff@peff.net> wrote:\n> On Wed, Oct 05, 2016 at 03:58:53PM +0200, Jakub Narębski wrote:\n>\n>> I would prefer the following:\n>>\n>> #   A --> B --> C --> D --> E --> F --> G --> H\n>> #      0     1     2     3     4     5     6\n>\n> Yeah, that is also more visually pleasing.\n>\n> Here's a squashable update that uses that and clarifies the points in\n> the discussion with Jacob.\n>\n> Junio, do you mind squashing this in to jk/alt-odb-cleanup?\n>\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index b393613..62170b7 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -39,13 +39,16 @@ test_expect_success 'preparing third repository' '\n>         )\n>  '\n>\n> -# Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n> -# the depth at 0 and count links, not repositories, so in a chain like:\n> +# Note: These tests depend on the hard-coded value of 5 as the maximum depth\n> +# we will follow recursion. We start the depth at 0 and count links, not\n> +# repositories. This means that in a chain like:\n>  #\n> -#   A -> B -> C -> D -> E -> F -> G -> H\n> -#      0    1    2    3    4    5    6\n> +#   A --> B --> C --> D --> E --> F --> G --> H\n> +#      0     1     2     3     4     5     6\n\nYea this looks much better (when I view it locally, gmail still looks\naweful here but...)\n\n>  #\n> -# we are OK at \"G\", but break at \"H\".\n> +# we are OK at \"G\", but break at \"H\", even though \"H\" is actually the 8th\n> +# repository, not the 6th, which you might expect. Counting the links allows\n> +# N+1 repositories, and counting from 0 to 5 inclusive allows 6 links.\n>  #\n\n... This is much more clear wording that helps me understand this a\nlot more. Thanks!\n\nRegards,\nJake\n\n>  # Note also that we must use \"--bare -l\" to make the link to H. The \"-l\"\n>  # ensures we do not do a connectivity check, and the \"--bare\" makes sure\n> @@ -59,11 +62,11 @@ test_expect_success 'creating too deep nesting' '\n>         git clone --bare -l -s G H\n>  '\n>\n> -test_expect_success 'validity of fifth-deep repository' '\n> +test_expect_success 'validity of seventh repository' '\n>         git -C G fsck\n>  '\n>\n> -test_expect_success 'invalidity of sixth-deep repository' '\n> +test_expect_success 'invalidity of eighth repository' '\n>         test_must_fail git -C H fsck\n>  '\n>\n"},{"id":"303419","messageId":"62a7966c-fe98-f987-db10-3fc3b5f4b7e6@web.de","threadId":"44205","inReplyTo":"20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] alternate object database cleanups","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-05T18:47:23Z","receivedAt":"2016-10-05T18:47:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2016 um 22:33 schrieb Jeff King:\n> This series is the result of René nerd-sniping me with the claim that we\n> could \"easily\" teach count-objects to print out the list of alternates\n> in:\n>\n>   http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/\n\n1. Send crappy patch\n2. ????\n3. PROFIT!!!\n\nSometimes it works. :)\n\nThank you!\nRené\n"},{"id":"303420","messageId":"5df3d609-62c8-c5e7-d457-76e044ea6438@web.de","threadId":"44205","inReplyTo":"20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net","subject":"Re: [PATCH 16/18] count-objects: report alternates via verbose mode","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-05T18:47:27Z","receivedAt":"2016-10-05T18:47:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2016 um 22:36 schrieb Jeff King:\n> There's no way to get the list of alternates that git\n> computes internally; our tests only infer it based on which\n> objects are available. In addition to testing, knowing this\n> list may be helpful for somebody debugging their alternates\n> setup.\n>\n> Let's add it to the \"count-objects -v\" output. We could give\n> it a separate flag, but there's not really any need.\n> \"count-objects -v\" is already a debugging catch-all for the\n> object database, its output is easily extensible to new data\n> items, and printing the alternates is not expensive (we\n> already had to find them to count the objects).\n\nGood idea.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Documentation/git-count-objects.txt |  5 +++++\n>  builtin/count-objects.c             | 10 ++++++++++\n>  t/t5613-info-alternate.sh           | 10 ++++++++++\n>  3 files changed, 25 insertions(+)\n>\n> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\n> index 2ff3568..cb9b4d2 100644\n> --- a/Documentation/git-count-objects.txt\n> +++ b/Documentation/git-count-objects.txt\n> @@ -38,6 +38,11 @@ objects nor valid packs\n>  +\n>  size-garbage: disk space consumed by garbage files, in KiB (unless -H is\n>  specified)\n> ++\n> +alternate: absolute path of alternate object databases; may appear\n> +multiple times, one line per path. Note that if the path contains\n> +non-printable characters, it may be surrounded by double-quotes and\n> +contain C-style backslashed escape sequences.\n>\n>  -H::\n>  --human-readable::\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index ba92919..a700409 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -8,6 +8,7 @@\n>  #include \"dir.h\"\n>  #include \"builtin.h\"\n>  #include \"parse-options.h\"\n> +#include \"quote.h\"\n>\n>  static unsigned long garbage;\n>  static off_t size_garbage;\n> @@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char *path, void *data)\n>  \treturn 0;\n>  }\n>\n> +static int print_alternate(struct alternate_object_database *alt, void *data)\n> +{\n> +\tprintf(\"alternate: \");\n> +\tquote_c_style(alt->path, NULL, stdout, 0);\n> +\tputchar('\\n');\n> +\treturn 0;\n> +}\n\nYeah, quoting paths makes sense.\n\n> +\n>  static char const * const count_objects_usage[] = {\n>  \tN_(\"git count-objects [-v] [-H | --human-readable]\"),\n>  \tNULL\n> @@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>  \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n>  \t\tprintf(\"garbage: %lu\\n\", garbage);\n>  \t\tprintf(\"size-garbage: %s\\n\", garbage_buf.buf);\n> +\t\tforeach_alt_odb(print_alternate, NULL);\n>  \t\tstrbuf_release(&loose_buf);\n>  \t\tstrbuf_release(&pack_buf);\n>  \t\tstrbuf_release(&garbage_buf);\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index b393613..74f6770 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '\n>  \t)\n>  '\n>\n> +test_expect_success 'count-objects shows the alternates' '\n> +\tcat >expect <<-EOF &&\n> +\talternate: $(pwd)/B/.git/objects\n> +\talternate: $(pwd)/A/.git/objects\n> +\tEOF\n> +\tgit -C C count-objects -v >actual &&\n> +\tgrep ^alternate: actual >actual.alternates &&\n> +\ttest_cmp expect actual.alternates\n> +'\n> +\n>  # Note: These tests depend on the hard-coded value of 5 as \"too deep\". We start\n>  # the depth at 0 and count links, not repositories, so in a chain like:\n>  #\n>\n"},{"id":"303421","messageId":"40d3920f-2267-f76d-a5e0-6868fb9f9be2@web.de","threadId":"44205","inReplyTo":"20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-05T18:47:29Z","receivedAt":"2016-10-05T18:47:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2016 um 22:34 schrieb Jeff King:\n> When we add a new alternate to the list, we try to normalize\n> out any redundant \"..\", etc. However, we do not look at the\n> return value of normalize_path_copy(), and will happily\n> continue with a path that could not be normalized. Worse,\n> the normalizing process is done in-place, so we are left\n> with whatever half-finished working state the normalizing\n> function was in.\n>\n> Fortunately, this cannot cause us to read past the end of\n> our buffer, as that working state will always leave the\n> NUL from the original path in place. And we do tend to\n> notice problems when we check is_directory() on the path.\n> But you can see the nonsense that we feed to is_directory\n> with an entry like:\n>\n>   this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n>\n> in your objects/info/alternates, which yields:\n>\n>   error: object directory\n>   /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve\n>   does not exist; check .git/objects/info/alternates.\n>\n> We can easily fix this just by checking the return value.\n> But that makes it hard to generate a good error message,\n> since we're normalizing in-place and our input value has\n> been overwritten by cruft.\n>\n> Instead, let's provide a strbuf helper that does an in-place\n> normalize, but restores the original contents on error. This\n> uses a second buffer under the hood, which is slightly less\n> efficient, but this is not a performance-critical code path.\n\nHmm, in-place functions are quite rare in the strbuf collection.  It \nlooks like a good fit for the two callers and makes sense in general, \nthough.\n"},{"id":"303424","messageId":"20161005190445.4kxc2sqbbpfrnsjf@sigill.intra.peff.net","threadId":"44205","inReplyTo":"40d3920f-2267-f76d-a5e0-6868fb9f9be2@web.de","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T19:04:45Z","receivedAt":"2016-10-05T19:04:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 05, 2016 at 08:47:29PM +0200, René Scharfe wrote:\n\n> > Instead, let's provide a strbuf helper that does an in-place\n> > normalize, but restores the original contents on error. This\n> > uses a second buffer under the hood, which is slightly less\n> > efficient, but this is not a performance-critical code path.\n> \n> Hmm, in-place functions are quite rare in the strbuf collection.  It looks\n> like a good fit for the two callers and makes sense in general, though.\n\nYeah, I almost wrote \"strbuf_add_normalized_path()\" instead. But then\nthe callers end up having to do the allocate-and-swap thing themselves.\nAnd I think we're still set in the future to add that if somebody wants\nit (and we can then implement the in-place version in terms of it).\n\nAnother alternative is to observe that the strbuf is generally used in\nthe first place to make the path absolute. So another interface is\nperhaps something like:\n\n  strbuf_add_path(struct strbuf *sb, const char *path,\n                  const char *relative_base)\n  {\n        struct strbuf scratch = STRBUF_INIT;\n        int ret;\n\n        if (is_absolute_path(path))\n                strbuf_grow(sb, strlen(path));\n        else {\n                if (relative_path)\n                        strbuf_addstr(&scratch, path);\n                else {\n                        if (strbuf_getcwd(&scratch))\n                                return -1;\n                }\n                strbuf_addch(&scratch, '/');\n                strbuf_addstr(&scratch, path);\n\n                strbuf_grow(sb, scratch.len);\n                path = scratch.buf;\n        }\n\n        ret = normalize_path_copy(sb.buf + sb.len, path);\n        strbuf_release(&scratch);\n        return ret;\n  }\n\nI don't think its worth the complexity of interface for the spots in\nthis series, but maybe there are other places that could use it. I'll\nleave that to somebody else to explore if the ywant to.\n\n-Peff\n"},{"id":"303429","messageId":"xmqqvax6oagq.fsf@gitster.mtv.corp.google.com","threadId":"44205","inReplyTo":"CA+P7+xrBX684an5EzUUk+_Dtu6Ep_F+nB1JyWDWsZjUANWcFoA@mail.gmail.com","subject":"Re: [PATCH 13/18] fill_sha1_file: write \"boring\" characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T19:35:33Z","receivedAt":"2016-10-05T19:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n>> The cost of fill function having to do the same thing repeatedly is\n>> negligible, so I am OK with the result, but for fairness, this was\n>> not \"make the callers do this extra thing\", but was \"the caller can\n>> prepare these unchanging parts just once, and the fill function that\n>> is repeatedly run does not have to.\"\n>\n> Sure, but it's a pretty minor optimization and I think the result is\n> easier to understand.\n\nYes; in case it wasn't clear, my comment was merely for fairness to\nthe original code.  I do agree that the end result of this series\nmakes a very pleasant read.\n"},{"id":"305520","messageId":"CAGyf7-HWAMF8S+Bw3wcwJCS1Subc28KHjpSCc1__0qn-GSMyvA@mail.gmail.com","threadId":"44205","inReplyTo":"20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2016-11-07T23:42:35Z","receivedAt":"2016-11-07T23:51:56Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:\n> When we add a new alternate to the list, we try to normalize\n> out any redundant \"..\", etc. However, we do not look at the\n> return value of normalize_path_copy(), and will happily\n> continue with a path that could not be normalized. Worse,\n> the normalizing process is done in-place, so we are left\n> with whatever half-finished working state the normalizing\n> function was in.\n>\n\n<snip>\n\n> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>         }\n>\n>         strbuf_add_absolute_path(&objdirbuf, get_object_directory());\n> -       normalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n> +       if (strbuf_normalize_path(&objdirbuf) < 0)\n> +               die(\"unable to normalize object directory: %s\",\n> +                   objdirbuf.buf);\n\nThis appears to break the ability to use a relative alternate via an\nenvironment variable, since normalize_path_copy_len is explicitly\ndocumented \"Returns failure (non-zero) if a \"..\" component appears as\nfirst path\"\n\nFor example, when trying to run a rev-list over commits in two\nrepositories using GIT_ALTERNATE_OBJECT_DIRECTORIES, in 2.10.x and\nprior the following command works. I know the alternate worked\npreviously because I'm passing a commit that does not exist in the\nrepository I'm running the command in; it only exists in a repository\nlinked by alternate, as shown by the \"fatal: bad object\" when the\nalternates are rejected.\n\nBefore, using Git 2.7.4 (but I've verified this behavior through to\nand including 2.10.2):\n\nbturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $\nGIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects git\nrev-list --format=\"%H\" 2d8897c9ac29ce42c3442cf80ac977057045e7f6\n74de5497dfca9731e455d60552f9a8906e5dc1ac\n^6053a1eaa1c009dd11092d09a72f3c41af1b59ad\n^017caf31eca7c46eb3d1800fcac431cfa7147a01 --\ncommit 74de5497dfca9731e455d60552f9a8906e5dc1ac\n74de5497dfca9731e455d60552f9a8906e5dc1ac\ncommit 3528cf690cb37f6adb85b7bd40cc7a6118d4b598\n3528cf690cb37f6adb85b7bd40cc7a6118d4b598\ncommit 2d8897c9ac29ce42c3442cf80ac977057045e7f6\n2d8897c9ac29ce42c3442cf80ac977057045e7f6\ncommit 9c05f43f859375e392d90d23a13717c16d0fdcda\n9c05f43f859375e392d90d23a13717c16d0fdcda\n\nNow, using Git 2.11.0-rc0\n\nbturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $\nGIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects\n/opt/git/2.11.0-rc0/bin/git rev-list --format=\"%H\"\n2d8897c9ac29ce42c3442cf80ac977057045e7f6\n74de5497dfca9731e455d60552f9a8906e5dc1ac\n^6053a1eaa1c009dd11092d09a72f3c41af1b59ad\n^017caf31eca7c46eb3d1800fcac431cfa7147a01 --\nerror: unable to normalize alternate object path: ../0/objects\nerror: unable to normalize alternate object path: ../1/objects\nfatal: bad object 74de5497dfca9731e455d60552f9a8906e5dc1ac\n\nOther commits, like [1], suggest the ability to use relative paths in\nalternates is something still actively developed and enhanced. Is it\nintentional that this breaks the ability to use relative alternates?\nIf this is to be the \"new normal\", is there any other option when\nusing environment variables besides using absolute paths?\n\nBest regards,\nBryan Turner\n\n[1]: https://github.com/git/git/commit/087b6d584062f5b704356286d6445bcc84d686fb\n-- Also newly tagged in 2.11.0-rc0\n"},{"id":"305521","messageId":"20161108003034.apydvv3bav3s7ehq@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CAGyf7-HWAMF8S+Bw3wcwJCS1Subc28KHjpSCc1__0qn-GSMyvA@mail.gmail.com","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-08T00:30:34Z","receivedAt":"2016-11-08T00:30:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:\n\n> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n> >         }\n> >\n> >         strbuf_add_absolute_path(&objdirbuf, get_object_directory());\n> > -       normalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n> > +       if (strbuf_normalize_path(&objdirbuf) < 0)\n> > +               die(\"unable to normalize object directory: %s\",\n> > +                   objdirbuf.buf);\n> \n> This appears to break the ability to use a relative alternate via an\n> environment variable, since normalize_path_copy_len is explicitly\n> documented \"Returns failure (non-zero) if a \"..\" component appears as\n> first path\"\n\nThat shouldn't happen, though, because the path we are normalizing has\nbeen converted to an absolute path via strbuf_add_absolute_path. IOW, if\nyour relative path is \"../../../foo\", we should be feeding something\nlike \"/path/to/repo/.git/objects/../../../foo\" and normalizing that to\n\"/path/to/foo\".\n\nBut in your example, you see:\n\n  error: unable to normalize alternate object path: ../0/objects\n\nwhich cannot come from the code above, which calls die(). It should be\ncoming from the call in link_alt_odb_entry().\n\nI think what is happening is that relative paths via environment\nvariables have always been slightly broken, but happened to mostly work.\nIn prepare_alt_odb(), we call link_alt_odb_entries() with a NULL\nrelative_base. That function does two things with it:\n\n  - it may unconditionally dereference it for an error message, which\n    would cause a segfault. This is impossible to trigger in practice,\n    though, because the error message is related to the depth, which we\n    know will always be 0 here.\n\n  - we pass the NULL along to the singular link_alt_odb_entry().\n    That function only creates an absolute path if given a non-NULL\n    relative_base; otherwise we have always fed the path to\n    normalize_path_copy, which is nonsense for a relative path.\n\n    So normalize_path_copy() was _always_ returning an error there, but\n    we ignored it and used whatever happened to be left in the buffer\n    anyway. And because of the way normalize_path_copy() is implemented,\n    that happened to be the untouched original string in most cases. But\n    that's mostly an accident. I think it would not be for something\n    like \"foo/../../bar\", which is technically valid (if done from a\n    relative base that has at least one path component).\n\n    Moreover, it means we don't have an absolute path to our alternate\n    odb. So the path is taken as relative whenever we do an object\n    lookup, meaning it will behave differently between a bare repository\n    (where we chdir to $GIT_DIR) and one with a working tree (where we\n    are generally in the root of the working tree). It can even behave\n    differently in the same process if we chdir between object lookups.\n\nSo it did happen to work, but I'm not sure it was planned (and obviously\nwe have no test coverage for it). More on that below.\n\n> Other commits, like [1], suggest the ability to use relative paths in\n> alternates is something still actively developed and enhanced. Is it\n> intentional that this breaks the ability to use relative alternates?\n> If this is to be the \"new normal\", is there any other option when\n> using environment variables besides using absolute paths?\n\nNo, I had no intention of disallowing relative alternates (and as you\nnoticed, a commit from the same series actually expands the use of\nrelative alternates). My use has been entirely within info/alternates\nfiles, though, not via the environment.\n\nAs I said, I'm not sure this was ever meant to work, but as far as I can\ntell it mostly _has_ worked, modulo some quirks. So I think we should\nconsider it a regression for it to stop working in v2.11.\n\nThe obvious solution is one of:\n\n  1. Stop calling normalize() at all when we do not have a relative base\n     and the path is not absolute. This restores the original quirky\n     behavior (plus makes the \"foo/../../bar\" case work).\n\n     If we want to do the minimum before releasing v2.11, it would be\n     that. I'm not sure it leaves things in a very sane state, but at\n     least v2.11 does no harm, and anybody who cares can build saner\n     semantics for v2.12.\n\n  2. Fix it for real. Pass a real relative_base when linking from the\n     environment. The question is: what is the correct relative base? I\n     suppose \"getcwd() at the time we prepare the alt odb\" is\n     reasonable, and would behave similarly to older versions ($GIT_DIR\n     for bare repos, top of the working tree otherwise).\n\n     If we were designing from scratch, I think saner semantics would\n     probably be always relative from $GIT_DIR, or even always relative\n     from the object directory (i.e., behave as if the paths were given\n     in objects/info/alternates). But that breaks compatibility with\n     older versions. If we are treating this as a regression, it is not\n     very friendly to say \"you are still broken, but you might just need\n     to add an extra '..' to your path\".\n\nSo I dunno. I guess that inclines me towards (1), as it lets us punt on\nthe harder question.\n\n-Peff\n"},{"id":"305530","messageId":"CAGyf7-FYvUgvOZm0xvFAJx=8hSc4ji=YQ5dUm3B1unU_WOcjeQ@mail.gmail.com","threadId":"44205","inReplyTo":"20161108003034.apydvv3bav3s7ehq@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2016-11-08T01:12:43Z","receivedAt":"2016-11-08T01:12:50Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Nov 7, 2016 at 4:30 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:\n>\n>> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>> >         }\n>> >\n>> >         strbuf_add_absolute_path(&objdirbuf, get_object_directory());\n>> > -       normalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n>> > +       if (strbuf_normalize_path(&objdirbuf) < 0)\n>> > +               die(\"unable to normalize object directory: %s\",\n>> > +                   objdirbuf.buf);\n>>\n>> This appears to break the ability to use a relative alternate via an\n>> environment variable, since normalize_path_copy_len is explicitly\n>> documented \"Returns failure (non-zero) if a \"..\" component appears as\n>> first path\"\n>\n> That shouldn't happen, though, because the path we are normalizing has\n> been converted to an absolute path via strbuf_add_absolute_path. IOW, if\n> your relative path is \"../../../foo\", we should be feeding something\n> like \"/path/to/repo/.git/objects/../../../foo\" and normalizing that to\n> \"/path/to/foo\".\n>\n> But in your example, you see:\n>\n>   error: unable to normalize alternate object path: ../0/objects\n>\n> which cannot come from the code above, which calls die(). It should be\n> coming from the call in link_alt_odb_entry().\n\nAh, of course. I should have looked more closely at the call.\n\n<snip>\n\n> No, I had no intention of disallowing relative alternates (and as you\n> noticed, a commit from the same series actually expands the use of\n> relative alternates). My use has been entirely within info/alternates\n> files, though, not via the environment.\n>\n> As I said, I'm not sure this was ever meant to work, but as far as I can\n> tell it mostly _has_ worked, modulo some quirks. So I think we should\n> consider it a regression for it to stop working in v2.11.\n>\n> The obvious solution is one of:\n>\n>   1. Stop calling normalize() at all when we do not have a relative base\n>      and the path is not absolute. This restores the original quirky\n>      behavior (plus makes the \"foo/../../bar\" case work).\n>\n>      If we want to do the minimum before releasing v2.11, it would be\n>      that. I'm not sure it leaves things in a very sane state, but at\n>      least v2.11 does no harm, and anybody who cares can build saner\n>      semantics for v2.12.\n>\n>   2. Fix it for real. Pass a real relative_base when linking from the\n>      environment. The question is: what is the correct relative base? I\n>      suppose \"getcwd() at the time we prepare the alt odb\" is\n>      reasonable, and would behave similarly to older versions ($GIT_DIR\n>      for bare repos, top of the working tree otherwise).\n>\n>      If we were designing from scratch, I think saner semantics would\n>      probably be always relative from $GIT_DIR, or even always relative\n>      from the object directory (i.e., behave as if the paths were given\n>      in objects/info/alternates). But that breaks compatibility with\n>      older versions. If we are treating this as a regression, it is not\n>      very friendly to say \"you are still broken, but you might just need\n>      to add an extra '..' to your path\".\n>\n> So I dunno. I guess that inclines me towards (1), as it lets us punt on\n> the harder question.\n\nIs there anything I can do to help? I'm happy to test out changes.\nI've got a set of ~1,040 tests that verify all sorts of different Git\nbehaviors (those tests flagged this change, for example, and found a\nregression in git diff-tree in 2.0.2/2.0.3, among other things). I run\nthem on the \"newest\" patch release for every feature-bearing line of\nGit from 1.8.x up to 2.10 (currently 1.8.0.3, 1.8.1.5, 1.8.2.3,\n1.8.3.4, 1.8.4.5, 1.8.5.6, 1.9.5, 2.0.5, 2.1.4, 2.2.3, 2.3.10, 2.4.11,\n2.5.5, 2.6.6, 2.7.4, 2.8.4, 2.9.3 and 2.10.2), and I add in RCs of new\nas soon as they become available. (I also test Git for Windows; at the\nmoment I've got 1.8.0, 1.8.1.2, 1.8.3, 1.8.4, 1.8.5.2 and 1.9.5.1 from\nmsysgit and 2.3.7.1, 2.4.6, 2.5.3, 2.6.4, 2.7.4, 2.8.4, 2.9.3 and\n2.10.2 from Git for Windows. 2.11.0-rc0 on Windows passes my test\nsuite; it looks like it's not tagging the same git/git commit as\nv2.11.0-rc0 is.) I wish there was an easy way for me to open this up.\nAt the moment, it's something I can really only run in-house, as it\nwere.\n\nAt the moment I have limited ability to actually try to submit patches\nmyself. I really need to sit down and setup a working development\nenvironment for Git. (My current dream, if I could get such an\nenvironment running, is to follow up on your git blame-tree work.\n\n>\n> -Peff\n"},{"id":"305535","messageId":"20161108053333.jta7bmqsyvy2ijoh@sigill.intra.peff.net","threadId":"44205","inReplyTo":"CAGyf7-FYvUgvOZm0xvFAJx=8hSc4ji=YQ5dUm3B1unU_WOcjeQ@mail.gmail.com","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-08T05:33:34Z","receivedAt":"2016-11-08T05:34:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2016 at 05:12:43PM -0800, Bryan Turner wrote:\n\n> > The obvious solution is one of:\n> >\n> >   1. Stop calling normalize() at all when we do not have a relative base\n> >      and the path is not absolute. This restores the original quirky\n> >      behavior (plus makes the \"foo/../../bar\" case work).\n\nActually, I think we want to keep normalizing, as it is possible for\nrelative paths to normalize correctly (e.g., \"foo/../bar\"). We just need\nto ignore the error, which is easy.\n\nThe patch is below, and is the absolute minimum I think we'd need for\nv2.11.\n\nBeyond that, we could go further:\n\n  a. Actually make a real absolute path based on getcwd(), which would\n     protect against later chdir() calls, and possibly help with\n     duplicate suppression. I'm not sure there are actually any\n     triggerable bugs here, so I went for the minimal fix.\n\n  b. Pick a more sane base for the absolute path, like $GIT_DIR. If we\n     did so, then people using relative paths in\n     GIT_ALTERNATE_OBJECT_DIRECTORIES from a bare repo would continue to\n     work, and people in non-bare repositories would have to add an\n     extra \"..\" to most of their paths. So a slight regression, but\n     saner overall semantics.\n\n     Making it relative to the object directory ($GIT_DIR/objects, or\n     even whatever is in $GIT_OBJECT_DIRECTORY) makes even more sense\n     to me, but would regress even the bare case (and would probably be\n     \"interesting\" along with the tmp-objdir stuff, which sets\n     $GIT_OBJECT_DIRECTORY on the fly, as that would invalidate\n     $GIT_ALTERNATE_OBJECT_DIRECTORIES).\n\nI'm inclined to leave those to anybody interested post-v2.11 (or never,\nif nobody cares). But it would be pretty trivial to do (a) as part of\nthis initial fix if anybody feels strongly.\n\n> Is there anything I can do to help? I'm happy to test out changes.\n\nThe patch at the end of his mail obviously passes the newly-added tests\nfor me, but please confirm that it fixes your test suite.\n\nI gather your suite is about noticing behavior changes between different\nversions. For cases where we know there is an obvious right behavior, it\nwould be nice if you could contribute them as patches to git's test\nsuite. This case was overlooked because there was no test coverage at\nall.\n\nBarring that, running your suite and giving easily-reproducible problem\nreports is valuable. The earlier the better. So I am happy to see this\non -rc0, and not on the final release. Periodically running it on\n\"master\" during the development cycle would have caught it even sooner.\n\n> At the moment I have limited ability to actually try to submit patches\n> myself. I really need to sit down and setup a working development\n> environment for Git. (My current dream, if I could get such an\n> environment running, is to follow up on your git blame-tree work.\n\nI would be happy for somebody to pick that up, too. It has been powering\nGitHub's tree-view for several years now, but I know there are some\nrough edges as well as opportunities to optimize it.\n\n-- >8 --\nSubject: [PATCH] alternates: re-allow relative paths from environment\n\nCommit 670c359da (link_alt_odb_entry: handle normalize_path\nerrors, 2016-10-03) regressed the handling of relative paths\nin the GIT_ALTERNATE_OBJECT_DIRECTORIES variable. It's not\nentirely clear this was ever meant to work, but it _has_\nworked for several years, so this commit restores the\noriginal behavior.\n\nWhen we get a path in GIT_ALTERNATE_OBJECT_DIRECTORIES, we\nadd it the path to the list of alternate object directories\nas if it were found in objects/info/alternates, but with one\ndifference: we do not provide the link_alt_odb_entry()\nfunction with a base for relative paths. That function\ndoesn't turn it into an absolute path, and we end up feeding\nthe relative path to the strbuf_normalize_path() function.\n\nMost relative paths break out of the top-level directory\n(e.g., \"../foo.git/objects\"), and thus normalizing fails.\nPrior to 670c359da, we simply ignored the error, and due to\nthe way normalize_path_copy() was implemented it happened to\nreturn the original path in this case. We then accessed the\nalternate objects using this relative path.\n\nBy storing the relative path in the alt_odb list, the path\nis relative to wherever we happen to be at the time we do an\nobject lookup. That means we look from $GIT_DIR in a bare\nrepository, and from the top of the worktree in a non-bare\nrepository.\n\nIf this were being designed from scratch, it would make\nsense to pick a stable location (probably $GIT_DIR, or even\nthe object directory) and use that as the relative base,\nturning the result into an absolute path.  However, given\nthe history, at this point the minimal fix is to match the\npre-670c359da behavior.\n\nWe can do this simply by ignoring the error when we have no\nrelative base and using the original value (which we now\nreliably have, thanks to strbuf_normalize_path()).\n\nThat still leaves us with a relative path that foils our\nduplicate detection, and may act strangely if we ever\nchdir() later in the process. We could solve that by storing\nan absolute path based on getcwd(). That may be a good\nfuture direction; for now we'll do just the minimum to fix\nthe regression.\n\nThe new t5615 script demonstrates the fix in its final three\ntests. Since we didn't have any tests of the alternates\nenvironment variable at all, it also adds some tests of\nabsolute paths.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c              |  2 +-\n t/t5615-alternate-env.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 72 insertions(+), 1 deletion(-)\n create mode 100755 t/t5615-alternate-env.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5457314e6..9c86d1924 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -296,7 +296,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,\n \t}\n \tstrbuf_addstr(&pathbuf, entry);\n \n-\tif (strbuf_normalize_path(&pathbuf) < 0) {\n+\tif (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {\n \t\terror(\"unable to normalize alternate object path: %s\",\n \t\t      pathbuf.buf);\n \t\tstrbuf_release(&pathbuf);\ndiff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh\nnew file mode 100755\nindex 000000000..22d9d8178\n--- /dev/null\n+++ b/t/t5615-alternate-env.sh\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+\n+test_description='handling of alternates in environment variables'\n+. ./test-lib.sh\n+\n+check_obj () {\n+\talt=$1; shift\n+\twhile read obj expect\n+\tdo\n+\t\techo \"$obj\" >&3 &&\n+\t\techo \"$obj $expect\" >&4\n+\tdone 3>input 4>expect &&\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$alt \\\n+\t\tgit \"$@\" cat-file --batch-check='%(objectname) %(objecttype)' \\\n+\t\t<input >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'create alternate repositories' '\n+\tgit init --bare one.git &&\n+\tone=$(echo one | git -C one.git hash-object -w --stdin) &&\n+\tgit init --bare two.git &&\n+\ttwo=$(echo two | git -C two.git hash-object -w --stdin)\n+'\n+\n+test_expect_success 'objects inaccessible without alternates' '\n+\tcheck_obj \"\" <<-EOF\n+\t$one missing\n+\t$two missing\n+\tEOF\n+'\n+\n+test_expect_success 'access alternate via absolute path' '\n+\tcheck_obj \"$(pwd)/one.git/objects\" <<-EOF\n+\t$one blob\n+\t$two missing\n+\tEOF\n+'\n+\n+test_expect_success 'access multiple alternates' '\n+\tcheck_obj \"$(pwd)/one.git/objects:$(pwd)/two.git/objects\" <<-EOF\n+\t$one blob\n+\t$two blob\n+\tEOF\n+'\n+\n+# bare paths are relative from $GIT_DIR\n+test_expect_success 'access alternate via relative path (bare)' '\n+\tgit init --bare bare.git &&\n+\tcheck_obj \"../one.git/objects\" -C bare.git <<-EOF\n+\t$one blob\n+\tEOF\n+'\n+\n+# non-bare paths are relative to top of worktree\n+test_expect_success 'access alternate via relative path (worktree)' '\n+\tgit init worktree &&\n+\tcheck_obj \"../one.git/objects\" -C worktree <<-EOF\n+\t$one blob\n+\tEOF\n+'\n+\n+# path is computed after moving to top-level of worktree\n+test_expect_success 'access alternate via relative path (subdir)' '\n+\tmkdir subdir &&\n+\tcheck_obj \"one.git/objects\" -C subdir <<-EOF\n+\t$one blob\n+\tEOF\n+'\n+\n+test_done\n-- \n2.11.0.rc0.263.g6f44bc3\n\n"},{"id":"305564","messageId":"CAGyf7-HVUN-kHhmccoYUVVDE_xJt1UsiE8bxZiADsW5GFGr+OA@mail.gmail.com","threadId":"44205","inReplyTo":"20161108053333.jta7bmqsyvy2ijoh@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2016-11-08T19:27:41Z","receivedAt":"2016-11-08T19:27:48Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":">\n>> Is there anything I can do to help? I'm happy to test out changes.\n>\n> The patch at the end of his mail obviously passes the newly-added tests\n> for me, but please confirm that it fixes your test suite.\n>\n> I gather your suite is about noticing behavior changes between different\n> versions. For cases where we know there is an obvious right behavior, it\n> would be nice if you could contribute them as patches to git's test\n> suite. This case was overlooked because there was no test coverage at\n> all.\n>\n> Barring that, running your suite and giving easily-reproducible problem\n> reports is valuable. The earlier the better. So I am happy to see this\n> on -rc0, and not on the final release. Periodically running it on\n> \"master\" during the development cycle would have caught it even sooner.\n\nI've applied your patch to the tip of the 2.11.0-rc0 tag (just to make\nsure I don't accidentally pick up anything else on master; I'll test\nthat separately) and my full test suite passes without issue.\n\nI'm going to investigate whether I can setup a version of this build\nthat runs \"periodically\" (I'm not sure what that period will be)\nagainst git/git master. I've got a lot of the infrastructure in place,\nbut I'm going to need to automate a few things to make it really\nviable.\n\nAs for contributing extensions to the test suite, that's a good idea.\nI need to fast track getting a development environment setup.\n"}]}