From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Baudis Subject: Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository Date: Fri, 11 Jul 2008 17:07:07 +0200 Message-ID: <20080711150707.GE32184@machine.or.cz> References: <20080710231853.21448.18643.stgit@rover.dkm.cz> <7vr6a1mhqi.fsf@gitster.siamese.dyndns.org> <7vr6a1kmpq.fsf@gitster.siamese.dyndns.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: git@vger.kernel.org To: Junio C Hamano X-From: git-owner@vger.kernel.org Fri Jul 11 17:08:18 2008 Return-path: Envelope-to: gcvg-git-2@gmane.org Received: from vger.kernel.org ([209.132.176.167]) by lo.gmane.org with esmtp (Exim 4.50) id 1KHKEI-000345-3C for gcvg-git-2@gmane.org; Fri, 11 Jul 2008 17:08:14 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757269AbYGKPHO (ORCPT ); Fri, 11 Jul 2008 11:07:14 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756290AbYGKPHO (ORCPT ); Fri, 11 Jul 2008 11:07:14 -0400 Received: from w241.dkm.cz ([62.24.88.241]:53682 "EHLO machine.or.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755330AbYGKPHM (ORCPT ); Fri, 11 Jul 2008 11:07:12 -0400 Received: by machine.or.cz (Postfix, from userid 2001) id 07D432C4C037; Fri, 11 Jul 2008 17:07:08 +0200 (CEST) Content-Disposition: inline In-Reply-To: <7vr6a1kmpq.fsf@gitster.siamese.dyndns.org> User-Agent: Mutt/1.5.16 (2007-06-09) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Thu, Jul 10, 2008 at 10:34:57PM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > Petr Baudis 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