{"thread":{"id":"51073","subject":"[PATCH] update-server-info: avoid needless overwrites","startedAt":"2019-05-11T01:34:59Z","lastAt":"2019-05-24T08:18:31Z","messageCount":26,"participants":["Eric Wong","Eric Sunshine","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"375331","messageId":"20190511013455.5886-1-e@80x24.org","threadId":"51073","inReplyTo":null,"subject":"[PATCH] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-11T01:34:55Z","receivedAt":"2019-05-11T01:34:59Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Do not change the existing info/refs and objects/info/packs\nfiles if they match the existing content on the filesystem.\nThis is intended to preserve mtime and make it easier for dumb\nHTTP pollers to rely on the If-Modified-Since header.\n\nCombined with stdio and kernel buffering; the kernel should be\nable to avoid block layer writes and reduce wear.\n\nSigned-off-by: Eric Wong \n---\n server-info.c | 61 +++++++++++++++++++++++++++++------\n t/t5200-update-server-info.sh | 41 +++++++++++++++++++++++\n 2 files changed, 92 insertions(+), 10 deletions(-)\n create mode 100755 t/t5200-update-server-info.sh\n\ndiff --git a/server-info.c b/server-info.c\nindex 41274d098b..34599e4817 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -6,37 +6,78 @@\n #include \"tag.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"strbuf.h\"\n+\n+static int files_differ(FILE *fp, const char *path)\n+{\n+\tstruct stat st;\n+\tgit_hash_ctx c;\n+\tstruct object_id oid_old, oid_new;\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\tlong new_len = ftell(fp);\n+\n+\tif (new_len < 0 || stat(path, &st) < 0)\n+\t\treturn 1;\n+\tif (!S_ISREG(st.st_mode))\n+\t\treturn 1;\n+\tif ((off_t)new_len != st.st_size)\n+\t\treturn 1;\n+\n+\trewind(fp);\n+\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n+\t\treturn 1;\n+\tthe_hash_algo->init_fn(&c);\n+\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n+\tthe_hash_algo->final_fn(oid_new.hash, &c);\n+\tstrbuf_release(&tmp);\n+\n+\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n+\t\treturn 1;\n+\tthe_hash_algo->init_fn(&c);\n+\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n+\tthe_hash_algo->final_fn(oid_old.hash, &c);\n+\tstrbuf_release(&tmp);\n+\n+\treturn hashcmp(oid_old.hash, oid_new.hash);\n+}\n \n /*\n * Create the file \"path\" by writing to a temporary file and renaming\n * it into place. The contents of the file come from \"generate\", which\n * should return non-zero if it encounters an error.\n */\n-static int update_info_file(char *path, int (*generate)(FILE *))\n+static int update_info_file(char *path, int (*generate)(FILE *), int force)\n {\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n \tint ret = -1;\n \tint fd = -1;\n \tFILE *fp = NULL, *to_close;\n+\tint do_update;\n \n \tsafe_create_leading_directories(path);\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n-\tto_close = fp = fdopen(fd, \"w\");\n+\tto_close = fp = fdopen(fd, \"w+\");\n \tif (!fp)\n \t\tgoto out;\n \tfd = -1;\n \tret = generate(fp);\n \tif (ret)\n \t\tgoto out;\n+\n+\tdo_update = force || files_differ(fp, path);\n \tfp = NULL;\n \tif (fclose(to_close))\n \t\tgoto out;\n-\tif (adjust_shared_perm(tmp) < 0)\n-\t\tgoto out;\n-\tif (rename(tmp, path) < 0)\n-\t\tgoto out;\n+\tif (do_update) {\n+\t\tif (adjust_shared_perm(tmp) < 0)\n+\t\t\tgoto out;\n+\t\tif (rename(tmp, path) < 0)\n+\t\t\tgoto out;\n+\t} else {\n+\t\tunlink(tmp);\n+\t}\n \tret = 0;\n \n out:\n@@ -78,10 +119,10 @@ static int generate_info_refs(FILE *fp)\n \treturn for_each_ref(add_info_ref, fp);\n }\n \n-static int update_info_refs(void)\n+static int update_info_refs(int force)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n-\tint ret = update_info_file(path, generate_info_refs);\n+\tint ret = update_info_file(path, generate_info_refs, force);\n \tfree(path);\n \treturn ret;\n }\n@@ -254,7 +295,7 @@ static int update_info_packs(int force)\n \tint ret;\n \n \tinit_pack_info(infofile, force);\n-\tret = update_info_file(infofile, write_pack_info_file);\n+\tret = update_info_file(infofile, write_pack_info_file, force);\n \tfree_pack_info();\n \tfree(infofile);\n \treturn ret;\n@@ -269,7 +310,7 @@ int update_server_info(int force)\n \t */\n \tint errs = 0;\n \n-\terrs = errs | update_info_refs();\n+\terrs = errs | update_info_refs(force);\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nnew file mode 100755\nindex 0000000000..f1cec46914\n--- /dev/null\n+++ b/t/t5200-update-server-info.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='Test git stash show configuration.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' 'test_commit file'\n+\n+test_expect_success 'create info/refs' '\n+\tgit update-server-info &&\n+\ttest_path_is_file .git/info/refs\n+'\n+\n+test_expect_success 'modify and store mtime' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >a\n+'\n+\n+test_expect_success 'info/refs is not needlessly overwritten' '\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b\n+'\n+\n+test_expect_success 'info/refs can be forced to update' '\n+\tgit update-server-info -f &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_expect_success 'info/refs updates when changes are made' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b &&\n+\tgit update-ref refs/heads/foo HEAD &&\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_done\n-- \nEW\n\n"},{"id":"375335","messageId":"CAPig+cRZy4yM+ZuHhZK3tPV_QRvSy63aHBZZj6AcFTu+5FNG0A@mail.gmail.com","threadId":"51073","inReplyTo":"20190511013455.5886-1-e@80x24.org","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-05-11T07:35:23Z","receivedAt":"2019-05-11T07:35:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 10, 2019 at 9:35 PM Eric Wong wrote:\n> Do not change the existing info/refs and objects/info/packs\n> files if they match the existing content on the filesystem.\n> This is intended to preserve mtime and make it easier for dumb\n> HTTP pollers to rely on the If-Modified-Since header.\n> [...]\n> Signed-off-by: Eric Wong \n> ---\n> diff --git a/server-info.c b/server-info.c\n> @@ -6,37 +6,78 @@\n> +static int files_differ(FILE *fp, const char *path)\n> +{\n> + [...]\n> + struct strbuf tmp = STRBUF_INIT;\n> + [...]\n> + if (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n> + return 1;\n\nAlthough strbuf_fread() will release 'tmp' automatically if the read\nactually fails, it won't do so otherwise. So, if it reads more or\nfewer bytes than expected, for some reason, then this early return\nwill leak the strbuf, won't it?\n\n> + the_hash_algo->init_fn(&c);\n> + the_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n> + the_hash_algo->final_fn(oid_new.hash, &c);\n> + strbuf_release(&tmp);\n> +\n> + if (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n> + return 1;\n\nSame issue.\n\n> diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\n> @@ -0,0 +1,41 @@\n> +test_description='Test git stash show configuration.'\n\nCopy/paste error for test description?\n"},{"id":"375339","messageId":"20190511204754.quj52fbwrdqxg6e7@dcvr","threadId":"51073","inReplyTo":"CAPig+cRZy4yM+ZuHhZK3tPV_QRvSy63aHBZZj6AcFTu+5FNG0A@mail.gmail.com","subject":"[PATCH v2] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-11T20:47:54Z","receivedAt":"2019-05-11T20:47:57Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine wrote:\n> On Fri, May 10, 2019 at 9:35 PM Eric Wong wrote:\n> > + if (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n> > + return 1;\n> \n> Although strbuf_fread() will release 'tmp' automatically if the read\n> actually fails, it won't do so otherwise. So, if it reads more or\n> fewer bytes than expected, for some reason, then this early return\n> will leak the strbuf, won't it?\n\n> > + if (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n> > + return 1;\n> \n> Same issue.\n> \n> > diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\n> > @@ -0,0 +1,41 @@\n> > +test_description='Test git stash show configuration.'\n> \n> Copy/paste error for test description?\n\nYup, thanks for the review. Updated patch below\n\n-----8<------\nSubject: [PATCH] update-server-info: avoid needless overwrites\n\nDo not change the existing info/refs and objects/info/packs\nfiles if they match the existing content on the filesystem.\nThis is intended to preserve mtime and make it easier for dumb\nHTTP pollers to rely on the If-Modified-Since header.\n\nCombined with stdio and kernel buffering; the kernel should be\nable to avoid block layer writes and reduce wear.\n\nSigned-off-by: Eric Wong \n---\nInterdiff:\n diff --git a/server-info.c b/server-info.c\n index 34599e4817..11515804d4 100644\n --- a/server-info.c\n +++ b/server-info.c\n @@ -24,15 +24,19 @@ static int files_differ(FILE *fp, const char *path)\n \t\treturn 1;\n \n \trewind(fp);\n -\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n +\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len) {\n +\t\tstrbuf_release(&tmp);\n \t\treturn 1;\n +\t}\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n \tthe_hash_algo->final_fn(oid_new.hash, &c);\n \tstrbuf_release(&tmp);\n \n -\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n +\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0) {\n +\t\tstrbuf_release(&tmp);\n \t\treturn 1;\n +\t}\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n \tthe_hash_algo->final_fn(oid_old.hash, &c);\n diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\n index f1cec46914..21a58eecb9 100755\n --- a/t/t5200-update-server-info.sh\n +++ b/t/t5200-update-server-info.sh\n @@ -1,6 +1,6 @@\n #!/bin/sh\n \n -test_description='Test git stash show configuration.'\n +test_description='Test git update-server-info'\n \n . ./test-lib.sh\n \n\n server-info.c | 65 +++++++++++++++++++++++++++++------\n t/t5200-update-server-info.sh | 41 ++++++++++++++++++++++\n 2 files changed, 96 insertions(+), 10 deletions(-)\n create mode 100755 t/t5200-update-server-info.sh\n\ndiff --git a/server-info.c b/server-info.c\nindex 41274d098b..11515804d4 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -6,37 +6,82 @@\n #include \"tag.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"strbuf.h\"\n+\n+static int files_differ(FILE *fp, const char *path)\n+{\n+\tstruct stat st;\n+\tgit_hash_ctx c;\n+\tstruct object_id oid_old, oid_new;\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\tlong new_len = ftell(fp);\n+\n+\tif (new_len < 0 || stat(path, &st) < 0)\n+\t\treturn 1;\n+\tif (!S_ISREG(st.st_mode))\n+\t\treturn 1;\n+\tif ((off_t)new_len != st.st_size)\n+\t\treturn 1;\n+\n+\trewind(fp);\n+\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len) {\n+\t\tstrbuf_release(&tmp);\n+\t\treturn 1;\n+\t}\n+\tthe_hash_algo->init_fn(&c);\n+\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n+\tthe_hash_algo->final_fn(oid_new.hash, &c);\n+\tstrbuf_release(&tmp);\n+\n+\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0) {\n+\t\tstrbuf_release(&tmp);\n+\t\treturn 1;\n+\t}\n+\tthe_hash_algo->init_fn(&c);\n+\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n+\tthe_hash_algo->final_fn(oid_old.hash, &c);\n+\tstrbuf_release(&tmp);\n+\n+\treturn hashcmp(oid_old.hash, oid_new.hash);\n+}\n \n /*\n * Create the file \"path\" by writing to a temporary file and renaming\n * it into place. The contents of the file come from \"generate\", which\n * should return non-zero if it encounters an error.\n */\n-static int update_info_file(char *path, int (*generate)(FILE *))\n+static int update_info_file(char *path, int (*generate)(FILE *), int force)\n {\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n \tint ret = -1;\n \tint fd = -1;\n \tFILE *fp = NULL, *to_close;\n+\tint do_update;\n \n \tsafe_create_leading_directories(path);\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n-\tto_close = fp = fdopen(fd, \"w\");\n+\tto_close = fp = fdopen(fd, \"w+\");\n \tif (!fp)\n \t\tgoto out;\n \tfd = -1;\n \tret = generate(fp);\n \tif (ret)\n \t\tgoto out;\n+\n+\tdo_update = force || files_differ(fp, path);\n \tfp = NULL;\n \tif (fclose(to_close))\n \t\tgoto out;\n-\tif (adjust_shared_perm(tmp) < 0)\n-\t\tgoto out;\n-\tif (rename(tmp, path) < 0)\n-\t\tgoto out;\n+\tif (do_update) {\n+\t\tif (adjust_shared_perm(tmp) < 0)\n+\t\t\tgoto out;\n+\t\tif (rename(tmp, path) < 0)\n+\t\t\tgoto out;\n+\t} else {\n+\t\tunlink(tmp);\n+\t}\n \tret = 0;\n \n out:\n@@ -78,10 +123,10 @@ static int generate_info_refs(FILE *fp)\n \treturn for_each_ref(add_info_ref, fp);\n }\n \n-static int update_info_refs(void)\n+static int update_info_refs(int force)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n-\tint ret = update_info_file(path, generate_info_refs);\n+\tint ret = update_info_file(path, generate_info_refs, force);\n \tfree(path);\n \treturn ret;\n }\n@@ -254,7 +299,7 @@ static int update_info_packs(int force)\n \tint ret;\n \n \tinit_pack_info(infofile, force);\n-\tret = update_info_file(infofile, write_pack_info_file);\n+\tret = update_info_file(infofile, write_pack_info_file, force);\n \tfree_pack_info();\n \tfree(infofile);\n \treturn ret;\n@@ -269,7 +314,7 @@ int update_server_info(int force)\n \t */\n \tint errs = 0;\n \n-\terrs = errs | update_info_refs();\n+\terrs = errs | update_info_refs(force);\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nnew file mode 100755\nindex 0000000000..21a58eecb9\n--- /dev/null\n+++ b/t/t5200-update-server-info.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='Test git update-server-info'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' 'test_commit file'\n+\n+test_expect_success 'create info/refs' '\n+\tgit update-server-info &&\n+\ttest_path_is_file .git/info/refs\n+'\n+\n+test_expect_success 'modify and store mtime' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >a\n+'\n+\n+test_expect_success 'info/refs is not needlessly overwritten' '\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b\n+'\n+\n+test_expect_success 'info/refs can be forced to update' '\n+\tgit update-server-info -f &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_expect_success 'info/refs updates when changes are made' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b &&\n+\tgit update-ref refs/heads/foo HEAD &&\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_done\n-- \nEW\n"},{"id":"375342","messageId":"20190511211749.5ghtdcuzgb7bkywo@dcvr","threadId":"51073","inReplyTo":"20190511013455.5886-1-e@80x24.org","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-11T21:17:49Z","receivedAt":"2019-05-11T21:17:52Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong wrote:\n> Combined with stdio and kernel buffering; the kernel should be\n> able to avoid block layer writes and reduce wear.\n\nAlong the same lines, I'd like to get repack to stop recreating\nidentical packs at some point (but that could be months from now...)\n"},{"id":"375346","messageId":"87v9ygwoj0.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190511013455.5886-1-e@80x24.org","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-11T23:37:55Z","receivedAt":"2019-05-11T23:38:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 11 2019, Eric Wong wrote:\n\n> Do not change the existing info/refs and objects/info/packs\n> files if they match the existing content on the filesystem.\n> This is intended to preserve mtime and make it easier for dumb\n> HTTP pollers to rely on the If-Modified-Since header.\n>\n> Combined with stdio and kernel buffering; the kernel should be\n> able to avoid block layer writes and reduce wear.\n>\n> Signed-off-by: Eric Wong \n> ---\n> server-info.c | 61 +++++++++++++++++++++++++++++------\n> t/t5200-update-server-info.sh | 41 +++++++++++++++++++++++\n> 2 files changed, 92 insertions(+), 10 deletions(-)\n> create mode 100755 t/t5200-update-server-info.sh\n>\n> diff --git a/server-info.c b/server-info.c\n> index 41274d098b..34599e4817 100644\n> --- a/server-info.c\n> +++ b/server-info.c\n> @@ -6,37 +6,78 @@\n> #include \"tag.h\"\n> #include \"packfile.h\"\n> #include \"object-store.h\"\n> +#include \"strbuf.h\"\n> +\n> +static int files_differ(FILE *fp, const char *path)\n> +{\n> +\tstruct stat st;\n> +\tgit_hash_ctx c;\n> +\tstruct object_id oid_old, oid_new;\n> +\tstruct strbuf tmp = STRBUF_INIT;\n> +\tlong new_len = ftell(fp);\n> +\n> +\tif (new_len < 0 || stat(path, &st) < 0)\n> +\t\treturn 1;\n> +\tif (!S_ISREG(st.st_mode))\n> +\t\treturn 1;\n> +\tif ((off_t)new_len != st.st_size)\n> +\t\treturn 1;\n> +\n> +\trewind(fp);\n> +\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n> +\t\treturn 1;\n> +\tthe_hash_algo->init_fn(&c);\n> +\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n> +\tthe_hash_algo->final_fn(oid_new.hash, &c);\n> +\tstrbuf_release(&tmp);\n> +\n> +\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n> +\t\treturn 1;\n> +\tthe_hash_algo->init_fn(&c);\n> +\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n> +\tthe_hash_algo->final_fn(oid_old.hash, &c);\n> +\tstrbuf_release(&tmp);\n> +\n> +\treturn hashcmp(oid_old.hash, oid_new.hash);\n> +}\n\nThis way of doing it just seems so weirdly convoluted. Read them one at\na time, compute the SHA-1, just to see if they're different. Why not\nsomething closer to a plain memcmp():\n\n\tstatic int files_differ(FILE *fp, const char *path)\n\t{\n\t\tstruct strbuf old = STRBUF_INIT, new = STRBUF_INIT;\n\t\tlong new_len = ftell(fp);\n\t\tint diff = 1;\n\n\t\trewind(fp);\n\t\tif (strbuf_fread(&new, (size_t)new_len, fp) != (size_t)new_len)\n\t\t\tgoto release_return;\n\t\tif (strbuf_read_file(&old, path, 0) < 0)\n\t\t\tgoto release_return;\n\n\t\tdiff = strbuf_cmp(&old, &new);\n\n\trelease_return:\n\t\tstrbuf_release(&old);\n\t\tstrbuf_release(&new);\n\n\t\treturn diff;\n\t}\n\nI.e. optimze for code simplicity with something close to a dumb \"cmp\n--silent\". I'm going to make the bold claim that nobody using \"dumb\nhttp\" is going to be impacted by the performance of reading their\nfor-each-ref or for-each-pack dump, hence opting for not even bothing to\nstat() to get the size before reading.\n\nBecause really, if we were *trying* to micro-optimize this for time or\nmemory use there's much better ways, e.g. reading the old file and\nmemcmp() as we go and stream the \"generate\" callback, but I just don't\nsee the point of trying in this case.\n\n> /*\n> * Create the file \"path\" by writing to a temporary file and renaming\n> * it into place. The contents of the file come from \"generate\", which\n> * should return non-zero if it encounters an error.\n> */\n> -static int update_info_file(char *path, int (*generate)(FILE *))\n> +static int update_info_file(char *path, int (*generate)(FILE *), int force)\n> {\n> \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n\nUnrelated to this, patch, but I hadn't thought about this nasty race\ncondition. We recommend users run this from the \"post-update\" (or\n\"post-receive\") hook, and don't juggle the lock along with the ref\nupdate, thus due to the vagaries of scheduling you can end up with two\nconcurrent ref updates where the \"old\" one wins.\n\nBut I guess that brings me back to something close to \"nobody with that\nsort of update rate is using 'dumb http'\" :)\n\nFor this to work properly we'd need to take some sort of global \"ref\nupdate/pack update\" lock, and I guess at that point this \"cmp\" case\nwould be a helper similar to commit_lock_file_to(),\ni.e. a commit_lock_file_to_if_different().\n\n> \tint ret = -1;\n> \tint fd = -1;\n> \tFILE *fp = NULL, *to_close;\n> +\tint do_update;\n>\n> \tsafe_create_leading_directories(path);\n> \tfd = git_mkstemp_mode(tmp, 0666);\n> \tif (fd < 0)\n> \t\tgoto out;\n> -\tto_close = fp = fdopen(fd, \"w\");\n> +\tto_close = fp = fdopen(fd, \"w+\");\n> \tif (!fp)\n> \t\tgoto out;\n> \tfd = -1;\n> \tret = generate(fp);\n> \tif (ret)\n> \t\tgoto out;\n> +\n> +\tdo_update = force || files_differ(fp, path);\n> [...]\n>\n> -static int update_info_refs(void)\n> +static int update_info_refs(int force)\n\nSo, I was going to say \"shouldn't we update the docs?\" which for --force\nsay \"Update the info files from scratch.\".\n\nBut reading through it that \"from scratch\" wording is from c743e6e3c0\n(\"Add a link from update-server-info documentation to repository\nlayout.\", 2005-09-04).\n\nThere it was a refrence to a bug since fixed in 60d0526aaa (\"Unoptimize\ninfo/refs creation.\", 2005-09-14), and then removed from the docs in\nc5fe5b6de9 (\"Remove obsolete bug warning in man git-update-server-info\",\n2009-04-25).\n\nThen in b3223761c8 (\"update_info_refs(): drop unused force parameter\",\n2019-04-05) Jeff removed the unused-for-a-decade \"force\" param.\n\nSo having gone through the history I think we're better off just\ndropping the --force argument entirely, or at least changing the\ndocs.\n\nBefore this change the only thing it was doing was pruning stuff we\nhaven't written since 2005-ish (see 3e15c67c90 (\"server-info: throw away\nT computation as well.\", 2005-12-04)), rather than \"detect if useless\"\nwe should just write out the file again, and then skip if changed\n(i.e. this logic).\n\nThat would also take care of the parse_pack_def() case by proxy, i.e. we\ndon't need some \"is stale if pack is missing\" special case if we just\nalways write out a new file (or not if it memcmp's the same as the \"old\"\none), we want to change the file if the packs are updated anyway\n"},{"id":"375347","messageId":"20190512003859.GA22361@dcvr","threadId":"51073","inReplyTo":"87v9ygwoj0.fsf@evledraar.gmail.com","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-12T00:38:59Z","receivedAt":"2019-05-12T00:39:48Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Sat, May 11 2019, Eric Wong wrote:\n> > +static int files_differ(FILE *fp, const char *path)\n> > +{\n> > +\tstruct stat st;\n> > +\tgit_hash_ctx c;\n> > +\tstruct object_id oid_old, oid_new;\n> > +\tstruct strbuf tmp = STRBUF_INIT;\n> > +\tlong new_len = ftell(fp);\n> > +\n> > +\tif (new_len < 0 || stat(path, &st) < 0)\n> > +\t\treturn 1;\n> > +\tif (!S_ISREG(st.st_mode))\n> > +\t\treturn 1;\n> > +\tif ((off_t)new_len != st.st_size)\n> > +\t\treturn 1;\n> > +\n> > +\trewind(fp);\n> > +\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)\n> > +\t\treturn 1;\n> > +\tthe_hash_algo->init_fn(&c);\n> > +\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n> > +\tthe_hash_algo->final_fn(oid_new.hash, &c);\n> > +\tstrbuf_release(&tmp);\n> > +\n> > +\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)\n> > +\t\treturn 1;\n> > +\tthe_hash_algo->init_fn(&c);\n> > +\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n> > +\tthe_hash_algo->final_fn(oid_old.hash, &c);\n> > +\tstrbuf_release(&tmp);\n> > +\n> > +\treturn hashcmp(oid_old.hash, oid_new.hash);\n> > +}\n> \n> This way of doing it just seems so weirdly convoluted. Read them one at\n> a time, compute the SHA-1, just to see if they're different. Why not\n> something closer to a plain memcmp():\n> \n> \tstatic int files_differ(FILE *fp, const char *path)\n> \t{\n> \t\tstruct strbuf old = STRBUF_INIT, new = STRBUF_INIT;\n> \t\tlong new_len = ftell(fp);\n> \t\tint diff = 1;\n> \n> \t\trewind(fp);\n> \t\tif (strbuf_fread(&new, (size_t)new_len, fp) != (size_t)new_len)\n> \t\t\tgoto release_return;\n> \t\tif (strbuf_read_file(&old, path, 0) < 0)\n> \t\t\tgoto release_return;\n> \n> \t\tdiff = strbuf_cmp(&old, &new);\n> \n> \trelease_return:\n> \t\tstrbuf_release(&old);\n> \t\tstrbuf_release(&new);\n> \n> \t\treturn diff;\n> \t}\n> \n> I.e. optimze for code simplicity with something close to a dumb \"cmp\n> --silent\". I'm going to make the bold claim that nobody using \"dumb\n> http\" is going to be impacted by the performance of reading their\n> for-each-ref or for-each-pack dump, hence opting for not even bothing to\n> stat() to get the size before reading.\n\nI've been trying to improve dumb HTTP for more cases; actually.\n(since it's much cheaper than smart HTTP in server memory/CPU)\n\n> Because really, if we were *trying* to micro-optimize this for time or\n> memory use there's much better ways, e.g. reading the old file and\n> memcmp() as we go and stream the \"generate\" callback, but I just don't\n> see the point of trying in this case.\n\nI was actually going towards that route; but wasn't sure if this\nidea would be accepted at all (and I've been trying to stay away\nfrom using non-scripting languages).\n\nI don't slurping all of info/refs into memory at all; so maybe\na streaming memcmp of the existing file is worth doing...\n\n> > /*\n> > * Create the file \"path\" by writing to a temporary file and renaming\n> > * it into place. The contents of the file come from \"generate\", which\n> > * should return non-zero if it encounters an error.\n> > */\n> > -static int update_info_file(char *path, int (*generate)(FILE *))\n> > +static int update_info_file(char *path, int (*generate)(FILE *), int force)\n> > {\n> > \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n> \n> Unrelated to this, patch, but I hadn't thought about this nasty race\n> condition. We recommend users run this from the \"post-update\" (or\n> \"post-receive\") hook, and don't juggle the lock along with the ref\n> update, thus due to the vagaries of scheduling you can end up with two\n> concurrent ref updates where the \"old\" one wins.\n> \n> But I guess that brings me back to something close to \"nobody with that\n> sort of update rate is using 'dumb http'\" :)\n> \n> For this to work properly we'd need to take some sort of global \"ref\n> update/pack update\" lock, and I guess at that point this \"cmp\" case\n> would be a helper similar to commit_lock_file_to(),\n> i.e. a commit_lock_file_to_if_different().\n\nWorth a separate patch, at some point, I think. I'm not too\nfamiliar with the existing locking in git, actually...\n\nAlong those lines, I think repack/gc should automatically\nupdate objects/info/packs if the file already exists.\n\n> > \tint ret = -1;\n> > \tint fd = -1;\n> > \tFILE *fp = NULL, *to_close;\n> > +\tint do_update;\n> >\n> > \tsafe_create_leading_directories(path);\n> > \tfd = git_mkstemp_mode(tmp, 0666);\n> > \tif (fd < 0)\n> > \t\tgoto out;\n> > -\tto_close = fp = fdopen(fd, \"w\");\n> > +\tto_close = fp = fdopen(fd, \"w+\");\n> > \tif (!fp)\n> > \t\tgoto out;\n> > \tfd = -1;\n> > \tret = generate(fp);\n> > \tif (ret)\n> > \t\tgoto out;\n> > +\n> > +\tdo_update = force || files_differ(fp, path);\n> > [...]\n> >\n> > -static int update_info_refs(void)\n> > +static int update_info_refs(int force)\n> \n> So, I was going to say \"shouldn't we update the docs?\" which for --force\n> say \"Update the info files from scratch.\".\n> \n> But reading through it that \"from scratch\" wording is from c743e6e3c0\n> (\"Add a link from update-server-info documentation to repository\n> layout.\", 2005-09-04).\n\nYes, that wording is awkward and I can update it. But maybe making\nit undocumented is sufficient and would save us the trouble\nof describing it :)\n\n\"--force\" might be seen as a performance optimization for cases\nwhere you're certain the result will differ, but I'm not\nsure if that's worth mentioning in the manpage.\n\n> There it was a refrence to a bug since fixed in 60d0526aaa (\"Unoptimize\n> info/refs creation.\", 2005-09-14), and then removed from the docs in\n> c5fe5b6de9 (\"Remove obsolete bug warning in man git-update-server-info\",\n> 2009-04-25).\n> \n> Then in b3223761c8 (\"update_info_refs(): drop unused force parameter\",\n> 2019-04-05) Jeff removed the unused-for-a-decade \"force\" param.\n> \n> So having gone through the history I think we're better off just\n> dropping the --force argument entirely, or at least changing the\n> docs.\n\nI can update the docs, or make it undocumented. Compatibility\nfrom the command-line needs to remain in case there are scripts\nusing it.\n"},{"id":"375348","messageId":"20190512040825.GA25370@sigill.intra.peff.net","threadId":"51073","inReplyTo":"87v9ygwoj0.fsf@evledraar.gmail.com","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-12T04:08:26Z","receivedAt":"2019-05-12T04:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 12, 2019 at 01:37:55AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This way of doing it just seems so weirdly convoluted. Read them one at\n> a time, compute the SHA-1, just to see if they're different. Why not\n> something closer to a plain memcmp():\n\nFWIW, I had the exact same thought on reading the patch. Checking sizes\nseems like an easy optimization and I don't mind it, but computing\nhashes when we could just compare the bytes seems pointless. I'd have\nexpected to stream 4k blocks as we go.\n\n> I.e. optimze for code simplicity with something close to a dumb \"cmp\n> --silent\". I'm going to make the bold claim that nobody using \"dumb\n> http\" is going to be impacted by the performance of reading their\n> for-each-ref or for-each-pack dump, hence opting for not even bothing to\n> stat() to get the size before reading.\n\nYou're probably right (especially because we'd just spent O(n) work\ngenerating the candidate file). But note that I have seen pathological\ncases where info/refs was gigabytes.\n\n> > /*\n> > * Create the file \"path\" by writing to a temporary file and renaming\n> > * it into place. The contents of the file come from \"generate\", which\n> > * should return non-zero if it encounters an error.\n> > */\n> > -static int update_info_file(char *path, int (*generate)(FILE *))\n> > +static int update_info_file(char *path, int (*generate)(FILE *), int force)\n> > {\n> > \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n> \n> Unrelated to this, patch, but I hadn't thought about this nasty race\n> condition. We recommend users run this from the \"post-update\" (or\n> \"post-receive\") hook, and don't juggle the lock along with the ref\n> update, thus due to the vagaries of scheduling you can end up with two\n> concurrent ref updates where the \"old\" one wins.\n> \n> But I guess that brings me back to something close to \"nobody with that\n> sort of update rate is using 'dumb http'\" :)\n> \n> For this to work properly we'd need to take some sort of global \"ref\n> update/pack update\" lock, and I guess at that point this \"cmp\" case\n> would be a helper similar to commit_lock_file_to(),\n> i.e. a commit_lock_file_to_if_different().\n\nI don't think our usual dot-locking is great here. What does the\nrace-loser do when it can't take the lock? It can't just skip its\nupdate, since it needs to make sure that its new pack is mentioned.\n\nSo it has to wait and poll until the other process finishes. I guess\nmaybe that isn't the end of the world.\n\n> Then in b3223761c8 (\"update_info_refs(): drop unused force parameter\",\n> 2019-04-05) Jeff removed the unused-for-a-decade \"force\" param.\n> \n> So having gone through the history I think we're better off just\n> dropping the --force argument entirely, or at least changing the\n> docs.\n> \n> Before this change the only thing it was doing was pruning stuff we\n> haven't written since 2005-ish (see 3e15c67c90 (\"server-info: throw away\n> T computation as well.\", 2005-12-04)), rather than \"detect if useless\"\n> we should just write out the file again, and then skip if changed\n> (i.e. this logic).\n\nYeah, my commit only ripped out the useless force parameter for\ninfo/refs. For info/packs, there's still that weird \"is is stale\"\ncomputation (which I fixed several bugs in). It's not entirely clear to\nme if that can just go away, but I agree that if we can it's simpler and\nmore desirable to just generate the candidate result and see if it's\nbit-for-bit identical or not.\n\nI'm not entirely sure of all of the magic that \"stale\" check is trying\nto accomplish. I think there's some bits in there that try to preserve\nthe existing ordering, but I don't know why anyone would care (and there\nare so many cases where the ordering gets thrown out that I think\nanybody who does care is likely to get disappointed).\n\n-Peff\n"},{"id":"375350","messageId":"87tve0w3ao.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190512040825.GA25370@sigill.intra.peff.net","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-12T07:16:31Z","receivedAt":"2019-05-12T07:23:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, May 12 2019, Jeff King wrote:\n\n> On Sun, May 12, 2019 at 01:37:55AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This way of doing it just seems so weirdly convoluted. Read them one at\n>> a time, compute the SHA-1, just to see if they're different. Why not\n>> something closer to a plain memcmp():\n>\n> FWIW, I had the exact same thought on reading the patch. Checking sizes\n> seems like an easy optimization and I don't mind it, but computing\n> hashes when we could just compare the bytes seems pointless. I'd have\n> expected to stream 4k blocks as we go.\n>\n>> I.e. optimze for code simplicity with something close to a dumb \"cmp\n>> --silent\". I'm going to make the bold claim that nobody using \"dumb\n>> http\" is going to be impacted by the performance of reading their\n>> for-each-ref or for-each-pack dump, hence opting for not even bothing to\n>> stat() to get the size before reading.\n>\n> You're probably right (especially because we'd just spent O(n) work\n> generating the candidate file). But note that I have seen pathological\n> cases where info/refs was gigabytes.\n\nWouldn't those users be calling update-server-info from something like a\npost-receive hook where they *know* the refs/packs just got updated?\n\nWell, there is \"transfer.unpackLimit\" to contend with, but that's just\nfor \"packs are same\", not \"refs are same\", and that file's presumably\nmuch smaller.\n\nSo I'd think that *if* this is a case where there's even a question of\nthe performance mattering here, and I'd assume \"refs changed but size is\nthe same\" is a common case (e.g. craploads or tags, but just \"master\"\npushed-to) we could make this an opt-in \"--update-if-changed\", because\nthe \"if changed\" is extra work, and we're already documenting that you\nshould be calling this on the \"I know it changed\" hook path.\n\nI don't care even if we use the initial SHA-1 method. It just struck me\nas odd to conflate the reasonable \"don't update mtime for optimizing\ndumb over-the-wire client\" with \"optimize resource use when updating\".\n\n>> > /*\n>> > * Create the file \"path\" by writing to a temporary file and renaming\n>> > * it into place. The contents of the file come from \"generate\", which\n>> > * should return non-zero if it encounters an error.\n>> > */\n>> > -static int update_info_file(char *path, int (*generate)(FILE *))\n>> > +static int update_info_file(char *path, int (*generate)(FILE *), int force)\n>> > {\n>> > \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n>>\n>> Unrelated to this, patch, but I hadn't thought about this nasty race\n>> condition. We recommend users run this from the \"post-update\" (or\n>> \"post-receive\") hook, and don't juggle the lock along with the ref\n>> update, thus due to the vagaries of scheduling you can end up with two\n>> concurrent ref updates where the \"old\" one wins.\n>>\n>> But I guess that brings me back to something close to \"nobody with that\n>> sort of update rate is using 'dumb http'\" :)\n>>\n>> For this to work properly we'd need to take some sort of global \"ref\n>> update/pack update\" lock, and I guess at that point this \"cmp\" case\n>> would be a helper similar to commit_lock_file_to(),\n>> i.e. a commit_lock_file_to_if_different().\n>\n> I don't think our usual dot-locking is great here. What does the\n> race-loser do when it can't take the lock? It can't just skip its\n> update, since it needs to make sure that its new pack is mentioned.\n\nRight, I mean a *global* .git/I_AM_DOING_WRITES.lock, because there's no\nway to square the ref backend's notion of per-ref ref lock enabling\nconcurrent pushes with update-server-info's desire to generate metadata\nshowing *all* the refs.\n\n> So it has to wait and poll until the other process finishes. I guess\n> maybe that isn't the end of the world.\n\nIf \"its\" is update-server-info this won't work. It's possible for two\nupdate-server-info processes to be racing in such a way that their\nfor_each_ref() reads a ref at a given value that'll be updated 1\nmillisecond later, but to then have that update's update-server-info\n\"win\" the race to update the info files (hypothetical locks on those\ninfo files and all).\n\nThus the \"info\" files will be updated to old values, since we'll see we\nneed changes, but change things to the old values.\n\nAll things that *can* be dealt with in some clever ways, but I think\njust further proof nobody's using this for anything serious :)\n\nI.e. the \"commit A happened before B\" but also \"commit B's post-* hooks\nfinished after A\" is a thing that happens quite frequently (per my\nlogs).\n\n>> Then in b3223761c8 (\"update_info_refs(): drop unused force parameter\",\n>> 2019-04-05) Jeff removed the unused-for-a-decade \"force\" param.\n>>\n>> So having gone through the history I think we're better off just\n>> dropping the --force argument entirely, or at least changing the\n>> docs.\n>>\n>> Before this change the only thing it was doing was pruning stuff we\n>> haven't written since 2005-ish (see 3e15c67c90 (\"server-info: throw away\n>> T computation as well.\", 2005-12-04)), rather than \"detect if useless\"\n>> we should just write out the file again, and then skip if changed\n>> (i.e. this logic).\n>\n> Yeah, my commit only ripped out the useless force parameter for\n> info/refs. For info/packs, there's still that weird \"is is stale\"\n> computation (which I fixed several bugs in). It's not entirely clear to\n> me if that can just go away, but I agree that if we can it's simpler and\n> more desirable to just generate the candidate result and see if it's\n> bit-for-bit identical or not.\n>\n> I'm not entirely sure of all of the magic that \"stale\" check is trying\n> to accomplish. I think there's some bits in there that try to preserve\n> the existing ordering, but I don't know why anyone would care (and there\n> are so many cases where the ordering gets thrown out that I think\n> anybody who does care is likely to get disappointed).\n\nMy reading of it is that it's premature optimization that can go away\n(and most of it has already), for \"it's cheap\" and \"if not it's called\nfrom the 'I know I had an update'\" hook case, as noted above.<\n"},{"id":"375432","messageId":"20190513231708.sdxs3krilrer4fga@dcvr","threadId":"51073","inReplyTo":"20190511013455.5886-1-e@80x24.org","subject":"[PATCH v3] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-13T23:17:08Z","receivedAt":"2019-05-13T23:17:11Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Do not change the existing info/refs and objects/info/packs\nfiles if they match the existing content on the filesystem.\nThis is intended to preserve mtime and make it easier for dumb\nHTTP pollers to rely on the If-Modified-Since header.\n\nCombined with stdio and kernel buffering; the kernel should be\nable to avoid block layer writes and reduce wear for small files.\n\nAs a result, the --force option is no longer needed. So stop\ndocumenting it, but let it remain for compatibility (and\ndebugging, if necessary).\n\nv3: perform incremental comparison while generating to avoid\n OOM with giant files. Remove documentation for --force.\n\nSigned-off-by: Eric Wong \n---\n OK, performing the incremental comparison wasn't nearly as\n bad as thought it'd be. I might be suffering from\n compiler-linker-slowness PTSD :x\n\nInterdiff:\n diff --git a/Documentation/git-update-server-info.txt b/Documentation/git-update-server-info.txt\n index bd0e36492f..969bb2e15f 100644\n --- a/Documentation/git-update-server-info.txt\n +++ b/Documentation/git-update-server-info.txt\n @@ -9,7 +9,7 @@ git-update-server-info - Update auxiliary info file to help dumb servers\n SYNOPSIS\n --------\n [verse]\n -'git update-server-info' [--force]\n +'git update-server-info'\n \n DESCRIPTION\n -----------\n @@ -19,15 +19,6 @@ $GIT_OBJECT_DIRECTORY/info directories to help clients discover\n what references and packs the server has. This command\n generates such auxiliary files.\n \n -\n -OPTIONS\n --------\n -\n --f::\n ---force::\n -\tUpdate the info files from scratch.\n -\n -\n OUTPUT\n ------\n \n diff --git a/server-info.c b/server-info.c\n index 11515804d4..e68f785c2f 100644\n --- a/server-info.c\n +++ b/server-info.c\n @@ -8,41 +8,54 @@\n #include \"object-store.h\"\n #include \"strbuf.h\"\n \n -static int files_differ(FILE *fp, const char *path)\n +struct update_info_ctx {\n +\tFILE *cur_fp;\n +\tFILE *old_fp; /* becomes NULL if it differs from cur_fp */\n +\tstruct strbuf cur_sb;\n +\tstruct strbuf old_sb;\n +};\n +\n +static void uic_mark_stale(struct update_info_ctx *uic)\n {\n -\tstruct stat st;\n -\tgit_hash_ctx c;\n -\tstruct object_id oid_old, oid_new;\n -\tstruct strbuf tmp = STRBUF_INIT;\n -\tlong new_len = ftell(fp);\n +\tfclose(uic->old_fp);\n +\tuic->old_fp = NULL;\n +}\n \n -\tif (new_len < 0 || stat(path, &st) < 0)\n -\t\treturn 1;\n -\tif (!S_ISREG(st.st_mode))\n -\t\treturn 1;\n -\tif ((off_t)new_len != st.st_size)\n -\t\treturn 1;\n +static int uic_is_stale(const struct update_info_ctx *uic)\n +{\n +\treturn uic->old_fp == NULL;\n +}\n \n -\trewind(fp);\n -\tif (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len) {\n -\t\tstrbuf_release(&tmp);\n -\t\treturn 1;\n -\t}\n -\tthe_hash_algo->init_fn(&c);\n -\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n -\tthe_hash_algo->final_fn(oid_new.hash, &c);\n -\tstrbuf_release(&tmp);\n +static int uic_printf(struct update_info_ctx *uic, const char *fmt, ...)\n +{\n +\tva_list ap;\n +\tint ret = -1;\n \n -\tif (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0) {\n -\t\tstrbuf_release(&tmp);\n -\t\treturn 1;\n +\tva_start(ap, fmt);\n +\n +\tif (uic_is_stale(uic)) {\n +\t\tret = vfprintf(uic->cur_fp, fmt, ap);\n +\t} else {\n +\t\tssize_t r;\n +\t\tstruct strbuf *cur = &uic->cur_sb;\n +\t\tstruct strbuf *old = &uic->old_sb;\n +\n +\t\tstrbuf_reset(cur);\n +\t\tstrbuf_vinsertf(cur, 0, fmt, ap);\n +\n +\t\tstrbuf_reset(old);\n +\t\tstrbuf_grow(old, cur->len);\n +\t\tr = fread(old->buf, 1, cur->len, uic->old_fp);\n +\t\tif (r != cur->len || memcmp(old->buf, cur->buf, r))\n +\t\t\tuic_mark_stale(uic);\n +\n +\t\tif (fwrite(cur->buf, 1, cur->len, uic->cur_fp) == cur->len)\n +\t\t\tret = 0;\n \t}\n -\tthe_hash_algo->init_fn(&c);\n -\tthe_hash_algo->update_fn(&c, tmp.buf, tmp.len);\n -\tthe_hash_algo->final_fn(oid_old.hash, &c);\n -\tstrbuf_release(&tmp);\n \n -\treturn hashcmp(oid_old.hash, oid_new.hash);\n +\tva_end(ap);\n +\n +\treturn ret;\n }\n \n /*\n @@ -50,31 +63,61 @@ static int files_differ(FILE *fp, const char *path)\n * it into place. The contents of the file come from \"generate\", which\n * should return non-zero if it encounters an error.\n */\n -static int update_info_file(char *path, int (*generate)(FILE *), int force)\n +static int update_info_file(char *path,\n +\t\t\tint (*generate)(struct update_info_ctx *),\n +\t\t\tint force)\n {\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n \tint ret = -1;\n \tint fd = -1;\n -\tFILE *fp = NULL, *to_close;\n -\tint do_update;\n +\tFILE *to_close;\n +\tstruct update_info_ctx uic = {\n +\t\t.cur_fp = NULL,\n +\t\t.old_fp = NULL,\n +\t\t.cur_sb = STRBUF_INIT,\n +\t\t.old_sb = STRBUF_INIT\n +\t};\n \n \tsafe_create_leading_directories(path);\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n -\tto_close = fp = fdopen(fd, \"w+\");\n -\tif (!fp)\n +\tto_close = uic.cur_fp = fdopen(fd, \"w\");\n +\tif (!uic.cur_fp)\n \t\tgoto out;\n \tfd = -1;\n -\tret = generate(fp);\n +\n +\t/* no problem on ENOENT and old_fp == NULL, it's stale, now */\n +\tif (!force)\n +\t\tuic.old_fp = fopen_or_warn(path, \"r\");\n +\n +\t/*\n +\t * uic_printf will compare incremental comparison aginst old_fp\n +\t * and mark uic as stale if needed\n +\t */\n +\tret = generate(&uic);\n \tif (ret)\n \t\tgoto out;\n \n -\tdo_update = force || files_differ(fp, path);\n -\tfp = NULL;\n +\t/* new file may be shorter than the old one, check here */\n +\tif (!uic_is_stale(&uic)) {\n +\t\tstruct stat st;\n +\t\tlong new_len = ftell(uic.cur_fp);\n +\t\tint old_fd = fileno(uic.old_fp);\n +\n +\t\tif (new_len < 0) {\n +\t\t\tret = -1;\n +\t\t\tgoto out;\n +\t\t}\n +\t\tif (fstat(old_fd, &st) || (st.st_size != (size_t)new_len))\n +\t\t\tuic_mark_stale(&uic);\n +\t}\n +\n +\tuic.cur_fp = NULL;\n \tif (fclose(to_close))\n \t\tgoto out;\n -\tif (do_update) {\n +\n +\tif (uic_is_stale(&uic)) {\n \t\tif (adjust_shared_perm(tmp) < 0)\n \t\t\tgoto out;\n \t\tif (rename(tmp, path) < 0)\n @@ -87,40 +130,44 @@ static int update_info_file(char *path, int (*generate)(FILE *), int force)\n out:\n \tif (ret) {\n \t\terror_errno(\"unable to update %s\", path);\n -\t\tif (fp)\n -\t\t\tfclose(fp);\n +\t\tif (uic.cur_fp)\n +\t\t\tfclose(uic.cur_fp);\n \t\telse if (fd >= 0)\n \t\t\tclose(fd);\n \t\tunlink(tmp);\n \t}\n \tfree(tmp);\n +\tif (uic.old_fp)\n +\t\tfclose(uic.old_fp);\n +\tstrbuf_release(&uic.old_sb);\n +\tstrbuf_release(&uic.cur_sb);\n \treturn ret;\n }\n \n static int add_info_ref(const char *path, const struct object_id *oid,\n \t\t\tint flag, void *cb_data)\n {\n -\tFILE *fp = cb_data;\n +\tstruct update_info_ctx *uic = cb_data;\n \tstruct object *o = parse_object(the_repository, oid);\n \tif (!o)\n \t\treturn -1;\n \n -\tif (fprintf(fp, \"%s\t%s\\n\", oid_to_hex(oid), path) < 0)\n +\tif (uic_printf(uic, \"%s\t%s\\n\", oid_to_hex(oid), path) < 0)\n \t\treturn -1;\n \n \tif (o->type == OBJ_TAG) {\n \t\to = deref_tag(the_repository, o, path, 0);\n \t\tif (o)\n -\t\t\tif (fprintf(fp, \"%s\t%s^{}\\n\",\n +\t\t\tif (uic_printf(uic, \"%s\t%s^{}\\n\",\n \t\t\t\toid_to_hex(&o->oid), path) < 0)\n \t\t\t\treturn -1;\n \t}\n \treturn 0;\n }\n \n -static int generate_info_refs(FILE *fp)\n +static int generate_info_refs(struct update_info_ctx *uic)\n {\n -\treturn for_each_ref(add_info_ref, fp);\n +\treturn for_each_ref(add_info_ref, uic);\n }\n \n static int update_info_refs(int force)\n @@ -281,14 +328,14 @@ static void free_pack_info(void)\n \tfree(info);\n }\n \n -static int write_pack_info_file(FILE *fp)\n +static int write_pack_info_file(struct update_info_ctx *uic)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n -\t\tif (fprintf(fp, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n +\t\tif (uic_printf(uic, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n \t\t\treturn -1;\n \t}\n -\tif (fputc('\\n', fp) == EOF)\n +\tif (uic_printf(uic, \"\\n\") < 0)\n \t\treturn -1;\n \treturn 0;\n }\n\n Documentation/git-update-server-info.txt | 11 +-\n server-info.c | 140 +++++++++++++++++++----\n t/t5200-update-server-info.sh | 41 +++++++\n 3 files changed, 158 insertions(+), 34 deletions(-)\n create mode 100755 t/t5200-update-server-info.sh\n\ndiff --git a/Documentation/git-update-server-info.txt b/Documentation/git-update-server-info.txt\nindex bd0e36492f..969bb2e15f 100644\n--- a/Documentation/git-update-server-info.txt\n+++ b/Documentation/git-update-server-info.txt\n@@ -9,7 +9,7 @@ git-update-server-info - Update auxiliary info file to help dumb servers\n SYNOPSIS\n --------\n [verse]\n-'git update-server-info' [--force]\n+'git update-server-info'\n \n DESCRIPTION\n -----------\n@@ -19,15 +19,6 @@ $GIT_OBJECT_DIRECTORY/info directories to help clients discover\n what references and packs the server has. This command\n generates such auxiliary files.\n \n-\n-OPTIONS\n--------\n-\n--f::\n---force::\n-\tUpdate the info files from scratch.\n-\n-\n OUTPUT\n ------\n \ndiff --git a/server-info.c b/server-info.c\nindex 41274d098b..e68f785c2f 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -6,82 +6,174 @@\n #include \"tag.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"strbuf.h\"\n+\n+struct update_info_ctx {\n+\tFILE *cur_fp;\n+\tFILE *old_fp; /* becomes NULL if it differs from cur_fp */\n+\tstruct strbuf cur_sb;\n+\tstruct strbuf old_sb;\n+};\n+\n+static void uic_mark_stale(struct update_info_ctx *uic)\n+{\n+\tfclose(uic->old_fp);\n+\tuic->old_fp = NULL;\n+}\n+\n+static int uic_is_stale(const struct update_info_ctx *uic)\n+{\n+\treturn uic->old_fp == NULL;\n+}\n+\n+static int uic_printf(struct update_info_ctx *uic, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tint ret = -1;\n+\n+\tva_start(ap, fmt);\n+\n+\tif (uic_is_stale(uic)) {\n+\t\tret = vfprintf(uic->cur_fp, fmt, ap);\n+\t} else {\n+\t\tssize_t r;\n+\t\tstruct strbuf *cur = &uic->cur_sb;\n+\t\tstruct strbuf *old = &uic->old_sb;\n+\n+\t\tstrbuf_reset(cur);\n+\t\tstrbuf_vinsertf(cur, 0, fmt, ap);\n+\n+\t\tstrbuf_reset(old);\n+\t\tstrbuf_grow(old, cur->len);\n+\t\tr = fread(old->buf, 1, cur->len, uic->old_fp);\n+\t\tif (r != cur->len || memcmp(old->buf, cur->buf, r))\n+\t\t\tuic_mark_stale(uic);\n+\n+\t\tif (fwrite(cur->buf, 1, cur->len, uic->cur_fp) == cur->len)\n+\t\t\tret = 0;\n+\t}\n+\n+\tva_end(ap);\n+\n+\treturn ret;\n+}\n \n /*\n * Create the file \"path\" by writing to a temporary file and renaming\n * it into place. The contents of the file come from \"generate\", which\n * should return non-zero if it encounters an error.\n */\n-static int update_info_file(char *path, int (*generate)(FILE *))\n+static int update_info_file(char *path,\n+\t\t\tint (*generate)(struct update_info_ctx *),\n+\t\t\tint force)\n {\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n \tint ret = -1;\n \tint fd = -1;\n-\tFILE *fp = NULL, *to_close;\n+\tFILE *to_close;\n+\tstruct update_info_ctx uic = {\n+\t\t.cur_fp = NULL,\n+\t\t.old_fp = NULL,\n+\t\t.cur_sb = STRBUF_INIT,\n+\t\t.old_sb = STRBUF_INIT\n+\t};\n \n \tsafe_create_leading_directories(path);\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n-\tto_close = fp = fdopen(fd, \"w\");\n-\tif (!fp)\n+\tto_close = uic.cur_fp = fdopen(fd, \"w\");\n+\tif (!uic.cur_fp)\n \t\tgoto out;\n \tfd = -1;\n-\tret = generate(fp);\n+\n+\t/* no problem on ENOENT and old_fp == NULL, it's stale, now */\n+\tif (!force)\n+\t\tuic.old_fp = fopen_or_warn(path, \"r\");\n+\n+\t/*\n+\t * uic_printf will compare incremental comparison aginst old_fp\n+\t * and mark uic as stale if needed\n+\t */\n+\tret = generate(&uic);\n \tif (ret)\n \t\tgoto out;\n-\tfp = NULL;\n+\n+\t/* new file may be shorter than the old one, check here */\n+\tif (!uic_is_stale(&uic)) {\n+\t\tstruct stat st;\n+\t\tlong new_len = ftell(uic.cur_fp);\n+\t\tint old_fd = fileno(uic.old_fp);\n+\n+\t\tif (new_len < 0) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tif (fstat(old_fd, &st) || (st.st_size != (size_t)new_len))\n+\t\t\tuic_mark_stale(&uic);\n+\t}\n+\n+\tuic.cur_fp = NULL;\n \tif (fclose(to_close))\n \t\tgoto out;\n-\tif (adjust_shared_perm(tmp) < 0)\n-\t\tgoto out;\n-\tif (rename(tmp, path) < 0)\n-\t\tgoto out;\n+\n+\tif (uic_is_stale(&uic)) {\n+\t\tif (adjust_shared_perm(tmp) < 0)\n+\t\t\tgoto out;\n+\t\tif (rename(tmp, path) < 0)\n+\t\t\tgoto out;\n+\t} else {\n+\t\tunlink(tmp);\n+\t}\n \tret = 0;\n \n out:\n \tif (ret) {\n \t\terror_errno(\"unable to update %s\", path);\n-\t\tif (fp)\n-\t\t\tfclose(fp);\n+\t\tif (uic.cur_fp)\n+\t\t\tfclose(uic.cur_fp);\n \t\telse if (fd >= 0)\n \t\t\tclose(fd);\n \t\tunlink(tmp);\n \t}\n \tfree(tmp);\n+\tif (uic.old_fp)\n+\t\tfclose(uic.old_fp);\n+\tstrbuf_release(&uic.old_sb);\n+\tstrbuf_release(&uic.cur_sb);\n \treturn ret;\n }\n \n static int add_info_ref(const char *path, const struct object_id *oid,\n \t\t\tint flag, void *cb_data)\n {\n-\tFILE *fp = cb_data;\n+\tstruct update_info_ctx *uic = cb_data;\n \tstruct object *o = parse_object(the_repository, oid);\n \tif (!o)\n \t\treturn -1;\n \n-\tif (fprintf(fp, \"%s\t%s\\n\", oid_to_hex(oid), path) < 0)\n+\tif (uic_printf(uic, \"%s\t%s\\n\", oid_to_hex(oid), path) < 0)\n \t\treturn -1;\n \n \tif (o->type == OBJ_TAG) {\n \t\to = deref_tag(the_repository, o, path, 0);\n \t\tif (o)\n-\t\t\tif (fprintf(fp, \"%s\t%s^{}\\n\",\n+\t\t\tif (uic_printf(uic, \"%s\t%s^{}\\n\",\n \t\t\t\toid_to_hex(&o->oid), path) < 0)\n \t\t\t\treturn -1;\n \t}\n \treturn 0;\n }\n \n-static int generate_info_refs(FILE *fp)\n+static int generate_info_refs(struct update_info_ctx *uic)\n {\n-\treturn for_each_ref(add_info_ref, fp);\n+\treturn for_each_ref(add_info_ref, uic);\n }\n \n-static int update_info_refs(void)\n+static int update_info_refs(int force)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n-\tint ret = update_info_file(path, generate_info_refs);\n+\tint ret = update_info_file(path, generate_info_refs, force);\n \tfree(path);\n \treturn ret;\n }\n@@ -236,14 +328,14 @@ static void free_pack_info(void)\n \tfree(info);\n }\n \n-static int write_pack_info_file(FILE *fp)\n+static int write_pack_info_file(struct update_info_ctx *uic)\n {\n \tint i;\n \tfor (i = 0; i < num_pack; i++) {\n-\t\tif (fprintf(fp, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n+\t\tif (uic_printf(uic, \"P %s\\n\", pack_basename(info[i]->p)) < 0)\n \t\t\treturn -1;\n \t}\n-\tif (fputc('\\n', fp) == EOF)\n+\tif (uic_printf(uic, \"\\n\") < 0)\n \t\treturn -1;\n \treturn 0;\n }\n@@ -254,7 +346,7 @@ static int update_info_packs(int force)\n \tint ret;\n \n \tinit_pack_info(infofile, force);\n-\tret = update_info_file(infofile, write_pack_info_file);\n+\tret = update_info_file(infofile, write_pack_info_file, force);\n \tfree_pack_info();\n \tfree(infofile);\n \treturn ret;\n@@ -269,7 +361,7 @@ int update_server_info(int force)\n \t */\n \tint errs = 0;\n \n-\terrs = errs | update_info_refs();\n+\terrs = errs | update_info_refs(force);\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nnew file mode 100755\nindex 0000000000..21a58eecb9\n--- /dev/null\n+++ b/t/t5200-update-server-info.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='Test git update-server-info'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' 'test_commit file'\n+\n+test_expect_success 'create info/refs' '\n+\tgit update-server-info &&\n+\ttest_path_is_file .git/info/refs\n+'\n+\n+test_expect_success 'modify and store mtime' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >a\n+'\n+\n+test_expect_success 'info/refs is not needlessly overwritten' '\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b\n+'\n+\n+test_expect_success 'info/refs can be forced to update' '\n+\tgit update-server-info -f &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_expect_success 'info/refs updates when changes are made' '\n+\ttest-tool chmtime =0 .git/info/refs &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\ttest_cmp a b &&\n+\tgit update-ref refs/heads/foo HEAD &&\n+\tgit update-server-info &&\n+\ttest-tool chmtime --get .git/info/refs >b &&\n+\t! test_cmp a b\n+'\n+\n+test_done\n-- \nEW\n"},{"id":"375492","messageId":"20190514094729.GA12256@sigill.intra.peff.net","threadId":"51073","inReplyTo":"87tve0w3ao.fsf@evledraar.gmail.com","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T09:47:29Z","receivedAt":"2019-05-14T09:47:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 12, 2019 at 09:16:31AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > You're probably right (especially because we'd just spent O(n) work\n> > generating the candidate file). But note that I have seen pathological\n> > cases where info/refs was gigabytes.\n> \n> Wouldn't those users be calling update-server-info from something like a\n> post-receive hook where they *know* the refs/packs just got updated?\n>\n> Well, there is \"transfer.unpackLimit\" to contend with, but that's just\n> for \"packs are same\", not \"refs are same\", and that file's presumably\n> much smaller.\n\nYeah, I think there's sort of an open question here of who is calling\nupdate-server-info when nothing got updated. I think the only place we\ncall it automatically is via receive-pack, but I'd guess Eric runs it as\npart of public-inbox scripts.\n\nI agree that this is probably mostly about info/packs. Not every push\n(or repo update) will create one, but every push _should_ be changing a\nref (if it succeeds at all). And I'd guess Eric's interest comes from\nthe use of Git in public-inbox, which is going to make frequent but\nsmall updates.\n\nSo this does seem like a really niche case; it avoids one single HTTP\nrequest of a small that should generally be small (unless you're letting\nyour pack list grow really big, but I think there are other issues with\nthat) in a case that we know will generate a bunch of other HTTP traffic\n(if somebody updated the server info, there was indeed a push, so you'd\nget a refreshed info/refs and presumably the new loose objects).\n\nThat said, the logic to preserve the mtime is pretty straightforward, so\nI don't mind it too much if Eric finds this really improves things for\nhim.\n\n> > I don't think our usual dot-locking is great here. What does the\n> > race-loser do when it can't take the lock? It can't just skip its\n> > update, since it needs to make sure that its new pack is mentioned.\n> \n> Right, I mean a *global* .git/I_AM_DOING_WRITES.lock, because there's no\n> way to square the ref backend's notion of per-ref ref lock enabling\n> concurrent pushes with update-server-info's desire to generate metadata\n> showing *all* the refs.\n\nOK. I agree that would work, but it's nasty to delay user-initiated\noperations for ongoing maintenance (another obvious place to want such a\nlock is for pruning, which can take quite a while).\n\n> > So it has to wait and poll until the other process finishes. I guess\n> > maybe that isn't the end of the world.\n> \n> If \"its\" is update-server-info this won't work. It's possible for two\n> update-server-info processes to be racing in such a way that their\n> for_each_ref() reads a ref at a given value that'll be updated 1\n> millisecond later, but to then have that update's update-server-info\n> \"win\" the race to update the info files (hypothetical locks on those\n> info files and all).\n> \n> Thus the \"info\" files will be updated to old values, since we'll see we\n> need changes, but change things to the old values.\n> \n> All things that *can* be dealt with in some clever ways, but I think\n> just further proof nobody's using this for anything serious :)\n> \n> I.e. the \"commit A happened before B\" but also \"commit B's post-* hooks\n> finished after A\" is a thing that happens quite frequently (per my\n> logs).\n\nI think it would work because any update-server-info, whether from A or\nB, will take into account the full current repo state (and we don't look\nat that state until we take the lock). So you might get an interleaved\n\"A-push, B-push, B-maint, A-maint\", but that's OK. A-maint will\nrepresent B's state when it runs.\n\n> > I'm not entirely sure of all of the magic that \"stale\" check is trying\n> > to accomplish. I think there's some bits in there that try to preserve\n> > the existing ordering, but I don't know why anyone would care (and there\n> > are so many cases where the ordering gets thrown out that I think\n> > anybody who does care is likely to get disappointed).\n> \n> My reading of it is that it's premature optimization that can go away\n> (and most of it has already), for \"it's cheap\" and \"if not it's called\n> from the 'I know I had an update'\" hook case, as noted above.<\n\nThat's my reading, too, but I didn't want to be responsible for\nregressing some obscure case. At least Eric seems to _use_\nupdate-server-info. ;)\n\n-Peff\n"},{"id":"375499","messageId":"87imudwck8.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190514094729.GA12256@sigill.intra.peff.net","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-14T10:33:11Z","receivedAt":"2019-05-14T10:33:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 14 2019, Jeff King wrote:\n\n> On Sun, May 12, 2019 at 09:16:31AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > You're probably right (especially because we'd just spent O(n) work\n>> > generating the candidate file). But note that I have seen pathological\n>> > cases where info/refs was gigabytes.\n>>\n>> Wouldn't those users be calling update-server-info from something like a\n>> post-receive hook where they *know* the refs/packs just got updated?\n>>\n>> Well, there is \"transfer.unpackLimit\" to contend with, but that's just\n>> for \"packs are same\", not \"refs are same\", and that file's presumably\n>> much smaller.\n>\n> Yeah, I think there's sort of an open question here of who is calling\n> update-server-info when nothing got updated. I think the only place we\n> call it automatically is via receive-pack, but I'd guess Eric runs it as\n> part of public-inbox scripts.\n>\n> I agree that this is probably mostly about info/packs. Not every push\n> (or repo update) will create one, but every push _should_ be changing a\n> ref (if it succeeds at all). And I'd guess Eric's interest comes from\n> the use of Git in public-inbox, which is going to make frequent but\n> small updates.\n>\n> So this does seem like a really niche case; it avoids one single HTTP\n> request of a small that should generally be small (unless you're letting\n> your pack list grow really big, but I think there are other issues with\n> that) in a case that we know will generate a bunch of other HTTP traffic\n> (if somebody updated the server info, there was indeed a push, so you'd\n> get a refreshed info/refs and presumably the new loose objects).\n>\n> That said, the logic to preserve the mtime is pretty straightforward, so\n> I don't mind it too much if Eric finds this really improves things for\n> him.\n>\n>> > I don't think our usual dot-locking is great here. What does the\n>> > race-loser do when it can't take the lock? It can't just skip its\n>> > update, since it needs to make sure that its new pack is mentioned.\n>>\n>> Right, I mean a *global* .git/I_AM_DOING_WRITES.lock, because there's no\n>> way to square the ref backend's notion of per-ref ref lock enabling\n>> concurrent pushes with update-server-info's desire to generate metadata\n>> showing *all* the refs.\n>\n> OK. I agree that would work, but it's nasty to delay user-initiated\n> operations for ongoing maintenance (another obvious place to want such a\n> lock is for pruning, which can take quite a while).\n>\n>> > So it has to wait and poll until the other process finishes. I guess\n>> > maybe that isn't the end of the world.\n>>\n>> If \"its\" is update-server-info this won't work. It's possible for two\n>> update-server-info processes to be racing in such a way that their\n>> for_each_ref() reads a ref at a given value that'll be updated 1\n>> millisecond later, but to then have that update's update-server-info\n>> \"win\" the race to update the info files (hypothetical locks on those\n>> info files and all).\n>>\n>> Thus the \"info\" files will be updated to old values, since we'll see we\n>> need changes, but change things to the old values.\n>>\n>> All things that *can* be dealt with in some clever ways, but I think\n>> just further proof nobody's using this for anything serious :)\n>>\n>> I.e. the \"commit A happened before B\" but also \"commit B's post-* hooks\n>> finished after A\" is a thing that happens quite frequently (per my\n>> logs).\n>\n> I think it would work because any update-server-info, whether from A or\n> B, will take into account the full current repo state (and we don't look\n> at that state until we take the lock). So you might get an interleaved\n> \"A-push, B-push, B-maint, A-maint\", but that's OK. A-maint will\n> represent B's state when it runs.\n\nMaybe we're talking about different things. I mean the following\nsequence:\n\n 1. Refs \"X\" and \"Y\" are at X=A Y=A\n 2. Concurrent push #1 happens, updating X from A..F\n 3. Concurrent push #2 happens, updating Y from A..F\n 4. Concurrent push #1 succeeds\n 5. Concurrent push #1 starts update-server-info. Reads X=F Y=A\n 5. Concurrent push #2 succeeds\n 6. Concurrent push #2 starts update-server-info. Reads X=F Y=F\n 7. Concurrent push #2's update-server-info finishes, X=F Y=F written to \"info\"\n 8. Concurrent push #1's update-server-info finishes, X=A Y=F written to \"info\"\n\nI.e. because we have per-ref locks and no lock at all on\nupdate-server-info (but that would need to be a global ref lock, not\njust on the \"info\" files) we can have a push that's already read \"X\"'s\nvalue as \"A\" while updating \"Y\" win the race against an\nupdate-server-info that updated \"X\"'s value to \"F\".\n\nIt will get fixed on the next push (at least as far as \"X\"'s value\ngoes), but until that time dumb clients will falsely see that \"X\" hasn't\nbeen updated.\n\n>> > I'm not entirely sure of all of the magic that \"stale\" check is trying\n>> > to accomplish. I think there's some bits in there that try to preserve\n>> > the existing ordering, but I don't know why anyone would care (and there\n>> > are so many cases where the ordering gets thrown out that I think\n>> > anybody who does care is likely to get disappointed).\n>>\n>> My reading of it is that it's premature optimization that can go away\n>> (and most of it has already), for \"it's cheap\" and \"if not it's called\n>> from the 'I know I had an update'\" hook case, as noted above.<\n>\n> That's my reading, too, but I didn't want to be responsible for\n> regressing some obscure case. At least Eric seems to _use_\n> update-server-info. ;)\n\nYeah I agree with that sentiment. I don't use it. I was just wondering\nif for those who still want it this wasn't a relatively obscure\nuse-case, so it should perhaps be hidden behind an option if there were\noptimization concerns.\n"},{"id":"375512","messageId":"20190514112451.GB26957@sigill.intra.peff.net","threadId":"51073","inReplyTo":"87imudwck8.fsf@evledraar.gmail.com","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T11:24:51Z","receivedAt":"2019-05-14T11:24:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 12:33:11PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I think it would work because any update-server-info, whether from A or\n> > B, will take into account the full current repo state (and we don't look\n> > at that state until we take the lock). So you might get an interleaved\n> > \"A-push, B-push, B-maint, A-maint\", but that's OK. A-maint will\n> > represent B's state when it runs.\n> \n> Maybe we're talking about different things. I mean the following\n> sequence:\n> \n> 1. Refs \"X\" and \"Y\" are at X=A Y=A\n> 2. Concurrent push #1 happens, updating X from A..F\n> 3. Concurrent push #2 happens, updating Y from A..F\n> 4. Concurrent push #1 succeeds\n> 5. Concurrent push #1 starts update-server-info. Reads X=F Y=A\n> 5. Concurrent push #2 succeeds\n> 6. Concurrent push #2 starts update-server-info. Reads X=F Y=F\n> 7. Concurrent push #2's update-server-info finishes, X=F Y=F written to \"info\"\n> 8. Concurrent push #1's update-server-info finishes, X=A Y=F written to \"info\"\n> \n> I.e. because we have per-ref locks and no lock at all on\n> update-server-info (but that would need to be a global ref lock, not\n> just on the \"info\" files) we can have a push that's already read \"X\"'s\n> value as \"A\" while updating \"Y\" win the race against an\n> update-server-info that updated \"X\"'s value to \"F\".\n> \n> It will get fixed on the next push (at least as far as \"X\"'s value\n> goes), but until that time dumb clients will falsely see that \"X\" hasn't\n> been updated.\n\nThat's the same situation. But I thought we were talking about having an\nupdate-server-info lock. In which case the #2 update-server-info or the\n#1 update-server-info runs in its entirety, and cannot have their read\nand write steps interleaved (that's what I meant by \"don't look at the\nstate until we take the lock\"). Then that gives us a strict ordering: we\nknow that _some_ update-server-info (be it #1 or #2's) will run after\nany given update.\n\n-Peff\n"},{"id":"375515","messageId":"20190514115047.oncvfq24fhnp64re@dcvr","threadId":"51073","inReplyTo":"20190514094729.GA12256@sigill.intra.peff.net","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-14T11:50:47Z","receivedAt":"2019-05-14T11:50:49Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> Yeah, I think there's sort of an open question here of who is calling\n> update-server-info when nothing got updated. I think the only place we\n> call it automatically is via receive-pack, but I'd guess Eric runs it as\n> part of public-inbox scripts.\n\nCorrect. post-update doesn't get run because public-inbox\nrelies on fast-import. I have mirrors using \"git fetch\", which\nalso doesn't call post-update, either so I was calling\nupdate-server-info in my mirror script.\n\nSince more people have taken an interest in mirroring things,\nI figured I'd make \"public-inbox-index\" (the script which\nupdates the Xapian and SQLite indices) call update-server-info\nitself.\n\nThat way, it's simpler to mirror (v1) inboxes:\n\n git fetch && git update-server-info && public-inbox-index\n\nbecomes:\n\n git fetch && public-inbox-index\n\nThat's a huge savings in cognitive overhead.\n\nSo, my eventual goal for this is we get to the point where any\ngit operation which changes refs will automatically update\ninfo/refs if it exists.\n\nDitto for objects/info/packs on any pack changes.\n\nThis, like my bitmap-by-default change is among the things\nI'm trying to make it easier for more users to start\nself-hosting (including mirroring) any type of git repo.\n\nAnyways, I am far from knowledgeable about the locking\ndiscussion for git, though :x\n\n> That's my reading, too, but I didn't want to be responsible for\n> regressing some obscure case. At least Eric seems to _use_\n> update-server-info. ;)\n\nI also have something else on my mind for abusing info files with :>\n(another email)\n"},{"id":"375516","messageId":"87h89xw8nk.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190514112451.GB26957@sigill.intra.peff.net","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-14T11:57:35Z","receivedAt":"2019-05-14T11:57:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 14 2019, Jeff King wrote:\n\n> On Tue, May 14, 2019 at 12:33:11PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I think it would work because any update-server-info, whether from A or\n>> > B, will take into account the full current repo state (and we don't look\n>> > at that state until we take the lock). So you might get an interleaved\n>> > \"A-push, B-push, B-maint, A-maint\", but that's OK. A-maint will\n>> > represent B's state when it runs.\n>>\n>> Maybe we're talking about different things. I mean the following\n>> sequence:\n>>\n>> 1. Refs \"X\" and \"Y\" are at X=A Y=A\n>> 2. Concurrent push #1 happens, updating X from A..F\n>> 3. Concurrent push #2 happens, updating Y from A..F\n>> 4. Concurrent push #1 succeeds\n>> 5. Concurrent push #1 starts update-server-info. Reads X=F Y=A\n>> 5. Concurrent push #2 succeeds\n>> 6. Concurrent push #2 starts update-server-info. Reads X=F Y=F\n>> 7. Concurrent push #2's update-server-info finishes, X=F Y=F written to \"info\"\n>> 8. Concurrent push #1's update-server-info finishes, X=A Y=F written to \"info\"\n>>\n>> I.e. because we have per-ref locks and no lock at all on\n>> update-server-info (but that would need to be a global ref lock, not\n>> just on the \"info\" files) we can have a push that's already read \"X\"'s\n>> value as \"A\" while updating \"Y\" win the race against an\n>> update-server-info that updated \"X\"'s value to \"F\".\n>>\n>> It will get fixed on the next push (at least as far as \"X\"'s value\n>> goes), but until that time dumb clients will falsely see that \"X\" hasn't\n>> been updated.\n>\n> That's the same situation. But I thought we were talking about having an\n> update-server-info lock. In which case the #2 update-server-info or the\n> #1 update-server-info runs in its entirety, and cannot have their read\n> and write steps interleaved (that's what I meant by \"don't look at the\n> state until we take the lock\"). Then that gives us a strict ordering: we\n> know that _some_ update-server-info (be it #1 or #2's) will run after\n> any given update.\n\nYeah you're right. I *thought* in my last E-mail we were talking about\nthe current state, but re-reading upthread I see that was a fail on my\npart.\n\nAn update-server-info lock would solve this indeed. We could still end\nup with a situation where whatever a naïve version of the lockfile API\nwould fail for the \"new\" update since the old one was underway, so we'd\nneed something similar to core.*Ref*Timeout, but if we ran into a *.lock\nor the timeout we could exit non-zero, as opposed to silently failing\nlike it does now when it races.\n"},{"id":"375520","messageId":"20190514121350.jugxtegpvcxr4vjs@dcvr","threadId":"51073","inReplyTo":"20190514115047.oncvfq24fhnp64re@dcvr","subject":"dumb HTTP things I want to do","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-14T12:13:50Z","receivedAt":"2019-05-14T12:13:52Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong wrote:\n> Jeff King wrote:\n> > That's my reading, too, but I didn't want to be responsible for\n> > regressing some obscure case. At least Eric seems to _use_\n> > update-server-info. ;)\n> \n> I also have something else on my mind for abusing info files with :>\n> (another email)\n\nI'm not sure when/if I'll have time for this; but this ought to\nbe possible:\n\n\tGIT_DIR=$HTTP_URL git \n\nAnd possible without existing admins to setup or change\nanything on their server.\n\nRight now, I could do it by setting up a WebDAV server\nand using fusedav[1] on the client.\n\nBut, not everybody runs a WebDAV server which allows PROPFIND\nfor listing files... However, info/refs and objects/info/packs\ncan give us all the info we need without needing PROPFIND. All\nwe'd need is the common GET/HEAD HTTP methods for read-only\naccess.\n\ngit doesn't need mmap; and curl + Range requests ought to be\nable to get us what we need to emulate pread. It'd be great for\nlow-latency LANs, maybe not so great with high latency; but\nprobably better in many cases than cloning a giant repo to cat\none blob.\n\nAlso, cloning on a static bundle ought to be doable with:\n\n\tgit clone $REMOTE_OR_LOCAL_PATH/foo.bundle\n\nAnd yeah, it also sucks that bundles double storage overhead\nfor admins; it would be nice if I could use bundles as alternates\nor packs...\n\nAnyways, all of this is probably a lot of work and I don't hack\nmuch, anymore.\n\n\n[1] I have many patches for fusedav, and the debian maintainer\n seems dead, and upstream's moved on: https://bugs.debian.org/fusedav\n davfs2 can't do Range requests, so it won't work for big repos...\n"},{"id":"375522","messageId":"87ftphw7mv.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190514115047.oncvfq24fhnp64re@dcvr","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-14T12:19:36Z","receivedAt":"2019-05-14T12:19:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 14 2019, Eric Wong wrote:\n\n> Jeff King wrote:\n>> Yeah, I think there's sort of an open question here of who is calling\n>> update-server-info when nothing got updated. I think the only place we\n>> call it automatically is via receive-pack, but I'd guess Eric runs it as\n>> part of public-inbox scripts.\n>\n> Correct. post-update doesn't get run because public-inbox\n> relies on fast-import. I have mirrors using \"git fetch\", which\n> also doesn't call post-update, either so I was calling\n> update-server-info in my mirror script.\n>\n> Since more people have taken an interest in mirroring things,\n> I figured I'd make \"public-inbox-index\" (the script which\n> updates the Xapian and SQLite indices) call update-server-info\n> itself.\n>\n> That way, it's simpler to mirror (v1) inboxes:\n>\n> git fetch && git update-server-info && public-inbox-index\n>\n> becomes:\n>\n> git fetch && public-inbox-index\n>\n> That's a huge savings in cognitive overhead.\n>\n> So, my eventual goal for this is we get to the point where any\n> git operation which changes refs will automatically update\n> info/refs if it exists.\n>\n> Ditto for objects/info/packs on any pack changes.\n>\n> This, like my bitmap-by-default change is among the things\n> I'm trying to make it easier for more users to start\n> self-hosting (including mirroring) any type of git repo.\n>\n> Anyways, I am far from knowledgeable about the locking\n> discussion for git, though :x\n>\n>> That's my reading, too, but I didn't want to be responsible for\n>> regressing some obscure case. At least Eric seems to _use_\n>> update-server-info. ;)\n>\n> I also have something else on my mind for abusing info files with :>\n> (another email)\n\nAside from this change, I wonder if making \"fetch\" optionally \"exit 1\"\nif no refs were updated would be useful, as in the below WIP. Of course\nit would be better to distinguish errors from \"no refs to update\".\n\nI've hacked around it not doing that in the past for similar \"fetch and\nindex\" things by looking at for-each-ref before/after, or only seeing if\nHEAD changed (so this wouldn't be a general solution...):\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4ba63d5ac6..da5414d9db 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -47,6 +47,7 @@ static int prune_tags = -1; /* unspecified */\n #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */\n\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;\n+static int exit_code;\n static int progress = -1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_children = 1;\n@@ -66,6 +67,7 @@ static struct refspec refmap = REFSPEC_INIT_FETCH;\n static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n+static int updated_refs;\n\n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -133,6 +135,8 @@ static struct option builtin_fetch_options[] = {\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n \t\t N_(\"control recursive fetching of submodules\"),\n \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },\n+\tOPT_BOOL(0, \"exit-code\", &exit_code,\n+\t\t N_(\"exit successfully if refs are updated\")),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n \t\t N_(\"dry run\")),\n \tOPT_BOOL('k', \"keep\", &keep, N_(\"keep downloaded pack\")),\n@@ -522,8 +526,10 @@ static int s_update_ref(const char *action,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret, df_conflict = 0;\n\n-\tif (dry_run)\n+\tif (dry_run) {\n+\t\tupdated_refs++;\n \t\treturn 0;\n+\t}\n \tif (!rla)\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n@@ -545,6 +551,7 @@ static int s_update_ref(const char *action,\n \tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \tfree(msg);\n+\tupdated_refs++;\n \treturn 0;\n fail:\n \tref_transaction_free(transaction);\n@@ -1680,5 +1687,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \trun_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);\n \targv_array_clear(&argv_gc_auto);\n\n-\treturn result;\n+\tif (result)\n+\t\treturn result;\n+\tif (exit_code)\n+\t\treturn !updated_refs;\n+\treturn 0;\n }\n"},{"id":"375524","messageId":"20190514122730.GB27276@sigill.intra.peff.net","threadId":"51073","inReplyTo":"20190514121350.jugxtegpvcxr4vjs@dcvr","subject":"Re: dumb HTTP things I want to do","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:27:30Z","receivedAt":"2019-05-14T12:27:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 12:13:50PM +0000, Eric Wong wrote:\n\n> I'm not sure when/if I'll have time for this; but this ought to\n> be possible:\n> \n> \tGIT_DIR=$HTTP_URL git \n> \n> And possible without existing admins to setup or change\n> anything on their server.\n> [...]\n> git doesn't need mmap; and curl + Range requests ought to be\n> able to get us what we need to emulate pread. It'd be great for\n> low-latency LANs, maybe not so great with high latency; but\n> probably better in many cases than cloning a giant repo to cat\n> one blob.\n\nMy first thought here is that we could probably just use the filesystem\nboundary as the appropriate layer, and have an httpfs fuse driver. Your\nmention of fusedav makes me think you've already gone this route.\n\nI guess non-dav http doesn't necessarily let us enumerate directories.\nBut it might be enough if we could insert ourselves in a few key spots.\nE.g., when we try to opendir(\"objects/packs\") and it fails with ENOSYS\nor similar, then we fallback to opening \"objects/info/packs\" to get the\nsame information (and ditto for loose ref traversal).\n\nThere'd still be _some_ work in Git, but it wouldn't require replacing\nevery single read with HTTP magic.\n\n> Also, cloning on a static bundle ought to be doable with:\n> \n> \tgit clone $REMOTE_OR_LOCAL_PATH/foo.bundle\n\nSome nearby discussion:\n\n https://public-inbox.org/git/20190514092900.GA11679@sigill.intra.peff.net/\n\n> And yeah, it also sucks that bundles double storage overhead\n> for admins; it would be nice if I could use bundles as alternates\n> or packs...\n\nYes. It's nice that they're a single file for some uses, but in terms of\nimplementation it would be easier if the bundle files just said \"here\nare some refs, and you can find my packfile in the relative filename\nXYZ.pack\". And then you'd just store the two of them together (and a\nmatching .idx if you wanted to use it as a real pack, but the bundle\nreaders wouldn't care).\n\nIt probably wouldn't be that hard to wedge that into the bundle format\nby bumping the version field.\n\n-Peff\n"},{"id":"375525","messageId":"20190514122957.GC27276@sigill.intra.peff.net","threadId":"51073","inReplyTo":"87ftphw7mv.fsf@evledraar.gmail.com","subject":"Re: [PATCH] update-server-info: avoid needless overwrites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:29:57Z","receivedAt":"2019-05-14T12:30:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 02:19:36PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Aside from this change, I wonder if making \"fetch\" optionally \"exit 1\"\n> if no refs were updated would be useful, as in the below WIP. Of course\n> it would be better to distinguish errors from \"no refs to update\".\n\nThat seems very sensible to me, as long as it's an optional flag as you\nhave it here.\n\n> @@ -133,6 +135,8 @@ static struct option builtin_fetch_options[] = {\n> \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n> \t\t N_(\"control recursive fetching of submodules\"),\n> \t\t PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },\n> +\tOPT_BOOL(0, \"exit-code\", &exit_code,\n> +\t\t N_(\"exit successfully if refs are updated\")),\n\nI think it makes sense to flip this explanation: \"exit non-zero if no\nrefs are updated\" or \"treat it as an error if no refs are updated\".\nSince the default behavior is already to exit successfully in this case. :)\n\nWe may also want to advertise a particular result code so callers can\ndistinguish it from regular die() errors.\n\n-Peff\n"},{"id":"375573","messageId":"20190515004551.emrxvboqemwnqh4g@dcvr","threadId":"51073","inReplyTo":"87ftphw7mv.fsf@evledraar.gmail.com","subject":"[PATCH 2/1] server-info: conditionally update on fetch","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-15T00:45:51Z","receivedAt":"2019-05-15T00:45:54Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason wrote:\n> Aside from this change, I wonder if making \"fetch\" optionally \"exit 1\"\n> if no refs were updated would be useful, as in the below WIP. Of course\n> it would be better to distinguish errors from \"no refs to update\".\n\nYes, we should've had this feature all along :)\n\nAnd it's easy for me to build off your WIP to have fetch\nupdate server info iff info/refs already exists:\n\n-------8<-------\nSubject: [PATCH 2/1] server-info: conditionally update on fetch\n\nSince fetch can invalidate existing server info files, use the\nnew `updated_refs' counter to update server info files iff\ninfo/refs already exists.\n\nNote: this depends on Ævar's WIP in:\n\thttps://public-inbox.org/git/87ftphw7mv.fsf@evledraar.gmail.com/\n\nSigned-off-by: Eric Wong \n---\n builtin/fetch.c | 3 +++\n server-info.c | 22 +++++++++++++++++++---\n t/t5513-fetch-track.sh | 21 +++++++++++++++++++++\n 3 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex da5414d9db..b35d4d105d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1681,6 +1681,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tclose_all_packs(the_repository->objects);\n \n+\tif (updated_refs)\n+\t\tupdate_server_info(-1);\n+\n \targv_array_pushl(&argv_gc_auto, \"gc\", \"--auto\", NULL);\n \tif (verbosity < 0)\n \t\targv_array_push(&argv_gc_auto, \"--quiet\");\ndiff --git a/server-info.c b/server-info.c\nindex e68f785c2f..d4065d56a3 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -7,6 +7,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"strbuf.h\"\n+#include \"dir.h\"\n \n struct update_info_ctx {\n \tFILE *cur_fp;\n@@ -170,10 +171,25 @@ static int generate_info_refs(struct update_info_ctx *uic)\n \treturn for_each_ref(add_info_ref, uic);\n }\n \n-static int update_info_refs(int force)\n+static int want_update(int *force, const char *path)\n+{\n+\tif (*force < 0) {\n+\t\tif (file_exists(path))\n+\t\t\t*force = 0; /* continue to normal update */\n+\t\telse\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+static int update_info_refs(int *force)\n {\n \tchar *path = git_pathdup(\"info/refs\");\n-\tint ret = update_info_file(path, generate_info_refs, force);\n+\tint ret = 0;\n+\n+\tif (want_update(force, path))\n+\t\tret = update_info_file(path, generate_info_refs, *force);\n+\n \tfree(path);\n \treturn ret;\n }\n@@ -361,7 +377,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(&force);\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\ndiff --git a/t/t5513-fetch-track.sh b/t/t5513-fetch-track.sh\nindex 65d1e05bd6..421f16ddfd 100755\n--- a/t/t5513-fetch-track.sh\n+++ b/t/t5513-fetch-track.sh\n@@ -27,4 +27,25 @@ test_expect_success fetch '\n \t)\n '\n \n+test_expect_success 'info/refs not created by fetch' '\n+\t(\n+\t\tcd other &&\n+\t\ttest_path_is_dir .git/info &&\n+\t\t! test_path_is_file .git/info/refs\n+\t)\n+'\n+\n+test_expect_success 'info/refs updated by fetch if it already exists' '\n+\tgit branch b/for-info-refs &&\n+\t(\n+\t\tcd other &&\n+\t\tgit update-server-info &&\n+\t\ttest_path_is_file .git/info/refs &&\n+\t\t! grep b/for-info-refs .git/info/refs &&\n+\t\tgit fetch &&\n+\t\ttest_path_is_file .git/info/refs &&\n+\t\tgrep b/for-info-refs .git/info/refs\n+\t)\n+'\n+\n test_done\n-- \nEW\n"},{"id":"375644","messageId":"20190515203839.zixx23bwzeyto6a3@dcvr","threadId":"51073","inReplyTo":"20190515004551.emrxvboqemwnqh4g@dcvr","subject":"[WIP] repack leaving stale entries in objects/info/packs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-15T20:38:39Z","receivedAt":"2019-05-15T20:38:41Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"I've also noticed objects/info/packs contains stale entries\nafter repack/gc runs on current git.\n\nTried adding reprepare_packed_git before update_server_info,\nbut that didn't seem to work; so maybe something isn't cleared.\nMight have time to investigate more this week, might not...\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 67f8978043..ec46f2099a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -559,8 +559,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tprune_shallow(PRUNE_QUICK);\n \t}\n \n-\tif (!no_update_server_info)\n+\tif (!no_update_server_info) {\n+\t\treprepare_packed_git(the_repository);\n \t\tupdate_server_info(0);\n+\t}\n \tremove_temporary_files();\n \n \tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 7411bf7fec..867d4fb0c7 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -71,6 +71,7 @@ test_expect_success 'gc --keep-largest-pack' '\n \t\tgit gc --keep-largest-pack &&\n \t\t( cd .git/objects/pack && ls *.pack ) >pack-list &&\n \t\ttest_line_count = 2 pack-list &&\n+\t\ttest_line_count = 2 .git/objects/info/packs &&\n \t\ttest_path_is_file $BASE_PACK &&\n \t\tgit fsck\n \t)\n\n"},{"id":"375656","messageId":"20190515214806.GA31693@sigill.intra.peff.net","threadId":"51073","inReplyTo":"20190515203839.zixx23bwzeyto6a3@dcvr","subject":"Re: [WIP] repack leaving stale entries in objects/info/packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-15T21:48:06Z","receivedAt":"2019-05-15T21:48:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2019 at 08:38:39PM +0000, Eric Wong wrote:\n\n> I've also noticed objects/info/packs contains stale entries\n> after repack/gc runs on current git.\n> \n> Tried adding reprepare_packed_git before update_server_info,\n> but that didn't seem to work; so maybe something isn't cleared.\n> Might have time to investigate more this week, might not...\n\nWe never delete entries from the in-memory packed_git list; a reprepare\nonly adds to the list. You'd need to teach update_server_info() to\nignore packs which are no longer present (or switch to exec-ing a\nseparate update-server-info binary).\n\n-Peff\n"},{"id":"376098","messageId":"20190523085959.4q76pokx2gy6wqq7@dcvr","threadId":"51073","inReplyTo":"20190515214806.GA31693@sigill.intra.peff.net","subject":"[PATCH] server-info: do not list unlinked packs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-23T08:59:59Z","receivedAt":"2019-05-23T09:00:01Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Wed, May 15, 2019 at 08:38:39PM +0000, Eric Wong wrote:\n> \n> > I've also noticed objects/info/packs contains stale entries\n> > after repack/gc runs on current git.\n> > \n> > Tried adding reprepare_packed_git before update_server_info,\n> > but that didn't seem to work; so maybe something isn't cleared.\n> > Might have time to investigate more this week, might not...\n> \n> We never delete entries from the in-memory packed_git list; a reprepare\n> only adds to the list. You'd need to teach update_server_info() to\n> ignore packs which are no longer present (or switch to exec-ing a\n> separate update-server-info binary).\n\nAh, checking files_exists() and setting a bit seems sufficient.\n\n--------8<---------\nSubject: [PATCH] server-info: do not list unlinked packs\n\nHaving non-existent packs in objects/info/packs causes\ndumb HTTP clients to abort.\n\nThere remains a small window where the old objects/info/packs\nfile can refer to unlinked packs. That's unavoidable even on a\nlocal FS given the time-of-use-time-of-check window between\nlisting and retrieving files.\n\nSigned-off-by: Eric Wong \n---\n I think the small window I refer to can be worked around by\n teaching the dumb HTTP client to reread objects/info/packs\n if it 404s while trying to GET a pack...\n\n object-store.h | 1 +\n server-info.c | 7 ++++++-\n t/t6500-gc.sh | 2 ++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 272e01e452..2c9facc8f2 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -77,6 +77,7 @@ struct packed_git {\n \t\t freshened:1,\n \t\t do_not_close:1,\n \t\t pack_promisor:1,\n+\t\t pack_unlinked:1,\n \t\t multi_pack_index:1;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct revindex_entry *revindex;\ndiff --git a/server-info.c b/server-info.c\nindex 41274d098b..69e2c5279b 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"dir.h\"\n #include \"repository.h\"\n #include \"refs.h\"\n #include \"object.h\"\n@@ -199,12 +200,16 @@ static void init_pack_info(const char *infofile, int force)\n \t\t */\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n+\t\tif (!file_exists(p->pack_name)) {\n+\t\t\tp->pack_unlinked = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\ti++;\n \t}\n \tnum_pack = i;\n \tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n \tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!p->pack_local)\n+\t\tif (!p->pack_local || p->pack_unlinked)\n \t\t\tcontinue;\n \t\tassert(i < num_pack);\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 515c6735e9..c0f04dc6b0 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -71,6 +71,8 @@ test_expect_success 'gc --keep-largest-pack' '\n \t\tgit gc --keep-largest-pack &&\n \t\t( cd .git/objects/pack && ls *.pack ) >pack-list &&\n \t\ttest_line_count = 2 pack-list &&\n+\t\tawk \"/^P /{print \\$2}\" <.git/objects/info/packs >pack-info &&\n+\t\ttest_line_count = 2 pack-info &&\n \t\ttest_path_is_file $BASE_PACK &&\n \t\tgit fsck\n \t)\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\n-- \nEW\n"},{"id":"376101","messageId":"20190523102456.GA6583@sigill.intra.peff.net","threadId":"51073","inReplyTo":"20190523085959.4q76pokx2gy6wqq7@dcvr","subject":"Re: [PATCH] server-info: do not list unlinked packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-23T10:24:56Z","receivedAt":"2019-05-23T10:25:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 23, 2019 at 08:59:59AM +0000, Eric Wong wrote:\n\n> > We never delete entries from the in-memory packed_git list; a reprepare\n> > only adds to the list. You'd need to teach update_server_info() to\n> > ignore packs which are no longer present (or switch to exec-ing a\n> > separate update-server-info binary).\n> \n> Ah, checking files_exists() and setting a bit seems sufficient.\n\nYes, though we do we even need to store the bit?\n\nI.e.,\n\n> @@ -199,12 +200,16 @@ static void init_pack_info(const char *infofile, int force)\n> \t\t */\n> \t\tif (!p->pack_local)\n> \t\t\tcontinue;\n> +\t\tif (!file_exists(p->pack_name)) {\n> +\t\t\tp->pack_unlinked = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\ti++;\n> \t}\n> \tnum_pack = i;\n> \tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n> \tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n> -\t\tif (!p->pack_local)\n> +\t\tif (!p->pack_local || p->pack_unlinked)\n> \t\t\tcontinue;\n> \t\tassert(i < num_pack);\n> \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n\nIf we just check file_exists() in the second loop, then this is entirely\nlocal to update_server_info(). And other users of packed_git do not have\nto wonder who is responsible for setting that flag in the global list.\n\nIt does mean you'd over-allocate the array (and num_pack would have to\nbe adjusted down to \"i\" after the second loop), but that's not a big\ndeal. I do think the whole two-loop thing would be more readable if we\nsimply grew it on the fly with ALLOC_GROW().\n\n-Peff\n"},{"id":"376108","messageId":"20190523172723.eny6smdt57zxau6z@dcvr","threadId":"51073","inReplyTo":"20190523102456.GA6583@sigill.intra.peff.net","subject":"[PATCH v2] server-info: do not list unlinked packs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-23T17:27:23Z","receivedAt":"2019-05-23T17:27:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Thu, May 23, 2019 at 08:59:59AM +0000, Eric Wong wrote:\n> \n> > > We never delete entries from the in-memory packed_git list; a reprepare\n> > > only adds to the list. You'd need to teach update_server_info() to\n> > > ignore packs which are no longer present (or switch to exec-ing a\n> > > separate update-server-info binary).\n> > \n> > Ah, checking files_exists() and setting a bit seems sufficient.\n> \n> Yes, though we do we even need to store the bit?\n\nI wanted to avoid the over-allocation, and I hit a bounds error\nbecause I forgot to adjust num_pack as you mentioned below.\n\n> I.e.,\n> \n> > @@ -199,12 +200,16 @@ static void init_pack_info(const char *infofile, int force)\n> > \t\t */\n> > \t\tif (!p->pack_local)\n> > \t\t\tcontinue;\n> > +\t\tif (!file_exists(p->pack_name)) {\n> > +\t\t\tp->pack_unlinked = 1;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > \t\ti++;\n> > \t}\n> > \tnum_pack = i;\n> > \tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n> > \tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n> > -\t\tif (!p->pack_local)\n> > +\t\tif (!p->pack_local || p->pack_unlinked)\n> > \t\t\tcontinue;\n> > \t\tassert(i < num_pack);\n> > \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n> \n> If we just check file_exists() in the second loop, then this is entirely\n> local to update_server_info(). And other users of packed_git do not have\n> to wonder who is responsible for setting that flag in the global list.\n> \n> It does mean you'd over-allocate the array (and num_pack would have to\n> be adjusted down to \"i\" after the second loop), but that's not a big\n> deal. I do think the whole two-loop thing would be more readable if we\n> simply grew it on the fly with ALLOC_GROW().\n\nALLOC_GROW makes the whole thing much nicer.\nThanks for the hint :>\n\n---------------------8<---------------------\nSubject: [PATCH] server-info: do not list unlinked packs\n\nHaving non-existent packs in objects/info/packs causes\ndumb HTTP clients to abort.\n\nv2: use single loop with ALLOC_GROW as suggested by Jeff King\n\nSigned-off-by: Eric Wong \nHelped-by: Jeff King \n---\nInterdiff:\n diff --git a/object-store.h b/object-store.h\n index 2c9facc8f2..272e01e452 100644\n --- a/object-store.h\n +++ b/object-store.h\n @@ -77,7 +77,6 @@ struct packed_git {\n \t\t freshened:1,\n \t\t do_not_close:1,\n \t\t pack_promisor:1,\n -\t\t pack_unlinked:1,\n \t\t multi_pack_index:1;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct revindex_entry *revindex;\n diff --git a/server-info.c b/server-info.c\n index 69e2c5279b..92187c70db 100644\n --- a/server-info.c\n +++ b/server-info.c\n @@ -192,30 +192,21 @@ static void init_pack_info(const char *infofile, int force)\n {\n \tstruct packed_git *p;\n \tint stale;\n -\tint i = 0;\n +\tint i;\n +\tsize_t alloc = 0;\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 \t\t */\n -\t\tif (!p->pack_local)\n -\t\t\tcontinue;\n -\t\tif (!file_exists(p->pack_name)) {\n -\t\t\tp->pack_unlinked = 1;\n -\t\t\tcontinue;\n -\t\t}\n -\t\ti++;\n -\t}\n -\tnum_pack = i;\n -\tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n -\tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n -\t\tif (!p->pack_local || p->pack_unlinked)\n +\t\tif (!p->pack_local || !file_exists(p->pack_name))\n \t\t\tcontinue;\n -\t\tassert(i < num_pack);\n +\n +\t\ti = num_pack++;\n +\t\tALLOC_GROW(info, num_pack, alloc);\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n \t\tinfo[i]->p = p;\n \t\tinfo[i]->old_num = -1;\n -\t\ti++;\n \t}\n \n \tif (infofile && !force)\n\n server-info.c | 18 +++++++-----------\n t/t6500-gc.sh | 2 ++\n 2 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 41274d098b..92187c70db 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"dir.h\"\n #include \"repository.h\"\n #include \"refs.h\"\n #include \"object.h\"\n@@ -191,26 +192,21 @@ static void init_pack_info(const char *infofile, int force)\n {\n \tstruct packed_git *p;\n \tint stale;\n-\tint i = 0;\n+\tint i;\n+\tsize_t alloc = 0;\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 \t\t */\n-\t\tif (!p->pack_local)\n-\t\t\tcontinue;\n-\t\ti++;\n-\t}\n-\tnum_pack = i;\n-\tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n-\tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!p->pack_local)\n+\t\tif (!p->pack_local || !file_exists(p->pack_name))\n \t\t\tcontinue;\n-\t\tassert(i < num_pack);\n+\n+\t\ti = num_pack++;\n+\t\tALLOC_GROW(info, num_pack, alloc);\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n \t\tinfo[i]->p = p;\n \t\tinfo[i]->old_num = -1;\n-\t\ti++;\n \t}\n \n \tif (infofile && !force)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 515c6735e9..c0f04dc6b0 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -71,6 +71,8 @@ test_expect_success 'gc --keep-largest-pack' '\n \t\tgit gc --keep-largest-pack &&\n \t\t( cd .git/objects/pack && ls *.pack ) >pack-list &&\n \t\ttest_line_count = 2 pack-list &&\n+\t\tawk \"/^P /{print \\$2}\" <.git/objects/info/packs >pack-info &&\n+\t\ttest_line_count = 2 pack-info &&\n \t\ttest_path_is_file $BASE_PACK &&\n \t\tgit fsck\n \t)\n-- \nEW\n"},{"id":"376142","messageId":"20190524060552.GA25694@sigill.intra.peff.net","threadId":"51073","inReplyTo":"20190523172723.eny6smdt57zxau6z@dcvr","subject":"Re: [PATCH v2] server-info: do not list unlinked packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T06:05:53Z","receivedAt":"2019-05-24T06:05:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 23, 2019 at 05:27:23PM +0000, Eric Wong wrote:\n\n> ALLOC_GROW makes the whole thing much nicer.\n> Thanks for the hint :>\n> \n> ---------------------8<---------------------\n> Subject: [PATCH] server-info: do not list unlinked packs\n\nThis looks fine to me (though the \"v2\" note is in your commit message).\nI'd probably break it into two separate patches, but I can live with it\neither way.\n\n-Peff\n"},{"id":"376155","messageId":"87sgt4fgqq.fsf@evledraar.gmail.com","threadId":"51073","inReplyTo":"20190523172723.eny6smdt57zxau6z@dcvr","subject":"Re: [PATCH v2] server-info: do not list unlinked packs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-24T07:34:21Z","receivedAt":"2019-05-24T08:18:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 23 2019, Eric Wong wrote:\n\n> Jeff King wrote:\n>> On Thu, May 23, 2019 at 08:59:59AM +0000, Eric Wong wrote:\n>>\n>> > > We never delete entries from the in-memory packed_git list; a reprepare\n>> > > only adds to the list. You'd need to teach update_server_info() to\n>> > > ignore packs which are no longer present (or switch to exec-ing a\n>> > > separate update-server-info binary).\n>> >\n>> > Ah, checking files_exists() and setting a bit seems sufficient.\n>>\n>> Yes, though we do we even need to store the bit?\n>\n> I wanted to avoid the over-allocation, and I hit a bounds error\n> because I forgot to adjust num_pack as you mentioned\n> below. [...]ALLOC_GROW makes the whole thing much nicer.\n\nIf you want to avoid over-allocation the last thing you want is\nALLOC_GROW() :)\n\nI.e. see alloc_nr() in cache.h, we explicitly over-allocate with it.\n\nBut as our extensive use of it shows this sort of pattern is the right\ntrade-off, both in terms of performance on modern hardware, and code\nreadability in cases like this where we're never going to realistically\nhave to worry about memory pressure.\n"}]}