{"thread":{"id":"14404","subject":"[PATCH] Fix backwards-incompatible handling of core.sharedRepository","startedAt":"2008-07-10T23:19:48Z","lastAt":"2008-07-12T03:44:10Z","messageCount":5,"participants":["Petr Baudis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"82891","messageId":"20080710231853.21448.18643.stgit@rover.dkm.cz","threadId":"14404","inReplyTo":null,"subject":"[PATCH] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-10T23:19:48Z","receivedAt":"2008-07-10T23:19:48Z","isPatch":true,"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.\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\nSigned-off-by: Petr Baudis \n---\n\n Sorry for the resend, StGIT kind of tricked me to adding two Cc headers and\n the first one just got dropped.\n\n path.c | 2 +-\n 1 files changed, 1 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;\n"},{"id":"82894","messageId":"7vr6a1mhqi.fsf@gitster.siamese.dyndns.org","threadId":"14404","inReplyTo":"20080710231853.21448.18643.stgit@rover.dkm.cz","subject":"Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T23:39:33Z","receivedAt":"2008-07-10T23:39:33Z","isPatch":true,"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.\n\nDonn't our existing tests catch this, and if the answer is no because we\ndon't have any, could you add some?\n\n> path.c | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n>\n> diff --git a/path.c b/path.c\n> index 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;\n\nI think this change is good. shared_repository has always been about\nwidening the access and not about limiting.\n"},{"id":"82921","messageId":"7vr6a1kmpq.fsf@gitster.siamese.dyndns.org","threadId":"14404","inReplyTo":"7vr6a1mhqi.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-11T05:34:57Z","receivedAt":"2008-07-11T05:34:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> 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.\n>\n> Donn't our existing tests catch this, and if the answer is no because we\n> don't have any, could you add some?\n> ...\n>> diff --git a/path.c b/path.c\n>> index 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;\n>\n> I think this change is good. shared_repository has always been about\n> widening the access and not about limiting.\n\nHaving said that, you really should protect this behaviour from regression\nwith a test case. I do not see practical difference for sane umask\nvalues.\n\nWhat umask are you using, and which file in the repository gets affected?\nIn the old code I see we do have checks for S_IXUSR and tweaks on S_IXGRP\nand S_IXOTH, but this should make a difference only if your umask blocks\nexecutable bit and the file in question is executable. Was it an\nexecutable hook copied from template?\n"},{"id":"82965","messageId":"20080711150707.GE32184@machine.or.cz","threadId":"14404","inReplyTo":"7vr6a1kmpq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-11T15:07:07Z","receivedAt":"2008-07-11T15:07:07Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Jul 10, 2008 at 10:34:57PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > 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.\n> >\n> > Donn't our existing tests catch this, and if the answer is no because we\n> > don't have any, could you add some?\n\n(Will do. Wanted to do it but forgot. :-)\n\n> >> diff --git a/path.c b/path.c\n> >> index 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;\n> >\n> > I think this change is good. shared_repository has always been about\n> > widening the access and not about limiting.\n> \n> Having said that, you really should protect this behaviour from regression\n> with a test case. I do not see practical difference for sane umask\n> values.\n> \n> What umask are you using, and which file in the repository gets affected?\n> In the old code I see we do have checks for S_IXUSR and tweaks on S_IXGRP\n> and S_IXOTH, but this should make a difference only if your umask blocks\n> executable bit and the file in question is executable. Was it an\n> executable hook copied from template?\n\nMy umask is 002, and the difference is visible at all the files in the\nrepository (in existing repositories, update will cause new object files\nhave the missing permission, making for the most confusing behaviour).\n\nWith sane Git versions, the permissions when shared=1 are rwxrwxr-x,\nwith recent Git however, it is rwxrwx--- (modulo the exec bits).\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nThe last good thing written in C++ was the Pachelbel Canon. -- J. Olson\n"},{"id":"83057","messageId":"7vprpjhilx.fsf@gitster.siamese.dyndns.org","threadId":"14404","inReplyTo":"20080711150707.GE32184@machine.or.cz","subject":"Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-12T03:44:10Z","receivedAt":"2008-07-12T03:44:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. Queued on 'maint'.\n"}]}