threads / patch / 14404

patchFix backwards-incompatible handling of core.sharedRepository

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

## tl;dr

5 messages between Jul 10, 2008 and Jul 12, 2008. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Petr Baudis· Jul 10, 2008, 23:19 UTC · lore

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(-)
Show changes to path.c +1 −1
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;
Junio C Hamano· Jul 10, 2008, 23:39 UTC · re: Petr Baudis · lore

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

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?

Show 17 quoted lines
>  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.

Junio C Hamano· Jul 11, 2008, 05:34 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> 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?

Petr Baudis· Jul 11, 2008, 15:07 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 10, 2008 at 10:34:57PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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. :-)
Show 26 quoted lines
> >> 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

← back to recent threads