[PATCH v2 00/12] Fix various overly aggressive protections in 2.45.1 and friends

[PATCH v2 00/12] Fix various overly aggressive protections in 2.45.1 and friends

[Junio C Hamano]

As people have seen, the latest "security fix" release turned out to
be a mixed bag of good vulnerability fixes with a bit over-eager
"layered defence" that broke real uses cases like git-lfs.  Let's
quickly get them in working order back first, with the vision that
we will then rebuild layered defence more carefully in the open on
top as necessary.

What we have here are the first "revert" part.

These patches are designed to apply to 2.39.4; the series may have
to grow as we discover more things to revert, but for now here are
the patches to

 - revert the over-eager "refusal to work" went into 2.39.4

 - adjust 2.39.4 codebase to cleanly build and test (at CI and
   locally) by backported fixes

It would have been better if we did not have to have the latter
class, but such is life.

Relative to the previous iteration, there are two differences, which
are:

 * Old [12/12] that reverted the repository ownership check for
   local case is gone.  A well known escape hatch is available that
   is easy to use when the repositories are trusted (most notably,
   in a hosting set-up, the repositories are trusted not to attack
   the 'nobody' user that is running 'git').

 * New [12/12] reverts a dubious checks for targets of symbolic
   links done in "git fsck" (and transfer).

Today's integration cycle is pretty much committed to have these in
'next' for the weekend, merge them down to 'master' by the end of
month, hoping that we can do 2.45.2 and friends sometime early next
month.


Jeff King (5):
  send-email: drop FakeTerm hack
  send-email: avoid creating more than one Term::ReadLine object
  ci: drop mention of BREW_INSTALL_PACKAGES variable
  ci: avoid bare "gcc" for osx-gcc job
  ci: stop installing "gcc-13" for osx-gcc

Johannes Schindelin (6):
  hook: plug a new memory leak
  init: use the correct path of the templates directory again
  Revert "core.hooksPath: add some protection while cloning"
  tests: verify that `clone -c core.hooksPath=/dev/null` works again
  clone: drop the protections where hooks aren't run
  Revert "Add a helper function to compare file contents"

Junio C Hamano (1):
  Revert "fsck: warn about symlink pointing inside a gitdir"

 .github/workflows/main.yml    |  3 +-
 Documentation/fsck-msgids.txt | 12 --------
 Makefile                      |  2 +-
 builtin/clone.c               | 12 +-------
 cache.h                       | 14 ---------
 ci/install-dependencies.sh    |  2 --
 config.c                      | 13 +-------
 copy.c                        | 58 -----------------------------------
 fsck.c                        | 56 ---------------------------------
 fsck.h                        | 12 --------
 git-send-email.perl           | 32 +++++++------------
 hook.c                        | 32 -------------------
 t/helper/test-path-utils.c    | 10 ------
 t/t0060-path-utils.sh         | 41 -------------------------
 t/t1350-config-hooks-path.sh  |  7 +++++
 t/t1450-fsck.sh               | 37 ----------------------
 t/t1800-hook.sh               | 15 ---------
 t/t5601-clone.sh              | 51 ------------------------------
 t/t9001-send-email.sh         |  5 +--
 19 files changed, 25 insertions(+), 389 deletions(-)

-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 01/12] send-email: drop FakeTerm hack

[Junio C Hamano]

From: Jeff King <peff@peff.net>

Back in 280242d1cc (send-email: do not barf when Term::ReadLine does not
like your terminal, 2006-07-02), we added a fallback for when
Term::ReadLine's constructor failed: we'd have a FakeTerm object
instead, which would then die if anybody actually tried to call
readline() on it. Since we instantiated the $term variable at program
startup, we needed this workaround to let the program run in modes when
we did not prompt the user.

But later, in f4dc9432fd (send-email: lazily load modules for a big
speedup, 2021-05-28), we started loading Term::ReadLine lazily only when
ask() is called. So at that point we know we're trying to prompt the
user, and we can just die if ReadLine instantiation fails, rather than
making this fake object to lazily delay showing the error.

This should be OK even if there is no tty (e.g., we're in a cron job),
because Term::ReadLine will return a stub object in that case whose "IN"
and "OUT" functions return undef. And since 5906f54e47 (send-email:
don't attempt to prompt if tty is closed, 2009-03-31), we check for that
case and skip prompting.

And we can be sure that FakeTerm was not kicking in for such a
situation, because it has actually been broken since that commit! It
does not define "IN" or "OUT" methods, so perl would barf with an error.
If FakeTerm was in use, we were neither honoring what 5906f54e47 tried
to do, nor producing the readable message that 280242d1cc intended.

So we're better off just dropping FakeTerm entirely, and letting the
error reported by constructing Term::ReadLine through.

[jc: cherry-picked from v2.42.0-rc2~6^2~1]

Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5861e99a6e..72d876f0a0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,18 +26,6 @@
 
 Getopt::Long::Configure qw/ pass_through /;
 
-package FakeTerm;
-sub new {
-	my ($class, $reason) = @_;
-	return bless \$reason, shift;
-}
-sub readline {
-	my $self = shift;
-	die "Cannot use readline on FakeTerm: $$self";
-}
-package main;
-
-
 sub usage {
 	print <<EOT;
 git send-email' [<options>] <file|directory>
@@ -930,16 +918,10 @@ sub get_patch_subject {
 }
 
 sub term {
-	my $term = eval {
-		require Term::ReadLine;
-		$ENV{"GIT_SEND_EMAIL_NOTTY"}
+	require Term::ReadLine;
+	return $ENV{"GIT_SEND_EMAIL_NOTTY"}
 			? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT)
 			: Term::ReadLine->new('git-send-email');
-	};
-	if ($@) {
-		$term = FakeTerm->new("$@: going non-interactive");
-	}
-	return $term;
 }
 
 sub ask {
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 02/12] send-email: avoid creating more than one Term::ReadLine object

[Junio C Hamano]

From: Jeff King <peff@peff.net>

Every time git-send-email calls its ask() function to prompt the user,
we call term(), which instantiates a new Term::ReadLine object. But in
v1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine
interface on some platforms), its constructor refuses to create a second
instance[1]. So on systems with that version of the module, most
git-send-email instances will fail (as we usually prompt for both "to"
and "in-reply-to" unless the user provided them on the command line).

We can fix this by keeping a single instance variable and returning it
for each call to term(). In perl 5.10 and up, we could do that with a
"state" variable. But since we only require 5.008, we'll do it the
old-fashioned way, with a lexical "my" in its own scope.

Note that the tests in t9001 detect this problem as-is, since the
failure mode is for the program to die. But let's also beef up the
"Prompting works" test to check that it correctly handles multiple
inputs (if we had chosen to keep our FakeTerm hack in the previous
commit, then the failure mode would be incorrectly ignoring prompts
after the first).

[1] For discussion of why multiple instances are forbidden, see:
    https://github.com/hirooih/perl-trg/issues/16

[jc: cherry-picked from v2.42.0-rc2~6^2]

Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl   | 18 +++++++++++++-----
 t/t9001-send-email.sh |  5 +++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 72d876f0a0..ad51508790 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -917,11 +917,19 @@ sub get_patch_subject {
 	do_edit(@files);
 }
 
-sub term {
-	require Term::ReadLine;
-	return $ENV{"GIT_SEND_EMAIL_NOTTY"}
-			? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT)
-			: Term::ReadLine->new('git-send-email');
+{
+	# Only instantiate one $term per program run, since some
+	# Term::ReadLine providers refuse to create a second instance.
+	my $term;
+	sub term {
+		require Term::ReadLine;
+		if (!defined $term) {
+			$term = $ENV{"GIT_SEND_EMAIL_NOTTY"}
+				? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT)
+				: Term::ReadLine->new('git-send-email');
+		}
+		return $term;
+	}
 }
 
 sub ask {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1130ef21b3..0f08a9542b 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -337,13 +337,14 @@ test_expect_success $PREREQ 'Show all headers' '
 test_expect_success $PREREQ 'Prompting works' '
 	clean_fake_sendmail &&
 	(echo "to@example.com" &&
-	 echo ""
+	 echo "my-message-id@example.com"
 	) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$patches \
 		2>errors &&
 		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
-		grep "^To: to@example.com\$" msgtxt1
+		grep "^To: to@example.com\$" msgtxt1 &&
+		grep "^In-Reply-To: <my-message-id@example.com>" msgtxt1
 '
 
 test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 03/12] ci: drop mention of BREW_INSTALL_PACKAGES variable

[Junio C Hamano]

From: Jeff King <peff@peff.net>

The last user of this variable went away in 4a6e4b9602 (CI: remove
Travis CI support, 2021-11-23), so it's doing nothing except making it
more confusing to find out which packages _are_ installed.

[jc: cherry-picked from v2.45.0-1-g9d4453e8d6]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/install-dependencies.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 4f407530d3..33039d516b 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -34,8 +34,6 @@ macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time
-	test -z "$BREW_INSTALL_PACKAGES" ||
-	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 	mkdir -p $HOME/bin
 	(
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 04/12] ci: avoid bare "gcc" for osx-gcc job

[Junio C Hamano]

From: Jeff King <peff@peff.net>

On macOS, a bare "gcc" (without a version) will invoke a wrapper for
clang, not actual gcc. Even when gcc is installed via homebrew, that
only provides version-specific links in /usr/local/bin (like "gcc-13"),
and never a version-agnostic "gcc" wrapper.

As far as I can tell, this has been the case for a long time, and this
osx-gcc job has largely been doing nothing. We can point it at "gcc-13",
which will pick up the homebrew-installed version.

The fix here is specific to the github workflow file, as the gitlab one
does not have a matching job.

It's a little unfortunate that we cannot just ask for the latest version
of gcc which homebrew provides, but as far as I can tell there is no
easy alias (you'd have to find the highest number gcc-* in
/usr/local/bin yourself).

[jc: cherry-picked from v2.45.0-2-g11c7001e3d]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .github/workflows/main.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 2dc0221f7f..583e7cd5f0 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -264,7 +264,7 @@ jobs:
             cc: clang
             pool: macos-13
           - jobname: osx-gcc
-            cc: gcc
+            cc: gcc-13
             cc_package: gcc-13
             pool: macos-13
           - jobname: linux-gcc-default
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 05/12] ci: stop installing "gcc-13" for osx-gcc

[Junio C Hamano]

From: Jeff King <peff@peff.net>

Our osx-gcc job explicitly asks to install gcc-13. But since the GitHub
runner image already comes with gcc-13 installed, this is mostly doing
nothing (or in some cases it may install an incremental update over the
runner image). But worse, it recently started causing errors like:

    ==> Fetching gcc@13
    ==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a
    ==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz
    Error: The `brew link` step did not complete successfully
    The formula built, but is not symlinked into /usr/local
    Could not symlink bin/c++-13
    Target /usr/local/bin/c++-13
    is a symlink belonging to gcc. You can unlink it:
      brew unlink gcc

which cause the whole CI job to bail.

I didn't track down the root cause, but I suspect it may be related to
homebrew recently switching the "gcc" default to gcc-14. And it may even
be fixed when a new runner image is released. But if we don't need to
run brew at all, it's one less thing for us to worry about.

[jc: cherry-picked from v2.45.0-3-g7df2405b38]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .github/workflows/main.yml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 583e7cd5f0..76e3f1e768 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,7 +265,6 @@ jobs:
             pool: macos-13
           - jobname: osx-gcc
             cc: gcc-13
-            cc_package: gcc-13
             pool: macos-13
           - jobname: linux-gcc-default
             cc: gcc
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 06/12] hook: plug a new memory leak

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 8db1e8743c0 (clone: prevent hooks from running during a clone,
2024-03-28), I introduced an inadvertent memory leak that was
unfortunately not caught before v2.45.1 was released. Here is a fix.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 hook.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hook.c b/hook.c
index 632b537b99..fc974cee1d 100644
--- a/hook.c
+++ b/hook.c
@@ -18,8 +18,10 @@ static int identical_to_template_hook(const char *name, const char *path)
 		found_template_hook = access(template_path.buf, X_OK) >= 0;
 	}
 #endif
-	if (!found_template_hook)
+	if (!found_template_hook) {
+		strbuf_release(&template_path);
 		return 0;
+	}
 
 	ret = do_files_match(template_path.buf, path);
 
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 07/12] init: use the correct path of the templates directory again

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In df93e407f06 (init: refactor the template directory discovery into its
own function, 2024-03-29), I refactored the way the templates directory
is discovered.

The refactoring was faithful, but missed a reference in the `Makefile`
where the `DEFAULT_GIT_TEMPLATE_DIR` constant is defined. As a
consequence, Git v2.45.1 and friends will always use the hard-coded path
`/usr/share/git-core/templates`.

Let's fix that by defining the `DEFAULT_GIT_TEMPLATE_DIR` when building
`setup.o`, where that constant is actually used.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 093829ae28..4b1502ba2c 100644
--- a/Makefile
+++ b/Makefile
@@ -2751,7 +2751,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \
 	'-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"'
 
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
-builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
+setup.sp setup.s setup.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
 config.sp config.s config.o: GIT-PREFIX
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 08/12] Revert "core.hooksPath: add some protection while cloning"

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This defense-in-depth was intended to protect the clone operation
against future escalations where bugs in `git clone` would allow
attackers to write arbitrary files in the `.git/` directory would allow
for Remote Code Execution attacks via maliciously-placed hooks.

However, it turns out that the `core.hooksPath` protection has
unintentional side effects so severe that they do not justify the
benefit of the protections. For example, it has been reported in
https://lore.kernel.org/git/FAFA34CB-9732-4A0A-87FB-BDB272E6AEE8@alchemists.io/
that the following invocation, which is intended to make `git clone`
safer, is itself broken by that protective measure:

	git clone --config core.hooksPath=/dev/null <url>

Since it turns out that the benefit does not justify the cost, let's revert
20f3588efc6 (core.hooksPath: add some protection while cloning,
2024-03-30).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c        | 13 +------------
 t/t1800-hook.sh | 15 ---------------
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/config.c b/config.c
index 85b37f2ee0..8c1c4071f0 100644
--- a/config.c
+++ b/config.c
@@ -1525,19 +1525,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "core.attributesfile"))
 		return git_config_pathname(&git_attributes_file, var, value);
 
-	if (!strcmp(var, "core.hookspath")) {
-		if (current_config_scope() == CONFIG_SCOPE_LOCAL &&
-		    git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0))
-			die(_("active `core.hooksPath` found in the local "
-			      "repository config:\n\t%s\nFor security "
-			      "reasons, this is disallowed by default.\nIf "
-			      "this is intentional and the hook should "
-			      "actually be run, please\nrun the command "
-			      "again with "
-			      "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
-			    value);
+	if (!strcmp(var, "core.hookspath"))
 		return git_config_pathname(&git_hooks_path, var, value);
-	}
 
 	if (!strcmp(var, "core.bare")) {
 		is_bare_repository_cfg = git_config_bool(var, value);
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 7ee12e6f48..2ef3579fa7 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -177,19 +177,4 @@ test_expect_success 'git hook run a hook with a bad shebang' '
 	test_cmp expect actual
 '
 
-test_expect_success 'clone protections' '
-	test_config core.hooksPath "$(pwd)/my-hooks" &&
-	mkdir -p my-hooks &&
-	write_script my-hooks/test-hook <<-\EOF &&
-	echo Hook ran $1
-	EOF
-
-	git hook run test-hook 2>err &&
-	grep "Hook ran" err &&
-	test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \
-		git hook run test-hook 2>err &&
-	grep "active .core.hooksPath" err &&
-	! grep "Hook ran" err
-'
-
 test_done
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 09/12] tests: verify that `clone -c core.hooksPath=/dev/null` works again

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As part of the protections added in Git v2.45.1 and friends,
repository-local `core.hooksPath` settings are no longer allowed, as a
defense-in-depth mechanism to prevent future Git vulnerabilities to
raise to critical level if those vulnerabilities inadvertently allow the
repository-local config to be written.

What the added protection did not anticipate is that such a
repository-local `core.hooksPath` can not only be used to point to
maliciously-placed scripts in the current worktree, but also to
_prevent_ hooks from being called altogether.

We just reverted the `core.hooksPath` protections, based on the Git
maintainer's recommendation in
https://lore.kernel.org/git/xmqq4jaxvm8z.fsf@gitster.g/ to address this
concern as well as related ones. Let's make sure that we won't regress
while trying to protect the clone operation further.

Reported-by: Brooke Kuhlmann <brooke@alchemists.io>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1350-config-hooks-path.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
index f6dc83e2aa..45a0492917 100755
--- a/t/t1350-config-hooks-path.sh
+++ b/t/t1350-config-hooks-path.sh
@@ -41,4 +41,11 @@ test_expect_success 'git rev-parse --git-path hooks' '
 	test .git/custom-hooks/abc = "$(cat actual)"
 '
 
+test_expect_success 'core.hooksPath=/dev/null' '
+	git clone -c core.hooksPath=/dev/null . no-templates &&
+	value="$(git -C no-templates config --local core.hooksPath)" &&
+	# The Bash used by Git for Windows rewrites `/dev/null` to `nul`
+	{ test /dev/null = "$value" || test nul = "$value"; }
+'
+
 test_done
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 10/12] clone: drop the protections where hooks aren't run

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As part of the security bug-fix releases v2.39.4, ..., v2.45.1, I
introduced logic to safeguard `git clone` from running hooks that were
installed _during_ the clone operation.

The rationale was that Git's CVE-2024-32002, CVE-2021-21300,
CVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349 should
have been low-severity vulnerabilities but were elevated to
critical/high severity by the attack vector that allows a weakness where
files inside `.git/` can be inadvertently written during a `git clone`
to escalate to a Remote Code Execution attack by virtue of installing a
malicious `post-checkout` hook that Git will then run at the end of the
operation without giving the user a chance to see what code is executed.

Unfortunately, Git LFS uses a similar strategy to install its own
`post-checkout` hook during a `git clone`; In fact, Git LFS is
installing four separate hooks while running the `smudge` filter.

While this pattern is probably in want of being improved by introducing
better support in Git for Git LFS and other tools wishing to register
hooks to be run at various stages of Git's commands, let's undo the
clone protections to unbreak Git LFS-enabled clones.

This reverts commit 8db1e8743c0 (clone: prevent hooks from running
during a clone, 2024-03-28).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clone.c  | 12 +-----------
 hook.c           | 34 --------------------------------
 t/t5601-clone.sh | 51 ------------------------------------------------
 3 files changed, 1 insertion(+), 96 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index e7721f5c22..9ec500d427 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -937,8 +937,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 	int filter_submodules = 0;
-	const char *template_dir;
-	char *template_dir_dup = NULL;
 
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
@@ -958,13 +956,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("You must specify a repository to clone."),
 			builtin_clone_usage, builtin_clone_options);
 
-	xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */);
-	template_dir = get_template_dir(option_template);
-	if (*template_dir && !is_absolute_path(template_dir))
-		template_dir = template_dir_dup =
-			absolute_pathdup(template_dir);
-	xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1);
-
 	if (option_depth || option_since || option_not.nr)
 		deepen = 1;
 	if (option_single_branch == -1)
@@ -1112,7 +1103,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	init_db(git_dir, real_git_dir, template_dir, GIT_HASH_UNKNOWN, NULL,
+	init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
 		INIT_DB_QUIET);
 
 	if (real_git_dir) {
@@ -1430,7 +1421,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	free(unborn_head);
 	free(dir);
 	free(path);
-	free(template_dir_dup);
 	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
diff --git a/hook.c b/hook.c
index fc974cee1d..22b274b60b 100644
--- a/hook.c
+++ b/hook.c
@@ -3,32 +3,6 @@
 #include "run-command.h"
 #include "config.h"
 
-static int identical_to_template_hook(const char *name, const char *path)
-{
-	const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
-	const char *template_dir = get_template_dir(env && *env ? env : NULL);
-	struct strbuf template_path = STRBUF_INIT;
-	int found_template_hook, ret;
-
-	strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name);
-	found_template_hook = access(template_path.buf, X_OK) >= 0;
-#ifdef STRIP_EXTENSION
-	if (!found_template_hook) {
-		strbuf_addstr(&template_path, STRIP_EXTENSION);
-		found_template_hook = access(template_path.buf, X_OK) >= 0;
-	}
-#endif
-	if (!found_template_hook) {
-		strbuf_release(&template_path);
-		return 0;
-	}
-
-	ret = do_files_match(template_path.buf, path);
-
-	strbuf_release(&template_path);
-	return ret;
-}
-
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
@@ -64,14 +38,6 @@ const char *find_hook(const char *name)
 		}
 		return NULL;
 	}
-	if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
-	    !identical_to_template_hook(name, path.buf))
-		die(_("active `%s` hook found during `git clone`:\n\t%s\n"
-		      "For security reasons, this is disallowed by default.\n"
-		      "If this is intentional and the hook should actually "
-		      "be run, please\nrun the command again with "
-		      "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
-		    name, path.buf);
 	return path.buf;
 }
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 20deca0231..fd02984330 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -771,57 +771,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
 '
 
-test_expect_success 'clone with init.templatedir runs hooks' '
-	git init tmpl/hooks &&
-	write_script tmpl/hooks/post-checkout <<-EOF &&
-	echo HOOK-RUN >&2
-	echo I was here >hook.run
-	EOF
-	git -C tmpl/hooks add . &&
-	test_tick &&
-	git -C tmpl/hooks commit -m post-checkout &&
-
-	test_when_finished "git config --global --unset init.templateDir || :" &&
-	test_when_finished "git config --unset init.templateDir || :" &&
-	(
-		sane_unset GIT_TEMPLATE_DIR &&
-		NO_SET_GIT_TEMPLATE_DIR=t &&
-		export NO_SET_GIT_TEMPLATE_DIR &&
-
-		git -c core.hooksPath="$(pwd)/tmpl/hooks" \
-			clone tmpl/hooks hook-run-hookspath 2>err &&
-		! grep "active .* hook found" err &&
-		test_path_is_file hook-run-hookspath/hook.run &&
-
-		git -c init.templateDir="$(pwd)/tmpl" \
-			clone tmpl/hooks hook-run-config 2>err &&
-		! grep "active .* hook found" err &&
-		test_path_is_file hook-run-config/hook.run &&
-
-		git clone --template=tmpl tmpl/hooks hook-run-option 2>err &&
-		! grep "active .* hook found" err &&
-		test_path_is_file hook-run-option/hook.run &&
-
-		git config --global init.templateDir "$(pwd)/tmpl" &&
-		git clone tmpl/hooks hook-run-global-config 2>err &&
-		git config --global --unset init.templateDir &&
-		! grep "active .* hook found" err &&
-		test_path_is_file hook-run-global-config/hook.run &&
-
-		# clone ignores local `init.templateDir`; need to create
-		# a new repository because we deleted `.git/` in the
-		# `setup` test case above
-		git init local-clone &&
-		cd local-clone &&
-
-		git config init.templateDir "$(pwd)/../tmpl" &&
-		git clone ../tmpl/hooks hook-run-local-config 2>err &&
-		git config --unset init.templateDir &&
-		! grep "active .* hook found" err &&
-		test_path_is_missing hook-run-local-config/hook.run
-	)
-'
-
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 11/12] Revert "Add a helper function to compare file contents"

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Now that during a `git clone`, the hooks' contents are no longer
compared to the templates' files', the caller for which the
`do_files_match()` function was introduced is gone, and therefore this
function can be retired, too.

This reverts commit 584de0b4c23 (Add a helper function to compare file
contents, 2024-03-30).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                    | 14 ---------
 copy.c                     | 58 --------------------------------------
 t/helper/test-path-utils.c | 10 -------
 t/t0060-path-utils.sh      | 41 ---------------------------
 4 files changed, 123 deletions(-)

diff --git a/cache.h b/cache.h
index 16b34799bf..8c5fb1e1ba 100644
--- a/cache.h
+++ b/cache.h
@@ -1785,20 +1785,6 @@ int copy_fd(int ifd, int ofd);
 int copy_file(const char *dst, const char *src, int mode);
 int copy_file_with_time(const char *dst, const char *src, int mode);
 
-/*
- * Compare the file mode and contents of two given files.
- *
- * If both files are actually symbolic links, the function returns 1 if the link
- * targets are identical or 0 if they are not.
- *
- * If any of the two files cannot be accessed or in case of read failures, this
- * function returns 0.
- *
- * If the file modes and contents are identical, the function returns 1,
- * otherwise it returns 0.
- */
-int do_files_match(const char *path1, const char *path2);
-
 void write_or_die(int fd, const void *buf, size_t count);
 void fsync_or_die(int fd, const char *);
 int fsync_component(enum fsync_component component, int fd);
diff --git a/copy.c b/copy.c
index 8492f6fc83..4de6a110f0 100644
--- a/copy.c
+++ b/copy.c
@@ -65,61 +65,3 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
 		return copy_times(dst, src);
 	return status;
 }
-
-static int do_symlinks_match(const char *path1, const char *path2)
-{
-	struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT;
-	int ret = 0;
-
-	if (!strbuf_readlink(&buf1, path1, 0) &&
-	    !strbuf_readlink(&buf2, path2, 0))
-		ret = !strcmp(buf1.buf, buf2.buf);
-
-	strbuf_release(&buf1);
-	strbuf_release(&buf2);
-	return ret;
-}
-
-int do_files_match(const char *path1, const char *path2)
-{
-	struct stat st1, st2;
-	int fd1 = -1, fd2 = -1, ret = 1;
-	char buf1[8192], buf2[8192];
-
-	if ((fd1 = open_nofollow(path1, O_RDONLY)) < 0 ||
-	    fstat(fd1, &st1) || !S_ISREG(st1.st_mode)) {
-		if (fd1 < 0 && errno == ELOOP)
-			/* maybe this is a symbolic link? */
-			return do_symlinks_match(path1, path2);
-		ret = 0;
-	} else if ((fd2 = open_nofollow(path2, O_RDONLY)) < 0 ||
-		   fstat(fd2, &st2) || !S_ISREG(st2.st_mode)) {
-		ret = 0;
-	}
-
-	if (ret)
-		/* to match, neither must be executable, or both */
-		ret = !(st1.st_mode & 0111) == !(st2.st_mode & 0111);
-
-	if (ret)
-		ret = st1.st_size == st2.st_size;
-
-	while (ret) {
-		ssize_t len1 = read_in_full(fd1, buf1, sizeof(buf1));
-		ssize_t len2 = read_in_full(fd2, buf2, sizeof(buf2));
-
-		if (len1 < 0 || len2 < 0 || len1 != len2)
-			ret = 0; /* read error or different file size */
-		else if (!len1) /* len2 is also 0; hit EOF on both */
-			break; /* ret is still true */
-		else
-			ret = !memcmp(buf1, buf2, len1);
-	}
-
-	if (fd1 >= 0)
-		close(fd1);
-	if (fd2 >= 0)
-		close(fd2);
-
-	return ret;
-}
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 0e0de21807..f69709d674 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -495,16 +495,6 @@ int cmd__path_utils(int argc, const char **argv)
 		return !!res;
 	}
 
-	if (argc == 4 && !strcmp(argv[1], "do_files_match")) {
-		int ret = do_files_match(argv[2], argv[3]);
-
-		if (ret)
-			printf("equal\n");
-		else
-			printf("different\n");
-		return !ret;
-	}
-
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 73d0e1a7f1..68e29c904a 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -560,45 +560,4 @@ test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works'
 	test_cmp expect actual
 '
 
-test_expect_success 'do_files_match()' '
-	test_seq 0 10 >0-10.txt &&
-	test_seq -1 10 >-1-10.txt &&
-	test_seq 1 10 >1-10.txt &&
-	test_seq 1 9 >1-9.txt &&
-	test_seq 0 8 >0-8.txt &&
-
-	test-tool path-utils do_files_match 0-10.txt 0-10.txt >out &&
-
-	assert_fails() {
-		test_must_fail \
-		test-tool path-utils do_files_match "$1" "$2" >out &&
-		grep different out
-	} &&
-
-	assert_fails 0-8.txt 1-9.txt &&
-	assert_fails -1-10.txt 0-10.txt &&
-	assert_fails 1-10.txt 1-9.txt &&
-	assert_fails 1-10.txt .git &&
-	assert_fails does-not-exist 1-10.txt &&
-
-	if test_have_prereq FILEMODE
-	then
-		cp 0-10.txt 0-10.x &&
-		chmod a+x 0-10.x &&
-		assert_fails 0-10.txt 0-10.x
-	fi &&
-
-	if test_have_prereq SYMLINKS
-	then
-		ln -sf 0-10.txt symlink &&
-		ln -s 0-10.txt another-symlink &&
-		ln -s over-the-ocean yet-another-symlink &&
-		ln -s "$PWD/0-10.txt" absolute-symlink &&
-		assert_fails 0-10.txt symlink &&
-		test-tool path-utils do_files_match symlink another-symlink &&
-		assert_fails symlink yet-another-symlink &&
-		assert_fails symlink absolute-symlink
-	fi
-'
-
 test_done
-- 
2.45.1-246-gb9cfe4845c

[PATCH v2 12/12] Revert "fsck: warn about symlink pointing inside a gitdir"

[Junio C Hamano]

This reverts commit a33fea08 (fsck: warn about symlink pointing
inside a gitdir, 2024-04-10), which warns against symbolic links
commonly created by git-annex.
---
 Documentation/fsck-msgids.txt | 12 --------
 fsck.c                        | 56 -----------------------------------
 fsck.h                        | 12 --------
 t/t1450-fsck.sh               | 37 -----------------------
 4 files changed, 117 deletions(-)

diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
index b06ec385af..12eae8a222 100644
--- a/Documentation/fsck-msgids.txt
+++ b/Documentation/fsck-msgids.txt
@@ -157,18 +157,6 @@
 `nullSha1`::
 	(WARN) Tree contains entries pointing to a null sha1.
 
-`symlinkPointsToGitDir`::
-	(WARN) Symbolic link points inside a gitdir.
-
-`symlinkTargetBlob`::
-	(ERROR) A non-blob found instead of a symbolic link's target.
-
-`symlinkTargetLength`::
-	(WARN) Symbolic link target longer than maximum path length.
-
-`symlinkTargetMissing`::
-	(ERROR) Unable to read symbolic link target's blob.
-
 `treeNotSorted`::
 	(ERROR) A tree is not properly sorted.
 
diff --git a/fsck.c b/fsck.c
index b85868e122..47eaeedd70 100644
--- a/fsck.c
+++ b/fsck.c
@@ -636,8 +636,6 @@ static int fsck_tree(const struct object_id *tree_oid,
 				retval += report(options, tree_oid, OBJ_TREE,
 						 FSCK_MSG_MAILMAP_SYMLINK,
 						 ".mailmap is a symlink");
-			oidset_insert(&options->symlink_targets_found,
-				      entry_oid);
 		}
 
 		if ((backslash = strchr(name, '\\'))) {
@@ -1230,56 +1228,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		}
 	}
 
-	if (oidset_contains(&options->symlink_targets_found, oid)) {
-		const char *ptr = buf;
-		const struct object_id *reported = NULL;
-
-		oidset_insert(&options->symlink_targets_done, oid);
-
-		if (!buf || size > PATH_MAX) {
-			/*
-			 * A missing buffer here is a sign that the caller found the
-			 * blob too gigantic to load into memory. Let's just consider
-			 * that an error.
-			 */
-			return report(options, oid, OBJ_BLOB,
-					FSCK_MSG_SYMLINK_TARGET_LENGTH,
-					"symlink target too long");
-		}
-
-		while (!reported && ptr) {
-			const char *p = ptr;
-			char c, *slash = strchrnul(ptr, '/');
-			char *backslash = memchr(ptr, '\\', slash - ptr);
-
-			c = *slash;
-			*slash = '\0';
-
-			while (!reported && backslash) {
-				*backslash = '\0';
-				if (is_ntfs_dotgit(p))
-					ret |= report(options, reported = oid, OBJ_BLOB,
-						      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
-						      "symlink target points to git dir");
-				*backslash = '\\';
-				p = backslash + 1;
-				backslash = memchr(p, '\\', slash - p);
-			}
-			if (!reported && is_ntfs_dotgit(p))
-				ret |= report(options, reported = oid, OBJ_BLOB,
-					      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
-					      "symlink target points to git dir");
-
-			if (!reported && is_hfs_dotgit(ptr))
-				ret |= report(options, reported = oid, OBJ_BLOB,
-					      FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR,
-					      "symlink target points to git dir");
-
-			*slash = c;
-			ptr = c ? slash + 1 : NULL;
-		}
-	}
-
 	return ret;
 }
 
@@ -1371,10 +1319,6 @@ int fsck_finish(struct fsck_options *options)
 			  FSCK_MSG_GITATTRIBUTES_MISSING, FSCK_MSG_GITATTRIBUTES_BLOB,
 			  options, ".gitattributes");
 
-	ret |= fsck_blobs(&options->symlink_targets_found, &options->symlink_targets_done,
-			  FSCK_MSG_SYMLINK_TARGET_MISSING, FSCK_MSG_SYMLINK_TARGET_BLOB,
-			  options, "<symlink-target>");
-
 	return ret;
 }
 
diff --git a/fsck.h b/fsck.h
index 130fa8d8f9..fcecf4101c 100644
--- a/fsck.h
+++ b/fsck.h
@@ -63,8 +63,6 @@ enum fsck_msg_type {
 	FUNC(GITATTRIBUTES_LARGE, ERROR) \
 	FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \
 	FUNC(GITATTRIBUTES_BLOB, ERROR) \
-	FUNC(SYMLINK_TARGET_MISSING, ERROR) \
-	FUNC(SYMLINK_TARGET_BLOB, ERROR) \
 	/* warnings */ \
 	FUNC(EMPTY_NAME, WARN) \
 	FUNC(FULL_PATHNAME, WARN) \
@@ -74,8 +72,6 @@ enum fsck_msg_type {
 	FUNC(NULL_SHA1, WARN) \
 	FUNC(ZERO_PADDED_FILEMODE, WARN) \
 	FUNC(NUL_IN_COMMIT, WARN) \
-	FUNC(SYMLINK_TARGET_LENGTH, WARN) \
-	FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_FILEMODE, INFO) \
 	FUNC(GITMODULES_PARSE, INFO) \
@@ -143,8 +139,6 @@ struct fsck_options {
 	struct oidset gitmodules_done;
 	struct oidset gitattributes_found;
 	struct oidset gitattributes_done;
-	struct oidset symlink_targets_found;
-	struct oidset symlink_targets_done;
 	kh_oid_map_t *object_names;
 };
 
@@ -154,8 +148,6 @@ struct fsck_options {
 	.gitmodules_done = OIDSET_INIT, \
 	.gitattributes_found = OIDSET_INIT, \
 	.gitattributes_done = OIDSET_INIT, \
-	.symlink_targets_found = OIDSET_INIT, \
-	.symlink_targets_done = OIDSET_INIT, \
 	.error_func = fsck_error_function \
 }
 #define FSCK_OPTIONS_STRICT { \
@@ -164,8 +156,6 @@ struct fsck_options {
 	.gitmodules_done = OIDSET_INIT, \
 	.gitattributes_found = OIDSET_INIT, \
 	.gitattributes_done = OIDSET_INIT, \
-	.symlink_targets_found = OIDSET_INIT, \
-	.symlink_targets_done = OIDSET_INIT, \
 	.error_func = fsck_error_function, \
 }
 #define FSCK_OPTIONS_MISSING_GITMODULES { \
@@ -174,8 +164,6 @@ struct fsck_options {
 	.gitmodules_done = OIDSET_INIT, \
 	.gitattributes_found = OIDSET_INIT, \
 	.gitattributes_done = OIDSET_INIT, \
-	.symlink_targets_found = OIDSET_INIT, \
-	.symlink_targets_done = OIDSET_INIT, \
 	.error_func = fsck_error_cb_print_missing_gitmodules, \
 }
 
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 5669872bc8..de0f6d5e7f 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1023,41 +1023,4 @@ test_expect_success 'fsck error on gitattributes with excessive size' '
 	test_cmp expected actual
 '
 
-test_expect_success 'fsck warning on symlink target with excessive length' '
-	symlink_target=$(printf "pattern %032769d" 1 | git hash-object -w --stdin) &&
-	test_when_finished "remove_object $symlink_target" &&
-	tree=$(printf "120000 blob %s\t%s\n" $symlink_target symlink | git mktree) &&
-	test_when_finished "remove_object $tree" &&
-	cat >expected <<-EOF &&
-	warning in blob $symlink_target: symlinkTargetLength: symlink target too long
-	EOF
-	git fsck --no-dangling >actual 2>&1 &&
-	test_cmp expected actual
-'
-
-test_expect_success 'fsck warning on symlink target pointing inside git dir' '
-	gitdir=$(printf ".git" | git hash-object -w --stdin) &&
-	ntfs_gitdir=$(printf "GIT~1" | git hash-object -w --stdin) &&
-	hfs_gitdir=$(printf ".${u200c}git" | git hash-object -w --stdin) &&
-	inside_gitdir=$(printf "nested/.git/config" | git hash-object -w --stdin) &&
-	benign_target=$(printf "legit/config" | git hash-object -w --stdin) &&
-	tree=$(printf "120000 blob %s\t%s\n" \
-		$benign_target benign_target \
-		$gitdir gitdir \
-		$hfs_gitdir hfs_gitdir \
-		$inside_gitdir inside_gitdir \
-		$ntfs_gitdir ntfs_gitdir |
-		git mktree) &&
-	for o in $gitdir $ntfs_gitdir $hfs_gitdir $inside_gitdir $benign_target $tree
-	do
-		test_when_finished "remove_object $o" || return 1
-	done &&
-	printf "warning in blob %s: symlinkPointsToGitDir: symlink target points to git dir\n" \
-		$gitdir $hfs_gitdir $inside_gitdir $ntfs_gitdir |
-	sort >expected &&
-	git fsck --no-dangling >actual 2>&1 &&
-	sort actual >actual.sorted &&
-	test_cmp expected actual.sorted
-'
-
 test_done
-- 
2.45.1-246-gb9cfe4845c