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

> 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?