[PATCH] Fix backwards-incompatible handling of core.sharedRepository

[PATCH] Fix backwards-incompatible handling of core.sharedRepository

[Petr Baudis]

The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
handling extension broke backwards compatibility; before, shared=1 meant
that Git merely ensured the repository is group-writable, not that it's
_only_ group-writable, which is the current behaviour.

Maybe it makes sense to provide the current semantics in some way too,
but that cannot be done at the expense of ditching backwards
compatibility; this bug has just wasted me two hours and broke
repo.or.cz pushing for several hours.

Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
---

 Sorry for the resend, StGIT kind of tricked me to adding two Cc headers and
 the first one just got dropped.

 path.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/path.c b/path.c
index 5983255..75c5915 100644
--- a/path.c
+++ b/path.c
@@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
 	mode = st.st_mode;
 
 	if (shared_repository) {
-		int tweak = shared_repository;
+		int tweak = (mode & 0777) | shared_repository;
 		if (!(mode & S_IWUSR))
 			tweak &= ~0222;
 		mode = (mode & ~0777) | tweak;

Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
> handling extension broke backwards compatibility; before, shared=1 meant
> that Git merely ensured the repository is group-writable, not that it's
> _only_ group-writable, which is the current behaviour.

Donn't our existing tests catch this, and if the answer is no because we
don't have any, could you add some?

>  path.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
>
> diff --git a/path.c b/path.c
> index 5983255..75c5915 100644
> --- a/path.c
> +++ b/path.c
> @@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
>  	mode = st.st_mode;
>  
>  	if (shared_repository) {
> -		int tweak = shared_repository;
> +		int tweak = (mode & 0777) | shared_repository;
>  		if (!(mode & S_IWUSR))
>  			tweak &= ~0222;
>  		mode = (mode & ~0777) | tweak;

I think this change is good.  shared_repository has always been about
widening the access and not about limiting.

Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Petr Baudis <pasky@suse.cz> writes:
>
>> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
>> handling extension broke backwards compatibility; before, shared=1 meant
>> that Git merely ensured the repository is group-writable, not that it's
>> _only_ group-writable, which is the current behaviour.
>
> Donn't our existing tests catch this, and if the answer is no because we
> don't have any, could you add some?
> ...
>> diff --git a/path.c b/path.c
>> index 5983255..75c5915 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
>>  	mode = st.st_mode;
>>  
>>  	if (shared_repository) {
>> -		int tweak = shared_repository;
>> +		int tweak = (mode & 0777) | shared_repository;
>>  		if (!(mode & S_IWUSR))
>>  			tweak &= ~0222;
>>  		mode = (mode & ~0777) | tweak;
>
> I think this change is good.  shared_repository has always been about
> widening the access and not about limiting.

Having said that, you really should protect this behaviour from regression
with a test case.  I do not see practical difference for sane umask
values.

What umask are you using, and which file in the repository gets affected?
In the old code I see we do have checks for S_IXUSR and tweaks on S_IXGRP
and S_IXOTH, but this should make a difference only if your umask blocks
executable bit and the file in question is executable.  Was it an
executable hook copied from template?

Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository

[Petr Baudis]

On Thu, Jul 10, 2008 at 10:34:57PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Petr Baudis <pasky@suse.cz> writes:
> >
> >> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
> >> handling extension broke backwards compatibility; before, shared=1 meant
> >> that Git merely ensured the repository is group-writable, not that it's
> >> _only_ group-writable, which is the current behaviour.
> >
> > Donn't our existing tests catch this, and if the answer is no because we
> > don't have any, could you add some?

(Will do. Wanted to do it but forgot. :-)

> >> diff --git a/path.c b/path.c
> >> index 5983255..75c5915 100644
> >> --- a/path.c
> >> +++ b/path.c
> >> @@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
> >>  	mode = st.st_mode;
> >>  
> >>  	if (shared_repository) {
> >> -		int tweak = shared_repository;
> >> +		int tweak = (mode & 0777) | shared_repository;
> >>  		if (!(mode & S_IWUSR))
> >>  			tweak &= ~0222;
> >>  		mode = (mode & ~0777) | tweak;
> >
> > I think this change is good.  shared_repository has always been about
> > widening the access and not about limiting.
> 
> Having said that, you really should protect this behaviour from regression
> with a test case.  I do not see practical difference for sane umask
> values.
> 
> What umask are you using, and which file in the repository gets affected?
> In the old code I see we do have checks for S_IXUSR and tweaks on S_IXGRP
> and S_IXOTH, but this should make a difference only if your umask blocks
> executable bit and the file in question is executable.  Was it an
> executable hook copied from template?

My umask is 002, and the difference is visible at all the files in the
repository (in existing repositories, update will cause new object files
have the missing permission, making for the most confusing behaviour).

With sane Git versions, the permissions when shared=1 are rwxrwxr-x,
with recent Git however, it is rwxrwx--- (modulo the exec bits).

-- 
				Petr "Pasky" Baudis
The last good thing written in C++ was the Pachelbel Canon. -- J. Olson

Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository

[Junio C Hamano]

Thanks.  Queued on 'maint'.