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

[PATCHv2] 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. Thus, with umask 002,
Git creates repository with /rw.rw.--./, this patch fixes it back to
/rw.rw.r-./.

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.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
---

  The patch description is clarified and a test is added to t1301.

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   19 +++++++++++++++++++
 2 files changed, 20 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;
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..d26191b 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,25 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+test_expect_success 'shared=1 does not override sane umask' '
+	mkdir sub &&
+	cd sub &&
+	umask 002 &&
+	git init --shared=1 &&
+	test 1 = $(git config core.sharedrepository) &&
+	actual="$(ls -l .git/HEAD)" &&
+	rm -rf sub &&
+	case "$actual" in
+	-rw-rw-r--*)
+		: happy
+		;;
+	*)
+		echo Oops, .git/HEAD is not 0664 but $actual
+		false
+		;;
+	esac
+'
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&

[PATCHv3] 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. Thus, with umask 002,
Git creates repository with /rw.rw.--./, this patch fixes it back to
/rw.rw.r-./.

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.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
---

  Oops, this testcase wouldn't really remove its test subrepository
after itself properly, though the testsuite would still pass; of course
this bug slipped through all the previous visual inspections of mine.

  Sorry for the continuous noise. :-)

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   20 ++++++++++++++++++++
 2 files changed, 21 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;
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..3fe485c 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,26 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+test_expect_success 'shared=1 does not override sane umask' '
+	mkdir sub &&
+	cd sub &&
+	umask 002 &&
+	git init --shared=1 &&
+	test 1 = $(git config core.sharedrepository) &&
+	actual="$(ls -l .git/HEAD)" &&
+	cd .. &&
+	rm -rf sub &&
+	case "$actual" in
+	-rw-rw-r--*)
+		: happy
+		;;
+	*)
+		echo Oops, .git/HEAD is not 0664 but $actual
+		false
+		;;
+	esac
+'
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&

Re: [PATCHv3] 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. Thus, with umask 002,
> Git creates repository with /rw.rw.--./, this patch fixes it back to
> /rw.rw.r-./.

Is it just me who finds the above unreadable blob of black ink?

	06cbe85 (Make core.sharedRepository more generic, 2008-04-16)
	broke the traditional setting core.sharedRepository to true,
        which was to make the repository group writable.

	The call to adjust_shared_perm() should only loosen the
	permission.  If the user has umask 002 (or 022) that allow others
	to read, the resulting files should be made readable and writable
	by group, without restricting the readability by others.

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

I do not think this gripe after the semicolon belongs before the
three-dash lines.

> Cc: Heikki Orsila <heikki.orsila@iki.fi>
> Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
> ---
>
>   Oops, this testcase wouldn't really remove its test subrepository
> after itself properly, though the testsuite would still pass; of course
> this bug slipped through all the previous visual inspections of mine.
>
>   Sorry for the continuous noise. :-)

Hmm, I am very puzzled.

I am afraid I have to ask you for another round.  I applied only your
update to t/t1301 without the change to path.c, and the test passes, which
means either (1) the test is incorrect and does not exposing the breakage,
or (2) the breakage is not real and existing code is correct.

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

[Petr Baudis]

06cbe85 (Make core.sharedRepository more generic, 2008-04-16) broke the
traditional setting of core.sharedRepository to true, which was to make
the repository group writable: with umask 022, it would clear the
permission bits for 'other'. (umask 002 did not exhibit this behaviour
since pre-chmod() check in adjust_shared_perm() fails in that case.)

The call to adjust_shared_perm() should only loosen the permission.
If the user has umask like 022 or 002 that allow others to read, the
resulting files should be made readable and writable by group, without
restricting the readability by others.

This patch fixes the adjust_shared_perm() mode tweak based on Junio's
suggestion and adds the appropriate tests to t/t1301-shared-repo.sh.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@suse.cz>
---

  Fourth iteration. The problem is in fact triggered with umask 022,
not 002; fixed the description and extended the testsuite appropriately.
Improved patch description as well as the adjust_shared_perm() fix based
on Junio's suggestions here and on #git, so by now it's more of his
patch than mine. :-)

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)


diff --git a/path.c b/path.c
index 5983255..504eae0 100644
--- a/path.c
+++ b/path.c
@@ -272,7 +272,7 @@ int adjust_shared_perm(const char *path)
 		int tweak = shared_repository;
 		if (!(mode & S_IWUSR))
 			tweak &= ~0222;
-		mode = (mode & ~0777) | tweak;
+		mode |= tweak;
 	} else {
 		/* Preserve old PERM_UMASK behaviour */
 		if (mode & S_IWUSR)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..4c6d0b6 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,28 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+for u in 002 022; do
+	test_expect_success "shared=1 does not clear bits preset by umask $u" '
+		mkdir sub &&
+		cd sub &&
+		umask $u &&
+		git init --shared=1 &&
+		test 1 = $(git config core.sharedrepository) &&
+		actual="$(ls -l .git/HEAD)" &&
+		cd .. &&
+		rm -rf sub &&
+		case "$actual" in
+		-rw-rw-r--*)
+			: happy
+			;;
+		*)
+			echo Oops, .git/HEAD is not 0664 but $actual
+			false
+			;;
+		esac
+	'
+done
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&