{"thread":{"id":"14416","subject":"[PATCHv2] Fix backwards-incompatible handling of core.sharedRepository","startedAt":"2008-07-11T23:39:46Z","lastAt":"2008-07-12T01:15:03Z","messageCount":4,"participants":["Petr Baudis","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"83041","messageId":"20080711233841.30916.75885.stgit@rover.dkm.cz","threadId":"14416","inReplyTo":null,"subject":"[PATCHv2] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-11T23:39:46Z","receivedAt":"2008-07-11T23:39:46Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository\nhandling extension broke backwards compatibility; before, shared=1 meant\nthat Git merely ensured the repository is group-writable, not that it's\n_only_ group-writable, which is the current behaviour. Thus, with umask 002,\nGit creates repository with /rw.rw.--./, this patch fixes it back to\n/rw.rw.r-./.\n\nMaybe it makes sense to provide the current semantics in some way too,\nbut that cannot be done at the expense of ditching backwards\ncompatibility; this bug has just wasted me two hours and broke\nrepo.or.cz pushing for several hours.\n\nCc: Heikki Orsila \nSigned-off-by: Petr Baudis \n---\n\n The patch description is clarified and a test is added to t1301.\n\n path.c | 2 +-\n t/t1301-shared-repo.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletions(-)\n\n\ndiff --git a/path.c b/path.c\nindex 5983255..75c5915 100644\n--- a/path.c\n+++ b/path.c\n@@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)\n \tmode = st.st_mode;\n \n \tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n+\t\tint tweak = (mode & 0777) | shared_repository;\n \t\tif (!(mode & S_IWUSR))\n \t\t\ttweak &= ~0222;\n \t\tmode = (mode & ~0777) | tweak;\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 6c78c8b..d26191b 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -17,6 +17,25 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \ttest $ret != \"0\"\n '\n \n+test_expect_success 'shared=1 does not override sane umask' '\n+\tmkdir sub &&\n+\tcd sub &&\n+\tumask 002 &&\n+\tgit init --shared=1 &&\n+\ttest 1 = $(git config core.sharedrepository) &&\n+\tactual=\"$(ls -l .git/HEAD)\" &&\n+\trm -rf sub &&\n+\tcase \"$actual\" in\n+\t-rw-rw-r--*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Oops, .git/HEAD is not 0664 but $actual\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n test_expect_success 'shared=all' '\n \tmkdir sub &&\n \tcd sub &&\n"},{"id":"83044","messageId":"20080711234257.16449.85447.stgit@rover.dkm.cz","threadId":"14416","inReplyTo":"20080711233841.30916.75885.stgit@rover.dkm.cz","subject":"[PATCHv3] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-11T23:45:00Z","receivedAt":"2008-07-11T23:45:00Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository\nhandling extension broke backwards compatibility; before, shared=1 meant\nthat Git merely ensured the repository is group-writable, not that it's\n_only_ group-writable, which is the current behaviour. Thus, with umask 002,\nGit creates repository with /rw.rw.--./, this patch fixes it back to\n/rw.rw.r-./.\n\nMaybe it makes sense to provide the current semantics in some way too,\nbut that cannot be done at the expense of ditching backwards\ncompatibility; this bug has just wasted me two hours and broke\nrepo.or.cz pushing for several hours.\n\nCc: Heikki Orsila \nSigned-off-by: Petr Baudis \n---\n\n Oops, this testcase wouldn't really remove its test subrepository\nafter itself properly, though the testsuite would still pass; of course\nthis bug slipped through all the previous visual inspections of mine.\n\n Sorry for the continuous noise. :-)\n\n path.c | 2 +-\n t/t1301-shared-repo.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletions(-)\n\n\ndiff --git a/path.c b/path.c\nindex 5983255..75c5915 100644\n--- a/path.c\n+++ b/path.c\n@@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)\n \tmode = st.st_mode;\n \n \tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n+\t\tint tweak = (mode & 0777) | shared_repository;\n \t\tif (!(mode & S_IWUSR))\n \t\t\ttweak &= ~0222;\n \t\tmode = (mode & ~0777) | tweak;\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 6c78c8b..3fe485c 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -17,6 +17,26 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \ttest $ret != \"0\"\n '\n \n+test_expect_success 'shared=1 does not override sane umask' '\n+\tmkdir sub &&\n+\tcd sub &&\n+\tumask 002 &&\n+\tgit init --shared=1 &&\n+\ttest 1 = $(git config core.sharedrepository) &&\n+\tactual=\"$(ls -l .git/HEAD)\" &&\n+\tcd .. &&\n+\trm -rf sub &&\n+\tcase \"$actual\" in\n+\t-rw-rw-r--*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Oops, .git/HEAD is not 0664 but $actual\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n test_expect_success 'shared=all' '\n \tmkdir sub &&\n \tcd sub &&\n"},{"id":"83048","messageId":"7vhcawhs1e.fsf@gitster.siamese.dyndns.org","threadId":"14416","inReplyTo":"20080711234257.16449.85447.stgit@rover.dkm.cz","subject":"Re: [PATCHv3] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-12T00:20:29Z","receivedAt":"2008-07-12T00:20:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis writes:\n\n> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository\n> handling extension broke backwards compatibility; before, shared=1 meant\n> that Git merely ensured the repository is group-writable, not that it's\n> _only_ group-writable, which is the current behaviour. Thus, with umask 002,\n> Git creates repository with /rw.rw.--./, this patch fixes it back to\n> /rw.rw.r-./.\n\nIs it just me who finds the above unreadable blob of black ink?\n\n\t06cbe85 (Make core.sharedRepository more generic, 2008-04-16)\n\tbroke the traditional setting core.sharedRepository to true,\n which was to make the repository group writable.\n\n\tThe call to adjust_shared_perm() should only loosen the\n\tpermission. If the user has umask 002 (or 022) that allow others\n\tto read, the resulting files should be made readable and writable\n\tby group, without restricting the readability by others.\n\n> Maybe it makes sense to provide the current semantics in some way too,\n> but that cannot be done at the expense of ditching backwards\n> compatibility; this bug has just wasted me two hours and broke\n> repo.or.cz pushing for several hours.\n\nI do not think this gripe after the semicolon belongs before the\nthree-dash lines.\n\n> Cc: Heikki Orsila \n> Signed-off-by: Petr Baudis \n> ---\n>\n> Oops, this testcase wouldn't really remove its test subrepository\n> after itself properly, though the testsuite would still pass; of course\n> this bug slipped through all the previous visual inspections of mine.\n>\n> Sorry for the continuous noise. :-)\n\nHmm, I am very puzzled.\n\nI am afraid I have to ask you for another round. I applied only your\nupdate to t/t1301 without the change to path.c, and the test passes, which\nmeans either (1) the test is incorrect and does not exposing the breakage,\nor (2) the breakage is not real and existing code is correct.\n"},{"id":"83051","messageId":"20080712011231.19745.48052.stgit@rover.dkm.cz","threadId":"14416","inReplyTo":"7vhcawhs1e.fsf@gitster.siamese.dyndns.org","subject":"[PATCHv4] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-12T01:15:03Z","receivedAt":"2008-07-12T01:15:03Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"06cbe85 (Make core.sharedRepository more generic, 2008-04-16) broke the\ntraditional setting of core.sharedRepository to true, which was to make\nthe repository group writable: with umask 022, it would clear the\npermission bits for 'other'. (umask 002 did not exhibit this behaviour\nsince pre-chmod() check in adjust_shared_perm() fails in that case.)\n\nThe call to adjust_shared_perm() should only loosen the permission.\nIf the user has umask like 022 or 002 that allow others to read, the\nresulting files should be made readable and writable by group, without\nrestricting the readability by others.\n\nThis patch fixes the adjust_shared_perm() mode tweak based on Junio's\nsuggestion and adds the appropriate tests to t/t1301-shared-repo.sh.\n\nCc: Heikki Orsila \nSigned-off-by: Petr Baudis \n---\n\n Fourth iteration. The problem is in fact triggered with umask 022,\nnot 002; fixed the description and extended the testsuite appropriately.\nImproved patch description as well as the adjust_shared_perm() fix based\non Junio's suggestions here and on #git, so by now it's more of his\npatch than mine. :-)\n\n path.c | 2 +-\n t/t1301-shared-repo.sh | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletions(-)\n\n\ndiff --git a/path.c b/path.c\nindex 5983255..504eae0 100644\n--- a/path.c\n+++ b/path.c\n@@ -272,7 +272,7 @@ int adjust_shared_perm(const char *path)\n \t\tint tweak = shared_repository;\n \t\tif (!(mode & S_IWUSR))\n \t\t\ttweak &= ~0222;\n-\t\tmode = (mode & ~0777) | tweak;\n+\t\tmode |= tweak;\n \t} else {\n \t\t/* Preserve old PERM_UMASK behaviour */\n \t\tif (mode & S_IWUSR)\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 6c78c8b..4c6d0b6 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -17,6 +17,28 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '\n \ttest $ret != \"0\"\n '\n \n+for u in 002 022; do\n+\ttest_expect_success \"shared=1 does not clear bits preset by umask $u\" '\n+\t\tmkdir sub &&\n+\t\tcd sub &&\n+\t\tumask $u &&\n+\t\tgit init --shared=1 &&\n+\t\ttest 1 = $(git config core.sharedrepository) &&\n+\t\tactual=\"$(ls -l .git/HEAD)\" &&\n+\t\tcd .. &&\n+\t\trm -rf sub &&\n+\t\tcase \"$actual\" in\n+\t\t-rw-rw-r--*)\n+\t\t\t: happy\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo Oops, .git/HEAD is not 0664 but $actual\n+\t\t\tfalse\n+\t\t\t;;\n+\t\tesac\n+\t'\n+done\n+\n test_expect_success 'shared=all' '\n \tmkdir sub &&\n \tcd sub &&\n"}]}