{"thread":{"id":"50876","subject":"[PATCH 0/12] a rabbit hole of update-server-info fixes","startedAt":"2019-04-04T23:21:08Z","lastAt":"2019-04-09T15:15:15Z","messageCount":48,"participants":["Jeff King","Ramsay Jones","Junio C Hamano","René Scharfe","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"373139","messageId":"20190404232104.GA27770@sigill.intra.peff.net","threadId":"50876","inReplyTo":null,"subject":"[PATCH 0/12] a rabbit hole of update-server-info fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:21:04Z","receivedAt":"2019-04-04T23:21:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch series started with patch 12: I just wanted to drop the\nunused \"force\" parameter from update_info_refs().\n\nBut that made me look at its sibling update_info_packs(), and whether it\nneeds its force parameter. It _does_ in fact do something (though I am\nslightly doubtful that anybody actually cares in practice).\n\nHowever, I did see a memory bug in its parser, which is fixed by patch\n6. And a bunch of other cleanups, which are patches 7-11. Fortunately\nthat buggy parser is not used to look at info/packs files we pull over\ndumb-http; it has its own parser. So I looked at that, and of course it\nhad a bug, too (fixed in patch 5).\n\nAnd in those cleanups, I needed to get the basename of a pack from its\npacked_git. So I looked for other code that did the same, in case we\ncould benefit from a shared helper. And behold, the midx code does. But\nyou guessed it, there was a bug (actually 2). Fixed in patches 4 (with\nnecessary refactoring in patch 3).\n\nAnd while constructing a test for that bug, I noticed that one of the\nother tests in t5319 was buggy. So that's patch 1 (with some cleanup in\npatch 2).\n\nAnd here we are. I present them here in reverse rabbit-hole order (which\nis also roughly important fixes first, and minor cleanups last). The\nindividual chunks are mostly independent, but the server-info cleanups\nrely on the shared pack_basename() helper added as part of the midx fix.\n\n  [01/12]: t5319: fix bogus cat-file argument\n  [02/12]: t5319: drop useless --buffer from cat-file\n  [03/12]: packfile: factor out .pack to .idx name conversion\n  [04/12]: packfile: check midx coverage with .idx rather than .pack\n  [05/12]: http: simplify parsing of remote objects/info/packs\n  [06/12]: server-info: fix blind pointer arithmetic\n  [07/12]: server-info: simplify cleanup in parse_pack_def()\n  [08/12]: server-info: use strbuf to read old info/packs file\n  [09/12]: server-info: drop nr_alloc struct member\n  [10/12]: packfile.h: drop extern from function declarations\n  [11/12]: server-info: drop objdirlen pointer arithmetic\n  [12/12]: update_info_refs(): drop unused force parameter\n\n http.c                      | 35 ++++++---------\n packfile.c                  | 31 ++++++++++---\n packfile.h                  | 86 ++++++++++++++++++++-----------------\n server-info.c               | 57 +++++++++++-------------\n t/t5319-multi-pack-index.sh | 29 ++++++++++---\n 5 files changed, 132 insertions(+), 106 deletions(-)\n\n-Peff\n"},{"id":"373140","messageId":"20190404232203.GA21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 01/12] t5319: fix bogus cat-file argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:22:03Z","receivedAt":"2019-04-04T23:22:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no such argument as \"--unordered\"; it's spelled \"--unsorted\".\nBut our test failed to notice that cat-file didn't run at all because:\n\n  1. It lost the exit code of git on the left-hand side of a pipe.\n\n  2. It was comparing two runs of the broken invocation with and without\n     a particular config variable (and indeed, both cases produced no\n     output!).\n\nLet's fix the option, but also tweak the helper function to check the\nexit code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5319-multi-pack-index.sh | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 70926b5bc0..42f4d6cd01 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -86,13 +86,14 @@ test_expect_success 'write midx with one v1 pack' '\n '\n \n midx_git_two_modes () {\n+\tgit -c core.multiPackIndex=false $1 >expect &&\n+\tgit -c core.multiPackIndex=true $1 >actual &&\n \tif [ \"$2\" = \"sorted\" ]\n \tthen\n-\t\tgit -c core.multiPackIndex=false $1 | sort >expect &&\n-\t\tgit -c core.multiPackIndex=true $1 | sort >actual\n-\telse\n-\t\tgit -c core.multiPackIndex=false $1 >expect &&\n-\t\tgit -c core.multiPackIndex=true $1 >actual\n+\t\tsort <expect >expect.sorted &&\n+\t\tmv expect.sorted expect &&\n+\t\tsort <actual >actual.sorted &&\n+\t\tmv actual.sorted actual\n \tfi &&\n \ttest_cmp expect actual\n }\n@@ -104,7 +105,7 @@ compare_results_with_midx () {\n \t\tmidx_git_two_modes \"log --raw\" &&\n \t\tmidx_git_two_modes \"count-objects --verbose\" &&\n \t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unsorted\" sorted\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unordered\" sorted\n \t'\n }\n \n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373141","messageId":"20190404232236.GB21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 02/12] t5319: drop useless --buffer from cat-file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:22:37Z","receivedAt":"2019-04-04T23:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The cat-file --buffer option is the default already when using\n--batch-all-objects. It doesn't hurt to specify it, but it's nice for\nthe test scripts to model good usage.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5319-multi-pack-index.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 42f4d6cd01..8c4d2bd849 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -104,8 +104,8 @@ compare_results_with_midx () {\n \t\tmidx_git_two_modes \"rev-list --objects --all\" &&\n \t\tmidx_git_two_modes \"log --raw\" &&\n \t\tmidx_git_two_modes \"count-objects --verbose\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unordered\" sorted\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check\" &&\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check --unordered\" sorted\n \t'\n }\n \n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373142","messageId":"20190404232259.GC21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 03/12] packfile: factor out .pack to .idx name conversion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:22:59Z","receivedAt":"2019-04-04T23:23:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we open a pack .idx, we have to convert the \"foo.pack\" name stored\nin the packed_git struct to \"foo.idx\". This isn't too complicated, but\nwe do encode some policy in the form of a BUG(). Let's pull this into\nits own function so it can be reused.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 16bcb75262..054269ae5d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -186,18 +186,24 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \treturn 0;\n }\n \n+static char *pack_name_to_idx(const char *pack_name)\n+{\n+\tsize_t len;\n+\n+\tif (!strip_suffix(pack_name, \".pack\", &len))\n+\t\tBUG(\"pack_name does not end in .pack\");\n+\treturn xstrfmt(\"%.*s.idx\", (int)len, pack_name);\n+}\n+\n int open_pack_index(struct packed_git *p)\n {\n \tchar *idx_name;\n-\tsize_t len;\n \tint ret;\n \n \tif (p->index_data)\n \t\treturn 0;\n \n-\tif (!strip_suffix(p->pack_name, \".pack\", &len))\n-\t\tBUG(\"pack_name does not end in .pack\");\n-\tidx_name = xstrfmt(\"%.*s.idx\", (int)len, p->pack_name);\n+\tidx_name = pack_name_to_idx(p->pack_name);\n \tret = check_packed_git_idx(idx_name, p);\n \tfree(idx_name);\n \treturn ret;\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373143","messageId":"20190404232546.GD21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:25:46Z","receivedAt":"2019-04-04T23:25:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have a .midx that covers many packfiles, we try to avoid opening\nthe .idx for those packfiles. However, there are a few problems with the\nfilename comparison we use:\n\n  - we ask midx_contains_pack() about the .pack name, not the .idx name.\n    But it compares to the latter.\n\n  - we compute the basename of the pack using strrchr() to find the\n    final slash. But that leaves an extra \"/\" at the start of our\n    string; we need to advance past it.\n\n    That also raises the question of what to do when the name does not\n    have a slash at all. This should generally not happen (we always\n    find files in \"pack/\"), but it doesn't hurt to be defensive here.\n\nThe tests don't notice because there's nothing about opening those .idx\nfiles that would cause us to give incorrect output. It's just a little\nslower. The new test checks this case by corrupting the covered .idx,\nand then making sure we don't complain about it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c                  | 17 ++++++++++++++---\n t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n 2 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 054269ae5d..e7ca135ed5 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -472,6 +472,16 @@ static unsigned int get_max_fd_limit(void)\n #endif\n }\n \n+static const char *pack_basename(struct packed_git *p)\n+{\n+\tconst char *ret = strrchr(p->pack_name, '/');\n+\tif (ret)\n+\t\tret = ret + 1; /* skip past slash */\n+\telse\n+\t\tret = p->pack_name; /* we only have a base */\n+\treturn ret;\n+}\n+\n /*\n  * Do not call this directly as this leaks p->pack_fd on error return;\n  * call open_packed_git() instead.\n@@ -486,15 +496,16 @@ static int open_packed_git_1(struct packed_git *p)\n \tssize_t read_result;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \n-\tif (!p->index_data) {\n+\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n \t\tstruct multi_pack_index *m;\n-\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n+\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n \n \t\tfor (m = the_repository->objects->multi_pack_index;\n \t\t     m; m = m->next) {\n-\t\t\tif (midx_contains_pack(m, pack_name))\n+\t\t\tif (midx_contains_pack(m, idx_name))\n \t\t\t\tbreak;\n \t\t}\n+\t\tfree(idx_name);\n \n \t\tif (!m && open_pack_index(p))\n \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 8c4d2bd849..1ebf19ec3c 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -117,6 +117,20 @@ test_expect_success 'write midx with one v2 pack' '\n \n compare_results_with_midx \"one v2 pack\"\n \n+test_expect_success 'corrupt idx not opened' '\n+\tidx=$(test-tool read-midx $objdir | grep \"\\.idx\\$\") &&\n+\tmv $objdir/pack/$idx backup-$idx &&\n+\ttest_when_finished \"mv backup-\\$idx \\$objdir/pack/\\$idx\" &&\n+\n+\t# This is the minimum size for a sha-1 based .idx; this lets\n+\t# us pass perfunctory tests, but anything that actually opens and reads\n+\t# the idx file will complain.\n+\ttest_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&\n+\n+\tgit -c core.multiPackIndex=true rev-list --objects --all 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'add more objects' '\n \tfor i in $(test_seq 6 10)\n \tdo\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373144","messageId":"20190404232704.GE21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 05/12] http: simplify parsing of remote objects/info/packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:27:04Z","receivedAt":"2019-04-04T23:27:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can use skip_prefix() and parse_oid_hex() to continuously increment\nour pointer, rather than dealing with magic numbers. This also fixes a\nfew small shortcomings:\n\n  - if we see a 'P' line that does not match our expectations, we'll\n    leave our \"i\" counter in the middle of the line. So we'll parse:\n    \"P P P pack-1234.pack\" as if there was just one \"P\" which was not\n    intentional (though probably not that harmful).\n\n  - if we see a line with the right prefix, suffix, and length, i.e.\n    matching /P pack-.{40}.pack\\n/, we'll interpret the middle part as\n    hex without checking if it could be parsed. This could lead to us\n    looking at uninitialized garbage in the hash array. In practice this\n    means we'll just make a garbage request to the server which will\n    fail, though it's interesting that a malicious server could convince\n    us to leak 40 bytes of uninitialized stack to them.\n\n  - the current code is picky about seeing a newline at the end of file,\n    but we can easily be more liberal\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 35 ++++++++++++++---------------------\n 1 file changed, 14 insertions(+), 21 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a32ad36ddf..2ef47bc779 100644\n--- a/http.c\n+++ b/http.c\n@@ -2147,11 +2147,11 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n {\n \tstruct http_get_options options = {0};\n-\tint ret = 0, i = 0;\n-\tchar *url, *data;\n+\tint ret = 0;\n+\tchar *url;\n+\tconst char *data;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tstruct object_id oid;\n \n \tend_url_with_slash(&buf, base_url);\n \tstrbuf_addstr(&buf, \"objects/info/packs\");\n@@ -2163,24 +2163,17 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \t\tgoto cleanup;\n \n \tdata = buf.buf;\n-\twhile (i < buf.len) {\n-\t\tswitch (data[i]) {\n-\t\tcase 'P':\n-\t\t\ti++;\n-\t\t\tif (i + hexsz + 12 <= buf.len &&\n-\t\t\t    starts_with(data + i, \" pack-\") &&\n-\t\t\t    starts_with(data + i + hexsz + 6, \".pack\\n\")) {\n-\t\t\t\tget_sha1_hex(data + i + 6, hash);\n-\t\t\t\tfetch_and_setup_pack_index(packs_head, hash,\n-\t\t\t\t\t\t      base_url);\n-\t\t\t\ti += hexsz + 11;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\tdefault:\n-\t\t\twhile (i < buf.len && data[i] != '\\n')\n-\t\t\t\ti++;\n+\twhile (*data) {\n+\t\tif (skip_prefix(data, \"P pack-\", &data) &&\n+\t\t    !parse_oid_hex(data, &oid, &data) &&\n+\t\t    skip_prefix(data, \".pack\", &data) &&\n+\t\t    (*data == '\\n' || *data == '\\0')) {\n+\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n+\t\t} else {\n+\t\t\tdata = strchrnul(data, '\\n');\n \t\t}\n-\t\ti++;\n+\t\tif (*data)\n+\t\t\tdata++; /* skip past newline */\n \t}\n \n cleanup:\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373145","messageId":"20190404232743.GF21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 06/12] server-info: fix blind pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:27:43Z","receivedAt":"2019-04-04T23:27:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're writing out a new objects/info/packs file, we read back the\nold one to try to keep the ordering the same. When we see a line\nstarting with \"P\", we expect \"P pack-1234...\" and blindly jump to \"line\n+ 2\" to parse the pack name. If we saw a line with _just_ \"P\" and\nnothing else, we'd jump past the end of the buffer and start reading\narbitrary memory.\n\nThis shouldn't be a big attack vector, as the files are local to the\nrepository and written by us, but it's clearly worth fixing (we do read\nremote copies of the file for dumb-http fetches, but using a totally\ndifferent parser!).\n\nLet's instead use skip_prefix() here, which avoids pointer arithmetic\naltogether. Note that this converts our switch statement to an if/else\nchain, making it slightly more verbose. But it will also make it easier\nto do a few follow-on cleanups.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex e2b2d6a27a..b61a6be4c2 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -112,9 +112,9 @@ static struct pack_info *find_pack_by_name(const char *name)\n /* Returns non-zero when we detect that the info in the\n  * old file is useless.\n  */\n-static int parse_pack_def(const char *line, int old_cnt)\n+static int parse_pack_def(const char *packname, int old_cnt)\n {\n-\tstruct pack_info *i = find_pack_by_name(line + 2);\n+\tstruct pack_info *i = find_pack_by_name(packname);\n \tif (i) {\n \t\ti->old_num = old_cnt;\n \t\treturn 0;\n@@ -139,24 +139,26 @@ static int read_pack_info_file(const char *infofile)\n \t\treturn 1; /* nonexistent is not an error. */\n \n \twhile (fgets(line, sizeof(line), fp)) {\n+\t\tconst char *arg;\n \t\tint len = strlen(line);\n \t\tif (len && line[len-1] == '\\n')\n \t\t\tline[--len] = 0;\n \n \t\tif (!len)\n \t\t\tcontinue;\n \n-\t\tswitch (line[0]) {\n-\t\tcase 'P': /* P name */\n-\t\t\tif (parse_pack_def(line, old_cnt++))\n+\t\tif (skip_prefix(line, \"P \", &arg)) {\n+\t\t\t/* P name */\n+\t\t\tif (parse_pack_def(arg, old_cnt++))\n \t\t\t\tgoto out_stale;\n-\t\t\tbreak;\n-\t\tcase 'D': /* we used to emit D but that was misguided. */\n-\t\tcase 'T': /* we used to emit T but nobody uses it. */\n+\t\t} else if (line[0] == 'D') {\n+\t\t\t/* we used to emit D but that was misguided. */\n \t\t\tgoto out_stale;\n-\t\tdefault:\n+\t\t} else if (line[0] == 'T') {\n+\t\t\t/* we used to emit T but nobody uses it. */\n+\t\t\tgoto out_stale;\n+\t\t} else {\n \t\t\terror(\"unrecognized: %s\", line);\n-\t\t\tbreak;\n \t\t}\n \t}\n \tfclose(fp);\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373146","messageId":"20190404232808.GG21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 07/12] server-info: simplify cleanup in parse_pack_def()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:28:08Z","receivedAt":"2019-04-04T23:28:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two exits from the function: either we jump to the out_stale\nlabel or not. But in both exits we repeat our cleanup, and the only\ndifference is our return value. Let's just use a variable for the return\nvalue to avoid repeating ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex b61a6be4c2..ba44cece7f 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -133,6 +133,7 @@ static int read_pack_info_file(const char *infofile)\n \tFILE *fp;\n \tchar line[1000];\n \tint old_cnt = 0;\n+\tint stale = 1;\n \n \tfp = fopen_or_warn(infofile, \"r\");\n \tif (!fp)\n@@ -161,11 +162,11 @@ static int read_pack_info_file(const char *infofile)\n \t\t\terror(\"unrecognized: %s\", line);\n \t\t}\n \t}\n-\tfclose(fp);\n-\treturn 0;\n+\tstale = 0;\n+\n  out_stale:\n \tfclose(fp);\n-\treturn 1;\n+\treturn stale;\n }\n \n static int compare_info(const void *a_, const void *b_)\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373147","messageId":"20190404232842.GH21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 08/12] server-info: use strbuf to read old info/packs file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:28:42Z","receivedAt":"2019-04-04T23:28:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This old code uses fgets with a fixed-size buffer. Let's use a strbuf\ninstead, so we don't have to wonder if \"1000\" is big enough, or what\nhappens if we see a long line.\n\nThis also lets us drop our custom code to trim the newline.\n\nProbably nobody actually cares about the 1000-char limit (after all, the\nlines generally only say \"P pack-[0-9a-f]{40}.pack\"), so this is mostly\njust about cleanup/readability.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex ba44cece7f..d4115fecbb 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -131,40 +131,38 @@ static int parse_pack_def(const char *packname, int old_cnt)\n static int read_pack_info_file(const char *infofile)\n {\n \tFILE *fp;\n-\tchar line[1000];\n+\tstruct strbuf line = STRBUF_INIT;\n \tint old_cnt = 0;\n \tint stale = 1;\n \n \tfp = fopen_or_warn(infofile, \"r\");\n \tif (!fp)\n \t\treturn 1; /* nonexistent is not an error. */\n \n-\twhile (fgets(line, sizeof(line), fp)) {\n+\twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tconst char *arg;\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len-1] == '\\n')\n-\t\t\tline[--len] = 0;\n \n-\t\tif (!len)\n+\t\tif (!line.len)\n \t\t\tcontinue;\n \n-\t\tif (skip_prefix(line, \"P \", &arg)) {\n+\t\tif (skip_prefix(line.buf, \"P \", &arg)) {\n \t\t\t/* P name */\n \t\t\tif (parse_pack_def(arg, old_cnt++))\n \t\t\t\tgoto out_stale;\n-\t\t} else if (line[0] == 'D') {\n+\t\t} else if (line.buf[0] == 'D') {\n \t\t\t/* we used to emit D but that was misguided. */\n \t\t\tgoto out_stale;\n-\t\t} else if (line[0] == 'T') {\n+\t\t} else if (line.buf[0] == 'T') {\n \t\t\t/* we used to emit T but nobody uses it. */\n \t\t\tgoto out_stale;\n \t\t} else {\n-\t\t\terror(\"unrecognized: %s\", line);\n+\t\t\terror(\"unrecognized: %s\", line.buf);\n \t\t}\n \t}\n \tstale = 0;\n \n  out_stale:\n+\tstrbuf_release(&line);\n \tfclose(fp);\n \treturn stale;\n }\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373148","messageId":"20190404232913.GI21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 09/12] server-info: drop nr_alloc struct member","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:29:13Z","receivedAt":"2019-04-04T23:29:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep an array of struct pointers, with each one representing a single\npackfile. But for some reason there is a nr_alloc parameter inside each\nstruct, which has never been used.\n\nThis is probably cruft left over from development, where we might have\nwanted a nr_alloc to dynamically grow the list. But as it turns out, we\ndo not dynamically grow the list at all, but rather count up the total\nnumber of packs and use that as a maximum size. So while we're thinking\nof this, let's add an assert() that documents (and checks!) that our\nallocation and fill loops stay in sync.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex d4115fecbb..c9fbfd3a51 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -91,7 +91,6 @@ static struct pack_info {\n \tstruct packed_git *p;\n \tint old_num;\n \tint new_num;\n-\tint nr_alloc;\n } **info;\n static int num_pack;\n static const char *objdir;\n@@ -213,6 +212,7 @@ static void init_pack_info(const char *infofile, int force)\n \tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n+\t\tassert(i < num_pack);\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n \t\tinfo[i]->p = p;\n \t\tinfo[i]->old_num = -1;\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373149","messageId":"20190404233008.GJ21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 10/12] packfile.h: drop extern from function declarations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:30:08Z","receivedAt":"2019-04-04T23:30:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As CodingGuidelines recommends, we do not need an \"extern\" when\ndeclaring a public function. Let's drop these. Note that we leave the\nextern on report_garbage(), as that is actually a function pointer, not\na function itself.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI hesitated on this one, because it does have a minor conflict with pu.\nBut I just couldn't bring myself to add a new extern, nor to stick a\nnon-extern one amidst a bunch of others.\n\n packfile.h | 80 +++++++++++++++++++++++++++---------------------------\n 1 file changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/packfile.h b/packfile.h\nindex d70c6d9afb..b40fc34fb2 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -15,23 +15,23 @@ struct object_info;\n  *\n  * Example: odb_pack_name(out, sha1, \"idx\") => \".git/objects/pack/pack-1234..idx\"\n  */\n-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n+char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n /*\n  * Return the name of the (local) packfile with the specified sha1 in\n  * its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_name(const unsigned char *sha1);\n+char *sha1_pack_name(const unsigned char *sha1);\n \n /*\n  * Return the name of the (local) pack index file with the specified\n  * sha1 in its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_index_name(const unsigned char *sha1);\n+char *sha1_pack_index_name(const unsigned char *sha1);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\n \t\t\t\t      const char *file_pach, void *data);\n@@ -45,8 +45,8 @@ void for_each_file_in_pack_dir(const char *objdir,\n #define PACKDIR_FILE_GARBAGE 4\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n-extern void reprepare_packed_git(struct repository *r);\n-extern void install_packed_git(struct repository *r, struct packed_git *pack);\n+void reprepare_packed_git(struct repository *r);\n+void install_packed_git(struct repository *r, struct packed_git *pack);\n \n struct packed_git *get_packed_git(struct repository *r);\n struct list_head *get_packed_git_mru(struct repository *r);\n@@ -59,32 +59,32 @@ struct packed_git *get_all_packs(struct repository *r);\n  */\n unsigned long approximate_object_count(void);\n \n-extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n-\t\t\t\t\t struct packed_git *packs);\n+struct packed_git *find_sha1_pack(const unsigned char *sha1,\n+\t\t\t\t  struct packed_git *packs);\n \n-extern void pack_report(void);\n+void pack_report(void);\n \n /*\n  * mmap the index file for the specified packfile (if it is not\n  * already mmapped).  Return 0 on success.\n  */\n-extern int open_pack_index(struct packed_git *);\n+int open_pack_index(struct packed_git *);\n \n /*\n  * munmap the index file for the specified packfile (if it is\n  * currently mmapped).\n  */\n-extern void close_pack_index(struct packed_git *);\n+void close_pack_index(struct packed_git *);\n \n-extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n+uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n \n-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n-extern void close_pack_windows(struct packed_git *);\n-extern void close_pack(struct packed_git *);\n-extern void close_all_packs(struct raw_object_store *o);\n-extern void unuse_pack(struct pack_window **);\n-extern void clear_delta_base_cache(void);\n-extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n+unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n+void close_pack_windows(struct packed_git *);\n+void close_pack(struct packed_git *);\n+void close_all_packs(struct raw_object_store *o);\n+void unuse_pack(struct pack_window **);\n+void clear_delta_base_cache(void);\n+struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n \n /*\n  * Make sure that a pointer access into an mmap'd index file is within bounds,\n@@ -94,7 +94,7 @@ extern struct packed_git *add_packed_git(const char *path, size_t path_len, int\n  * (like the 64-bit extended offset table), as we compare the size to the\n  * fixed-length parts when we open the file.\n  */\n-extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n+void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n \n /*\n  * Perform binary search on a pack-index for a given oid. Packfile is expected to\n@@ -110,59 +110,59 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32\n  * at the SHA-1 within the mmapped index.  Return NULL if there is an\n  * error.\n  */\n-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n+const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n /*\n  * Like nth_packed_object_sha1, but write the data into the object specified by\n  * the the first argument.  Returns the first argument on success, and NULL on\n  * error.\n  */\n-extern const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n+const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n \n /*\n  * Return the offset of the nth object within the specified packfile.\n  * The index must already be opened.\n  */\n-extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n+off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n \n /*\n  * If the object named sha1 is present in the specified packfile,\n  * return its offset within the packfile; otherwise, return 0.\n  */\n-extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n+off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n \n-extern int is_pack_valid(struct packed_git *);\n-extern void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);\n-extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n-extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n-extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n+int is_pack_valid(struct packed_git *);\n+void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);\n+unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n+unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n+int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n \n-extern void release_pack_memory(size_t);\n+void release_pack_memory(size_t);\n \n /* global flag to enable extra checks when accessing packed objects */\n-extern int do_check_packed_object_crc;\n+int do_check_packed_object_crc;\n \n-extern int packed_object_info(struct repository *r,\n+int packed_object_info(struct repository *r,\n \t\t\t      struct packed_git *pack,\n \t\t\t      off_t offset, struct object_info *);\n \n-extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n-extern const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n+void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n+const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n \n /*\n  * Iff a pack file in the given repository contains the object named by sha1,\n  * return true and store its location to e.\n  */\n-extern int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e);\n+int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e);\n \n-extern int has_object_pack(const struct object_id *oid);\n+int has_object_pack(const struct object_id *oid);\n \n-extern int has_pack_index(const unsigned char *sha1);\n+int has_pack_index(const unsigned char *sha1);\n \n /*\n  * Return 1 if an object in a promisor packfile is or refers to the given\n  * object, 0 otherwise.\n  */\n-extern int is_promisor_object(const struct object_id *oid);\n+int is_promisor_object(const struct object_id *oid);\n \n /*\n  * Expose a function for fuzz testing.\n@@ -174,7 +174,7 @@ extern int is_promisor_object(const struct object_id *oid);\n  * have a convenient entry-point for fuzz testing. For real uses, you should\n  * probably use open_pack_index() or parse_pack_index() instead.\n  */\n-extern int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n-\t\t    size_t idx_size, struct packed_git *p);\n+int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n+\t     size_t idx_size, struct packed_git *p);\n \n #endif\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373150","messageId":"20190404233048.GK21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 11/12] server-info: drop objdirlen pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:30:48Z","receivedAt":"2019-04-04T23:30:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When writing objects/info/packs, we use the basename of each pack\n(i.e., just the \"pack-1234abcd.pack\" part). We compute that manually by\nadding \"objdirlen + 6\" to the name.\n\nThis _should_ work consistently, as we do not include non-local packs,\nmeaning everything should be in $objdir/pack/. Before f13d7db4af\n(server-info.c: use pack_local like everybody else., 2005-12-05), this\nwas definitely true, since we computed \"local\" based on comparing the\nobjdir string.  Since then, we're relying on the code on packfile.c to\nmatch our expectations of p->pack_name and p->local.\n\nI think our expectations do still hold today, but we can be a bit more\ndefensive by just using pack_basename() to get the base. That\nfuture-proofs us, and should hopefully be more obviously safe to\nsomebody reading the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c    |  2 +-\n packfile.h    |  6 ++++++\n server-info.c | 10 ++--------\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex e7ca135ed5..a0f92b7eef 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -472,7 +472,7 @@ static unsigned int get_max_fd_limit(void)\n #endif\n }\n \n-static const char *pack_basename(struct packed_git *p)\n+const char *pack_basename(struct packed_git *p)\n {\n \tconst char *ret = strrchr(p->pack_name, '/');\n \tif (ret)\ndiff --git a/packfile.h b/packfile.h\nindex b40fc34fb2..45bf792d79 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,6 +31,12 @@ char *sha1_pack_name(const unsigned char *sha1);\n  */\n char *sha1_pack_index_name(const unsigned char *sha1);\n \n+/*\n+ * Return the basename of the packfile, omitting any containing directory\n+ * (e.g., \"pack-1234abcd[...].pack\").\n+ */\n+const char *pack_basename(struct packed_git *p);\n+\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\ndiff --git a/server-info.c b/server-info.c\nindex c9fbfd3a51..ab03c1b3c2 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -93,16 +93,13 @@ static struct pack_info {\n \tint new_num;\n } **info;\n static int num_pack;\n-static const char *objdir;\n-static int objdirlen;\n \n static struct pack_info *find_pack_by_name(const char *name)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n \t\tstruct packed_git *p = info[i]->p;\n-\t\t/* skip \"/pack/\" after \".git/objects\" */\n-\t\tif (!strcmp(p->pack_name + objdirlen + 6, name))\n+\t\tif (!strcmp(pack_basename(p), name))\n \t\t\treturn info[i];\n \t}\n \treturn NULL;\n@@ -196,9 +193,6 @@ static void init_pack_info(const char *infofile, int force)\n \tint stale;\n \tint i = 0;\n \n-\tobjdir = get_object_directory();\n-\tobjdirlen = strlen(objdir);\n-\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t/* we ignore things on alternate path since they are\n \t\t * not available to the pullers in general.\n@@ -246,7 +240,7 @@ static int write_pack_info_file(FILE *fp)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n-\t\tif (fprintf(fp, \"P %s\\n\", info[i]->p->pack_name + objdirlen + 6) < 0)\n+\t\tif (fprintf(fp, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n \t\t\treturn -1;\n \t}\n \tif (fputc('\\n', fp) == EOF)\n-- \n2.21.0.714.gd1be1d035b\n\n"},{"id":"373151","messageId":"20190404233105.GL21839@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH 12/12] update_info_refs(): drop unused force parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:31:05Z","receivedAt":"2019-04-04T23:31:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time the force flag meant something when writing info/refs,\nbut it hasn't done anything since 60d0526aaa (Unoptimize info/refs\ncreation., 2005-09-14).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex ab03c1b3c2..41274d098b 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -78,7 +78,7 @@ static int generate_info_refs(FILE *fp)\n \treturn for_each_ref(add_info_ref, fp);\n }\n \n-static int update_info_refs(int force)\n+static int update_info_refs(void)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n \tint ret = update_info_file(path, generate_info_refs);\n@@ -269,7 +269,7 @@ int update_server_info(int force)\n \t */\n \tint errs = 0;\n \n-\terrs = errs | update_info_refs(force);\n+\terrs = errs | update_info_refs();\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\n-- \n2.21.0.714.gd1be1d035b\n"},{"id":"373156","messageId":"69f35074-7f83-ea74-41be-c6ea079a9de8@ramsayjones.plus.com","threadId":"50876","inReplyTo":"20190404232203.GA21839@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] t5319: fix bogus cat-file argument","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-04-05T00:44:09Z","receivedAt":"2019-04-05T00:44:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/04/2019 00:22, Jeff King wrote:\n> There's no such argument as \"--unordered\"; it's spelled \"--unsorted\".\n\nErr, isn't this back-to-front? (i.e. cat-file has the _option_\n\"--unordered\" but not \"--unsorted\").\n\nI suspect that I am not reading that right! :-D\n\nATB,\nRamsay Jones\n\n"},{"id":"373168","messageId":"20190405014110.GA25017@sigill.intra.peff.net","threadId":"50876","inReplyTo":"69f35074-7f83-ea74-41be-c6ea079a9de8@ramsayjones.plus.com","subject":"Re: [PATCH 01/12] t5319: fix bogus cat-file argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T01:41:10Z","receivedAt":"2019-04-05T01:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 01:44:09AM +0100, Ramsay Jones wrote:\n\n> \n> \n> On 05/04/2019 00:22, Jeff King wrote:\n> > There's no such argument as \"--unordered\"; it's spelled \"--unsorted\".\n> \n> Err, isn't this back-to-front? (i.e. cat-file has the _option_\n> \"--unordered\" but not \"--unsorted\").\n\nOops, yes. The patch is right; we are going the other way.\n\nSorry for the confusion.\n\n-Peff\n"},{"id":"373169","messageId":"20190405014649.GA25326@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405014110.GA25017@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] t5319: fix bogus cat-file argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T01:46:49Z","receivedAt":"2019-04-05T01:46:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 09:41:10PM -0400, Jeff King wrote:\n\n> On Fri, Apr 05, 2019 at 01:44:09AM +0100, Ramsay Jones wrote:\n> \n> > \n> > \n> > On 05/04/2019 00:22, Jeff King wrote:\n> > > There's no such argument as \"--unordered\"; it's spelled \"--unsorted\".\n> > \n> > Err, isn't this back-to-front? (i.e. cat-file has the _option_\n> > \"--unordered\" but not \"--unsorted\").\n> \n> Oops, yes. The patch is right; we are going the other way.\n\nReading my response I might have been unclear again. :) I mean the\nactual diff text of the patch is doing the right thing. It's just the\ndescription here which is wrong.\n\n-Peff\n"},{"id":"373184","messageId":"xmqqpnq1c8dc.fsf@gitster-ct.c.googlers.com","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"Re: [PATCH 0/12] a rabbit hole of update-server-info fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-05T05:46:07Z","receivedAt":"2019-04-05T05:46:12Z","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> This patch series started with patch 12: I just wanted to drop the\n> unused \"force\" parameter from update_info_refs().\n>\n> But that made me look at its sibling update_info_packs(), and whether it\n> ...\n> And here we are. I present them here in reverse rabbit-hole order (which\n> is also roughly important fixes first, and minor cleanups last). The\n> individual chunks are mostly independent, but the server-info cleanups\n> rely on the shared pack_basename() helper added as part of the midx fix.\n\nA kind of cover letter to make readers chuckle.  Well written.\n\nAnd of course, thanks.  It's a delight to read a nicely constructed\nseries like this one.\n\n>\n>   [01/12]: t5319: fix bogus cat-file argument\n>   [02/12]: t5319: drop useless --buffer from cat-file\n>   [03/12]: packfile: factor out .pack to .idx name conversion\n>   [04/12]: packfile: check midx coverage with .idx rather than .pack\n>   [05/12]: http: simplify parsing of remote objects/info/packs\n>   [06/12]: server-info: fix blind pointer arithmetic\n>   [07/12]: server-info: simplify cleanup in parse_pack_def()\n>   [08/12]: server-info: use strbuf to read old info/packs file\n>   [09/12]: server-info: drop nr_alloc struct member\n>   [10/12]: packfile.h: drop extern from function declarations\n>   [11/12]: server-info: drop objdirlen pointer arithmetic\n>   [12/12]: update_info_refs(): drop unused force parameter\n>\n>  http.c                      | 35 ++++++---------\n>  packfile.c                  | 31 ++++++++++---\n>  packfile.h                  | 86 ++++++++++++++++++++-----------------\n>  server-info.c               | 57 +++++++++++-------------\n>  t/t5319-multi-pack-index.sh | 29 ++++++++++---\n>  5 files changed, 132 insertions(+), 106 deletions(-)\n>\n> -Peff\n"},{"id":"373186","messageId":"726ad2d6-ab39-8f5e-1b2d-f991c8765a08@web.de","threadId":"50876","inReplyTo":"20190404232546.GD21839@sigill.intra.peff.net","subject":"Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-04-05T08:05:29Z","receivedAt":"2019-04-05T08:05:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.2019 um 01:25 schrieb Jeff King:\n> When we have a .midx that covers many packfiles, we try to avoid opening\n> the .idx for those packfiles. However, there are a few problems with the\n> filename comparison we use:\n>\n>    - we ask midx_contains_pack() about the .pack name, not the .idx name.\n>      But it compares to the latter.\n>\n>    - we compute the basename of the pack using strrchr() to find the\n>      final slash. But that leaves an extra \"/\" at the start of our\n>      string; we need to advance past it.\n>\n>      That also raises the question of what to do when the name does not\n>      have a slash at all. This should generally not happen (we always\n>      find files in \"pack/\"), but it doesn't hurt to be defensive here.\n>\n> The tests don't notice because there's nothing about opening those .idx\n> files that would cause us to give incorrect output. It's just a little\n> slower. The new test checks this case by corrupting the covered .idx,\n> and then making sure we don't complain about it.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   packfile.c                  | 17 ++++++++++++++---\n>   t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n>   2 files changed, 28 insertions(+), 3 deletions(-)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 054269ae5d..e7ca135ed5 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n\n> @@ -486,15 +496,16 @@ static int open_packed_git_1(struct packed_git *p)\n>   \tssize_t read_result;\n>   \tconst unsigned hashsz = the_hash_algo->rawsz;\n>\n> -\tif (!p->index_data) {\n> +\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n\nSo if there is no multi_pack_index, we skip this block now...\n\n>   \t\tstruct multi_pack_index *m;\n> -\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n> +\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n>\n>   \t\tfor (m = the_repository->objects->multi_pack_index;\n>   \t\t     m; m = m->next) {\n> -\t\t\tif (midx_contains_pack(m, pack_name))\n> +\t\t\tif (midx_contains_pack(m, idx_name))\n>   \t\t\t\tbreak;\n>   \t\t}\n> +\t\tfree(idx_name);\n>\n>   \t\tif (!m && open_pack_index(p))\n>   \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n\n... which also means this open_pack_index() call isn't done anymore if\nthere's no .midx file at all.  You don't mention this change in the\ncommit message; is it intended?\n\nAnd I wonder if it would be easier overall to let midx_contains_pack()\naccept .pack names in addition to .idx names.  Perhaps with something\nlike this?\n\nint cmp_idx_or_pack_name(const char *idx_or_pack_name, const char *idx_name)\n{\n\twhile (*idx_name && *idx_name == *idx_or_pack_name) {\n\t\tidx_name++;\n\t\tidx_or_pack_name++;\n\t}\n\tif (!strcmp(idx_name, \".idx\") && !strcmp(idx_or_pack_name, \".pack\"))\n\t\treturn 0;\n\treturn strcmp(idx_or_pack_name, idx_name);\n}\n\nRené\n"},{"id":"373192","messageId":"83129937-dcd0-f16e-c8aa-97eceec9769a@web.de","threadId":"50876","inReplyTo":"20190404232704.GE21839@sigill.intra.peff.net","subject":"Re: [PATCH 05/12] http: simplify parsing of remote objects/info/packs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-04-05T10:41:27Z","receivedAt":"2019-04-05T10:41:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.2019 um 01:27 schrieb Jeff King:\n> We can use skip_prefix() and parse_oid_hex() to continuously increment\n> our pointer, rather than dealing with magic numbers. This also fixes a\n> few small shortcomings:\n>\n>   - if we see a 'P' line that does not match our expectations, we'll\n>     leave our \"i\" counter in the middle of the line. So we'll parse:\n>     \"P P P pack-1234.pack\" as if there was just one \"P\" which was not\n>     intentional (though probably not that harmful).\n\nHow so?  The default case, which we'd fall through to, skips the rest\nof such a line, doesn't it?\n\n>\n>   - if we see a line with the right prefix, suffix, and length, i.e.\n>     matching /P pack-.{40}.pack\\n/, we'll interpret the middle part as\n>     hex without checking if it could be parsed. This could lead to us\n>     looking at uninitialized garbage in the hash array. In practice this\n>     means we'll just make a garbage request to the server which will\n>     fail, though it's interesting that a malicious server could convince\n>     us to leak 40 bytes of uninitialized stack to them.\n>\n>   - the current code is picky about seeing a newline at the end of file,\n>     but we can easily be more liberal\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http.c | 35 ++++++++++++++---------------------\n>  1 file changed, 14 insertions(+), 21 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index a32ad36ddf..2ef47bc779 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2147,11 +2147,11 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n>  int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n>  {\n>  \tstruct http_get_options options = {0};\n> -\tint ret = 0, i = 0;\n> -\tchar *url, *data;\n> +\tint ret = 0;\n> +\tchar *url;\n> +\tconst char *data;\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tunsigned char hash[GIT_MAX_RAWSZ];\n> -\tconst unsigned hexsz = the_hash_algo->hexsz;\n> +\tstruct object_id oid;\n>\n>  \tend_url_with_slash(&buf, base_url);\n>  \tstrbuf_addstr(&buf, \"objects/info/packs\");\n> @@ -2163,24 +2163,17 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n>  \t\tgoto cleanup;\n>\n>  \tdata = buf.buf;\n> -\twhile (i < buf.len) {\n> -\t\tswitch (data[i]) {\n> -\t\tcase 'P':\n> -\t\t\ti++;\n> -\t\t\tif (i + hexsz + 12 <= buf.len &&\n> -\t\t\t    starts_with(data + i, \" pack-\") &&\n> -\t\t\t    starts_with(data + i + hexsz + 6, \".pack\\n\")) {\n> -\t\t\t\tget_sha1_hex(data + i + 6, hash);\n> -\t\t\t\tfetch_and_setup_pack_index(packs_head, hash,\n> -\t\t\t\t\t\t      base_url);\n> -\t\t\t\ti += hexsz + 11;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\tdefault:\n> -\t\t\twhile (i < buf.len && data[i] != '\\n')\n> -\t\t\t\ti++;\n> +\twhile (*data) {\n> +\t\tif (skip_prefix(data, \"P pack-\", &data) &&\n> +\t\t    !parse_oid_hex(data, &oid, &data) &&\n> +\t\t    skip_prefix(data, \".pack\", &data) &&\n> +\t\t    (*data == '\\n' || *data == '\\0')) {\n> +\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n> +\t\t} else {\n> +\t\t\tdata = strchrnul(data, '\\n');\n>  \t\t}\n> -\t\ti++;\n> +\t\tif (*data)\n> +\t\t\tdata++; /* skip past newline */\n\nSo much simpler, *and* converted to object_id -- I like it!\n\nParsing \"P\" and \"pack-\" together crosses logical token boundaries,\nbut that I don't mind it here.\n\nRené\n\n"},{"id":"373195","messageId":"20190405120120.GA8796@szeder.dev","threadId":"50876","inReplyTo":"20190404232546.GD21839@sigill.intra.peff.net","subject":"Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-05T12:01:20Z","receivedAt":"2019-04-05T12:01:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 04, 2019 at 07:25:46PM -0400, Jeff King wrote:\n> When we have a .midx that covers many packfiles, we try to avoid opening\n> the .idx for those packfiles. However, there are a few problems with the\n> filename comparison we use:\n> \n>   - we ask midx_contains_pack() about the .pack name, not the .idx name.\n>     But it compares to the latter.\n> \n>   - we compute the basename of the pack using strrchr() to find the\n>     final slash. But that leaves an extra \"/\" at the start of our\n>     string; we need to advance past it.\n> \n>     That also raises the question of what to do when the name does not\n>     have a slash at all. This should generally not happen (we always\n>     find files in \"pack/\"), but it doesn't hurt to be defensive here.\n> \n> The tests don't notice because there's nothing about opening those .idx\n> files that would cause us to give incorrect output. It's just a little\n> slower. The new test checks this case by corrupting the covered .idx,\n> and then making sure we don't complain about it.\n\nWhen the test suite is run with GIT_TEST_MULTI_PACK_INDEX=1, the\nfollowing tests fail with this patch:\n\n  t1006-cat-file.sh                                (Wstat: 256 Tests: 111 Failed: 1)\n    Failed test:  87\n    Non-zero exit status: 1\n  t5320-delta-islands.sh                           (Wstat: 256 Tests: 15 Failed: 10)\n    Failed tests:  2-4, 6-10, 12-13\n    Non-zero exit status: 1\n  t5310-pack-bitmaps.sh                            (Wstat: 256 Tests: 46 Failed: 1)\n    Failed test:  44\n    Non-zero exit status: 1\n  t5570-git-daemon.sh                              (Wstat: 256 Tests: 21 Failed: 1)\n    Failed test:  10\n    Non-zero exit status: 1\n\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  packfile.c                  | 17 ++++++++++++++---\n>  t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n>  2 files changed, 28 insertions(+), 3 deletions(-)\n> \n> diff --git a/packfile.c b/packfile.c\n> index 054269ae5d..e7ca135ed5 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -472,6 +472,16 @@ static unsigned int get_max_fd_limit(void)\n>  #endif\n>  }\n>  \n> +static const char *pack_basename(struct packed_git *p)\n> +{\n> +\tconst char *ret = strrchr(p->pack_name, '/');\n> +\tif (ret)\n> +\t\tret = ret + 1; /* skip past slash */\n> +\telse\n> +\t\tret = p->pack_name; /* we only have a base */\n> +\treturn ret;\n> +}\n> +\n>  /*\n>   * Do not call this directly as this leaks p->pack_fd on error return;\n>   * call open_packed_git() instead.\n> @@ -486,15 +496,16 @@ static int open_packed_git_1(struct packed_git *p)\n>  \tssize_t read_result;\n>  \tconst unsigned hashsz = the_hash_algo->rawsz;\n>  \n> -\tif (!p->index_data) {\n> +\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n>  \t\tstruct multi_pack_index *m;\n> -\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n> +\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n>  \n>  \t\tfor (m = the_repository->objects->multi_pack_index;\n>  \t\t     m; m = m->next) {\n> -\t\t\tif (midx_contains_pack(m, pack_name))\n> +\t\t\tif (midx_contains_pack(m, idx_name))\n>  \t\t\t\tbreak;\n>  \t\t}\n> +\t\tfree(idx_name);\n>  \n>  \t\tif (!m && open_pack_index(p))\n>  \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 8c4d2bd849..1ebf19ec3c 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -117,6 +117,20 @@ test_expect_success 'write midx with one v2 pack' '\n>  \n>  compare_results_with_midx \"one v2 pack\"\n>  \n> +test_expect_success 'corrupt idx not opened' '\n> +\tidx=$(test-tool read-midx $objdir | grep \"\\.idx\\$\") &&\n> +\tmv $objdir/pack/$idx backup-$idx &&\n> +\ttest_when_finished \"mv backup-\\$idx \\$objdir/pack/\\$idx\" &&\n> +\n> +\t# This is the minimum size for a sha-1 based .idx; this lets\n> +\t# us pass perfunctory tests, but anything that actually opens and reads\n> +\t# the idx file will complain.\n> +\ttest_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&\n> +\n> +\tgit -c core.multiPackIndex=true rev-list --objects --all 2>err &&\n> +\ttest_must_be_empty err\n> +'\n> +\n>  test_expect_success 'add more objects' '\n>  \tfor i in $(test_seq 6 10)\n>  \tdo\n> -- \n> 2.21.0.714.gd1be1d035b\n> \n"},{"id":"373196","messageId":"20190405132125.GA1460@sigill.intra.peff.net","threadId":"50876","inReplyTo":"726ad2d6-ab39-8f5e-1b2d-f991c8765a08@web.de","subject":"Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T13:21:25Z","receivedAt":"2019-04-05T13:21:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 10:05:29AM +0200, René Scharfe wrote:\n\n> > @@ -486,15 +496,16 @@ static int open_packed_git_1(struct packed_git *p)\n> >   \tssize_t read_result;\n> >   \tconst unsigned hashsz = the_hash_algo->rawsz;\n> >\n> > -\tif (!p->index_data) {\n> > +\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n> \n> So if there is no multi_pack_index, we skip this block now...\n> \n> >   \t\tstruct multi_pack_index *m;\n> > -\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n> > +\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n> >\n> >   \t\tfor (m = the_repository->objects->multi_pack_index;\n> >   \t\t     m; m = m->next) {\n> > -\t\t\tif (midx_contains_pack(m, pack_name))\n> > +\t\t\tif (midx_contains_pack(m, idx_name))\n> >   \t\t\t\tbreak;\n> >   \t\t}\n> > +\t\tfree(idx_name);\n> >\n> >   \t\tif (!m && open_pack_index(p))\n> >   \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n> \n> ... which also means this open_pack_index() call isn't done anymore if\n> there's no .midx file at all.  You don't mention this change in the\n> commit message; is it intended?\n\nDoh, thank you for catching that. I made that switch at the last minute\nbecause I didn't want to pay the malloc/free cost when we had no list to\ncompare to. I'm surprised it works at all. :-/\n\nI guess it doesn't, from the other message in the thread.\n\n> And I wonder if it would be easier overall to let midx_contains_pack()\n> accept .pack names in addition to .idx names.  Perhaps with something\n> like this?\n> \n> int cmp_idx_or_pack_name(const char *idx_or_pack_name, const char *idx_name)\n> {\n> \twhile (*idx_name && *idx_name == *idx_or_pack_name) {\n> \t\tidx_name++;\n> \t\tidx_or_pack_name++;\n> \t}\n> \tif (!strcmp(idx_name, \".idx\") && !strcmp(idx_or_pack_name, \".pack\"))\n> \t\treturn 0;\n> \treturn strcmp(idx_or_pack_name, idx_name);\n> }\n\nHmm, maybe. It does a binary search, so I'd have to scratch my head for\na minute of whether this loose comparison is correct. I think it is\nbecause of that final strcmp.\n\n-Peff\n"},{"id":"373197","messageId":"20190405133044.GB1460@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405132125.GA1460@sigill.intra.peff.net","subject":"Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T13:30:44Z","receivedAt":"2019-04-05T13:30:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 09:21:25AM -0400, Jeff King wrote:\n\n> > ... which also means this open_pack_index() call isn't done anymore if\n> > there's no .midx file at all.  You don't mention this change in the\n> > commit message; is it intended?\n> \n> Doh, thank you for catching that. I made that switch at the last minute\n> because I didn't want to pay the malloc/free cost when we had no list to\n> compare to. I'm surprised it works at all. :-/\n> \n> I guess it doesn't, from the other message in the thread.\n\nHmm, no, that's a different problem. I think we'd generally already have\nthe pack open unless there _is_ a midx, so this conditional probably\nnever kicks in unless we have a midx anyway. Meaning my malloc-avoidance\nwas over-thought. So we could just get rid of it. Or do it like this:\n\ndiff --git a/packfile.c b/packfile.c\nindex e7ca135ed5..ef0f959311 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -496,17 +496,22 @@ static int open_packed_git_1(struct packed_git *p)\n \tssize_t read_result;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \n-\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n-\t\tstruct multi_pack_index *m;\n-\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n+\tif (!p->index_data) {\n+\t\tstruct multi_pack_index *m =\n+\t\t\tthe_repository->objects->multi_pack_index;\n \n-\t\tfor (m = the_repository->objects->multi_pack_index;\n-\t\t     m; m = m->next) {\n-\t\t\tif (midx_contains_pack(m, idx_name))\n-\t\t\t\tbreak;\n+\t\tif (m) {\n+\t\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n+\n+\t\t\tfor (; m; m = m->next) {\n+\t\t\t\tif (midx_contains_pack(m, idx_name))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfree(idx_name);\n \t\t}\n-\t\tfree(idx_name);\n \n+\t\tif (!m)\n+\t\t\twarning(\"opening anyway\");\n \t\tif (!m && open_pack_index(p))\n \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \t}\n\nI'll have to re-roll to address the other problem, though, so I'll give\nit some thought.\n\n-Peff\n"},{"id":"373199","messageId":"20190405134037.GC1460@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405120120.GA8796@szeder.dev","subject":"Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T13:40:38Z","receivedAt":"2019-04-05T13:40:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 02:01:20PM +0200, SZEDER Gábor wrote:\n\n> > The tests don't notice because there's nothing about opening those .idx\n> > files that would cause us to give incorrect output. It's just a little\n> > slower. The new test checks this case by corrupting the covered .idx,\n> > and then making sure we don't complain about it.\n> \n> When the test suite is run with GIT_TEST_MULTI_PACK_INDEX=1, the\n> following tests fail with this patch:\n> \n>   t1006-cat-file.sh                                (Wstat: 256 Tests: 111 Failed: 1)\n>     Failed test:  87\n>     Non-zero exit status: 1\n>   t5320-delta-islands.sh                           (Wstat: 256 Tests: 15 Failed: 10)\n>     Failed tests:  2-4, 6-10, 12-13\n>     Non-zero exit status: 1\n>   t5310-pack-bitmaps.sh                            (Wstat: 256 Tests: 46 Failed: 1)\n>     Failed test:  44\n>     Non-zero exit status: 1\n>   t5570-git-daemon.sh                              (Wstat: 256 Tests: 21 Failed: 1)\n>     Failed test:  10\n>     Non-zero exit status: 1\n\nThanks. It looks like this is not a bug in my patch per se, but rather\nthat the feature of the midx code it is un-breaking is not actually a\ngood idea.\n\nThe problem is with generating the pack revindex. It needs the actual\nindex for each pack to be loaded, and now open_packed_git() will\nsometimes quietly not do that.\n\nProbably we _could_ be generating a big revindex off of the midx, but in\nthe meantime, the revindex code needs to make sure that the index is\nloaded before looking at it.\n\nThis patch clears up those test failures (I'll re-roll the whole series\nwith it in the proper spot).\n\n---\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 4695aaf6b4..3960ad94c8 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -308,7 +308,8 @@ static int load_pack_bitmap(struct bitmap_index *bitmap_git)\n \n \tbitmap_git->bitmaps = kh_init_sha1();\n \tbitmap_git->ext_index.positions = kh_init_sha1_pos();\n-\tload_pack_revindex(bitmap_git->pack);\n+\tif (load_pack_revindex(bitmap_git->pack))\n+\t\tgoto failed;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 50891f77a2..d28a7e43d0 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pack-revindex.h\"\n #include \"object-store.h\"\n+#include \"packfile.h\"\n \n /*\n  * Pack index for existing packs give us easy access to the offsets into\n@@ -158,10 +159,14 @@ static void create_pack_revindex(struct packed_git *p)\n \tsort_revindex(p->revindex, num_ent, p->pack_size);\n }\n \n-void load_pack_revindex(struct packed_git *p)\n+int load_pack_revindex(struct packed_git *p)\n {\n-\tif (!p->revindex)\n+\tif (!p->revindex) {\n+\t\tif (open_pack_index(p))\n+\t\t\treturn -1;\n \t\tcreate_pack_revindex(p);\n+\t}\n+\treturn 0;\n }\n \n int find_revindex_position(struct packed_git *p, off_t ofs)\n@@ -188,7 +193,9 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)\n {\n \tint pos;\n \n-\tload_pack_revindex(p);\n+\tif (load_pack_revindex(p))\n+\t\treturn NULL;\n+\n \tpos = find_revindex_position(p, ofs);\n \n \tif (pos < 0)\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex e262f3efe8..848331d5d6 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -8,7 +8,7 @@ struct revindex_entry {\n \tunsigned int nr;\n };\n \n-void load_pack_revindex(struct packed_git *p);\n+int load_pack_revindex(struct packed_git *p);\n int find_revindex_position(struct packed_git *p, off_t ofs);\n \n struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs);\ndiff --git a/packfile.c b/packfile.c\nindex e7ca135ed5..ca73a5b12a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2040,8 +2040,10 @@ int for_each_object_in_pack(struct packed_git *p,\n \tuint32_t i;\n \tint r = 0;\n \n-\tif (flags & FOR_EACH_OBJECT_PACK_ORDER)\n-\t\tload_pack_revindex(p);\n+\tif (flags & FOR_EACH_OBJECT_PACK_ORDER) {\n+\t\tif (load_pack_revindex(p))\n+\t\t\treturn -1;\n+\t}\n \n \tfor (i = 0; i < p->num_objects; i++) {\n \t\tuint32_t pos;\n"},{"id":"373225","messageId":"20190405180306.GA21113@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190404232104.GA27770@sigill.intra.peff.net","subject":"[PATCH v2 0/13] a rabbit hole of update-server-info (and midx!) fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:03:07Z","receivedAt":"2019-04-05T18:03:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 07:21:04PM -0400, Jeff King wrote:\n\n> This patch series started with patch 12: I just wanted to drop the\n> unused \"force\" parameter from update_info_refs().\n> [...]\n> And here we are. I present them here in reverse rabbit-hole order (which\n> is also roughly important fixes first, and minor cleanups last). The\n> individual chunks are mostly independent, but the server-info cleanups\n> rely on the shared pack_basename() helper added as part of the midx fix.\n\nHere's a v2 which addresses the feedback on the last round (thank you\nboth). In particular:\n\n  - the rabbit hole goes one deeper, as fixing the midx bug reveals that\n    the revindex code just assumes that the pack .idx has been loaded.\n    Patch 2 corrects this, which addresses most of the test failures\n    with GIT_TEST_MULTI_PACK_INDEX.\n\n  - There was one other failure, which is that t5570 corrupted a .idx\n    and expected us to notice (which no longer happens when the midx is\n    able to provide the answer). I've tweaked the test to address this.\n\n  - I took René's suggestion of making midx_contains_pack() friendlier\n    by handling either a .pack or a .idx argument. I had to tweak the\n    suggested code just a little, and I added comments, since it took me\n    a little staring to make sure everything was correct.\n\n    I split that out from the basename fix, so that's now patches 5 and\n    6. It also lets us drop the patch to factor out pack_name_to_idx().\n\nOther than that, it's substantially the same as v1. I did move the\npublic declaration of pack_basename() to the patch where it is added,\nrather than doing it later, which shows up in the range diff below.\n\n  [01/13]: packfile.h: drop extern from function declarations\n  [02/13]: pack-revindex: open index if necessary\n  [03/13]: t5319: fix bogus cat-file argument\n  [04/13]: t5319: drop useless --buffer from cat-file\n  [05/13]: midx: check both pack and index names for containment\n  [06/13]: packfile: fix pack basename computation\n  [07/13]: http: simplify parsing of remote objects/info/packs\n  [08/13]: server-info: fix blind pointer arithmetic\n  [09/13]: server-info: simplify cleanup in parse_pack_def()\n  [10/13]: server-info: use strbuf to read old info/packs file\n  [11/13]: server-info: drop nr_alloc struct member\n  [12/13]: server-info: drop objdirlen pointer arithmetic\n  [13/13]: update_info_refs(): drop unused force parameter\n\n http.c                      | 35 ++++++---------\n midx.c                      | 36 +++++++++++++++-\n midx.h                      |  2 +-\n pack-bitmap.c               |  3 +-\n pack-revindex.c             | 13 ++++--\n pack-revindex.h             |  2 +-\n packfile.c                  | 18 ++++++--\n packfile.h                  | 86 ++++++++++++++++++++-----------------\n server-info.c               | 57 +++++++++++-------------\n t/t5319-multi-pack-index.sh | 29 ++++++++++---\n t/t5570-git-daemon.sh       |  1 +\n 11 files changed, 172 insertions(+), 110 deletions(-)\n\n10:  bead52a5dc =  1:  829e666ec9 packfile.h: drop extern from function declarations\n -:  ---------- >  2:  d46c0eafd0 pack-revindex: open index if necessary\n 1:  3d1978ccaf =  3:  5b965de360 t5319: fix bogus cat-file argument\n 2:  ce74312004 =  4:  32ef03e059 t5319: drop useless --buffer from cat-file\n 3:  ea637c9cf1 <  -:  ---------- packfile: factor out .pack to .idx name conversion\n -:  ---------- >  5:  d87a2aa2bf midx: check both pack and index names for containment\n 4:  4723e1b952 !  6:  c2bed91a3f packfile: check midx coverage with .idx rather than .pack\n    @@ -1,35 +1,40 @@\n     Author: Jeff King <peff@peff.net>\n     \n    -    packfile: check midx coverage with .idx rather than .pack\n    +    packfile: fix pack basename computation\n     \n    -    When we have a .midx that covers many packfiles, we try to avoid opening\n    -    the .idx for those packfiles. However, there are a few problems with the\n    -    filename comparison we use:\n    +    When we have a multi-pack-index that covers many packfiles, we try to\n    +    avoid opening the .idx for those packfiles. To do that we feed the pack\n    +    name to midx_contains_pack(). But that function wants to see only the\n    +    basename, which we compute using strrchr() to find the final slash. But\n    +    that leaves an extra \"/\" at the start of our string.\n     \n    -      - we ask midx_contains_pack() about the .pack name, not the .idx name.\n    -        But it compares to the latter.\n    +    We can fix this by incrementing the pointer. That also raises the\n    +    question of what to do when the name does not have a '/' at all. This\n    +    should generally not happen (we always find files in \"pack/\"), but it\n    +    doesn't hurt to be defensive here.\n     \n    -      - we compute the basename of the pack using strrchr() to find the\n    -        final slash. But that leaves an extra \"/\" at the start of our\n    -        string; we need to advance past it.\n    -\n    -        That also raises the question of what to do when the name does not\n    -        have a slash at all. This should generally not happen (we always\n    -        find files in \"pack/\"), but it doesn't hurt to be defensive here.\n    +    Let's wrap all of that up in a helper function and make it publicly\n    +    available, since a later patch will need to use it, too.\n     \n         The tests don't notice because there's nothing about opening those .idx\n         files that would cause us to give incorrect output. It's just a little\n         slower. The new test checks this case by corrupting the covered .idx,\n         and then making sure we don't complain about it.\n     \n    +    We also have to tweak t5570, which intentionally corrupts a .idx file\n    +    and expects us to notice it. When run with GIT_TEST_MULTI_PACK_INDEX,\n    +    this will fail since we now will (correctly) not bother opening the .idx\n    +    at all. We can fix that by unconditionally dropping any midx that's\n    +    there, which ensures we'll have to read the .idx.\n    +\n      diff --git a/packfile.c b/packfile.c\n      --- a/packfile.c\n      +++ b/packfile.c\n     @@\n      #endif\n      }\n      \n    -+static const char *pack_basename(struct packed_git *p)\n    ++const char *pack_basename(struct packed_git *p)\n     +{\n     +\tconst char *ret = strrchr(p->pack_name, '/');\n     +\tif (ret)\n    @@ -43,25 +48,31 @@\n       * Do not call this directly as this leaks p->pack_fd on error return;\n       * call open_packed_git() instead.\n     @@\n    - \tssize_t read_result;\n    - \tconst unsigned hashsz = the_hash_algo->rawsz;\n      \n    --\tif (!p->index_data) {\n    -+\tif (!p->index_data && the_repository->objects->multi_pack_index) {\n    + \tif (!p->index_data) {\n      \t\tstruct multi_pack_index *m;\n     -\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n    -+\t\tchar *idx_name = pack_name_to_idx(pack_basename(p));\n    ++\t\tconst char *pack_name = pack_basename(p);\n      \n      \t\tfor (m = the_repository->objects->multi_pack_index;\n      \t\t     m; m = m->next) {\n    --\t\t\tif (midx_contains_pack(m, pack_name))\n    -+\t\t\tif (midx_contains_pack(m, idx_name))\n    - \t\t\t\tbreak;\n    - \t\t}\n    -+\t\tfree(idx_name);\n    +\n    + diff --git a/packfile.h b/packfile.h\n    + --- a/packfile.h\n    + +++ b/packfile.h\n    +@@\n    +  */\n    + char *sha1_pack_index_name(const unsigned char *sha1);\n      \n    - \t\tif (!m && open_pack_index(p))\n    - \t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n    ++/*\n    ++ * Return the basename of the packfile, omitting any containing directory\n    ++ * (e.g., \"pack-1234abcd[...].pack\").\n    ++ */\n    ++const char *pack_basename(struct packed_git *p);\n    ++\n    + struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n    + \n    + typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\n     \n      diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n      --- a/t/t5319-multi-pack-index.sh\n    @@ -87,3 +98,15 @@\n      test_expect_success 'add more objects' '\n      \tfor i in $(test_seq 6 10)\n      \tdo\n    +\n    + diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n    + --- a/t/t5570-git-daemon.sh\n    + +++ b/t/t5570-git-daemon.sh\n    +@@\n    + test_expect_success 'fetch notices corrupt idx' '\n    + \tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n    + \t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n    ++\t rm -f objects/pack/multi-pack-index &&\n    + \t p=$(ls objects/pack/pack-*.idx) &&\n    + \t chmod u+w $p &&\n    + \t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n 5:  0bf3fb7758 =  7:  15a7d0c690 http: simplify parsing of remote objects/info/packs\n 6:  3f711f74f1 =  8:  aee572065c server-info: fix blind pointer arithmetic\n 7:  5a77637699 =  9:  a9d226960a server-info: simplify cleanup in parse_pack_def()\n 8:  cbc7b6fa71 = 10:  388e7bf2be server-info: use strbuf to read old info/packs file\n 9:  9601aa240c = 11:  76ca72f4bf server-info: drop nr_alloc struct member\n11:  23ff6ba3dc ! 12:  8753c537a7 server-info: drop objdirlen pointer arithmetic\n    @@ -18,36 +18,6 @@\n         future-proofs us, and should hopefully be more obviously safe to\n         somebody reading the code.\n     \n    - diff --git a/packfile.c b/packfile.c\n    - --- a/packfile.c\n    - +++ b/packfile.c\n    -@@\n    - #endif\n    - }\n    - \n    --static const char *pack_basename(struct packed_git *p)\n    -+const char *pack_basename(struct packed_git *p)\n    - {\n    - \tconst char *ret = strrchr(p->pack_name, '/');\n    - \tif (ret)\n    -\n    - diff --git a/packfile.h b/packfile.h\n    - --- a/packfile.h\n    - +++ b/packfile.h\n    -@@\n    -  */\n    - char *sha1_pack_index_name(const unsigned char *sha1);\n    - \n    -+/*\n    -+ * Return the basename of the packfile, omitting any containing directory\n    -+ * (e.g., \"pack-1234abcd[...].pack\").\n    -+ */\n    -+const char *pack_basename(struct packed_git *p);\n    -+\n    - struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n    - \n    - typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\n    -\n      diff --git a/server-info.c b/server-info.c\n      --- a/server-info.c\n      +++ b/server-info.c\n12:  3f9cff4360 = 13:  96c5a26a99 update_info_refs(): drop unused force parameter\n"},{"id":"373226","messageId":"20190405180340.GA32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:03:41Z","receivedAt":"2019-04-05T18:03:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As CodingGuidelines recommends, we do not need an \"extern\" when\ndeclaring a public function. Let's drop these. Note that we leave the\nextern on report_garbage(), as that is actually a function pointer, not\na function itself.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is bumped to the front in v2 since we now add our public function\nto it much earlier.\n\n packfile.h | 80 +++++++++++++++++++++++++++---------------------------\n 1 file changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/packfile.h b/packfile.h\nindex d70c6d9afb..b40fc34fb2 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -15,23 +15,23 @@ struct object_info;\n  *\n  * Example: odb_pack_name(out, sha1, \"idx\") => \".git/objects/pack/pack-1234..idx\"\n  */\n-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n+char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n /*\n  * Return the name of the (local) packfile with the specified sha1 in\n  * its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_name(const unsigned char *sha1);\n+char *sha1_pack_name(const unsigned char *sha1);\n \n /*\n  * Return the name of the (local) pack index file with the specified\n  * sha1 in its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_index_name(const unsigned char *sha1);\n+char *sha1_pack_index_name(const unsigned char *sha1);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\n \t\t\t\t      const char *file_pach, void *data);\n@@ -45,8 +45,8 @@ void for_each_file_in_pack_dir(const char *objdir,\n #define PACKDIR_FILE_GARBAGE 4\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n-extern void reprepare_packed_git(struct repository *r);\n-extern void install_packed_git(struct repository *r, struct packed_git *pack);\n+void reprepare_packed_git(struct repository *r);\n+void install_packed_git(struct repository *r, struct packed_git *pack);\n \n struct packed_git *get_packed_git(struct repository *r);\n struct list_head *get_packed_git_mru(struct repository *r);\n@@ -59,32 +59,32 @@ struct packed_git *get_all_packs(struct repository *r);\n  */\n unsigned long approximate_object_count(void);\n \n-extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n-\t\t\t\t\t struct packed_git *packs);\n+struct packed_git *find_sha1_pack(const unsigned char *sha1,\n+\t\t\t\t  struct packed_git *packs);\n \n-extern void pack_report(void);\n+void pack_report(void);\n \n /*\n  * mmap the index file for the specified packfile (if it is not\n  * already mmapped).  Return 0 on success.\n  */\n-extern int open_pack_index(struct packed_git *);\n+int open_pack_index(struct packed_git *);\n \n /*\n  * munmap the index file for the specified packfile (if it is\n  * currently mmapped).\n  */\n-extern void close_pack_index(struct packed_git *);\n+void close_pack_index(struct packed_git *);\n \n-extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n+uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n \n-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n-extern void close_pack_windows(struct packed_git *);\n-extern void close_pack(struct packed_git *);\n-extern void close_all_packs(struct raw_object_store *o);\n-extern void unuse_pack(struct pack_window **);\n-extern void clear_delta_base_cache(void);\n-extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n+unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n+void close_pack_windows(struct packed_git *);\n+void close_pack(struct packed_git *);\n+void close_all_packs(struct raw_object_store *o);\n+void unuse_pack(struct pack_window **);\n+void clear_delta_base_cache(void);\n+struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n \n /*\n  * Make sure that a pointer access into an mmap'd index file is within bounds,\n@@ -94,7 +94,7 @@ extern struct packed_git *add_packed_git(const char *path, size_t path_len, int\n  * (like the 64-bit extended offset table), as we compare the size to the\n  * fixed-length parts when we open the file.\n  */\n-extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n+void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n \n /*\n  * Perform binary search on a pack-index for a given oid. Packfile is expected to\n@@ -110,59 +110,59 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32\n  * at the SHA-1 within the mmapped index.  Return NULL if there is an\n  * error.\n  */\n-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n+const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n /*\n  * Like nth_packed_object_sha1, but write the data into the object specified by\n  * the the first argument.  Returns the first argument on success, and NULL on\n  * error.\n  */\n-extern const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n+const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n \n /*\n  * Return the offset of the nth object within the specified packfile.\n  * The index must already be opened.\n  */\n-extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n+off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n \n /*\n  * If the object named sha1 is present in the specified packfile,\n  * return its offset within the packfile; otherwise, return 0.\n  */\n-extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n+off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n \n-extern int is_pack_valid(struct packed_git *);\n-extern void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);\n-extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n-extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n-extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n+int is_pack_valid(struct packed_git *);\n+void *unpack_entry(struct repository *r, struct packed_git *, off_t, enum object_type *, unsigned long *);\n+unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n+unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n+int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n \n-extern void release_pack_memory(size_t);\n+void release_pack_memory(size_t);\n \n /* global flag to enable extra checks when accessing packed objects */\n-extern int do_check_packed_object_crc;\n+int do_check_packed_object_crc;\n \n-extern int packed_object_info(struct repository *r,\n+int packed_object_info(struct repository *r,\n \t\t\t      struct packed_git *pack,\n \t\t\t      off_t offset, struct object_info *);\n \n-extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n-extern const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n+void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n+const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n \n /*\n  * Iff a pack file in the given repository contains the object named by sha1,\n  * return true and store its location to e.\n  */\n-extern int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e);\n+int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e);\n \n-extern int has_object_pack(const struct object_id *oid);\n+int has_object_pack(const struct object_id *oid);\n \n-extern int has_pack_index(const unsigned char *sha1);\n+int has_pack_index(const unsigned char *sha1);\n \n /*\n  * Return 1 if an object in a promisor packfile is or refers to the given\n  * object, 0 otherwise.\n  */\n-extern int is_promisor_object(const struct object_id *oid);\n+int is_promisor_object(const struct object_id *oid);\n \n /*\n  * Expose a function for fuzz testing.\n@@ -174,7 +174,7 @@ extern int is_promisor_object(const struct object_id *oid);\n  * have a convenient entry-point for fuzz testing. For real uses, you should\n  * probably use open_pack_index() or parse_pack_index() instead.\n  */\n-extern int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n-\t\t    size_t idx_size, struct packed_git *p);\n+int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n+\t     size_t idx_size, struct packed_git *p);\n \n #endif\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373227","messageId":"20190405180423.GB32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 02/13] pack-revindex: open index if necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:04:24Z","receivedAt":"2019-04-05T18:04:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can't create a pack revindex if we haven't actually looked at the\nindex. Normally we would never get as far as creating a revindex without\nhaving already been looking in the pack, so this code never bothered to\ndouble-check that pack->index_data had been loaded.\n\nBut with the new multi-pack-index feature, many code paths might not\nload the individual pack .idx at all (they'd find objects via the midx\nand then open the .pack, but not its index).\n\nThis can't yet be triggered in practice, because a bug in the midx code\nmeans we accidentally open up the individual .idx files anyway. But in\npreparation for fixing that, let's have the revindex code check that\neverything it needs has been loaded.\n\nIn most cases this will just be a quick noop. But note that this does\nintroduce a possibility of error (if we have to open the index and it's\ncorrupt), so load_pack_revindex() now returns a result code, and callers\nneed to handle the error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-bitmap.c   |  3 ++-\n pack-revindex.c | 13 ++++++++++---\n pack-revindex.h |  2 +-\n packfile.c      |  6 ++++--\n 4 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 4695aaf6b4..3960ad94c8 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -308,7 +308,8 @@ static int load_pack_bitmap(struct bitmap_index *bitmap_git)\n \n \tbitmap_git->bitmaps = kh_init_sha1();\n \tbitmap_git->ext_index.positions = kh_init_sha1_pos();\n-\tload_pack_revindex(bitmap_git->pack);\n+\tif (load_pack_revindex(bitmap_git->pack))\n+\t\tgoto failed;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 50891f77a2..d28a7e43d0 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pack-revindex.h\"\n #include \"object-store.h\"\n+#include \"packfile.h\"\n \n /*\n  * Pack index for existing packs give us easy access to the offsets into\n@@ -158,10 +159,14 @@ static void create_pack_revindex(struct packed_git *p)\n \tsort_revindex(p->revindex, num_ent, p->pack_size);\n }\n \n-void load_pack_revindex(struct packed_git *p)\n+int load_pack_revindex(struct packed_git *p)\n {\n-\tif (!p->revindex)\n+\tif (!p->revindex) {\n+\t\tif (open_pack_index(p))\n+\t\t\treturn -1;\n \t\tcreate_pack_revindex(p);\n+\t}\n+\treturn 0;\n }\n \n int find_revindex_position(struct packed_git *p, off_t ofs)\n@@ -188,7 +193,9 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)\n {\n \tint pos;\n \n-\tload_pack_revindex(p);\n+\tif (load_pack_revindex(p))\n+\t\treturn NULL;\n+\n \tpos = find_revindex_position(p, ofs);\n \n \tif (pos < 0)\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex e262f3efe8..848331d5d6 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -8,7 +8,7 @@ struct revindex_entry {\n \tunsigned int nr;\n };\n \n-void load_pack_revindex(struct packed_git *p);\n+int load_pack_revindex(struct packed_git *p);\n int find_revindex_position(struct packed_git *p, off_t ofs);\n \n struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs);\ndiff --git a/packfile.c b/packfile.c\nindex 16bcb75262..6e40bd89c7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2023,8 +2023,10 @@ int for_each_object_in_pack(struct packed_git *p,\n \tuint32_t i;\n \tint r = 0;\n \n-\tif (flags & FOR_EACH_OBJECT_PACK_ORDER)\n-\t\tload_pack_revindex(p);\n+\tif (flags & FOR_EACH_OBJECT_PACK_ORDER) {\n+\t\tif (load_pack_revindex(p))\n+\t\t\treturn -1;\n+\t}\n \n \tfor (i = 0; i < p->num_objects; i++) {\n \t\tuint32_t pos;\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373228","messageId":"20190405180456.GC32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 03/13] t5319: fix bogus cat-file argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:04:56Z","receivedAt":"2019-04-05T18:04:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no such argument as \"--unsorted\"; it's spelled \"--unordered\".\nBut our test failed to notice that cat-file didn't run at all because:\n\n  1. It lost the exit code of git on the left-hand side of a pipe.\n\n  2. It was comparing two runs of the broken invocation with and without\n     a particular config variable (and indeed, both cases produced no\n     output!).\n\nLet's fix the option, but also tweak the helper function to check the\nexit code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5319-multi-pack-index.sh | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 70926b5bc0..42f4d6cd01 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -86,13 +86,14 @@ test_expect_success 'write midx with one v1 pack' '\n '\n \n midx_git_two_modes () {\n+\tgit -c core.multiPackIndex=false $1 >expect &&\n+\tgit -c core.multiPackIndex=true $1 >actual &&\n \tif [ \"$2\" = \"sorted\" ]\n \tthen\n-\t\tgit -c core.multiPackIndex=false $1 | sort >expect &&\n-\t\tgit -c core.multiPackIndex=true $1 | sort >actual\n-\telse\n-\t\tgit -c core.multiPackIndex=false $1 >expect &&\n-\t\tgit -c core.multiPackIndex=true $1 >actual\n+\t\tsort <expect >expect.sorted &&\n+\t\tmv expect.sorted expect &&\n+\t\tsort <actual >actual.sorted &&\n+\t\tmv actual.sorted actual\n \tfi &&\n \ttest_cmp expect actual\n }\n@@ -104,7 +105,7 @@ compare_results_with_midx () {\n \t\tmidx_git_two_modes \"log --raw\" &&\n \t\tmidx_git_two_modes \"count-objects --verbose\" &&\n \t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unsorted\" sorted\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unordered\" sorted\n \t'\n }\n \n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373229","messageId":"20190405180503.GD32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 04/13] t5319: drop useless --buffer from cat-file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:05:03Z","receivedAt":"2019-04-05T18:05:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The cat-file --buffer option is the default already when using\n--batch-all-objects. It doesn't hurt to specify it, but it's nice for\nthe test scripts to model good usage.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5319-multi-pack-index.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 42f4d6cd01..8c4d2bd849 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -104,8 +104,8 @@ compare_results_with_midx () {\n \t\tmidx_git_two_modes \"rev-list --objects --all\" &&\n \t\tmidx_git_two_modes \"log --raw\" &&\n \t\tmidx_git_two_modes \"count-objects --verbose\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check\" &&\n-\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unordered\" sorted\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check\" &&\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --batch-check --unordered\" sorted\n \t'\n }\n \n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373230","messageId":"20190405180604.GE32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 05/13] midx: check both pack and index names for containment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:06:04Z","receivedAt":"2019-04-05T18:06:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A midx file (and the struct we parse from it) contains a list of all of\nthe covered packfiles, mentioned by their \".idx\" names (e.g.,\n\"pack-1234.idx\", etc). And thus calls to midx_contains_pack() expect\ncallers to provide the idx name.\n\nThis works for most of the calls, but the one in open_packed_git_1()\ntries to feed a packed_git->pack_name, which is the \".pack\" name,\nmeaning we'll never find a match (even if the pack is covered by the\nmidx).\n\nWe can fix this by converting the \".pack\" to \".idx\" in the caller.\nHowever, that requires allocating a new string. Instead, let's make\nmidx_contains_pack() a bit friendlier, and allow it take _either_ the\n.pack or .idx variant.\n\nAll cleverness in the matching code is credited to René. Bugs are mine.\n\nThere's no test here, because while this does fix _a_ bug, it's masked\nby another bug in that same caller. That will be covered (with a test)\nin the next patch.\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI was tempted to suggest that the midx struct just store the base name\nwithout \".idx\" at all, but having callers pass that is no less tricky\nthan passing \".idx\" (they still have to allocate a new string).\n\n midx.c | 36 ++++++++++++++++++++++++++++++++++--\n midx.h |  2 +-\n 2 files changed, 35 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8a505fd423..0ceca1938f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -307,7 +307,39 @@ int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct mu\n \treturn nth_midxed_pack_entry(m, e, pos);\n }\n \n-int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n+/* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n+static int cmp_idx_or_pack_name(const char *idx_or_pack_name,\n+\t\t\t\tconst char *idx_name)\n+{\n+\t/* Skip past any initial matching prefix. */\n+\twhile (*idx_name && *idx_name == *idx_or_pack_name) {\n+\t\tidx_name++;\n+\t\tidx_or_pack_name++;\n+\t}\n+\n+\t/*\n+\t * If we didn't match completely, we may have matched \"pack-1234.\" and\n+\t * be left with \"idx\" and \"pack\" respectively, which is also OK. We do\n+\t * not have to check for \"idx\" and \"idx\", because that would have been\n+\t * a complete match (and in that case these strcmps will be false, but\n+\t * we'll correctly return 0 from the final strcmp() below.\n+\t *\n+\t * Technically this matches \"fooidx\" and \"foopack\", but we'd never have\n+\t * such names in the first place.\n+\t */\n+\tif (!strcmp(idx_name, \"idx\") && !strcmp(idx_or_pack_name, \"pack\"))\n+\t\treturn 0;\n+\n+\t/*\n+\t * This not only checks for a complete match, but also orders based on\n+\t * the first non-identical character, which means our ordering will\n+\t * match a raw strcmp(). That makes it OK to use this to binary search\n+\t * a naively-sorted list.\n+\t */\n+\treturn strcmp(idx_or_pack_name, idx_name);\n+}\n+\n+int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name)\n {\n \tuint32_t first = 0, last = m->num_packs;\n \n@@ -317,7 +349,7 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n \t\tint cmp;\n \n \t\tcurrent = m->pack_names[mid];\n-\t\tcmp = strcmp(idx_name, current);\n+\t\tcmp = cmp_idx_or_pack_name(idx_or_pack_name, current);\n \t\tif (!cmp)\n \t\t\treturn 1;\n \t\tif (cmp > 0) {\ndiff --git a/midx.h b/midx.h\nindex 774f652530..26dd042d63 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -43,7 +43,7 @@ struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t n);\n int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n-int midx_contains_pack(struct multi_pack_index *m, const char *idx_name);\n+int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name);\n int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n \n int write_midx_file(const char *object_dir);\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373231","messageId":"20190405180622.GF32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 06/13] packfile: fix pack basename computation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:06:22Z","receivedAt":"2019-04-05T18:06:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have a multi-pack-index that covers many packfiles, we try to\navoid opening the .idx for those packfiles. To do that we feed the pack\nname to midx_contains_pack(). But that function wants to see only the\nbasename, which we compute using strrchr() to find the final slash. But\nthat leaves an extra \"/\" at the start of our string.\n\nWe can fix this by incrementing the pointer. That also raises the\nquestion of what to do when the name does not have a '/' at all. This\nshould generally not happen (we always find files in \"pack/\"), but it\ndoesn't hurt to be defensive here.\n\nLet's wrap all of that up in a helper function and make it publicly\navailable, since a later patch will need to use it, too.\n\nThe tests don't notice because there's nothing about opening those .idx\nfiles that would cause us to give incorrect output. It's just a little\nslower. The new test checks this case by corrupting the covered .idx,\nand then making sure we don't complain about it.\n\nWe also have to tweak t5570, which intentionally corrupts a .idx file\nand expects us to notice it. When run with GIT_TEST_MULTI_PACK_INDEX,\nthis will fail since we now will (correctly) not bother opening the .idx\nat all. We can fix that by unconditionally dropping any midx that's\nthere, which ensures we'll have to read the .idx.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c                  | 12 +++++++++++-\n packfile.h                  |  6 ++++++\n t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n t/t5570-git-daemon.sh       |  1 +\n 4 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 6e40bd89c7..7a2dd2fdbe 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -466,6 +466,16 @@ static unsigned int get_max_fd_limit(void)\n #endif\n }\n \n+const char *pack_basename(struct packed_git *p)\n+{\n+\tconst char *ret = strrchr(p->pack_name, '/');\n+\tif (ret)\n+\t\tret = ret + 1; /* skip past slash */\n+\telse\n+\t\tret = p->pack_name; /* we only have a base */\n+\treturn ret;\n+}\n+\n /*\n  * Do not call this directly as this leaks p->pack_fd on error return;\n  * call open_packed_git() instead.\n@@ -482,7 +492,7 @@ static int open_packed_git_1(struct packed_git *p)\n \n \tif (!p->index_data) {\n \t\tstruct multi_pack_index *m;\n-\t\tconst char *pack_name = strrchr(p->pack_name, '/');\n+\t\tconst char *pack_name = pack_basename(p);\n \n \t\tfor (m = the_repository->objects->multi_pack_index;\n \t\t     m; m = m->next) {\ndiff --git a/packfile.h b/packfile.h\nindex b40fc34fb2..45bf792d79 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,6 +31,12 @@ char *sha1_pack_name(const unsigned char *sha1);\n  */\n char *sha1_pack_index_name(const unsigned char *sha1);\n \n+/*\n+ * Return the basename of the packfile, omitting any containing directory\n+ * (e.g., \"pack-1234abcd[...].pack\").\n+ */\n+const char *pack_basename(struct packed_git *p);\n+\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 8c4d2bd849..1ebf19ec3c 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -117,6 +117,20 @@ test_expect_success 'write midx with one v2 pack' '\n \n compare_results_with_midx \"one v2 pack\"\n \n+test_expect_success 'corrupt idx not opened' '\n+\tidx=$(test-tool read-midx $objdir | grep \"\\.idx\\$\") &&\n+\tmv $objdir/pack/$idx backup-$idx &&\n+\ttest_when_finished \"mv backup-\\$idx \\$objdir/pack/\\$idx\" &&\n+\n+\t# This is the minimum size for a sha-1 based .idx; this lets\n+\t# us pass perfunctory tests, but anything that actually opens and reads\n+\t# the idx file will complain.\n+\ttest_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&\n+\n+\tgit -c core.multiPackIndex=true rev-list --objects --all 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'add more objects' '\n \tfor i in $(test_seq 6 10)\n \tdo\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 58ee787685..19e271bda6 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -90,6 +90,7 @@ test_expect_success 'fetch notices corrupt pack' '\n test_expect_success 'fetch notices corrupt idx' '\n \tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n \t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t rm -f objects/pack/multi-pack-index &&\n \t p=$(ls objects/pack/pack-*.idx) &&\n \t chmod u+w $p &&\n \t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373232","messageId":"20190405181132.GA32401@sigill.intra.peff.net","threadId":"50876","inReplyTo":"83129937-dcd0-f16e-c8aa-97eceec9769a@web.de","subject":"Re: [PATCH 05/12] http: simplify parsing of remote objects/info/packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:11:32Z","receivedAt":"2019-04-05T18:11:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 12:41:27PM +0200, René Scharfe wrote:\n\n> Am 05.04.2019 um 01:27 schrieb Jeff King:\n> > We can use skip_prefix() and parse_oid_hex() to continuously increment\n> > our pointer, rather than dealing with magic numbers. This also fixes a\n> > few small shortcomings:\n> >\n> >   - if we see a 'P' line that does not match our expectations, we'll\n> >     leave our \"i\" counter in the middle of the line. So we'll parse:\n> >     \"P P P pack-1234.pack\" as if there was just one \"P\" which was not\n> >     intentional (though probably not that harmful).\n> \n> How so?  The default case, which we'd fall through to, skips the rest\n> of such a line, doesn't it?\n\nOh, you're right. I didn't notice the fall-through, which is quite\nsubtle (I'm actually surprised -Wimplicit-fallthrough doesn't complain\nabout this).\n\nI'll fix up the commit message (the cleanup is still very much worth it\nfor the garbage-oid issue, IMHO).\n\n> > +\twhile (*data) {\n> > +\t\tif (skip_prefix(data, \"P pack-\", &data) &&\n> > +\t\t    !parse_oid_hex(data, &oid, &data) &&\n> > +\t\t    skip_prefix(data, \".pack\", &data) &&\n> > +\t\t    (*data == '\\n' || *data == '\\0')) {\n> > +\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n> > +\t\t} else {\n> > +\t\t\tdata = strchrnul(data, '\\n');\n> >  \t\t}\n> > -\t\ti++;\n> > +\t\tif (*data)\n> > +\t\t\tdata++; /* skip past newline */\n> \n> So much simpler, *and* converted to object_id -- I like it!\n> \n> Parsing \"P\" and \"pack-\" together crosses logical token boundaries,\n> but that I don't mind it here.\n\nYeah, I was tempted to write:\n\n  if (skip_prefix(data, \"P \", &data) &&\n      skip_prefix(data, \"pack-\", &data) &&\n      ...\n\nbut that felt a little silly. I dunno. I guess it is probably not any\nless efficient, because we'd expect skip_prefix() and its loop to get\ninlined here anyway.\n\n-Peff\n"},{"id":"373233","messageId":"20190405181255.GG32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 07/13] http: simplify parsing of remote objects/info/packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:12:55Z","receivedAt":"2019-04-05T18:12:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can use skip_prefix() and parse_oid_hex() to continuously increment\nour pointer, rather than dealing with magic numbers. This also fixes a\nfew small shortcomings:\n\n  - if we see a line with the right prefix, suffix, and length, i.e.\n    matching /P pack-.{40}.pack\\n/, we'll interpret the middle part as\n    hex without checking if it could be parsed. This could lead to us\n    looking at uninitialized garbage in the hash array. In practice this\n    means we'll just make a garbage request to the server which will\n    fail, though it's interesting that a malicious server could convince\n    us to leak 40 bytes of uninitialized stack to them.\n\n  - the current code is picky about seeing a newline at the end of file,\n    but we can easily be more liberal\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis drops an incorrect claim from the v1 commit message. Sorry, I only\nremembered to deal with it as I was sending the patch out, so it is not\nreflected in the range-diff in the cover letter.\n\n http.c | 35 ++++++++++++++---------------------\n 1 file changed, 14 insertions(+), 21 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a32ad36ddf..2ef47bc779 100644\n--- a/http.c\n+++ b/http.c\n@@ -2147,11 +2147,11 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n {\n \tstruct http_get_options options = {0};\n-\tint ret = 0, i = 0;\n-\tchar *url, *data;\n+\tint ret = 0;\n+\tchar *url;\n+\tconst char *data;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tstruct object_id oid;\n \n \tend_url_with_slash(&buf, base_url);\n \tstrbuf_addstr(&buf, \"objects/info/packs\");\n@@ -2163,24 +2163,17 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \t\tgoto cleanup;\n \n \tdata = buf.buf;\n-\twhile (i < buf.len) {\n-\t\tswitch (data[i]) {\n-\t\tcase 'P':\n-\t\t\ti++;\n-\t\t\tif (i + hexsz + 12 <= buf.len &&\n-\t\t\t    starts_with(data + i, \" pack-\") &&\n-\t\t\t    starts_with(data + i + hexsz + 6, \".pack\\n\")) {\n-\t\t\t\tget_sha1_hex(data + i + 6, hash);\n-\t\t\t\tfetch_and_setup_pack_index(packs_head, hash,\n-\t\t\t\t\t\t      base_url);\n-\t\t\t\ti += hexsz + 11;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\tdefault:\n-\t\t\twhile (i < buf.len && data[i] != '\\n')\n-\t\t\t\ti++;\n+\twhile (*data) {\n+\t\tif (skip_prefix(data, \"P pack-\", &data) &&\n+\t\t    !parse_oid_hex(data, &oid, &data) &&\n+\t\t    skip_prefix(data, \".pack\", &data) &&\n+\t\t    (*data == '\\n' || *data == '\\0')) {\n+\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n+\t\t} else {\n+\t\t\tdata = strchrnul(data, '\\n');\n \t\t}\n-\t\ti++;\n+\t\tif (*data)\n+\t\t\tdata++; /* skip past newline */\n \t}\n \n cleanup:\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373235","messageId":"20190405181310.GH32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 08/13] server-info: fix blind pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:13:10Z","receivedAt":"2019-04-05T18:13:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're writing out a new objects/info/packs file, we read back the\nold one to try to keep the ordering the same. When we see a line\nstarting with \"P\", we expect \"P pack-1234...\" and blindly jump to \"line\n+ 2\" to parse the pack name. If we saw a line with _just_ \"P\" and\nnothing else, we'd jump past the end of the buffer and start reading\narbitrary memory.\n\nThis shouldn't be a big attack vector, as the files are local to the\nrepository and written by us, but it's clearly worth fixing (we do read\nremote copies of the file for dumb-http fetches, but using a totally\ndifferent parser!).\n\nLet's instead use skip_prefix() here, which avoids pointer arithmetic\naltogether. Note that this converts our switch statement to an if/else\nchain, making it slightly more verbose. But it will also make it easier\nto do a few follow-on cleanups.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex e2b2d6a27a..b61a6be4c2 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -112,9 +112,9 @@ static struct pack_info *find_pack_by_name(const char *name)\n /* Returns non-zero when we detect that the info in the\n  * old file is useless.\n  */\n-static int parse_pack_def(const char *line, int old_cnt)\n+static int parse_pack_def(const char *packname, int old_cnt)\n {\n-\tstruct pack_info *i = find_pack_by_name(line + 2);\n+\tstruct pack_info *i = find_pack_by_name(packname);\n \tif (i) {\n \t\ti->old_num = old_cnt;\n \t\treturn 0;\n@@ -139,24 +139,26 @@ static int read_pack_info_file(const char *infofile)\n \t\treturn 1; /* nonexistent is not an error. */\n \n \twhile (fgets(line, sizeof(line), fp)) {\n+\t\tconst char *arg;\n \t\tint len = strlen(line);\n \t\tif (len && line[len-1] == '\\n')\n \t\t\tline[--len] = 0;\n \n \t\tif (!len)\n \t\t\tcontinue;\n \n-\t\tswitch (line[0]) {\n-\t\tcase 'P': /* P name */\n-\t\t\tif (parse_pack_def(line, old_cnt++))\n+\t\tif (skip_prefix(line, \"P \", &arg)) {\n+\t\t\t/* P name */\n+\t\t\tif (parse_pack_def(arg, old_cnt++))\n \t\t\t\tgoto out_stale;\n-\t\t\tbreak;\n-\t\tcase 'D': /* we used to emit D but that was misguided. */\n-\t\tcase 'T': /* we used to emit T but nobody uses it. */\n+\t\t} else if (line[0] == 'D') {\n+\t\t\t/* we used to emit D but that was misguided. */\n \t\t\tgoto out_stale;\n-\t\tdefault:\n+\t\t} else if (line[0] == 'T') {\n+\t\t\t/* we used to emit T but nobody uses it. */\n+\t\t\tgoto out_stale;\n+\t\t} else {\n \t\t\terror(\"unrecognized: %s\", line);\n-\t\t\tbreak;\n \t\t}\n \t}\n \tfclose(fp);\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373236","messageId":"20190405181313.GI32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 09/13] server-info: simplify cleanup in parse_pack_def()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:13:14Z","receivedAt":"2019-04-05T18:13:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two exits from the function: either we jump to the out_stale\nlabel or not. But in both exits we repeat our cleanup, and the only\ndifference is our return value. Let's just use a variable for the return\nvalue to avoid repeating ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex b61a6be4c2..ba44cece7f 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -133,6 +133,7 @@ static int read_pack_info_file(const char *infofile)\n \tFILE *fp;\n \tchar line[1000];\n \tint old_cnt = 0;\n+\tint stale = 1;\n \n \tfp = fopen_or_warn(infofile, \"r\");\n \tif (!fp)\n@@ -161,11 +162,11 @@ static int read_pack_info_file(const char *infofile)\n \t\t\terror(\"unrecognized: %s\", line);\n \t\t}\n \t}\n-\tfclose(fp);\n-\treturn 0;\n+\tstale = 0;\n+\n  out_stale:\n \tfclose(fp);\n-\treturn 1;\n+\treturn stale;\n }\n \n static int compare_info(const void *a_, const void *b_)\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373237","messageId":"20190405181356.GJ32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 10/13] server-info: use strbuf to read old info/packs file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:13:56Z","receivedAt":"2019-04-05T18:13:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This old code uses fgets with a fixed-size buffer. Let's use a strbuf\ninstead, so we don't have to wonder if \"1000\" is big enough, or what\nhappens if we see a long line.\n\nThis also lets us drop our custom code to trim the newline.\n\nProbably nobody actually cares about the 1000-char limit (after all, the\nlines generally only say \"P pack-[0-9a-f]{40}.pack\"), so this is mostly\njust about cleanup/readability.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex ba44cece7f..d4115fecbb 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -131,40 +131,38 @@ static int parse_pack_def(const char *packname, int old_cnt)\n static int read_pack_info_file(const char *infofile)\n {\n \tFILE *fp;\n-\tchar line[1000];\n+\tstruct strbuf line = STRBUF_INIT;\n \tint old_cnt = 0;\n \tint stale = 1;\n \n \tfp = fopen_or_warn(infofile, \"r\");\n \tif (!fp)\n \t\treturn 1; /* nonexistent is not an error. */\n \n-\twhile (fgets(line, sizeof(line), fp)) {\n+\twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tconst char *arg;\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len-1] == '\\n')\n-\t\t\tline[--len] = 0;\n \n-\t\tif (!len)\n+\t\tif (!line.len)\n \t\t\tcontinue;\n \n-\t\tif (skip_prefix(line, \"P \", &arg)) {\n+\t\tif (skip_prefix(line.buf, \"P \", &arg)) {\n \t\t\t/* P name */\n \t\t\tif (parse_pack_def(arg, old_cnt++))\n \t\t\t\tgoto out_stale;\n-\t\t} else if (line[0] == 'D') {\n+\t\t} else if (line.buf[0] == 'D') {\n \t\t\t/* we used to emit D but that was misguided. */\n \t\t\tgoto out_stale;\n-\t\t} else if (line[0] == 'T') {\n+\t\t} else if (line.buf[0] == 'T') {\n \t\t\t/* we used to emit T but nobody uses it. */\n \t\t\tgoto out_stale;\n \t\t} else {\n-\t\t\terror(\"unrecognized: %s\", line);\n+\t\t\terror(\"unrecognized: %s\", line.buf);\n \t\t}\n \t}\n \tstale = 0;\n \n  out_stale:\n+\tstrbuf_release(&line);\n \tfclose(fp);\n \treturn stale;\n }\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373238","messageId":"20190405181400.GK32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 11/13] server-info: drop nr_alloc struct member","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:14:00Z","receivedAt":"2019-04-05T18:14:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep an array of struct pointers, with each one representing a single\npackfile. But for some reason there is a nr_alloc parameter inside each\nstruct, which has never been used.\n\nThis is probably cruft left over from development, where we might have\nwanted a nr_alloc to dynamically grow the list. But as it turns out, we\ndo not dynamically grow the list at all, but rather count up the total\nnumber of packs and use that as a maximum size. So while we're thinking\nof this, let's add an assert() that documents (and checks!) that our\nallocation and fill loops stay in sync.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex d4115fecbb..c9fbfd3a51 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -91,7 +91,6 @@ static struct pack_info {\n \tstruct packed_git *p;\n \tint old_num;\n \tint new_num;\n-\tint nr_alloc;\n } **info;\n static int num_pack;\n static const char *objdir;\n@@ -213,6 +212,7 @@ static void init_pack_info(const char *infofile, int force)\n \tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n+\t\tassert(i < num_pack);\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n \t\tinfo[i]->p = p;\n \t\tinfo[i]->old_num = -1;\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373239","messageId":"20190405181403.GL32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 12/13] server-info: drop objdirlen pointer arithmetic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:14:04Z","receivedAt":"2019-04-05T18:14:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When writing objects/info/packs, we use the basename of each pack\n(i.e., just the \"pack-1234abcd.pack\" part). We compute that manually by\nadding \"objdirlen + 6\" to the name.\n\nThis _should_ work consistently, as we do not include non-local packs,\nmeaning everything should be in $objdir/pack/. Before f13d7db4af\n(server-info.c: use pack_local like everybody else., 2005-12-05), this\nwas definitely true, since we computed \"local\" based on comparing the\nobjdir string.  Since then, we're relying on the code on packfile.c to\nmatch our expectations of p->pack_name and p->local.\n\nI think our expectations do still hold today, but we can be a bit more\ndefensive by just using pack_basename() to get the base. That\nfuture-proofs us, and should hopefully be more obviously safe to\nsomebody reading the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex c9fbfd3a51..ab03c1b3c2 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -93,16 +93,13 @@ static struct pack_info {\n \tint new_num;\n } **info;\n static int num_pack;\n-static const char *objdir;\n-static int objdirlen;\n \n static struct pack_info *find_pack_by_name(const char *name)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n \t\tstruct packed_git *p = info[i]->p;\n-\t\t/* skip \"/pack/\" after \".git/objects\" */\n-\t\tif (!strcmp(p->pack_name + objdirlen + 6, name))\n+\t\tif (!strcmp(pack_basename(p), name))\n \t\t\treturn info[i];\n \t}\n \treturn NULL;\n@@ -196,9 +193,6 @@ static void init_pack_info(const char *infofile, int force)\n \tint stale;\n \tint i = 0;\n \n-\tobjdir = get_object_directory();\n-\tobjdirlen = strlen(objdir);\n-\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t/* we ignore things on alternate path since they are\n \t\t * not available to the pullers in general.\n@@ -246,7 +240,7 @@ static int write_pack_info_file(FILE *fp)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n-\t\tif (fprintf(fp, \"P %s\\n\", info[i]->p->pack_name + objdirlen + 6) < 0)\n+\t\tif (fprintf(fp, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n \t\t\treturn -1;\n \t}\n \tif (fputc('\\n', fp) == EOF)\n-- \n2.21.0.729.g7d31bf3764\n\n"},{"id":"373240","messageId":"20190405181407.GM32243@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"[PATCH v2 13/13] update_info_refs(): drop unused force parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:14:07Z","receivedAt":"2019-04-05T18:14:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time the force flag meant something when writing info/refs,\nbut it hasn't done anything since 60d0526aaa (Unoptimize info/refs\ncreation., 2005-09-14).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex ab03c1b3c2..41274d098b 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -78,7 +78,7 @@ static int generate_info_refs(FILE *fp)\n \treturn for_each_ref(add_info_ref, fp);\n }\n \n-static int update_info_refs(int force)\n+static int update_info_refs(void)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n \tint ret = update_info_file(path, generate_info_refs);\n@@ -269,7 +269,7 @@ int update_server_info(int force)\n \t */\n \tint errs = 0;\n \n-\terrs = errs | update_info_refs(force);\n+\terrs = errs | update_info_refs();\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\n-- \n2.21.0.729.g7d31bf3764\n"},{"id":"373241","messageId":"20190405181630.GA2284@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405181313.GI32243@sigill.intra.peff.net","subject":"Re: [PATCH v2 09/13] server-info: simplify cleanup in parse_pack_def()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:16:31Z","receivedAt":"2019-04-05T18:16:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 02:13:13PM -0400, Jeff King wrote:\n\n> Subject: server-info: simplify cleanup in parse_pack_def()\n> We have two exits from the function: either we jump to the out_stale\n> label or not. But in both exits we repeat our cleanup, and the only\n> difference is our return value. Let's just use a variable for the return\n> value to avoid repeating ourselves.\n\nThe subject name is misleading here. The change is actually in\nread_pack_info_file(), which sees an error when calling\nparse_pack_def().\n\nProbably not worth a re-roll by itself.\n\n-Peff\n"},{"id":"373242","messageId":"20190405181900.GB2284@sigill.intra.peff.net","threadId":"50876","inReplyTo":"20190405180306.GA21113@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/13] a rabbit hole of update-server-info (and midx!) fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T18:19:01Z","receivedAt":"2019-04-05T18:19:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 02:03:06PM -0400, Jeff King wrote:\n\n> Other than that, it's substantially the same as v1. I did move the\n> public declaration of pack_basename() to the patch where it is added,\n> rather than doing it later, which shows up in the range diff below.\n\nSorry, two things I noticed and fixed up while sending out the patches\nthemselves, but which were too late to be in the cover letter or range\ndiff:\n\n>   [03/13]: t5319: fix bogus cat-file argument\n\nThis fixes the flipped unsorted/unordered words in the commit message,\nnoticed by Ramsay.\n\n>   [07/13]: http: simplify parsing of remote objects/info/packs\n\nThis drops the incorrect claim in the commit message noticed by René.\n\n-Peff\n"},{"id":"373250","messageId":"c21b923f-54aa-defa-ecfd-11ecd6f8a664@ramsayjones.plus.com","threadId":"50876","inReplyTo":"20190405180340.GA32243@sigill.intra.peff.net","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-04-05T19:19:30Z","receivedAt":"2019-04-05T19:19:39Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/04/2019 19:03, Jeff King wrote:\n> As CodingGuidelines recommends, we do not need an \"extern\" when\n> declaring a public function. Let's drop these. Note that we leave the\n> extern on report_garbage(), as that is actually a function pointer, not\n> a function itself.\n\nHmm, perhaps we need to edit CodingGuidelines to make it clear\nthat the 'extern' keyword is not needed on _function_ declarations\nonly. ;-)\n\nBecause, ...\n\n[snip]\n\n>  /* global flag to enable extra checks when accessing packed objects */\n> -extern int do_check_packed_object_crc;\n> +int do_check_packed_object_crc;\n\n... removing this 'extern' on an int variable sends 'sparse'\ninto a frenzy of warnings! :-D\n\n[You didn't use a global s/extern// by any chance?]\n\nATB,\nRamsay Jones\n"},{"id":"373251","messageId":"20190405192534.GA4496@sigill.intra.peff.net","threadId":"50876","inReplyTo":"c21b923f-54aa-defa-ecfd-11ecd6f8a664@ramsayjones.plus.com","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T19:25:35Z","receivedAt":"2019-04-05T19:25:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:\n\n> >  /* global flag to enable extra checks when accessing packed objects */\n> > -extern int do_check_packed_object_crc;\n> > +int do_check_packed_object_crc;\n> \n> ... removing this 'extern' on an int variable sends 'sparse'\n> into a frenzy of warnings! :-D\n> \n> [You didn't use a global s/extern// by any chance?]\n\nOh my. I did look at each one, but probably via replace-and-confirm in\nvim. I don't know how I managed to botch that one so badly.\n\n-Peff\n"},{"id":"373255","messageId":"85b12bbd-15a8-09a1-395b-dc1606ab34e7@web.de","threadId":"50876","inReplyTo":"20190405181132.GA32401@sigill.intra.peff.net","subject":"Re: [PATCH 05/12] http: simplify parsing of remote objects/info/packs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-04-05T20:17:07Z","receivedAt":"2019-04-05T20:17:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.2019 um 20:11 schrieb Jeff King:\n> On Fri, Apr 05, 2019 at 12:41:27PM +0200, René Scharfe wrote:\n>> Parsing \"P\" and \"pack-\" together crosses logical token boundaries,\n>> but that I don't mind it here.\n>\n> Yeah, I was tempted to write:\n>\n>   if (skip_prefix(data, \"P \", &data) &&\n>       skip_prefix(data, \"pack-\", &data) &&\n>       ...\n>\n> but that felt a little silly. I dunno. I guess it is probably not any\n> less efficient, because we'd expect skip_prefix() and its loop to get\n> inlined here anyway.\n\nDidn't think of inlining.  Clang unrolls the whole comparison (except on\npowerpc64), but the other compilers available at the Compiler Explorer\nwebsite keep the consecutive calls separate: https://godbolt.org/z/7eTarV\n\nRené\n"},{"id":"373256","messageId":"8f188559-c1a9-7717-4b2c-a7397cfaa6bc@web.de","threadId":"50876","inReplyTo":"20190405180604.GE32243@sigill.intra.peff.net","subject":"Re: [PATCH v2 05/13] midx: check both pack and index names for containment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-04-05T20:18:17Z","receivedAt":"2019-04-05T20:18:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.04.2019 um 20:06 schrieb Jeff King:\n> A midx file (and the struct we parse from it) contains a list of all of\n> the covered packfiles, mentioned by their \".idx\" names (e.g.,\n> \"pack-1234.idx\", etc). And thus calls to midx_contains_pack() expect\n> callers to provide the idx name.\n>\n> This works for most of the calls, but the one in open_packed_git_1()\n> tries to feed a packed_git->pack_name, which is the \".pack\" name,\n> meaning we'll never find a match (even if the pack is covered by the\n> midx).\n>\n> We can fix this by converting the \".pack\" to \".idx\" in the caller.\n> However, that requires allocating a new string. Instead, let's make\n> midx_contains_pack() a bit friendlier, and allow it take _either_ the\n> .pack or .idx variant.\n>\n> All cleverness in the matching code is credited to René. Bugs are mine.\n\nI didn't consider it to be particularly tricky -- but then kept the dots\nin both filename extension strings, which is not going to fly, as they\nare skipped by the common-prefix loop..  Thanks for fixing that.\n\n> There's no test here, because while this does fix _a_ bug, it's masked\n> by another bug in that same caller. That will be covered (with a test)\n> in the next patch.\n>\n> Helped-by: René Scharfe <l.s.r@web.de>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I was tempted to suggest that the midx struct just store the base name\n> without \".idx\" at all, but having callers pass that is no less tricky\n> than passing \".idx\" (they still have to allocate a new string).\n\nThe middle part of the comparison function would become:\n\n\tif (!*idx_name && (!strcmp(idx_or_pack_name, \".idx\") ||\n\t\t\t   !strcmp(idx_or_pack_name, \".pack\"))\n\t\treturn 0;\n\nNo allocations needed -- except when building the list.\n\n(And this time we'd actually need the dots.)\n\n>\n>  midx.c | 36 ++++++++++++++++++++++++++++++++++--\n>  midx.h |  2 +-\n>  2 files changed, 35 insertions(+), 3 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 8a505fd423..0ceca1938f 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -307,7 +307,39 @@ int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct mu\n>  \treturn nth_midxed_pack_entry(m, e, pos);\n>  }\n>\n> -int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n> +/* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n> +static int cmp_idx_or_pack_name(const char *idx_or_pack_name,\n> +\t\t\t\tconst char *idx_name)\n> +{\n> +\t/* Skip past any initial matching prefix. */\n> +\twhile (*idx_name && *idx_name == *idx_or_pack_name) {\n> +\t\tidx_name++;\n> +\t\tidx_or_pack_name++;\n> +\t}\n> +\n> +\t/*\n> +\t * If we didn't match completely, we may have matched \"pack-1234.\" and\n> +\t * be left with \"idx\" and \"pack\" respectively, which is also OK. We do\n> +\t * not have to check for \"idx\" and \"idx\", because that would have been\n> +\t * a complete match (and in that case these strcmps will be false, but\n> +\t * we'll correctly return 0 from the final strcmp() below.\n> +\t *\n> +\t * Technically this matches \"fooidx\" and \"foopack\", but we'd never have\n> +\t * such names in the first place.\n> +\t */\n> +\tif (!strcmp(idx_name, \"idx\") && !strcmp(idx_or_pack_name, \"pack\"))\n> +\t\treturn 0;\n\nThis is asymmetric, and thus the function should not be used for sorting,\nwhere it would be called for random pairs of values. For the binary search\nit's fine, assuming the list contains only index filenames (ending with\n\".idx\"), as the search string is always passed in as idx_or_pack_name.\n\nAnd an extension-insensitive lookup works fine in a strcmp()-sorted list\nbecause the order wouldn't change when sorting it again with a looser\nstable extension-sensitive sort.\n\n> +\n> +\t/*\n> +\t * This not only checks for a complete match, but also orders based on\n> +\t * the first non-identical character, which means our ordering will\n> +\t * match a raw strcmp(). That makes it OK to use this to binary search\n> +\t * a naively-sorted list.\n> +\t */\n> +\treturn strcmp(idx_or_pack_name, idx_name);\n\nAt this point we could also compare the chars like this:\n\n\treturn (unsigned char)(*idx_or_pack_name) - (unsigned char)(*idx_name);\n\nThis avoids a function call, but doesn't look very pretty.  And I'm not\nfully sure the casts are correct.\n\n> +}\n> +\n> +int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name)\n>  {\n>  \tuint32_t first = 0, last = m->num_packs;\n>\n> @@ -317,7 +349,7 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n>  \t\tint cmp;\n>\n>  \t\tcurrent = m->pack_names[mid];\n> -\t\tcmp = strcmp(idx_name, current);\n> +\t\tcmp = cmp_idx_or_pack_name(idx_or_pack_name, current);\n>  \t\tif (!cmp)\n>  \t\t\treturn 1;\n>  \t\tif (cmp > 0) {\n> diff --git a/midx.h b/midx.h\n> index 774f652530..26dd042d63 100644\n> --- a/midx.h\n> +++ b/midx.h\n> @@ -43,7 +43,7 @@ struct object_id *nth_midxed_object_oid(struct object_id *oid,\n>  \t\t\t\t\tstruct multi_pack_index *m,\n>  \t\t\t\t\tuint32_t n);\n>  int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n> -int midx_contains_pack(struct multi_pack_index *m, const char *idx_name);\n> +int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name);\n>  int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n>\n>  int write_midx_file(const char *object_dir);\n>\n\n"},{"id":"373393","messageId":"xmqqzhp19j0y.fsf@gitster-ct.c.googlers.com","threadId":"50876","inReplyTo":"20190405192534.GA4496@sigill.intra.peff.net","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T05:13:17Z","receivedAt":"2019-04-08T05:13:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:\n>\n>> >  /* global flag to enable extra checks when accessing packed objects */\n>> > -extern int do_check_packed_object_crc;\n>> > +int do_check_packed_object_crc;\n>> \n>> ... removing this 'extern' on an int variable sends 'sparse'\n>> into a frenzy of warnings! :-D\n>> \n>> [You didn't use a global s/extern// by any chance?]\n>\n> Oh my. I did look at each one, but probably via replace-and-confirm in\n> vim. I don't know how I managed to botch that one so badly.\n\nPerhaps we should keep 'extern' even when declaring (not defining) a\npublic function in the header file to avoid a gotcha like this?\n\nWhat was the reasoning behind the insn in CodingGuidelines?  \"As it\nis already the default\" does qualify as a reasonable justification\nfor telling \"extern is not needed for functions\" to our readers, but\nnot quite enough for \"extern should not be used for functions\".\n\n"},{"id":"373439","messageId":"20190408203252.GA5696@sigill.intra.peff.net","threadId":"50876","inReplyTo":"xmqqzhp19j0y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-08T20:32:52Z","receivedAt":"2019-04-08T20:32:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2019 at 02:13:17PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:\n> >\n> >> >  /* global flag to enable extra checks when accessing packed objects */\n> >> > -extern int do_check_packed_object_crc;\n> >> > +int do_check_packed_object_crc;\n> >> \n> >> ... removing this 'extern' on an int variable sends 'sparse'\n> >> into a frenzy of warnings! :-D\n> >> \n> >> [You didn't use a global s/extern// by any chance?]\n> >\n> > Oh my. I did look at each one, but probably via replace-and-confirm in\n> > vim. I don't know how I managed to botch that one so badly.\n> \n> Perhaps we should keep 'extern' even when declaring (not defining) a\n> public function in the header file to avoid a gotcha like this?\n> \n> What was the reasoning behind the insn in CodingGuidelines?  \"As it\n> is already the default\" does qualify as a reasonable justification\n> for telling \"extern is not needed for functions\" to our readers, but\n> not quite enough for \"extern should not be used for functions\".\n\nI think the reasoning is just that it's useless noise, so it makes the\nresulting lines longer (which are often already too-long) and harder to\nread.\n\nFor this particular patch, I don't care that much about the existing\nfunctions, which I'm not touching, but rather was adding a new one. And\nmy options were:\n\n  - use \"extern\" there to match; even if we don't want to go through the\n    code churn of cleaning up existing cases, I think we shouldn't be\n    encouraging it in new ones. Even more crazy to me would actually\n    be review telling somebody to add an extern.\n\n  - intermingle it with the existing extern ones. Low risk, but leaves\n    people wondering why some have extern and some do not.\n\n  - clean up the existing cases first\n\nI dunno. Like all code churn, these kinds of clean-ups have the\npossibility of accidentally screwing something up. But they are at least\na one-time pain, as long as we do not keep changing our mind back and\nforth.\n\n-Peff\n"},{"id":"373491","messageId":"xmqqsgur43nm.fsf@gitster-ct.c.googlers.com","threadId":"50876","inReplyTo":"20190405192534.GA4496@sigill.intra.peff.net","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T15:08:45Z","receivedAt":"2019-04-09T15:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:\n>\n>> >  /* global flag to enable extra checks when accessing packed objects */\n>> > -extern int do_check_packed_object_crc;\n>> > +int do_check_packed_object_crc;\n>> \n>> ... removing this 'extern' on an int variable sends 'sparse'\n>> into a frenzy of warnings! :-D\n>> \n>> [You didn't use a global s/extern// by any chance?]\n>\n> Oh my. I did look at each one, but probably via replace-and-confirm in\n> vim. I don't know how I managed to botch that one so badly.\n\nIn any case, I think I have a correct SQUASH??? immediately on top\nof this step queued.  If there is nothing else needed, I think we\nare ready to squash that in and merge the whole thing to 'next'.\n\n"},{"id":"373494","messageId":"20190409151507.GA12627@sigill.intra.peff.net","threadId":"50876","inReplyTo":"xmqqsgur43nm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-09T15:15:07Z","receivedAt":"2019-04-09T15:15:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 10, 2019 at 12:08:45AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:\n> >\n> >> >  /* global flag to enable extra checks when accessing packed objects */\n> >> > -extern int do_check_packed_object_crc;\n> >> > +int do_check_packed_object_crc;\n> >> \n> >> ... removing this 'extern' on an int variable sends 'sparse'\n> >> into a frenzy of warnings! :-D\n> >> \n> >> [You didn't use a global s/extern// by any chance?]\n> >\n> > Oh my. I did look at each one, but probably via replace-and-confirm in\n> > vim. I don't know how I managed to botch that one so badly.\n> \n> In any case, I think I have a correct SQUASH??? immediately on top\n> of this step queued.  If there is nothing else needed, I think we\n> are ready to squash that in and merge the whole thing to 'next'.\n\nYes, your squash looks good, and I don't think there were any other\nunaddressed comments. Thanks for fixing my bug.\n\n-Peff\n"}]}