{"thread":{"id":"49843","subject":"[PATCH v2] read-cache: write all indexes with the same permissions","startedAt":"2018-11-16T17:31:27Z","lastAt":"2018-11-18T19:04:41Z","messageCount":16,"participants":["Christian Couder","Duy Nguyen","SZEDER Gábor","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"363518","messageId":"20181116173105.21784-1-chriscool@tuxfamily.org","threadId":"49843","inReplyTo":null,"subject":"[PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-11-16T17:31:05Z","receivedAt":"2018-11-16T17:31:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nChange the code that writes out the shared index to use\nmks_tempfile_sm() instead of mks_tempfile().\n\nThe create_tempfile() function is used to write out the main\n\".git/index\" (via \".git/index.lock\") using lock_file(). The\ncreate_tempfile() function respects the umask, as it uses open() with\n0666, whereas the mks_tempfile() function uses open() with 0600.\n\nSo mks_tempfile() which is used to create the shared index file is\nlikely to create such a file with restricted permissions compared to\nthe main \".git/index\" file.\n\nA bug related to this was spotted, fixed and tested for in df801f3f9f\n(\"read-cache: use shared perms when writing shared index\", 2017-06-25)\nand 3ee83f48e5 (\"t1700: make sure split-index respects\ncore.sharedrepository\", 2017-06-25).\n\nHowever, as noted in those commits we'd still create the file as 0600,\nand would just re-chmod it depending on the setting of\ncore.sharedRepository. So without core.splitIndex a system with\ne.g. the umask set to group writeability would work for the members of\nthe group, but not with core.splitIndex set, as members of the group\nwould not be able to access the shared index file.\n\nLet's instead make the two consistent by using mks_tempfile_sm() and\npassing 0666 in its `mode` argument.\n\nNote that we cannot use the create_tempfile() function itself that is\nused to write the main \".git/index\" file because we want the XXXXXX\npart of the \"sharedindex_XXXXXX\" argument to be replaced by a pseudo\nrandom value and create_tempfile() doesn't do that.\n\nIdeally we'd split up the adjust_shared_perm() function to one that\ncan give us the mode we want so we could just call open() instead of\nopen() followed by chmod(), but that's an unrelated cleanup. We\nalready have that minor issue with the \"index\" file #leftoverbits.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n\nThis is a simpler fix iterating from Ævar's RFC patch and the\nfollowing discussions:\n\nhttps://public-inbox.org/git/20181113153235.25402-1-avarab@gmail.com/\n\n read-cache.c           |  3 ++-\n t/t1700-split-index.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 8c924506dd..ea80600bff 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tstruct tempfile *temp;\n \t\tint saved_errno;\n \n-\t\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\t\t/* Same permissions as the main .git/index file */\n+\t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n \t\tif (!temp) {\n \t\t\toidclr(&si->base_oid);\n \t\t\tret = do_write_locked_index(istate, lock, flags);\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 2ac47aa0e4..fa1d3d468b 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n \n+test_expect_success POSIXPERM 'same mode for index & split index' '\n+\tgit init same-mode &&\n+\t(\n+\t\tcd same-mode &&\n+\t\ttest_commit A &&\n+\t\ttest_modebits .git/index >index_mode &&\n+\t\ttest_must_fail git config core.sharedRepository &&\n+\t\tgit -c core.splitIndex=true status &&\n+\t\tshared=$(ls .git/sharedindex.*) &&\n+\t\tcase \"$shared\" in\n+\t\t*\" \"*)\n+\t\t\t# we have more than one???\n+\t\t\tfalse ;;\n+\t\t*)\n+\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n+\t\t\ttest_cmp index_mode split_index_mode ;;\n+\t\tesac\n+\t)\n+'\n+\n while read -r mode modebits\n do\n \ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n-- \n2.19.1.1053.g063ed687ac\n\n"},{"id":"363520","messageId":"CACsJy8Cdk8YQWJM1HAFYWB6qJpepNQoj86yrTqF9Rg3oN0TeUA@mail.gmail.com","threadId":"49843","inReplyTo":"20181116173105.21784-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-16T18:02:56Z","receivedAt":"2018-11-16T18:03:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 16, 2018 at 6:31 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n> diff --git a/read-cache.c b/read-cache.c\n> index 8c924506dd..ea80600bff 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n>                 struct tempfile *temp;\n>                 int saved_errno;\n>\n> -               temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n> +               /* Same permissions as the main .git/index file */\n\nIf the permission is already correct from the beginning (of this temp\nfile), should df801f3f9f be reverted since we don't need to adjust\npermission anymore?\n\nOr does $GIT_DIR/index go through the same adjust_shared_perm() anyway\nin the end, which means df801f3f9f must stay? If so, perhaps clarify\nthat in the commit message.\n\n> +               temp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n>                 if (!temp) {\n>                         oidclr(&si->base_oid);\n>                         ret = do_write_locked_index(istate, lock, flags);\n-- \nDuy\n"},{"id":"363521","messageId":"20181116182934.GN30222@szeder.dev","threadId":"49843","inReplyTo":"20181116173105.21784-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-16T18:29:34Z","receivedAt":"2018-11-16T18:29:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 16, 2018 at 06:31:05PM +0100, Christian Couder wrote:\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 2ac47aa0e4..fa1d3d468b 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n>  \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n>  '\n>  \n> +test_expect_success POSIXPERM 'same mode for index & split index' '\n> +\tgit init same-mode &&\n> +\t(\n> +\t\tcd same-mode &&\n> +\t\ttest_commit A &&\n> +\t\ttest_modebits .git/index >index_mode &&\n> +\t\ttest_must_fail git config core.sharedRepository &&\n> +\t\tgit -c core.splitIndex=true status &&\n> +\t\tshared=$(ls .git/sharedindex.*) &&\n\nI think the command substitution and 'ls' are unnecessary, and\n\n  shared=.git/sharedindex.*\n\nwould work as well.\n\n> +\t\tcase \"$shared\" in\n> +\t\t*\" \"*)\n> +\t\t\t# we have more than one???\n> +\t\t\tfalse ;;\n> +\t\t*)\n> +\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n> +\t\t\ttest_cmp index_mode split_index_mode ;;\n> +\t\tesac\n> +\t)\n> +'\n> +\n>  while read -r mode modebits\n>  do\n>  \ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n> -- \n> 2.19.1.1053.g063ed687ac\n> \n"},{"id":"363524","messageId":"CAP8UFD2Y4dC4GfjgPDtR3gyrG_3hOvn-bRHMDNVSutaCF49i8g@mail.gmail.com","threadId":"49843","inReplyTo":"CACsJy8Cdk8YQWJM1HAFYWB6qJpepNQoj86yrTqF9Rg3oN0TeUA@mail.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-11-16T19:10:44Z","receivedAt":"2018-11-16T19:10:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Nov 16, 2018 at 7:03 PM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Fri, Nov 16, 2018 at 6:31 PM Christian Couder\n> <christian.couder@gmail.com> wrote:\n> > diff --git a/read-cache.c b/read-cache.c\n> > index 8c924506dd..ea80600bff 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n> >                 struct tempfile *temp;\n> >                 int saved_errno;\n> >\n> > -               temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n> > +               /* Same permissions as the main .git/index file */\n>\n> If the permission is already correct from the beginning (of this temp\n> file), should df801f3f9f be reverted since we don't need to adjust\n> permission anymore?\n\ndf801f3f9f (read-cache: use shared perms when writing shared index,\n2017-06-25) was fixing the bug that permissions of the shared index\nfile did not take into account the shared permissions (which are about\ncore.sharedRepository; \"shared\" has a different meaning in \"shared\nindex file\" and in \"shared permissions\").\n\nThis fix only changes permissions before the shared permissions are\ntaken into account (so before adjust_shared_perm() is called).\n\n> Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway\n> in the end, which means df801f3f9f must stay?\n\nYeah, $GIT_DIR/index goes through adjust_shared_perm() too because\ncreate_tempfile() calls adjust_shared_perm(). So indeed df801f3f9f\nmust stay.\n\n> If so, perhaps clarify\n> that in the commit message.\n\nThere is already the following about df801f3f9f:\n\n---\nA bug related to this was spotted, fixed and tested for in df801f3f9f\n(\"read-cache: use shared perms when writing shared index\", 2017-06-25)\nand 3ee83f48e5 (\"t1700: make sure split-index respects\ncore.sharedrepository\", 2017-06-25).\n\nHowever, as noted in those commits we'd still create the file as 0600,\nand would just re-chmod it depending on the setting of\ncore.sharedRepository.\n---\n\nSo I would think that df801f3f9f should perhaps have explained that\ncreate_tempfile() calls adjust_shared_perm(), but I don't think that\nit is very relevant in this commit message. We are already talking\nabout df801f3f9f which should be enough to explain the issue\ndf801f3f9f fixed, and I think we should not need to explain in more\ndetails why df801f3f9f did a good job. It's not as if we are reverting\nit. We are just complementing it with another fix related to what\nhappens before adjust_shared_perm() is called.\n\nI think rewording the comment a bit might help though, for example maybe:\n\n/* Same initial permissions as the main .git/index file */\n\ninstead of:\n\n/* Same permissions as the main .git/index file */\n\nThanks,\nChristian.\n"},{"id":"363525","messageId":"CACsJy8ACgDq_EkV5-d9jPiy4LEMuzK8EDdgsdSANsQu=U9TZgQ@mail.gmail.com","threadId":"49843","inReplyTo":"CAP8UFD2Y4dC4GfjgPDtR3gyrG_3hOvn-bRHMDNVSutaCF49i8g@mail.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-16T19:16:41Z","receivedAt":"2018-11-16T19:17:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 16, 2018 at 8:10 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Fri, Nov 16, 2018 at 7:03 PM Duy Nguyen <pclouds@gmail.com> wrote:\n> >\n> > On Fri, Nov 16, 2018 at 6:31 PM Christian Couder\n> > <christian.couder@gmail.com> wrote:\n> > > diff --git a/read-cache.c b/read-cache.c\n> > > index 8c924506dd..ea80600bff 100644\n> > > --- a/read-cache.c\n> > > +++ b/read-cache.c\n> > > @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n> > >                 struct tempfile *temp;\n> > >                 int saved_errno;\n> > >\n> > > -               temp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n> > > +               /* Same permissions as the main .git/index file */\n> >\n> > If the permission is already correct from the beginning (of this temp\n> > file), should df801f3f9f be reverted since we don't need to adjust\n> > permission anymore?\n>\n> df801f3f9f (read-cache: use shared perms when writing shared index,\n> 2017-06-25) was fixing the bug that permissions of the shared index\n> file did not take into account the shared permissions (which are about\n> core.sharedRepository; \"shared\" has a different meaning in \"shared\n> index file\" and in \"shared permissions\").\n>\n> This fix only changes permissions before the shared permissions are\n> taken into account (so before adjust_shared_perm() is called).\n>\n> > Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway\n> > in the end, which means df801f3f9f must stay?\n>\n> Yeah, $GIT_DIR/index goes through adjust_shared_perm() too because\n> create_tempfile() calls adjust_shared_perm(). So indeed df801f3f9f\n> must stay.\n\nAh thanks. By the time I got to this part\n\n> Let's instead make the two consistent by using mks_tempfile_sm() and\n> passing 0666 in its `mode` argument.\n\nwent look at that function and back, I forgot about the paragraph above it.\n-- \nDuy\n"},{"id":"363528","messageId":"87h8gg25bf.fsf@evledraar.gmail.com","threadId":"49843","inReplyTo":"20181116182934.GN30222@szeder.dev","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-16T19:25:08Z","receivedAt":"2018-11-16T19:25:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 16 2018, SZEDER Gábor wrote:\n\n> On Fri, Nov 16, 2018 at 06:31:05PM +0100, Christian Couder wrote:\n>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n>> index 2ac47aa0e4..fa1d3d468b 100755\n>> --- a/t/t1700-split-index.sh\n>> +++ b/t/t1700-split-index.sh\n>> @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n>>  \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n>>  '\n>>\n>> +test_expect_success POSIXPERM 'same mode for index & split index' '\n>> +\tgit init same-mode &&\n>> +\t(\n>> +\t\tcd same-mode &&\n>> +\t\ttest_commit A &&\n>> +\t\ttest_modebits .git/index >index_mode &&\n>> +\t\ttest_must_fail git config core.sharedRepository &&\n>> +\t\tgit -c core.splitIndex=true status &&\n>> +\t\tshared=$(ls .git/sharedindex.*) &&\n>\n> I think the command substitution and 'ls' are unnecessary, and\n>\n>   shared=.git/sharedindex.*\n>\n> would work as well.\n\nLooks like it. FWIW I just copy/pasted what an adjacent test was doing\nfor consistency, which was added in one of Christian's earlier changes\nto this behavior.\n\nBut yeah, if the test can be made simpler in a portable way it would\nmake sense to make this a two-parter test cleanup & bug fix series.\n\n>> +\t\tcase \"$shared\" in\n>> +\t\t*\" \"*)\n>> +\t\t\t# we have more than one???\n>> +\t\t\tfalse ;;\n>> +\t\t*)\n>> +\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n>> +\t\t\ttest_cmp index_mode split_index_mode ;;\n>> +\t\tesac\n>> +\t)\n>> +'\n>> +\n>>  while read -r mode modebits\n>>  do\n>>  \ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n>> --\n>> 2.19.1.1053.g063ed687ac\n>>\n"},{"id":"363529","messageId":"CAP8UFD2tC9B_m=NmK4DTCJP=o+L+RKy9C_2ra9fgeNMzOuCZ3A@mail.gmail.com","threadId":"49843","inReplyTo":"20181116182934.GN30222@szeder.dev","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-11-16T19:25:43Z","receivedAt":"2018-11-16T19:25:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Nov 16, 2018 at 7:29 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> On Fri, Nov 16, 2018 at 06:31:05PM +0100, Christian Couder wrote:\n> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > index 2ac47aa0e4..fa1d3d468b 100755\n> > --- a/t/t1700-split-index.sh\n> > +++ b/t/t1700-split-index.sh\n> > @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n> >       test $(ls .git/sharedindex.* | wc -l) -le 2\n> >  '\n> >\n> > +test_expect_success POSIXPERM 'same mode for index & split index' '\n> > +     git init same-mode &&\n> > +     (\n> > +             cd same-mode &&\n> > +             test_commit A &&\n> > +             test_modebits .git/index >index_mode &&\n> > +             test_must_fail git config core.sharedRepository &&\n> > +             git -c core.splitIndex=true status &&\n> > +             shared=$(ls .git/sharedindex.*) &&\n>\n> I think the command substitution and 'ls' are unnecessary, and\n>\n>   shared=.git/sharedindex.*\n>\n> would work as well.\n\nIf there is no shared index file with the above we would get:\n\nshared=.git/sharedindex.*\n$ echo $shared\n.git/sharedindex.*\n\nwhich seems bug prone to me.\n"},{"id":"363547","messageId":"xmqqy39sgjyp.fsf@gitster-ct.c.googlers.com","threadId":"49843","inReplyTo":"CAP8UFD2tC9B_m=NmK4DTCJP=o+L+RKy9C_2ra9fgeNMzOuCZ3A@mail.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-17T08:57:18Z","receivedAt":"2018-11-17T08:57:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n>> > +test_expect_success POSIXPERM 'same mode for index & split index' '\n>> > +     git init same-mode &&\n>> > +     (\n>> > +             cd same-mode &&\n>> > +             test_commit A &&\n>> > +             test_modebits .git/index >index_mode &&\n>> > +             test_must_fail git config core.sharedRepository &&\n>> > +             git -c core.splitIndex=true status &&\n>> > +             shared=$(ls .git/sharedindex.*) &&\n>>\n>> I think the command substitution and 'ls' are unnecessary, and\n>>\n>>   shared=.git/sharedindex.*\n>>\n>> would work as well.\n>\n> If there is no shared index file with the above we would get:\n>\n> shared=.git/sharedindex.*\n> $ echo $shared\n> .git/sharedindex.*\n>\n> which seems bug prone to me.\n\nIt is not bug *PRONE*, but is already wrong, as the way this\nvariable is used is\n\n        +\t\tcase \"$shared\" in\n        +\t\t*\" \"*)\n        +\t\t\t# we have more than one???\n        +\t\t\tfalse ;;\n        +\t\t*)\n        +\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n        +\t\t\ttest_cmp index_mode split_index_mode ;;\n        +\t\tesac\n\ni.e. we'd think there is a singleton \".git/shared/index.*\" and we\ncan feed it to test_modebits.\n\nSo what you wrote originally is corrrect.\n"},{"id":"363549","messageId":"xmqqpnv4gigi.fsf@gitster-ct.c.googlers.com","threadId":"49843","inReplyTo":"20181116173105.21784-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-17T09:29:49Z","receivedAt":"2018-11-17T09:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> However, as noted in those commits we'd still create the file as 0600,\n> and would just re-chmod it depending on the setting of\n> core.sharedRepository. So without core.splitIndex a system with\n> e.g. the umask set to group writeability would work for the members of\n> the group, but not with core.splitIndex set, as members of the group\n> would not be able to access the shared index file.\n\nI am not sure what the above wants to say.  If we are not making\nnecessary call to adjust-shared-perm, then it is irrelevant that the\nlack of the call does not immediately cause an apparent problem for\nusers who happens to have non-restrictive group perm bit in their\numask.  Another group member whose umask is tighter will eventually\nuse the repository and end up creating a file unreadable to group\nmembers.\n\nAre you saying that we _lack_ necessary call when core.sharedRepository\nis set?  If so, a commit that fixes such a bug would be the best\nplace to have a paragraph like the above.  If not, the above description\nsimply misleads the readers.\n\n> Let's instead make the two consistent by using mks_tempfile_sm() and\n> passing 0666 in its `mode` argument.\n\nOn the other hand, this is a relevant description; this patch kills\nan inconsistency that is very short lived (I am assuming that there\nis no bug in the current code before this patch and we make\nnecessary calls to adjust-shared-perm when core.sharedrepository is\nset).\n\n> Note that we cannot use the create_tempfile() function itself that is\n> used to write the main \".git/index\" file because we want the XXXXXX\n> part of the \"sharedindex_XXXXXX\" argument to be replaced by a pseudo\n> random value and create_tempfile() doesn't do that.\n\nSure.  Pseudo-random-ness is less important than the resulting\nfilename being unique.  \"Because we are asking for a unique file to\nbe created, we cannot use create_tempfile() interface that is\ndesigned to be used to create a file with known name.\"\n\nBut is that really worth saying, I wonder.\n\n> Ideally we'd split up the adjust_shared_perm() function to one that\n> can give us the mode we want so we could just call open() instead of\n> open() followed by chmod(), but that's an unrelated cleanup.\n\nI would drop this paragraph, as I think this is totally incorrect.\nImagine your umask is tighter than the target permission.  You ask\nsuch a helper function and get \"you want 0660\".  Doing open(0660)\nwould not help you an iota---you'd need chmod() or fchmod() to\nadjust the result anyway, which already is done by\nadjust-shared-perm.\n\n> We already have that minor issue with the \"index\" file\n> #leftoverbits.\n\nThe above \"Ideally\", which I suspect is totally bogus, would show up\nwhey people look for that keyword in the list archive.  This is one\nof the reasons why I try to write it after at least one person\nsanity checks that an idea floated is worth remembering.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>\n> This is a simpler fix iterating from Ævar's RFC patch and the\n> following discussions:\n>\n> https://public-inbox.org/git/20181113153235.25402-1-avarab@gmail.com/\n>\n>  read-cache.c           |  3 ++-\n>  t/t1700-split-index.sh | 20 ++++++++++++++++++++\n>  2 files changed, 22 insertions(+), 1 deletion(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 8c924506dd..ea80600bff 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n>  \t\tstruct tempfile *temp;\n>  \t\tint saved_errno;\n>  \n> -\t\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n> +\t\t/* Same permissions as the main .git/index file */\n> +\t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n>  \t\tif (!temp) {\n>  \t\t\toidclr(&si->base_oid);\n>  \t\t\tret = do_write_locked_index(istate, lock, flags);\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 2ac47aa0e4..fa1d3d468b 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n>  \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n>  '\n>  \n> +test_expect_success POSIXPERM 'same mode for index & split index' '\n> +\tgit init same-mode &&\n> +\t(\n> +\t\tcd same-mode &&\n> +\t\ttest_commit A &&\n> +\t\ttest_modebits .git/index >index_mode &&\n> +\t\ttest_must_fail git config core.sharedRepository &&\n> +\t\tgit -c core.splitIndex=true status &&\n> +\t\tshared=$(ls .git/sharedindex.*) &&\n> +\t\tcase \"$shared\" in\n> +\t\t*\" \"*)\n> +\t\t\t# we have more than one???\n> +\t\t\tfalse ;;\n> +\t\t*)\n> +\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n> +\t\t\ttest_cmp index_mode split_index_mode ;;\n> +\t\tesac\n> +\t)\n> +'\n> +\n>  while read -r mode modebits\n>  do\n>  \ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n"},{"id":"363550","messageId":"CAP8UFD0f_oD2cm61exc9mCczD59ze0Qj1cHGn-MvtSMWNXA+gg@mail.gmail.com","threadId":"49843","inReplyTo":"xmqqpnv4gigi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-11-17T11:19:41Z","receivedAt":"2018-11-17T11:19:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Nov 17, 2018 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> > However, as noted in those commits we'd still create the file as 0600,\n> > and would just re-chmod it depending on the setting of\n> > core.sharedRepository. So without core.splitIndex a system with\n> > e.g. the umask set to group writeability would work for the members of\n> > the group, but not with core.splitIndex set, as members of the group\n> > would not be able to access the shared index file.\n>\n> I am not sure what the above wants to say.\n\nI tried to improve from Ævar's previous commit message but I agree\nthat the above is not very clear.\n\n> If we are not making\n> necessary call to adjust-shared-perm,\n\nThe issue is that adjust_shared_perm() returns immediately when\ncore.sharedRepository is unset (or false). So when it is unset (or\nfalse), and when the umask is 0022 or 0002 for example, then the index\nand the shared index will not have the same permissions because one is\ncreated using open() with mode 0666 and the other with mode 0600.\n\n> then it is irrelevant that the\n> lack of the call does not immediately cause an apparent problem for\n> users who happens to have non-restrictive group perm bit in their\n> umask.  Another group member whose umask is tighter will eventually\n> use the repository and end up creating a file unreadable to group\n> members.\n\nThe issue is that a group member with non-restrictive group perm bit\nin their umask, like 0022 or 0002, will currently create an unreadable\nshared index when using the repo.\n\nI agree that it is much safer to just set core.sharedRepository to\n\"true\" or \"all\", but maybe in some setups/systems it might be ok to\nrely on everyone having non-restrictive group perm bit in their umask.\n\n> Are you saying that we _lack_ necessary call when core.sharedRepository\n> is set?\n\nNo, I am saying that, when it is unset, adjust_shared_perm() does nothing.\n\n> If so, a commit that fixes such a bug would be the best\n> place to have a paragraph like the above.  If not, the above description\n> simply misleads the readers.\n\nI agree that it is a bit misleading. Maybe something like:\n\n\"However, as noted in those commits we'd still create the file as\n0600, and would just re-chmod it only if core.sharedRepository is set\nto \"true\" or \"all\". If core.sharedRepository is unset or set to\n\"false\", then the file mode will not be changed, so without\ncore.splitIndex a system with e.g. the umask set to group writeability\nwould work for a group member, but not with core.splitIndex set, as\ngroup members would not be able to access the shared index file.\n\n> > Let's instead make the two consistent by using mks_tempfile_sm() and\n> > passing 0666 in its `mode` argument.\n>\n> On the other hand, this is a relevant description; this patch kills\n> an inconsistency that is very short lived (I am assuming that there\n> is no bug in the current code before this patch and we make\n> necessary calls to adjust-shared-perm when core.sharedrepository is\n> set).\n\nIt is unfortunately not short lived when core.sharedrepository is\nunset for example as adjust_shared_perm() starts with:\n\nint adjust_shared_perm(const char *path)\n{\n        int old_mode, new_mode;\n\n        if (!get_shared_repository())\n                return 0;\n\nbut get_shared_repository() will return PERM_UMASK which is 0 when\ngit_config_get_value(\"core.sharedrepository\", ...) returns a non zero\nvalue which happens when \"core.sharedrepository\" is unset.\n\nMaybe there is a bug somewhere in adjust_shared_perm() or the\nfunctions it calls, but I don't know this part of the code base much.\n\n> > Note that we cannot use the create_tempfile() function itself that is\n> > used to write the main \".git/index\" file because we want the XXXXXX\n> > part of the \"sharedindex_XXXXXX\" argument to be replaced by a pseudo\n> > random value and create_tempfile() doesn't do that.\n>\n> Sure.  Pseudo-random-ness is less important than the resulting\n> filename being unique.  \"Because we are asking for a unique file to\n> be created, we cannot use create_tempfile() interface that is\n> designed to be used to create a file with known name.\"\n>\n> But is that really worth saying, I wonder.\n\nI am ok with either your version or removing the above from the commit message.\n\n> > Ideally we'd split up the adjust_shared_perm() function to one that\n> > can give us the mode we want so we could just call open() instead of\n> > open() followed by chmod(), but that's an unrelated cleanup.\n>\n> I would drop this paragraph, as I think this is totally incorrect.\n> Imagine your umask is tighter than the target permission.  You ask\n> such a helper function and get \"you want 0660\".  Doing open(0660)\n> would not help you an iota---you'd need chmod() or fchmod() to\n> adjust the result anyway, which already is done by\n> adjust-shared-perm.\n\nIt seems to me that it is not done when \"core.sharedrepository\" is unset.\n\n> > We already have that minor issue with the \"index\" file\n> > #leftoverbits.\n>\n> The above \"Ideally\", which I suspect is totally bogus, would show up\n> whey people look for that keyword in the list archive.  This is one\n> of the reasons why I try to write it after at least one person\n> sanity checks that an idea floated is worth remembering.\n\nIt was in Ævar's commit message and I thought it might be better to\nkeep it so that people looking for that keyword could find the above\nas well as the previous RFC patch.\n"},{"id":"363551","messageId":"20181117122417.GP30222@szeder.dev","threadId":"49843","inReplyTo":"CAP8UFD2tC9B_m=NmK4DTCJP=o+L+RKy9C_2ra9fgeNMzOuCZ3A@mail.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-17T12:24:17Z","receivedAt":"2018-11-17T12:24:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 16, 2018 at 08:25:43PM +0100, Christian Couder wrote:\n> On Fri, Nov 16, 2018 at 7:29 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> >\n> > On Fri, Nov 16, 2018 at 06:31:05PM +0100, Christian Couder wrote:\n> > > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > > index 2ac47aa0e4..fa1d3d468b 100755\n> > > --- a/t/t1700-split-index.sh\n> > > +++ b/t/t1700-split-index.sh\n> > > @@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n> > >       test $(ls .git/sharedindex.* | wc -l) -le 2\n> > >  '\n> > >\n> > > +test_expect_success POSIXPERM 'same mode for index & split index' '\n> > > +     git init same-mode &&\n> > > +     (\n> > > +             cd same-mode &&\n> > > +             test_commit A &&\n> > > +             test_modebits .git/index >index_mode &&\n> > > +             test_must_fail git config core.sharedRepository &&\n> > > +             git -c core.splitIndex=true status &&\n> > > +             shared=$(ls .git/sharedindex.*) &&\n> >\n> > I think the command substitution and 'ls' are unnecessary, and\n> >\n> >   shared=.git/sharedindex.*\n> >\n> > would work as well.\n> \n> If there is no shared index file with the above we would get:\n> \n> shared=.git/sharedindex.*\n> $ echo $shared\n> .git/sharedindex.*\n> \n> which seems bug prone to me.\n\nThat's just a non-existing file, for which 'test_modebits' will print\nnothing, which, in turn, will not match the modebits of '.git/index'.\nAnd the \"there are more than one shared index files\" case is handled\nby the case statement that was snipped from the email context.\n\n"},{"id":"363554","messageId":"xmqqftvzhn22.fsf@gitster-ct.c.googlers.com","threadId":"49843","inReplyTo":"CAP8UFD0f_oD2cm61exc9mCczD59ze0Qj1cHGn-MvtSMWNXA+gg@mail.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-17T13:05:09Z","receivedAt":"2018-11-17T13:05:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> \"However, as noted in those commits we'd still create the file as\n> 0600, and would just re-chmod it only if core.sharedRepository is set\n> to \"true\" or \"all\". If core.sharedRepository is unset or set to\n> \"false\", then the file mode will not be changed, so without\n> core.splitIndex a system with e.g. the umask set to group writeability\n> would work for a group member, but not with core.splitIndex set, as\n> group members would not be able to access the shared index file.\n\nThat is irrelevant.  The repository needs to be configured properly\nif it wanted to be used by the members of the group, period.\n\n> It is unfortunately not short lived when core.sharedrepository is\n> unset for example as adjust_shared_perm() starts with:\n>\n> int adjust_shared_perm(const char *path)\n> {\n>         int old_mode, new_mode;\n>\n>         if (!get_shared_repository())\n>                 return 0;\n>\n> but get_shared_repository() will return PERM_UMASK which is 0 when\n> git_config_get_value(\"core.sharedrepository\", ...) returns a non zero\n> value which happens when \"core.sharedrepository\" is unset.\n\nWhich is to say, you get an unwanted result when your repository is\nnot configured properly.  It is not a news, and I have no sympathy.\n\nJust configure your repository properly and you'll be fine.\n\n>> > Ideally we'd split up the adjust_shared_perm() function to one that\n>> > can give us the mode we want so we could just call open() instead of\n>> > open() followed by chmod(), but that's an unrelated cleanup.\n>>\n>> I would drop this paragraph, as I think this is totally incorrect.\n>> Imagine your umask is tighter than the target permission.  You ask\n>> such a helper function and get \"you want 0660\".  Doing open(0660)\n>> would not help you an iota---you'd need chmod() or fchmod() to\n>> adjust the result anyway, which already is done by\n>> adjust-shared-perm.\n>\n> It seems to me that it is not done when \"core.sharedrepository\" is unset.\n\nSo?  You are assuming that the repository is misconfigured and it is\nnot set to widen the perm bit in the first place, no?\n\n>> > We already have that minor issue with the \"index\" file\n>> > #leftoverbits.\n>>\n>> The above \"Ideally\", which I suspect is totally bogus, would show up\n>> whey people look for that keyword in the list archive.  This is one\n>> of the reasons why I try to write it after at least one person\n>> sanity checks that an idea floated is worth remembering.\n>\n> It was in Ævar's commit message and I thought it might be better to\n> keep it so that people looking for that keyword could find the above\n> as well as the previous RFC patch.\n\nSo do you agree that open(0660) does not guarantee the result will\nbe group writable, the above \"Ideally\" is misguided nonsense, and\ngiving the #leftoverbits label to it will clutter the search result\nand harm readers?  That's good.\n\nThanks.\n"},{"id":"363561","messageId":"87ftvz1k5u.fsf@evledraar.gmail.com","threadId":"49843","inReplyTo":"xmqqftvzhn22.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-17T21:14:21Z","receivedAt":"2018-11-17T21:14:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 17 2018, Junio C Hamano wrote:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> \"However, as noted in those commits we'd still create the file as\n>> 0600, and would just re-chmod it only if core.sharedRepository is set\n>> to \"true\" or \"all\". If core.sharedRepository is unset or set to\n>> \"false\", then the file mode will not be changed, so without\n>> core.splitIndex a system with e.g. the umask set to group writeability\n>> would work for a group member, but not with core.splitIndex set, as\n>> group members would not be able to access the shared index file.\n>\n> That is irrelevant.  The repository needs to be configured properly\n> if it wanted to be used by the members of the group, period.\n>\n>> It is unfortunately not short lived when core.sharedrepository is\n>> unset for example as adjust_shared_perm() starts with:\n>>\n>> int adjust_shared_perm(const char *path)\n>> {\n>>         int old_mode, new_mode;\n>>\n>>         if (!get_shared_repository())\n>>                 return 0;\n>>\n>> but get_shared_repository() will return PERM_UMASK which is 0 when\n>> git_config_get_value(\"core.sharedrepository\", ...) returns a non zero\n>> value which happens when \"core.sharedrepository\" is unset.\n>\n> Which is to say, you get an unwanted result when your repository is\n> not configured properly.  It is not a news, and I have no sympathy.\n>\n> Just configure your repository properly and you'll be fine.\n>\n>>> > Ideally we'd split up the adjust_shared_perm() function to one that\n>>> > can give us the mode we want so we could just call open() instead of\n>>> > open() followed by chmod(), but that's an unrelated cleanup.\n>>>\n>>> I would drop this paragraph, as I think this is totally incorrect.\n>>> Imagine your umask is tighter than the target permission.  You ask\n>>> such a helper function and get \"you want 0660\".  Doing open(0660)\n>>> would not help you an iota---you'd need chmod() or fchmod() to\n>>> adjust the result anyway, which already is done by\n>>> adjust-shared-perm.\n>>\n>> It seems to me that it is not done when \"core.sharedrepository\" is unset.\n>\n> So?  You are assuming that the repository is misconfigured and it is\n> not set to widen the perm bit in the first place, no?\n>\n>>> > We already have that minor issue with the \"index\" file\n>>> > #leftoverbits.\n>>>\n>>> The above \"Ideally\", which I suspect is totally bogus, would show up\n>>> whey people look for that keyword in the list archive.  This is one\n>>> of the reasons why I try to write it after at least one person\n>>> sanity checks that an idea floated is worth remembering.\n>>\n>> It was in Ævar's commit message and I thought it might be better to\n>> keep it so that people looking for that keyword could find the above\n>> as well as the previous RFC patch.\n>\n> So do you agree that open(0660) does not guarantee the result will\n> be group writable, the above \"Ideally\" is misguided nonsense, and\n> giving the #leftoverbits label to it will clutter the search result\n> and harm readers?  That's good.\n\nAside from issues with the clarity of the commit message, which I'll fix\n& thanks for pointing them out. I think we may have stumbled on\nsomething more important here.\n\nDo you mean that you don't agree that following should always create\nboth \"foo\" and e.g. \".git/refs/heads/master\" with the same 644\n(-rw-rw-r--) mode:\n\n    (\n        rm -rf /tmp/repo &&\n        umask 022 &&\n        git init /tmp/repo &&\n        cd /tmp/repo &&\n        echo hi >foo &&\n        git add foo &&\n        git commit -m\"first\"\n    )\n\nTo me what we should do with the standard umask and what\ncore.sharedRepository are for are completely different things.\n\nWe should in git be creating files such that if I set my umask to\ne.g. 022 all users on the system can read what I'm creating.\n\nE.g. I tend to use this on something like a production server where\nothers (if I'm asleep) might want to look at my .bash_history as a last\nresort, and also some one-off repo I've created without setting\ncore.sharedRepository.\n\nI've yet to run into a case where this doesn't just work, aside from\ncore.splitIndex where before the patch here we're using a tempfile API\nfor something that isn't a tempfile.\n\nThis is distinct from the core.sharedRepository use-case, where you'd\nlike to on a per-repo basis override what you'd otherwise get with the\numask. E.g. if you have a shared server hosting a shared git repo, where\nusers with umask 077 will still be forced to create e.g. group rw files.\n"},{"id":"363567","messageId":"xmqqsgzyg8ux.fsf@gitster-ct.c.googlers.com","threadId":"49843","inReplyTo":"87ftvz1k5u.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-18T07:09:26Z","receivedAt":"2018-11-18T07:09:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Do you mean that you don't agree that following should always create\n> both \"foo\" and e.g. \".git/refs/heads/master\" with the same 644\n> (-rw-rw-r--) mode:\n>\n>     (\n>         rm -rf /tmp/repo &&\n>         umask 022 &&\n>         git init /tmp/repo &&\n>         cd /tmp/repo &&\n>         echo hi >foo &&\n>         git add foo &&\n>         git commit -m\"first\"\n>     )\n>\n> To me what we should do with the standard umask and what\n> core.sharedRepository are for are completely different things.\n\nAhh, of course.  If you put it that way, I do agree that it gives us\na valid use case where core.sharedRepository is false and the umask\nof repository owner is set to 022 (or anything that does not allow\nwrite to group or others, and allows read to group) to let group\nmembers only peek but not touch the contents of the repository.\n\nI think I was distracted by the mention of ore.sharedRepository in\nthe proposed log message.\n"},{"id":"363574","messageId":"87efbi1tk4.fsf@evledraar.gmail.com","threadId":"49843","inReplyTo":"xmqqsgzyg8ux.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] read-cache: write all indexes with the same permissions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-18T12:03:39Z","receivedAt":"2018-11-18T12:03:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 18 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> Do you mean that you don't agree that following should always create\n>> both \"foo\" and e.g. \".git/refs/heads/master\" with the same 644\n>> (-rw-rw-r--) mode:\n>>\n>>     (\n>>         rm -rf /tmp/repo &&\n>>         umask 022 &&\n>>         git init /tmp/repo &&\n>>         cd /tmp/repo &&\n>>         echo hi >foo &&\n>>         git add foo &&\n>>         git commit -m\"first\"\n>>     )\n>>\n>> To me what we should do with the standard umask and what\n>> core.sharedRepository are for are completely different things.\n>\n> Ahh, of course.  If you put it that way, I do agree that it gives us\n> a valid use case where core.sharedRepository is false and the umask\n> of repository owner is set to 022 (or anything that does not allow\n> write to group or others, and allows read to group) to let group\n> members only peek but not touch the contents of the repository.\n>\n> I think I was distracted by the mention of ore.sharedRepository in\n> the proposed log message.\n\nThanks. I'll submit a v3 with a less confusing commit message.\n"},{"id":"363593","messageId":"20181118190429.15581-1-avarab@gmail.com","threadId":"49843","inReplyTo":"87efbi1tk4.fsf@evledraar.gmail.com","subject":"[PATCH v3] read-cache: make the split index obey umask settings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-18T19:04:29Z","receivedAt":"2018-11-18T19:04:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the split index write out its .git/sharedindex_* files with the\nsame permissions as .git/index. This only changes the behavior when\ncore.sharedRepository isn't set, i.e. the user's umask settings will\nbe respected.\n\nThis hasn't been the case ever since the split index was originally\nimplemented in c18b80a0e8 (\"update-index: new options to\nenable/disable split index mode\", 2014-06-13). A mkstemp()-like\nfunction has always been used to create it. First mkstemp() itself,\nand then later our own mkstemp()-like in\nf6ecc62dbf (\"write_shared_index(): use tempfile module\", 2015-08-10)\n\nA related bug was fixed in df801f3f9f (\"read-cache: use shared perms\nwhen writing shared index\", 2017-06-25). Since then the split index\nhas respected core.sharedRepository.\n\nHowever, using that setting should not be required simply to make git\nobey the user's umask setting. It's intended for the use-case of\noverriding whatever that umask is set to. This fixes cases where the\nuser has e.g. set his umask to 022 on a shared server in anticipation\nof other user's needing to run \"status\", \"log\" etc. in his repository.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nHere it is with a rewritten commit message & adjusted comment as\ndiscussed in the v2 discussion.\n\n read-cache.c           |  3 ++-\n t/t1700-split-index.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 4ca81286c0..e7e77e780b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3150,7 +3150,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tstruct tempfile *temp;\n \t\tint saved_errno;\n \n-\t\ttemp = mks_tempfile(git_path(\"sharedindex_XXXXXX\"));\n+\t\t/* Same initial permissions as the main .git/index file */\n+\t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n \t\tif (!temp) {\n \t\t\toidclr(&si->base_oid);\n \t\t\tret = do_write_locked_index(istate, lock, flags);\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 2ac47aa0e4..fa1d3d468b 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"\n \ttest $(ls .git/sharedindex.* | wc -l) -le 2\n '\n \n+test_expect_success POSIXPERM 'same mode for index & split index' '\n+\tgit init same-mode &&\n+\t(\n+\t\tcd same-mode &&\n+\t\ttest_commit A &&\n+\t\ttest_modebits .git/index >index_mode &&\n+\t\ttest_must_fail git config core.sharedRepository &&\n+\t\tgit -c core.splitIndex=true status &&\n+\t\tshared=$(ls .git/sharedindex.*) &&\n+\t\tcase \"$shared\" in\n+\t\t*\" \"*)\n+\t\t\t# we have more than one???\n+\t\t\tfalse ;;\n+\t\t*)\n+\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n+\t\t\ttest_cmp index_mode split_index_mode ;;\n+\t\tesac\n+\t)\n+'\n+\n while read -r mode modebits\n do\n \ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n-- \n2.19.1.1182.g4ecb1133ce\n\n"}]}