{"thread":{"id":"47666","subject":"[PATCH 0/3] nd/shared-index-fix update","startedAt":"2018-01-22T11:03:52Z","lastAt":"2018-01-24T18:09:45Z","messageCount":11,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"337055","messageId":"20180122110334.4411-1-pclouds@gmail.com","threadId":"47666","inReplyTo":null,"subject":"[PATCH 0/3] nd/shared-index-fix update","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-22T11:03:31Z","receivedAt":"2018-01-22T11:03:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This only changes the last patch to correct the test prerequisite and\na couple minor refinements. Interdiff:\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 5494389dbb..d2a8e0312a 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -401,7 +401,7 @@ done <<\\EOF\n 0642 -rw-r---w-\n EOF\n \n-test_expect_success POSIXPERM 'graceful handling splitting index is not allowed' '\n+test_expect_success SANITY 'graceful handling when splitting index is not allowed' '\n \ttest_create_repo ro &&\n \t(\n \t\tcd ro &&\n@@ -409,11 +409,11 @@ test_expect_success POSIXPERM 'graceful handling splitting index is not allowed'\n \t\tgit update-index --split-index &&\n \t\ttest -f .git/sharedindex.*\n \t) &&\n-\ttest_when_finished \"chmod -R u+w ro\" &&\n-\tchmod -R u-w ro &&\n \tcp ro/.git/index new-index &&\n+\ttest_when_finished \"chmod u+w ro/.git\" &&\n+\tchmod u-w ro/.git &&\n \tGIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n-\tchmod -R u+w ro &&\n+\tchmod u+w ro/.git &&\n \trm ro/.git/sharedindex.* &&\n \tGIT_INDEX_FILE=new-index git ls-files >actual &&\n \techo initial.t >expected &&\n\nNguyễn Thái Ngọc Duy (3):\n  read-cache.c: change type of \"temp\" in write_shared_index()\n  read-cache.c: move tempfile creation/cleanup out of write_shared_index\n  read-cache: don't write index twice if we can't write shared index\n\n read-cache.c           | 40 ++++++++++++++++++++++------------------\n t/t1700-split-index.sh | 19 +++++++++++++++++++\n 2 files changed, 41 insertions(+), 18 deletions(-)\n\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337056","messageId":"20180122110334.4411-2-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180122110334.4411-1-pclouds@gmail.com","subject":"[PATCH 1/3] read-cache.c: change type of \"temp\" in write_shared_index()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-22T11:03:32Z","receivedAt":"2018-01-22T11:03:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This local variable 'temp' will be passed in from the caller in the next\npatch. To reduce patch noise, let's change its type now while it's still\na local variable and get all the trival conversion out of the next patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n read-cache.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..536086e1fe 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char *current_hex)\n static int write_shared_index(struct index_state *istate,\n \t\t\t      struct lock_file *lock, unsigned flags)\n {\n-\tstruct tempfile *temp;\n+\tstruct tempfile *real_temp;\n+\tstruct tempfile **temp = &real_temp;\n \tstruct split_index *si = istate->split_index;\n \tint ret;\n \n-\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n-\tif (!temp) {\n+\treal_temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\tif (!real_temp) {\n \t\thashclr(si->base_sha1);\n \t\treturn do_write_locked_index(istate, lock, flags);\n \t}\n+\ttemp = &real_temp;\n \tmove_cache_to_base_index(istate);\n-\tret = do_write_index(si->base, temp, 1);\n+\tret = do_write_index(si->base, *temp, 1);\n \tif (ret) {\n-\t\tdelete_tempfile(&temp);\n+\t\tdelete_tempfile(temp);\n \t\treturn ret;\n \t}\n-\tret = adjust_shared_perm(get_tempfile_path(temp));\n+\tret = adjust_shared_perm(get_tempfile_path(*temp));\n \tif (ret) {\n \t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(temp));\n-\t\tdelete_tempfile(&temp);\n+\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(*temp));\n+\t\tdelete_tempfile(temp);\n \t\terrno = save_errno;\n \t\treturn ret;\n \t}\n-\tret = rename_tempfile(&temp,\n+\tret = rename_tempfile(temp,\n \t\t\t      git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n \tif (!ret) {\n \t\thashcpy(si->base_sha1, si->base->sha1);\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337057","messageId":"20180122110334.4411-3-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180122110334.4411-1-pclouds@gmail.com","subject":"[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-22T11:03:33Z","receivedAt":"2018-01-22T11:03:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"For one thing, we have more consistent cleanup procedure now and always\nkeep errno intact.\n\nThe real purpose is the ability to break out of write_locked_index()\nearly when mks_tempfile() fails in the next patch. It's more awkward to\ndo it if this mks_tempfile() is still inside write_shared_index().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n read-cache.c | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 536086e1fe..c568643f55 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char *current_hex)\n }\n \n static int write_shared_index(struct index_state *istate,\n-\t\t\t      struct lock_file *lock, unsigned flags)\n+\t\t\t      struct tempfile **temp)\n {\n-\tstruct tempfile *real_temp;\n-\tstruct tempfile **temp = &real_temp;\n \tstruct split_index *si = istate->split_index;\n \tint ret;\n \n-\treal_temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n-\tif (!real_temp) {\n-\t\thashclr(si->base_sha1);\n-\t\treturn do_write_locked_index(istate, lock, flags);\n-\t}\n-\ttemp = &real_temp;\n \tmove_cache_to_base_index(istate);\n \tret = do_write_index(si->base, *temp, 1);\n-\tif (ret) {\n-\t\tdelete_tempfile(temp);\n+\tif (ret)\n \t\treturn ret;\n-\t}\n \tret = adjust_shared_perm(get_tempfile_path(*temp));\n \tif (ret) {\n-\t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(*temp));\n-\t\tdelete_tempfile(temp);\n-\t\terrno = save_errno;\n \t\treturn ret;\n \t}\n \tret = rename_tempfile(temp,\n@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tnew_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;\n \n \tif (new_shared_index) {\n-\t\tret = write_shared_index(istate, lock, flags);\n+\t\tstruct tempfile *temp;\n+\t\tint saved_errno;\n+\n+\t\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\t\tif (!temp) {\n+\t\t\thashclr(si->base_sha1);\n+\t\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\t} else\n+\t\t\tret = write_shared_index(istate, &temp);\n+\n+\t\tsaved_errno = errno;\n+\t\tif (is_tempfile_active(temp))\n+\t\t\tdelete_tempfile(&temp);\n+\t\terrno = saved_errno;\n+\n \t\tif (ret)\n \t\t\tgoto out;\n \t}\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337058","messageId":"20180122110334.4411-4-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180122110334.4411-1-pclouds@gmail.com","subject":"[PATCH 3/3] read-cache: don't write index twice if we can't write shared index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-22T11:03:34Z","receivedAt":"2018-01-22T11:04:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In a0a967568e (\"update-index --split-index: do not split if $GIT_DIR is\nread only\", 2014-06-13), we tried to make sure we can still write an\nindex, even if the shared index can not be written.\n\nWe did so by just calling 'do_write_locked_index()' just before\n'write_shared_index()'.  'do_write_locked_index()' always at least\ncloses the tempfile nowadays, and used to close or commit the lockfile\nif COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was\nintroduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of\n'write_locked_index()'.\n\nAfter calling 'write_shared_index()', we call 'write_split_index()',\nwhich calls 'do_write_locked_index()' again, which then tries to use the\nclosed lockfile again, but in fact fails to do so as it's already\nclosed. This eventually leads to a segfault.\n\nMake sure to write the main index only once.\n\n[nd: most of the commit message and investigation done by Thomas, I only\ntweaked the solution a bit]\n\nHelped-by: Thomas Gummerer <t.gummerer@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n read-cache.c           |  5 +++--\n t/t1700-split-index.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c568643f55..c58c0a978a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tif (!temp) {\n \t\t\thashclr(si->base_sha1);\n \t\t\tret = do_write_locked_index(istate, lock, flags);\n-\t\t} else\n-\t\t\tret = write_shared_index(istate, &temp);\n+\t\t\tgoto out;\n+\t\t}\n+\t\tret = write_shared_index(istate, &temp);\n \n \t\tsaved_errno = errno;\n \t\tif (is_tempfile_active(temp))\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex af9b847761..d2a8e0312a 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -401,4 +401,23 @@ done <<\\EOF\n 0642 -rw-r---w-\n EOF\n \n+test_expect_success SANITY 'graceful handling when splitting index is not allowed' '\n+\ttest_create_repo ro &&\n+\t(\n+\t\tcd ro &&\n+\t\ttest_commit initial &&\n+\t\tgit update-index --split-index &&\n+\t\ttest -f .git/sharedindex.*\n+\t) &&\n+\tcp ro/.git/index new-index &&\n+\ttest_when_finished \"chmod u+w ro/.git\" &&\n+\tchmod u-w ro/.git &&\n+\tGIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n+\tchmod u+w ro/.git &&\n+\trm ro/.git/sharedindex.* &&\n+\tGIT_INDEX_FILE=new-index git ls-files >actual &&\n+\techo initial.t >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337117","messageId":"xmqqfu6xjxrx.fsf@gitster.mtv.corp.google.com","threadId":"47666","inReplyTo":"20180122110334.4411-4-pclouds@gmail.com","subject":"Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-22T23:09:54Z","receivedAt":"2018-01-22T23:10:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index af9b847761..d2a8e0312a 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -401,4 +401,23 @@ done <<\\EOF\n>  0642 -rw-r---w-\n>  EOF\n>  \n> +test_expect_success SANITY 'graceful handling when splitting index is not allowed' '\n\nIs SANITY the only prereq we want, or do we want both it and POSIXPERM?\n\nIn \"git grep SANITY t/\" output, we see that they are almost always\nused together.\n\n> +\ttest_create_repo ro &&\n> +\t(\n> +\t\tcd ro &&\n> +\t\ttest_commit initial &&\n> +\t\tgit update-index --split-index &&\n> +\t\ttest -f .git/sharedindex.*\n> +\t) &&\n> +\tcp ro/.git/index new-index &&\n> +\ttest_when_finished \"chmod u+w ro/.git\" &&\n> +\tchmod u-w ro/.git &&\n> +\tGIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n> +\tchmod u+w ro/.git &&\n> +\trm ro/.git/sharedindex.* &&\n> +\tGIT_INDEX_FILE=new-index git ls-files >actual &&\n> +\techo initial.t >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_done\n"},{"id":"337144","messageId":"CACsJy8B059uev+HpL=JGDdh+FM19vSFObhp7DB4eAt-u6d7GnQ@mail.gmail.com","threadId":"47666","inReplyTo":"xmqqfu6xjxrx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-23T04:06:02Z","receivedAt":"2018-01-23T04:06:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 23, 2018 at 6:09 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n>> index af9b847761..d2a8e0312a 100755\n>> --- a/t/t1700-split-index.sh\n>> +++ b/t/t1700-split-index.sh\n>> @@ -401,4 +401,23 @@ done <<\\EOF\n>>  0642 -rw-r---w-\n>>  EOF\n>>\n>> +test_expect_success SANITY 'graceful handling when splitting index is not allowed' '\n>\n> Is SANITY the only prereq we want, or do we want both it and POSIXPERM?\n>\n> In \"git grep SANITY t/\" output, we see that they are almost always\n> used together.\n\nSANITY test does more or less the same as this one (chmod then verify)\nwhich is the reason I removed POSIXPERM. Looking at other tests\nthough, they don't do anything different than what I do here and still\nrequire both SANITY and POSIXPERM. I'm adding POSIXPERM back.\n\n>\n>> +     test_create_repo ro &&\n>> +     (\n>> +             cd ro &&\n>> +             test_commit initial &&\n>> +             git update-index --split-index &&\n>> +             test -f .git/sharedindex.*\n>> +     ) &&\n>> +     cp ro/.git/index new-index &&\n>> +     test_when_finished \"chmod u+w ro/.git\" &&\n>> +     chmod u-w ro/.git &&\n>> +     GIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n>> +     chmod u+w ro/.git &&\n>> +     rm ro/.git/sharedindex.* &&\n>> +     GIT_INDEX_FILE=new-index git ls-files >actual &&\n>> +     echo initial.t >expected &&\n>> +     test_cmp expected actual\n>> +'\n>> +\n>>  test_done\n\n\n\n-- \nDuy\n"},{"id":"337213","messageId":"20180124093829.12966-1-pclouds@gmail.com","threadId":"47666","inReplyTo":"xmqqfu6xjxrx.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] nd/shared-index-fix updates","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:38:26Z","receivedAt":"2018-01-24T09:39:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This makes the new test need both prerequisites SANITY and POSIXPERM\ninstead of just SANITY (on 'pu').  This is how most other tests do it\nso we do the same to be safe.\n\nNguyễn Thái Ngọc Duy (3):\n  read-cache.c: change type of \"temp\" in write_shared_index()\n  read-cache.c: move tempfile creation/cleanup out of write_shared_index\n  read-cache: don't write index twice if we can't write shared index\n\n read-cache.c           | 40 ++++++++++++++++++++++------------------\n t/t1700-split-index.sh | 19 +++++++++++++++++++\n 2 files changed, 41 insertions(+), 18 deletions(-)\n\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337214","messageId":"20180124093829.12966-2-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180124093829.12966-1-pclouds@gmail.com","subject":"[PATCH 1/3] read-cache.c: change type of \"temp\" in write_shared_index()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:38:27Z","receivedAt":"2018-01-24T09:39:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This local variable 'temp' will be passed in from the caller in the next\npatch. To reduce patch noise, let's change its type now while it's still\na local variable and get all the trival conversion out of the next patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n read-cache.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..536086e1fe 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char *current_hex)\n static int write_shared_index(struct index_state *istate,\n \t\t\t      struct lock_file *lock, unsigned flags)\n {\n-\tstruct tempfile *temp;\n+\tstruct tempfile *real_temp;\n+\tstruct tempfile **temp = &real_temp;\n \tstruct split_index *si = istate->split_index;\n \tint ret;\n \n-\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n-\tif (!temp) {\n+\treal_temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\tif (!real_temp) {\n \t\thashclr(si->base_sha1);\n \t\treturn do_write_locked_index(istate, lock, flags);\n \t}\n+\ttemp = &real_temp;\n \tmove_cache_to_base_index(istate);\n-\tret = do_write_index(si->base, temp, 1);\n+\tret = do_write_index(si->base, *temp, 1);\n \tif (ret) {\n-\t\tdelete_tempfile(&temp);\n+\t\tdelete_tempfile(temp);\n \t\treturn ret;\n \t}\n-\tret = adjust_shared_perm(get_tempfile_path(temp));\n+\tret = adjust_shared_perm(get_tempfile_path(*temp));\n \tif (ret) {\n \t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(temp));\n-\t\tdelete_tempfile(&temp);\n+\t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(*temp));\n+\t\tdelete_tempfile(temp);\n \t\terrno = save_errno;\n \t\treturn ret;\n \t}\n-\tret = rename_tempfile(&temp,\n+\tret = rename_tempfile(temp,\n \t\t\t      git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n \tif (!ret) {\n \t\thashcpy(si->base_sha1, si->base->sha1);\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337215","messageId":"20180124093829.12966-3-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180124093829.12966-1-pclouds@gmail.com","subject":"[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:38:28Z","receivedAt":"2018-01-24T09:39:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"For one thing, we have more consistent cleanup procedure now and always\nkeep errno intact.\n\nThe real purpose is the ability to break out of write_locked_index()\nearly when mks_tempfile() fails in the next patch. It's more awkward to\ndo it if this mks_tempfile() is still inside write_shared_index().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n read-cache.c | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 536086e1fe..c568643f55 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char *current_hex)\n }\n \n static int write_shared_index(struct index_state *istate,\n-\t\t\t      struct lock_file *lock, unsigned flags)\n+\t\t\t      struct tempfile **temp)\n {\n-\tstruct tempfile *real_temp;\n-\tstruct tempfile **temp = &real_temp;\n \tstruct split_index *si = istate->split_index;\n \tint ret;\n \n-\treal_temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n-\tif (!real_temp) {\n-\t\thashclr(si->base_sha1);\n-\t\treturn do_write_locked_index(istate, lock, flags);\n-\t}\n-\ttemp = &real_temp;\n \tmove_cache_to_base_index(istate);\n \tret = do_write_index(si->base, *temp, 1);\n-\tif (ret) {\n-\t\tdelete_tempfile(temp);\n+\tif (ret)\n \t\treturn ret;\n-\t}\n \tret = adjust_shared_perm(get_tempfile_path(*temp));\n \tif (ret) {\n-\t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", get_tempfile_path(*temp));\n-\t\tdelete_tempfile(temp);\n-\t\terrno = save_errno;\n \t\treturn ret;\n \t}\n \tret = rename_tempfile(temp,\n@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tnew_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;\n \n \tif (new_shared_index) {\n-\t\tret = write_shared_index(istate, lock, flags);\n+\t\tstruct tempfile *temp;\n+\t\tint saved_errno;\n+\n+\t\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\t\tif (!temp) {\n+\t\t\thashclr(si->base_sha1);\n+\t\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\t} else\n+\t\t\tret = write_shared_index(istate, &temp);\n+\n+\t\tsaved_errno = errno;\n+\t\tif (is_tempfile_active(temp))\n+\t\t\tdelete_tempfile(&temp);\n+\t\terrno = saved_errno;\n+\n \t\tif (ret)\n \t\t\tgoto out;\n \t}\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337216","messageId":"20180124093829.12966-4-pclouds@gmail.com","threadId":"47666","inReplyTo":"20180124093829.12966-1-pclouds@gmail.com","subject":"[PATCH 3/3] read-cache: don't write index twice if we can't write shared index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:38:29Z","receivedAt":"2018-01-24T09:39:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In a0a967568e (\"update-index --split-index: do not split if $GIT_DIR is\nread only\", 2014-06-13), we tried to make sure we can still write an\nindex, even if the shared index can not be written.\n\nWe did so by just calling 'do_write_locked_index()' just before\n'write_shared_index()'.  'do_write_locked_index()' always at least\ncloses the tempfile nowadays, and used to close or commit the lockfile\nif COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was\nintroduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of\n'write_locked_index()'.\n\nAfter calling 'write_shared_index()', we call 'write_split_index()',\nwhich calls 'do_write_locked_index()' again, which then tries to use the\nclosed lockfile again, but in fact fails to do so as it's already\nclosed. This eventually leads to a segfault.\n\nMake sure to write the main index only once.\n\n[nd: most of the commit message and investigation done by Thomas, I only\ntweaked the solution a bit]\n\nHelped-by: Thomas Gummerer <t.gummerer@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n read-cache.c           |  5 +++--\n t/t1700-split-index.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c568643f55..c58c0a978a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tif (!temp) {\n \t\t\thashclr(si->base_sha1);\n \t\t\tret = do_write_locked_index(istate, lock, flags);\n-\t\t} else\n-\t\t\tret = write_shared_index(istate, &temp);\n+\t\t\tgoto out;\n+\t\t}\n+\t\tret = write_shared_index(istate, &temp);\n \n \t\tsaved_errno = errno;\n \t\tif (is_tempfile_active(temp))\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex af9b847761..cbcefa6e5f 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -401,4 +401,23 @@ done <<\\EOF\n 0642 -rw-r---w-\n EOF\n \n+test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is not allowed' '\n+\ttest_create_repo ro &&\n+\t(\n+\t\tcd ro &&\n+\t\ttest_commit initial &&\n+\t\tgit update-index --split-index &&\n+\t\ttest -f .git/sharedindex.*\n+\t) &&\n+\tcp ro/.git/index new-index &&\n+\ttest_when_finished \"chmod u+w ro/.git\" &&\n+\tchmod u-w ro/.git &&\n+\tGIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n+\tchmod u+w ro/.git &&\n+\trm ro/.git/sharedindex.* &&\n+\tGIT_INDEX_FILE=new-index git ls-files >actual &&\n+\techo initial.t >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337276","messageId":"xmqq1siff7rx.fsf@gitster.mtv.corp.google.com","threadId":"47666","inReplyTo":"20180124093829.12966-1-pclouds@gmail.com","subject":"Re: [PATCH 0/3] nd/shared-index-fix updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-24T18:09:38Z","receivedAt":"2018-01-24T18:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This makes the new test need both prerequisites SANITY and POSIXPERM\n> instead of just SANITY (on 'pu').  This is how most other tests do it\n> so we do the same to be safe.\n>\n> Nguyễn Thái Ngọc Duy (3):\n>   read-cache.c: change type of \"temp\" in write_shared_index()\n>   read-cache.c: move tempfile creation/cleanup out of write_shared_index\n>   read-cache: don't write index twice if we can't write shared index\n>\n>  read-cache.c           | 40 ++++++++++++++++++++++------------------\n>  t/t1700-split-index.sh | 19 +++++++++++++++++++\n>  2 files changed, 41 insertions(+), 18 deletions(-)\n\nThanks; let's move this to 'next'.\n"}]}