diff mbox series

[12/12] Revert "fetch/clone: detect dubious ownership of local repositories"

Message ID 20240521195659.870714-13-gitster@pobox.com (mailing list archive)
State Superseded
Headers show
Series Fix various overly aggressive protections in 2.45.1 and friends | expand

Commit Message

Junio C Hamano May 21, 2024, 7:56 p.m. UTC
This partially reverts f4aa8c8b (fetch/clone: detect dubious
ownership of local repositories, 2024-04-10) that broke typical
read-only use cases (e.g. by git-daemon serving fetches and clones)
where "nobody" who has no write permission serves repositories owned
by others.  The function to die upon seeing dubious ownership is
still kept, as there are other users of it, but calls to it from the
generic repository discovery code path, which triggered in cases far
wider than originally intended (i.e. to stop local clones), have
been removed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 path.c                        | 2 --
 t/t0411-clone-from-partial.sh | 6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Junio C Hamano May 21, 2024, 8:43 p.m. UTC | #1
Junio C Hamano <gitster@pobox.com> writes:

> This partially reverts f4aa8c8b (fetch/clone: detect dubious
> ownership of local repositories, 2024-04-10) that broke typical
> read-only use cases (e.g. by git-daemon serving fetches and clones)
> where "nobody" who has no write permission serves repositories owned
> by others.  The function to die upon seeing dubious ownership is
> still kept, as there are other users of it, but calls to it from the
> generic repository discovery code path, which triggered in cases far
> wider than originally intended (i.e. to stop local clones), have
> been removed.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I am inclined to propose dropping this step, actually.

cf. https://lore.kernel.org/git/xmqq7cfmaofl.fsf@gitster.g/
Johannes Schindelin May 22, 2024, 7:27 a.m. UTC | #2
Hi Junio,

On Tue, 21 May 2024, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > This partially reverts f4aa8c8b (fetch/clone: detect dubious
> > ownership of local repositories, 2024-04-10) that broke typical
> > read-only use cases (e.g. by git-daemon serving fetches and clones)
> > where "nobody" who has no write permission serves repositories owned
> > by others.  The function to die upon seeing dubious ownership is
> > still kept, as there are other users of it, but calls to it from the
> > generic repository discovery code path, which triggered in cases far
> > wider than originally intended (i.e. to stop local clones), have
> > been removed.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
>
> I am inclined to propose dropping this step, actually.
>
> cf. https://lore.kernel.org/git/xmqq7cfmaofl.fsf@gitster.g/

To https://github.com/dscho/git
 + c6da96aa5f0...f71d7009814 maint-2.39 -> tentative/maint-2.39 (forced update)
 + fff57b200d1...21dc6c4d521 maint-2.40 -> tentative/maint-2.40 (forced update)
 + 616450032a0...0d21b3451cd maint-2.41 -> tentative/maint-2.41 (forced update)
 + b1ea89bc2d6...e9bd0c8f8c4 maint-2.42 -> tentative/maint-2.42 (forced update)
 + 093c42a6c6b...9926037ce8c maint-2.43 -> tentative/maint-2.43 (forced update)
 + 3c7a7b923b3...aec5a9bf52c maint-2.44 -> tentative/maint-2.44 (forced update)
 + aeddcb02756...d3c56966d13 maint-2.45 -> tentative/maint-2.45 (forced update)

This command-line comes up with no differences (meaning: you resolved the
merge conflicts, even the ones without conflict markers, in the same way
as I did, which is good):

  for i in $(seq 39 45)
  do
    case $i in
    39) gitster=jc/fix-2.45.1-and-friends-for-2.39;;
    45) gitster=jc/fix-2.45.1-and-friends-for-maint;;
    *)  gitster=fixes/2.45.1/2.$i;;
    esac &&
    git diff gitster/$gitster..dscho/tentative/maint-2.$i -- \
      ':(exclude)Documentation/RelNotes/' \
      ':(exclude)GIT-VERSION-GEN' \
      ':(exclude)RelNotes'
  done

Ciao,
Johannes
Junio C Hamano May 22, 2024, 5:20 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> To https://github.com/dscho/git
>  + c6da96aa5f0...f71d7009814 maint-2.39 -> tentative/maint-2.39 (forced update)
>  + fff57b200d1...21dc6c4d521 maint-2.40 -> tentative/maint-2.40 (forced update)
>  + 616450032a0...0d21b3451cd maint-2.41 -> tentative/maint-2.41 (forced update)
>  + b1ea89bc2d6...e9bd0c8f8c4 maint-2.42 -> tentative/maint-2.42 (forced update)
>  + 093c42a6c6b...9926037ce8c maint-2.43 -> tentative/maint-2.43 (forced update)
>  + 3c7a7b923b3...aec5a9bf52c maint-2.44 -> tentative/maint-2.44 (forced update)
>  + aeddcb02756...d3c56966d13 maint-2.45 -> tentative/maint-2.45 (forced update)
>
> This command-line comes up with no differences (meaning: you resolved the
> merge conflicts, even the ones without conflict markers, in the same way
> as I did, which is good):

Yeah, but I am afraid that it is a bit too premature to worry about
the integration to merge them up.  What's your take on the symlink
stuff Joey raised recently?

Thanks.
diff mbox series

Patch

diff --git a/path.c b/path.c
index d61f70e87d..492e17ad12 100644
--- a/path.c
+++ b/path.c
@@ -840,7 +840,6 @@  const char *enter_repo(const char *path, int strict)
 		if (!suffix[i])
 			return NULL;
 		gitfile = read_gitfile(used_path.buf);
-		die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
@@ -851,7 +850,6 @@  const char *enter_repo(const char *path, int strict)
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
-		die_upon_dubious_ownership(gitfile, NULL, path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh
index b3d6ddc4bc..a481ed97b7 100755
--- a/t/t0411-clone-from-partial.sh
+++ b/t/t0411-clone-from-partial.sh
@@ -23,7 +23,7 @@  test_expect_success 'create evil repo' '
 	>evil/.git/shallow
 '
 
-test_expect_success 'local clone must not fetch from promisor remote and execute script' '
+test_expect_failure 'local clone must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
@@ -33,7 +33,7 @@  test_expect_success 'local clone must not fetch from promisor remote and execute
 	test_path_is_missing script-executed
 '
 
-test_expect_success 'clone from file://... must not fetch from promisor remote and execute script' '
+test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
@@ -43,7 +43,7 @@  test_expect_success 'clone from file://... must not fetch from promisor remote a
 	test_path_is_missing script-executed
 '
 
-test_expect_success 'fetch from file://... must not fetch from promisor remote and execute script' '
+test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' '
 	rm -f script-executed &&
 	test_must_fail git fetch \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \