git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config
@ 2021-12-12 20:13 Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 01/13] t0001: fix gaps in "TEMPLATE DIRECTORY" coverage Ævar Arnfjörð Bjarmason
                   ` (12 more replies)
  0 siblings, 13 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

[This series does a couple of different things that could be split up,
but I thought that it was easier to review it in one piece].

This series changes our tests to not rely on the default template that
"git init" populates new repositories with. This reliance hid a bug
where a recently added mode of "sparse-checkout" had a hard dependency
on git's default template, i.e. it would potentially break on
repositories with a custom --template.

So this changes all the tests that relied on those, and declares that
we'll always create the "info" directory whatever the template says
(which will fix that sparse-checkout isssue).

We also add a new "git [init|clone] --no-template" option, and make
the existing init.templateDir accept "false" as a way of doing what
the existing-but-undocumented --template= would do, i.e. use no
template at all.

I would like to eventually follow-up and make something like
init.templateDir=false the default (or at least something closer to
it). Notably we litter the rather large sample hooks in every
repository ever cloned, but this series doesn't chane anything about
what we do by default.

This topic was mentioned (item 16) in the recent contributor
summit[1]. The notes only reflect that I mentioned .git/branches, but
I think it was in reply to someone's question about that or the
default templates.

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211148400.56@tvgsbejvaqbjf.bet/

Ævar Arnfjörð Bjarmason (13):
  t0001: fix gaps in "TEMPLATE DIRECTORY" coverage
  init: split out template population from create_default_files()
  init: unconditionally create the "info" directory
  t0008: don't rely on default ".git/info/exclude"
  init & clone: add a --no-template option
  init & clone: add init.templateDir=[bool]
  test-lib: create test data with "git init --no-template" (almost)
  tests: don't depend on template-created .git/branches
  t5540: don't rely on "hook/post-update.sample"
  test-lib-functions: add and use a "write_hook" wrapper
  tests: change "cat && chmod +x" to use "write_hook"
  tests: migrate miscellaneous "write_script" to "write_hooks"
  tests: don't depend on template-created .git/hooks

 Documentation/git-clone.txt            |   8 +-
 Documentation/git-init.txt             |  31 +++++++-
 Documentation/gitrepository-layout.txt |  17 ++++-
 builtin/clone.c                        |  15 +++-
 builtin/init-db.c                      |  75 +++++++++++++------
 cache.h                                |   2 +
 config.c                               |  34 +++++++++
 config.h                               |  17 +++++
 t/t0001-init.sh                        | 100 ++++++++++++++++++++++---
 t/t0008-ignores.sh                     |  10 +--
 t/t1416-ref-transaction-hooks.sh       |  14 ++--
 t/t3412-rebase-root.sh                 |  18 ++---
 t/t3413-rebase-hook.sh                 |  18 ++---
 t/t3430-rebase-merges.sh               |   5 +-
 t/t5401-update-hooks.sh                |  62 +++++++--------
 t/t5402-post-merge-hook.sh             |  16 ++--
 t/t5406-remote-rejects.sh              |   2 +-
 t/t5407-post-rewrite-hook.sh           |  14 ++--
 t/t5409-colorize-remote-messages.sh    |   2 +-
 t/t5411-proc-receive-hook.sh           |   4 +-
 t/t5505-remote.sh                      |   2 +
 t/t5510-fetch.sh                       |   6 +-
 t/t5516-fetch-push.sh                  |  26 +++----
 t/t5521-pull-options.sh                |   4 +-
 t/t5540-http-push-webdav.sh            |   4 +-
 t/t5541-http-push-smart.sh             |   4 +-
 t/t5547-push-quarantine.sh             |   4 +-
 t/t5548-push-porcelain.sh              |   2 +-
 t/t5601-clone.sh                       |   4 +-
 t/t6500-gc.sh                          |   4 +-
 t/t7450-bad-git-dotfiles.sh            |   1 +
 t/test-lib-functions.sh                |  30 ++++++++
 t/test-lib.sh                          |   5 +-
 wrap-for-bin.sh                        |   4 +-
 34 files changed, 391 insertions(+), 173 deletions(-)

-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 01/13] t0001: fix gaps in "TEMPLATE DIRECTORY" coverage
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 02/13] init: split out template population from create_default_files() Ævar Arnfjörð Bjarmason
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Add tests to assert that the priority order described in the "TEMPLATE
DIRECTORY" section in the "init" documentation is correct.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0001-init.sh | 85 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 3 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 7603ad2f82b..e0b965cdc8f 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -173,6 +173,55 @@ test_expect_success 'reinit' '
 	test_must_be_empty again/err2
 '
 
+setup_template_priority() {
+	test_when_finished "rm -rf template" &&
+	mkdir template &&
+	touch template/file &&
+
+	test_when_finished "rm -rf template2" &&
+	mkdir template2 &&
+	touch template2/file2 &&
+
+	# Created by the caller
+	test_when_finished "rm -rf repo"
+}
+
+test_expect_success 'usage priority: --template only' '
+	setup_template_priority &&
+	git init --template=template repo &&
+	test_path_is_file repo/.git/file
+'
+
+test_expect_success 'usage priority: --template takes precedence over GIT_TEMPLATE_DIR' '
+	setup_template_priority &&
+	GIT_TEMPLATE_DIR="$PWD/template2" git init --template=template repo &&
+	test_path_is_file repo/.git/file
+'
+
+test_expect_success 'usage priority: --template takes precedence over init.templateDir' '
+	setup_template_priority &&
+	git -c init.templateDir="$PWD/template2" init --template=template repo &&
+	test_path_is_file repo/.git/file
+'
+
+test_expect_success 'usage priority: --no-template takes precedence over init.templateDir' '
+	setup_template_priority &&
+	git -c init.templateDir="$PWD/template" init --no-template repo &&
+	test_path_is_missing repo/.git/file
+'
+
+test_expect_success 'usage priority: --no-template takes precedence over GIT_TEMPLATE_DIR' '
+	setup_template_priority &&
+	GIT_TEMPLATE_DIR="$PWD/template" git init --no-template repo &&
+	test_path_is_missing repo/.git/file
+'
+
+test_expect_success 'usage priority: GIT_NO_TEMPLATE_DIR=true takes precedence over GIT_TEMPLATE_DIR' '
+	setup_template_priority &&
+	GIT_TEMPLATE_DIR="$PWD/template" GIT_NO_TEMPLATE_DIR=true git init repo &&
+	test_path_is_missing repo/.git/file
+'
+
 test_expect_success 'init with --template' '
 	mkdir template-source &&
 	echo content >template-source/file &&
@@ -187,11 +236,15 @@ test_expect_success 'init with --template (blank)' '
 	test_path_is_missing template-blank/.git/info/exclude
 '
 
+no_templatedir_env () {
+	sane_unset GIT_TEMPLATE_DIR &&
+	NO_SET_GIT_TEMPLATE_DIR=t &&
+	export NO_SET_GIT_TEMPLATE_DIR
+}
+
 init_no_templatedir_env () {
 	(
-		sane_unset GIT_TEMPLATE_DIR &&
-		NO_SET_GIT_TEMPLATE_DIR=t &&
-		export NO_SET_GIT_TEMPLATE_DIR &&
+		no_templatedir_env &&
 		git init "$1"
 	)
 }
@@ -214,6 +267,32 @@ test_expect_success 'init with init.templatedir using ~ expansion' '
 	test_cmp templatedir-source/file templatedir-expansion/.git/file
 '
 
+test_expect_success 'init with init.templateDir=does-not-exist' '
+	test_when_finished "rm -rf repo" &&
+	(
+		no_templatedir_env &&
+
+		cat >expect <<-\EOF &&
+		warning: templates not found in does-not-exist
+		EOF
+		git -c init.templateDir=does-not-exist init repo 2>actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'init with init.templateDir=[bool]' '
+	test_when_finished "rm -rf repo" &&
+	(
+		no_templatedir_env &&
+
+		cat >expect <<-\EOF &&
+		warning: templates not found in false
+		EOF
+		git -c init.templateDir=false init repo 2>actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'init --bare/--shared overrides system/global config' '
 	test_config_global core.bare false &&
 	test_config_global core.sharedRepository 0640 &&
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 02/13] init: split out template population from create_default_files()
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 01/13] t0001: fix gaps in "TEMPLATE DIRECTORY" coverage Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 03/13] init: unconditionally create the "info" directory Ævar Arnfjörð Bjarmason
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

The create_default_files() function only has one caller,
init_db(). Let's have it call a separate create_template_files()
function.

This refactoring changes nothing about how the repository is
initialized, but makes subsequent changes to create_template_files()
easier to read, e.g. because new variables we'll need will be scoped
to that function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/init-db.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2167796ff2a..3cf834eddd2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -187,21 +187,9 @@ void initialize_repository_version(int hash_algo, int reinit)
 		git_config_set_gently("extensions.objectformat", NULL);
 }
 
-static int create_default_files(const char *template_path,
-				const char *original_git_dir,
-				const char *initial_branch,
-				const struct repository_format *fmt,
-				int quiet)
+static void create_template_files(const char *template_path)
 {
-	struct stat st1;
-	struct strbuf buf = STRBUF_INIT;
-	char *path;
-	char junk[2];
-	int reinit;
-	int filemode;
-	struct strbuf err = STRBUF_INIT;
 	const char *init_template_dir = NULL;
-	const char *work_tree = get_git_work_tree();
 
 	/*
 	 * First copy the templates -- we might have the default
@@ -218,6 +206,21 @@ static int create_default_files(const char *template_path,
 	git_config_clear();
 	reset_shared_repository();
 	git_config(git_default_config, NULL);
+}
+
+static int create_default_files(const char *original_git_dir,
+				const char *initial_branch,
+				const struct repository_format *fmt,
+				int quiet)
+{
+	struct stat st1;
+	struct strbuf buf = STRBUF_INIT;
+	char *path;
+	char junk[2];
+	int reinit;
+	int filemode;
+	struct strbuf err = STRBUF_INIT;
+	const char *work_tree = get_git_work_tree();
 
 	/*
 	 * We must make sure command-line options continue to override any
@@ -425,7 +428,8 @@ int init_db(const char *git_dir, const char *real_git_dir,
 
 	validate_hash_algorithm(&repo_fmt, hash);
 
-	reinit = create_default_files(template_dir, original_git_dir,
+	create_template_files(template_dir);
+	reinit = create_default_files(original_git_dir,
 				      initial_branch, &repo_fmt,
 				      flags & INIT_DB_QUIET);
 	if (reinit && initial_branch)
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 01/13] t0001: fix gaps in "TEMPLATE DIRECTORY" coverage Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 02/13] init: split out template population from create_default_files() Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-20 15:59   ` Derrick Stolee
  2021-12-12 20:13 ` [PATCH 04/13] t0008: don't rely on default ".git/info/exclude" Ævar Arnfjörð Bjarmason
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

In preceding commits the test suite has been taught to run without a
template directory, but in doing so we needed to fix code that relied
on the "hooks" and "branches" directories.

The "hooks" code was all specific to our own test suite. The
"branches" directory is intentionally created, but has been "slightly
deprecated" for a while, so it's not created when not using the
default template.

However "info" is different. Trying to omit its creation would lead to
a lot of test suite failures. Many of these we should arguably fix,
the common pattern being to add an exclude to "info/excludes".

But we've also grown a hard dependency on this directory within git
itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
subcommand, 2019-11-21) released with v2.25.0 the "git
sparse-checkout" command has wanted to add exclusions to
"info/sparse-checkout". It didn't check or create the leading
directory, so if it's omitted the command will die.

Even if that behavior were fixed we'd be left with older versions of
"git" dying if that was attempted if they used a repository
initialized without a template.

So let's just bite the bullet and make the "info" directory mandatory,
and document it as such. Let's also note that in the documentation
that this doesn't apply to the "hooks" and "branches" directories.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/gitrepository-layout.txt | 17 ++++++++++++++++-
 builtin/init-db.c                      |  6 ++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
index 1a2ef4c1505..eb58ab08817 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -160,7 +160,10 @@ branches::
 	and not likely to be found in modern repositories. This
 	directory is ignored if $GIT_COMMON_DIR is set and
 	"$GIT_COMMON_DIR/branches" will be used instead.
-
++
+This directory is created by the default linkgit:git-init[1]
+template. It will not be created when using a custom template that
+doesn't contain it.
 
 hooks::
 	Hooks are customization scripts used by various Git
@@ -171,6 +174,10 @@ hooks::
 	Read linkgit:githooks[5] for more details about
 	each hook. This directory is ignored if $GIT_COMMON_DIR is set
 	and "$GIT_COMMON_DIR/hooks" will be used instead.
++
+This directory is created by the default linkgit:git-init[1]
+template. It will not be created when using a custom template that
+doesn't contain it.
 
 common::
 	When multiple working trees are used, most of files in
@@ -190,6 +197,14 @@ info::
 	Additional information about the repository is recorded
 	in this directory. This directory is ignored if $GIT_COMMON_DIR
 	is set and "$GIT_COMMON_DIR/info" will be used instead.
++
+This directory is created by the default linkgit:git-init[1]
+template.
++
+It will be created even when when using a custom template that doesn't
+contain it. On older versions of git this was not the case, as various
+tools came to rely on its creation (including parts of git itself)
+it's now unconditionally (re-)created on 'git init'.
 
 info/refs::
 	This file helps dumb transports discover what refs are
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3cf834eddd2..75495c9c8c6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -222,6 +222,12 @@ static int create_default_files(const char *original_git_dir,
 	struct strbuf err = STRBUF_INIT;
 	const char *work_tree = get_git_work_tree();
 
+	/*
+	 * We may not have a info/ if the template explicitly omitted
+	 * it.
+	 */
+	safe_create_dir(git_path("info"), 1);
+
 	/*
 	 * We must make sure command-line options continue to override any
 	 * values we might have just re-read from the config.
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 04/13] t0008: don't rely on default ".git/info/exclude"
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 03/13] init: unconditionally create the "info" directory Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 05/13] init & clone: add a --no-template option Ævar Arnfjörð Bjarmason
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Change a test added in 368aa52952d (add git-check-ignore sub-command,
2013-01-06) to clobber .git/info/exclude rather than append to
it.

These tests would break if the "templates/info--exclude" file added in
d3af621b147 (Redo the templates generation and installation.,
2005-08-06) wasn't exactly 6 lines (of only comments).

Let's instead clobber the default .git/info/excludes file, and test
only our own expected content. This is not strictly needed for
anything in this series, but is a good cleanup while we're at it.

As discussed in the preceding commit a lot of things depend on the
"info" directory being created, but this was the only test that relied
on the specific content in the "templates/info--exclude" file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0008-ignores.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 42d23148049..6bc08ee2537 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -225,7 +225,7 @@ test_expect_success 'setup' '
 		!globaltwo
 		globalthree
 	EOF
-	cat <<-\EOF >>.git/info/exclude
+	cat <<-\EOF >.git/info/exclude
 		per-repo
 	EOF
 '
@@ -543,9 +543,9 @@ test_expect_success_multi 'submodule from subdirectory' '' '
 
 test_expect_success 'global ignore not yet enabled' '
 	expect_from_stdin <<-\EOF &&
-		.git/info/exclude:7:per-repo	per-repo
+		.git/info/exclude:1:per-repo	per-repo
 		a/.gitignore:2:*three	a/globalthree
-		.git/info/exclude:7:per-repo	a/per-repo
+		.git/info/exclude:1:per-repo	a/per-repo
 	EOF
 	test_check_ignore "-v globalone per-repo a/globalthree a/per-repo not-ignored a/globaltwo"
 '
@@ -566,10 +566,10 @@ test_expect_success 'global ignore with -v' '
 	enable_global_excludes &&
 	expect_from_stdin <<-EOF &&
 		$global_excludes:1:globalone	globalone
-		.git/info/exclude:7:per-repo	per-repo
+		.git/info/exclude:1:per-repo	per-repo
 		$global_excludes:3:globalthree	globalthree
 		a/.gitignore:2:*three	a/globalthree
-		.git/info/exclude:7:per-repo	a/per-repo
+		.git/info/exclude:1:per-repo	a/per-repo
 		$global_excludes:2:!globaltwo	globaltwo
 	EOF
 	test_check_ignore "-v globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo"
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 05/13] init & clone: add a --no-template option
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 04/13] t0008: don't rely on default ".git/info/exclude" Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 06/13] init & clone: add init.templateDir=[bool] Ævar Arnfjörð Bjarmason
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Add a new "--no-template" convenience option to "git init" and "git
clone".

This option is functionally equivalent to the long-standing "trick" of
providing an empty parameter to --template, i.e. "--template=". See
172035f044e (init: handle empty "template" parameter, 2008-07-28).

But that long-standing trick has never been documented, and isn't
obvious. Instead of documenting it let's provide an alternate way of
doing this that conforms with how we usually handle other such cases.

Let's also add a GIT_NO_TEMPLATE_DIR environment variable. For now
this is for consistency with the existing "GIT_TEMPLATE_DIR", but in a
subsequent commit we'll make use of it within "t/test-lib.sh".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-clone.txt |  8 +++++++-
 Documentation/git-init.txt  | 24 +++++++++++++++++++++---
 builtin/clone.c             | 15 ++++++++++++---
 builtin/init-db.c           | 34 ++++++++++++++++++++++++----------
 cache.h                     |  2 ++
 t/t0001-init.sh             |  4 ++++
 6 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 9685ea06915..0227ee9a4c0 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -9,7 +9,9 @@ git-clone - Clone a repository into a new directory
 SYNOPSIS
 --------
 [verse]
-'git clone' [--template=<template-directory>]
+
+
+'git clone' [--no-template | --template=<template-directory>]
 	  [-l] [-s] [--no-hardlinks] [-q] [-n] [--bare] [--mirror]
 	  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
 	  [--dissociate] [--separate-git-dir <git-dir>]
@@ -211,6 +213,10 @@ objects from the source repository into a pack in the cloned repository.
 	via ssh, this specifies a non-default path for the command
 	run on the other end.
 
+--no-template:
+	Specify that no template directory will be used;
+	(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
+
 --template=<template-directory>::
 	Specify the directory from which templates will be used;
 	(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index ad921fe782e..d0ce9b185a5 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -9,7 +9,8 @@ git-init - Create an empty Git repository or reinitialize an existing one
 SYNOPSIS
 --------
 [verse]
-'git init' [-q | --quiet] [--bare] [--template=<template-directory>]
+'git init' [-q | --quiet] [--bare]
+	  [--no-template | --template=<template-directory>]
 	  [--separate-git-dir <git-dir>] [--object-format=<format>]
 	  [-b <branch-name> | --initial-branch=<branch-name>]
 	  [--shared[=<permissions>]] [<directory>]
@@ -62,6 +63,12 @@ include::object-format-disclaimer.txt[]
 Specify the directory from which templates will be used.  (See the "TEMPLATE
 DIRECTORY" section below.)
 
+--no-template::
+
+Do not use any template directory. On older versions of git this can
+be emulated by providing a `--template' argument where
+'<template_directory>' is a path to an empty directory.
+
 --separate-git-dir=<git-dir>::
 
 Instead of initializing the repository as a directory to either `$GIT_DIR` or
@@ -132,10 +139,21 @@ does not exist, it will be created.
 TEMPLATE DIRECTORY
 ------------------
 
-Files and directories in the template directory whose name do not start with a
+If a template directory is in use files and directories in the
+template directory whose name do not start with a
 dot will be copied to the `$GIT_DIR` after it is created.
 
-The template directory will be one of the following (in order):
+Using a template template directory can be disabled by any of of:
+
+ - The `--no-template` option being given. This option is incompatible
+   with `--template`.
+
+ - The `GIT_NO_TEMPLATE_DIR` variable being set to `true` in the
+   environment (or any other `true` value. See the discussion of
+   boolean canonicalization in linkgit:git-config[1]).
+
+If none of those conditions are true, then the template directory
+will be one of the following (in order):
 
  - the argument given with the `--template` option;
 
diff --git a/builtin/clone.c b/builtin/clone.c
index fb377b27657..a57ba4da31d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,6 +53,7 @@ static int option_shallow_submodules;
 static int option_reject_shallow = -1;    /* unspecified */
 static int config_reject_shallow = -1;    /* unspecified */
 static int deepen;
+static int option_no_template;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *remote_name = NULL;
@@ -113,8 +114,11 @@ static struct option builtin_clone_options[] = {
 	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	OPT_INTEGER('j', "jobs", &max_jobs,
 		    N_("number of submodules cloned in parallel")),
-	OPT_STRING(0, "template", &option_template, N_("template-directory"),
-		   N_("directory from which templates will be used")),
+	OPT_BOOL_F(0, "no-template", &option_no_template, N_("do not use a template"),
+		   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
+	OPT_STRING_F(0, "template", &option_template, N_("template-directory"),
+		     N_("directory from which templates will be used"),
+		     PARSE_OPT_NONEG),
 	OPT_STRING_LIST(0, "reference", &option_required_reference, N_("repo"),
 			N_("reference repository")),
 	OPT_STRING_LIST(0, "reference-if-able", &option_optional_reference,
@@ -907,6 +911,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
+	if (option_no_template && option_template)
+		die(_("--no-template and --template are incompatible."));
+
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
@@ -1037,7 +1044,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
+	init_db(git_dir, real_git_dir,
+		option_no_template, option_template,
+		GIT_HASH_UNKNOWN, NULL,
 		INIT_DB_QUIET);
 
 	if (real_git_dir) {
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 75495c9c8c6..4c4ff6fe412 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -93,7 +93,8 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
-static void copy_templates(const char *template_dir, const char *init_template_dir)
+static void copy_templates(int no_template, const char *template_dir,
+			   const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;
@@ -103,6 +104,11 @@ static void copy_templates(const char *template_dir, const char *init_template_d
 	DIR *dir;
 	char *to_free = NULL;
 
+	if (no_template)
+		return;
+	if (!template_dir && !init_template_dir &&
+	    git_env_bool(GIT_NO_TEMPLATE_DIR_ENVIRONMENT, 0))
+		return;
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
@@ -187,7 +193,7 @@ void initialize_repository_version(int hash_algo, int reinit)
 		git_config_set_gently("extensions.objectformat", NULL);
 }
 
-static void create_template_files(const char *template_path)
+static void create_template_files(int no_template, const char *template_path)
 {
 	const char *init_template_dir = NULL;
 
@@ -201,7 +207,7 @@ static void create_template_files(const char *template_path)
 	 * disk).
 	 */
 	git_config_get_pathname("init.templatedir", &init_template_dir);
-	copy_templates(template_path, init_template_dir);
+	copy_templates(no_template, template_path, init_template_dir);
 	free((char *)init_template_dir);
 	git_config_clear();
 	reset_shared_repository();
@@ -391,7 +397,8 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash
 }
 
 int init_db(const char *git_dir, const char *real_git_dir,
-	    const char *template_dir, int hash, const char *initial_branch,
+	    int no_template, const char *template_dir,
+	    int hash, const char *initial_branch,
 	    unsigned int flags)
 {
 	int reinit;
@@ -434,7 +441,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 
 	validate_hash_algorithm(&repo_fmt, hash);
 
-	create_template_files(template_dir);
+	create_template_files(no_template, template_dir);
 	reinit = create_default_files(original_git_dir,
 				      initial_branch, &repo_fmt,
 				      flags & INIT_DB_QUIET);
@@ -525,7 +532,7 @@ static int shared_callback(const struct option *opt, const char *arg, int unset)
 }
 
 static const char *const init_db_usage[] = {
-	N_("git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]] [<directory>]"),
+	N_("git init [-q | --quiet] [--bare] [--no-template | --template=<template-directory>] [--shared[=<permissions>]] [<directory>]"),
 	NULL
 };
 
@@ -540,14 +547,18 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *real_git_dir = NULL;
 	const char *work_tree;
+	int no_template = 0;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
 	const char *object_format = NULL;
 	const char *initial_branch = NULL;
 	int hash_algo = GIT_HASH_UNKNOWN;
 	const struct option init_db_options[] = {
-		OPT_STRING(0, "template", &template_dir, N_("template-directory"),
-				N_("directory from which templates will be used")),
+		OPT_BOOL_F(0, "no-template", &no_template, N_("do not use a template"),
+			   PARSE_OPT_NONEG),
+		OPT_STRING_F(0, "template", &template_dir, N_("template-directory"),
+			     N_("directory from which templates will be used"),
+			     PARSE_OPT_NONEG),
 		OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
 				N_("create a bare repository"), 1),
 		{ OPTION_CALLBACK, 0, "shared", &init_shared_repository,
@@ -572,6 +583,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	if (real_git_dir && !is_absolute_path(real_git_dir))
 		real_git_dir = real_pathdup(real_git_dir, 1);
 
+	if (no_template && template_dir)
+		die(_("--no-template and --template are incompatible"));
 	if (template_dir && *template_dir && !is_absolute_path(template_dir)) {
 		template_dir = absolute_pathdup(template_dir);
 		UNLEAK(template_dir);
@@ -701,6 +714,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	UNLEAK(work_tree);
 
 	flags |= INIT_DB_EXIST_OK;
-	return init_db(git_dir, real_git_dir, template_dir, hash_algo,
-		       initial_branch, flags);
+	return init_db(git_dir, real_git_dir,
+		       no_template, template_dir,
+		       hash_algo, initial_branch, flags);
 }
diff --git a/cache.h b/cache.h
index d5cafba17d4..d2c7b5e1899 100644
--- a/cache.h
+++ b/cache.h
@@ -498,6 +498,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE"
 #define GIT_SHALLOW_FILE_ENVIRONMENT "GIT_SHALLOW_FILE"
 #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
+#define GIT_NO_TEMPLATE_DIR_ENVIRONMENT "GIT_NO_TEMPLATE_DIR"
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
 #define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
 #define CONFIG_COUNT_ENVIRONMENT "GIT_CONFIG_COUNT"
@@ -656,6 +657,7 @@ int path_inside_repo(const char *prefix, const char *path);
 #define INIT_DB_EXIST_OK 0x0002
 
 int init_db(const char *git_dir, const char *real_git_dir,
+	    int no_template,
 	    const char *template_dir, int hash_algo,
 	    const char *initial_branch, unsigned int flags);
 void initialize_repository_version(int hash_algo, int reinit);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e0b965cdc8f..388c28062c2 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -173,6 +173,10 @@ test_expect_success 'reinit' '
 	test_must_be_empty again/err2
 '
 
+test_expect_success 'usage: init with --no-template --template' '
+	test_expect_code 128 git init --no-template --template=$PWD
+'
+
 setup_template_priority() {
 	test_when_finished "rm -rf template" &&
 	mkdir template &&
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 06/13] init & clone: add init.templateDir=[bool]
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (4 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 05/13] init & clone: add a --no-template option Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 07/13] test-lib: create test data with "git init --no-template" (almost) Ævar Arnfjörð Bjarmason
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Add the ability to specify init.templateDir=false in the configuration
to always initialize or clone repositories as though "--no-template"
was provided. This makes it easy to entirely opt-out of getting
default template content such as the sample hooks.

Not all of what's being added here to the config API is needed for
this change, but let's be consistent with the existing boilerplate
there and add the full set of relevant functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-init.txt |  7 ++++++-
 builtin/clone.c            |  4 ++--
 builtin/init-db.c          | 10 +++++++---
 config.c                   | 34 ++++++++++++++++++++++++++++++++++
 config.h                   | 17 +++++++++++++++++
 t/t0001-init.sh            |  8 +++-----
 6 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index d0ce9b185a5..ef933ee9145 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -148,6 +148,9 @@ Using a template template directory can be disabled by any of of:
  - The `--no-template` option being given. This option is incompatible
    with `--template`.
 
+ - The `init.templateDirectory' option being set to 'false' (or
+   another stand-in for 'false', see linkgit:git-config[1]).
+
  - The `GIT_NO_TEMPLATE_DIR` variable being set to `true` in the
    environment (or any other `true` value. See the discussion of
    boolean canonicalization in linkgit:git-config[1]).
@@ -159,7 +162,9 @@ will be one of the following (in order):
 
  - the contents of the `$GIT_TEMPLATE_DIR` environment variable;
 
- - the `init.templateDir` configuration variable; or
+ - the `init.templateDir` configuration variable, if set to 'true' (or
+   another stand-in for 'true', see linkgit:git-config[1]) the default
+   template directory; or
 
  - the default template directory: `/usr/share/git-core/templates`.
 
diff --git a/builtin/clone.c b/builtin/clone.c
index a57ba4da31d..671d8356f1c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,7 +53,7 @@ static int option_shallow_submodules;
 static int option_reject_shallow = -1;    /* unspecified */
 static int config_reject_shallow = -1;    /* unspecified */
 static int deepen;
-static int option_no_template;
+static int option_no_template = -1; /* unspecified */
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *remote_name = NULL;
@@ -911,7 +911,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (option_no_template && option_template)
+	if (option_no_template != -1 && option_template)
 		die(_("--no-template and --template are incompatible."));
 
 	repo_name = argv[0];
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 4c4ff6fe412..fcf538193c8 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -196,6 +196,7 @@ void initialize_repository_version(int hash_algo, int reinit)
 static void create_template_files(int no_template, const char *template_path)
 {
 	const char *init_template_dir = NULL;
+	int is_bool, ret;
 
 	/*
 	 * First copy the templates -- we might have the default
@@ -206,7 +207,10 @@ static void create_template_files(int no_template, const char *template_path)
 	 * values (since we've just potentially changed what's available on
 	 * disk).
 	 */
-	git_config_get_pathname("init.templatedir", &init_template_dir);
+	ret = git_config_get_bool_or_pathname("init.templatedir", &is_bool,
+					      &init_template_dir);
+	if (no_template == -1)
+		no_template = is_bool ? !ret : 0;
 	copy_templates(no_template, template_path, init_template_dir);
 	free((char *)init_template_dir);
 	git_config_clear();
@@ -547,7 +551,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *real_git_dir = NULL;
 	const char *work_tree;
-	int no_template = 0;
+	int no_template = -1;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
 	const char *object_format = NULL;
@@ -583,7 +587,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	if (real_git_dir && !is_absolute_path(real_git_dir))
 		real_git_dir = real_pathdup(real_git_dir, 1);
 
-	if (no_template && template_dir)
+	if (no_template != -1 && template_dir)
 		die(_("--no-template and --template are incompatible"));
 	if (template_dir && *template_dir && !is_absolute_path(template_dir)) {
 		template_dir = absolute_pathdup(template_dir);
diff --git a/config.c b/config.c
index c5873f3a706..f64ce2a1e1e 100644
--- a/config.c
+++ b/config.c
@@ -1260,6 +1260,18 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+int git_config_bool_or_pathname(const char **dest, const char *name,
+				const char *value, int *is_bool)
+{
+	int v = git_parse_maybe_bool_text(value);
+	if (0 <= v) {
+		*is_bool = 1;
+		return v;
+	}
+	*is_bool = 0;
+	return git_config_pathname(dest, name, value);
+}
+
 int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)
 {
 	if (!value)
@@ -2251,6 +2263,16 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
 		return 1;
 }
 
+int git_configset_get_bool_or_pathname(struct config_set *cs, const char *key,
+				       int *is_bool, const char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value))
+		return git_config_bool_or_pathname(dest, key, value, is_bool);
+	else
+		return 1;
+}
+
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
@@ -2384,6 +2406,13 @@ int repo_config_get_pathname(struct repository *repo,
 	return ret;
 }
 
+int repo_config_get_bool_or_pathname(struct repository *repo,
+				     const char *key, int *is_bool, const char **dest)
+{
+	git_config_check_init(repo);
+	return git_configset_get_bool_or_pathname(repo->config, key, is_bool, dest);
+}
+
 /* Functions used historically to read configuration from 'the_repository' */
 void git_config(config_fn_t fn, void *data)
 {
@@ -2445,6 +2474,11 @@ int git_config_get_pathname(const char *key, const char **dest)
 	return repo_config_get_pathname(the_repository, key, dest);
 }
 
+int git_config_get_bool_or_pathname(const char *key, int *is_bool, const char **dest)
+{
+	return repo_config_get_bool_or_pathname(the_repository, key, is_bool, dest);
+}
+
 int git_config_get_expiry(const char *key, const char **output)
 {
 	int ret = git_config_get_string(key, (char **)output);
diff --git a/config.h b/config.h
index f119de01309..d482d55d9ae 100644
--- a/config.h
+++ b/config.h
@@ -241,6 +241,15 @@ int git_config_string(const char **, const char *, const char *);
  */
 int git_config_pathname(const char **, const char *, const char *);
 
+/**
+ * Same as `git_config_pathname`, except that we'll first try to parse
+ * the value as a boolean. Check the `is_bool` to see if it was a
+ * boolen. Otherwise it'll have the same semantics as
+ * `git_config_pathname`.
+ */
+int git_config_bool_or_pathname(const char **dest, const char *name,
+				const char *value, int *is_bool);
+
 int git_config_expiry_date(timestamp_t *, const char *, const char *);
 int git_config_color(char *, const char *, const char *);
 int git_config_set_in_file_gently(const char *, const char *, const char *);
@@ -493,6 +502,8 @@ int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);
 int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);
 int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);
 int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);
+int git_configset_get_bool_or_pathname(struct config_set *cs, const char *key,
+				       int *is_bool, const char **dest);
 
 /* Functions for reading a repository's config */
 struct repository;
@@ -517,6 +528,9 @@ int repo_config_get_maybe_bool(struct repository *repo,
 			       const char *key, int *dest);
 int repo_config_get_pathname(struct repository *repo,
 			     const char *key, const char **dest);
+int repo_config_get_bool_or_pathname(struct repository *repo,
+				     const char *key, int *is_bool,
+				     const char **dest);
 
 /**
  * Querying For Specific Variables
@@ -607,6 +621,9 @@ int git_config_get_maybe_bool(const char *key, int *dest);
  */
 int git_config_get_pathname(const char *key, const char **dest);
 
+int git_config_get_bool_or_pathname(const char *key, int *is_bool,
+				    const char **dest);
+
 int git_config_get_index_threads(int *dest);
 int git_config_get_split_index(void);
 int git_config_get_max_percent_split_change(void);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 388c28062c2..39cf132e9a0 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -289,11 +289,9 @@ test_expect_success 'init with init.templateDir=[bool]' '
 	(
 		no_templatedir_env &&
 
-		cat >expect <<-\EOF &&
-		warning: templates not found in false
-		EOF
-		git -c init.templateDir=false init repo 2>actual &&
-		test_cmp expect actual
+		git -c init.templateDir=false init repo 2>err &&
+		test_must_be_empty err &&
+		test_path_is_missing repo/.git/description
 	)
 '
 
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 07/13] test-lib: create test data with "git init --no-template" (almost)
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (5 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 06/13] init & clone: add init.templateDir=[bool] Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 08/13] tests: don't depend on template-created .git/branches Ævar Arnfjörð Bjarmason
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Change "t/test-lib.sh" to set the newly added
"GIT_NO_TEMPLATE_DIR=true" option in the environment, which makes any
"git init" invocation take the equivalent of a "--no-template" option,
unless explicitly overridden by config or CLI option.

Well, "almost". We're also adding a test-only
"GIT_TEST_BARE_TEMPLATE=true", when it's set we'll interpret
"GIT_NO_TEMPLATE_DIR=true" as meaning no template, except for empty
"hooks" and "branches" directories.

This is because we've added various implicit dependencies on having
these created by "git init". In subsequent commits we'll address some
of those. More on that below.

The code being added here in "builtin/init-db.c" doesn't require
careful review, since this lazy_mkdir_strbuf_or_die_setlen() function
and the "no_template" condition will be gone in a few commits as we
fix up a few tests to create their own "hooks" and "branches"
directories.

This change reduces the I/O the test suite generates by quite a
bit. Before this running it with --debug (so that trash is retained)
results in ~770MB of trash* (according to "du -shc"), after it's
reduced to around 590M, so almost 1/4 bytes we wrote were the same
sample hooks and other repetitive data. The number of files created
went down from ~151k to ~110k, which around the same reduction
of (more than) 1/4.

This will also make our tests more reliable as we're now forced to
check whether our software works with any arbitrary --template that
may be in use in the wild, as opposed to our relatively "fat" current
default.

On the "more on that below", these are the directories we're
creating (only in the test suite) for now:

- "hooks": Removing it will be relatively simple, mostly a matter of a
  few tests needing a "mkdir .git/hooks", or the tests assume that
  they'll need to explicitly disable .git/hooks.

- "branches": Similarly trivial. The last attempt to get rid of it was
  aborted in c8a58ac5a52 (Revert "Don't create the $GIT_DIR/branches
  directory on init", 2009-10-31), but only a couple of tests files
  depend on it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/init-db.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 t/t0001-init.sh   | 19 ++++++++-----------
 t/test-lib.sh     |  6 ++++--
 wrap-for-bin.sh   |  4 +---
 4 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index fcf538193c8..8dddb47bdc4 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -93,10 +93,36 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
+static void lazy_mkdir_strbuf_or_die_setlen(struct strbuf *path, size_t oldlen,
+					    const char *dir)
+{
+	strbuf_addstr(path, dir);
+	if (mkdir(path->buf, 0777) < 0) {
+		int saved_errno = errno;
+		struct stat st;
+
+		/*
+		 * Unfortunately there's no EEXIST_{DIR,FILE}, and
+		 * we'd like to pass these only if the path is already
+		 * what we want it to be, not if it's a normal.
+		 */
+		if (lstat(path->buf, &st))
+			die_errno(_("cannot stat '%s'"), path->buf);
+		else if (S_ISDIR(st.st_mode))
+			goto cleanup;
+
+		errno = saved_errno;
+		die_errno(_("cannot mkdir '%s'"), path->buf);
+	}
+cleanup:
+	strbuf_setlen(path, oldlen);
+}
+
 static void copy_templates(int no_template, const char *template_dir,
 			   const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
+	size_t len;
 	struct strbuf template_path = STRBUF_INIT;
 	size_t template_len;
 	struct repository_format template_format = REPOSITORY_FORMAT_INIT;
@@ -108,7 +134,7 @@ static void copy_templates(int no_template, const char *template_dir,
 		return;
 	if (!template_dir && !init_template_dir &&
 	    git_env_bool(GIT_NO_TEMPLATE_DIR_ENVIRONMENT, 0))
-		return;
+		goto no_template;
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
@@ -157,6 +183,19 @@ static void copy_templates(int no_template, const char *template_dir,
 	strbuf_release(&path);
 	strbuf_release(&template_path);
 	clear_repository_format(&template_format);
+	return;
+no_template:
+	if (!git_env_bool("GIT_TEST_BARE_TEMPLATE", 0))
+		return;
+
+	strbuf_addstr(&path, get_git_common_dir());
+	strbuf_complete(&path, '/');
+	len = path.len;
+
+	lazy_mkdir_strbuf_or_die_setlen(&path, len, "hooks");
+	lazy_mkdir_strbuf_or_die_setlen(&path, len, "branches");
+
+	strbuf_release(&path);
 }
 
 /*
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 39cf132e9a0..a91de28aadc 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -235,7 +235,7 @@ test_expect_success 'init with --template' '
 
 test_expect_success 'init with --template (blank)' '
 	git init template-plain &&
-	test_path_is_file template-plain/.git/info/exclude &&
+	test_path_is_dir template-plain/.git/info &&
 	git init --template= template-blank &&
 	test_path_is_missing template-blank/.git/info/exclude
 '
@@ -246,19 +246,12 @@ no_templatedir_env () {
 	export NO_SET_GIT_TEMPLATE_DIR
 }
 
-init_no_templatedir_env () {
-	(
-		no_templatedir_env &&
-		git init "$1"
-	)
-}
-
 test_expect_success 'init with init.templatedir set' '
 	mkdir templatedir-source &&
 	echo Content >templatedir-source/file &&
 	test_config_global init.templatedir "${HOME}/templatedir-source" &&
 
-	init_no_templatedir_env templatedir-set &&
+	git init templatedir-set &&
 	test_cmp templatedir-source/file templatedir-set/.git/file
 '
 
@@ -267,7 +260,7 @@ test_expect_success 'init with init.templatedir using ~ expansion' '
 	echo Content >templatedir-source/file &&
 	test_config_global init.templatedir "~/templatedir-source" &&
 
-	init_no_templatedir_env templatedir-expansion &&
+	git init templatedir-expansion &&
 	test_cmp templatedir-source/file templatedir-expansion/.git/file
 '
 
@@ -563,15 +556,19 @@ test_expect_success 'remote init from does not use config from cwd' '
 '
 
 test_expect_success 're-init from a linked worktree' '
-	git init main-worktree &&
 	(
+		git init main-worktree &&
+
 		cd main-worktree &&
 		test_commit first &&
 		git worktree add ../linked-worktree &&
+		>empty &&
+		cp empty .git/info/exclude &&
 		mv .git/info/exclude expected-exclude &&
 		cp .git/config expected-config &&
 		find .git/worktrees -print | sort >expected &&
 		git -C ../linked-worktree init &&
+		cp empty .git/info/exclude &&
 		test_cmp expected-exclude .git/info/exclude &&
 		test_cmp expected-config .git/config &&
 		find .git/worktrees -print | sort >actual &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 57efcc5e97a..bd09d691da3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1355,11 +1355,13 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:
 		PATH="$GIT_BUILD_DIR:$GIT_BUILD_DIR/t/helper:$PATH"
 	fi
 fi
-GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
+GIT_NO_TEMPLATE_DIR=true
+GIT_TEST_BARE_TEMPLATE=true
+export GIT_NO_TEMPLATE_DIR GIT_TEST_BARE_TEMPLATE
 GIT_CONFIG_NOSYSTEM=1
 GIT_ATTR_NOSYSTEM=1
 GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/.."
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM GIT_CEILING_DIRECTORIES
+export PATH GIT_EXEC_PATH GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM GIT_CEILING_DIRECTORIES
 
 if test -z "$GIT_TEST_CMP"
 then
diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index 95851b85b6b..26efe0b60ac 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -7,10 +7,8 @@
 # @@BUILD_DIR@@ and @@PROG@@.
 
 GIT_EXEC_PATH='@@BUILD_DIR@@'
-if test -n "$NO_SET_GIT_TEMPLATE_DIR"
+if test -z "$GIT_TEMPLATE_DIR" && test -z "$GIT_NO_TEMPLATE_DIR"
 then
-	unset GIT_TEMPLATE_DIR
-else
 	GIT_TEMPLATE_DIR='@@BUILD_DIR@@/templates/blt'
 	export GIT_TEMPLATE_DIR
 fi
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 08/13] tests: don't depend on template-created .git/branches
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (6 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 07/13] test-lib: create test data with "git init --no-template" (almost) Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 09/13] t5540: don't rely on "hook/post-update.sample" Ævar Arnfjörð Bjarmason
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

As noted in c8a58ac5a52 (Revert "Don't create the $GIT_DIR/branches
directory on init", 2009-10-31) there was an attempt long ago in
0cc5691a8b0 (Don't create the $GIT_DIR/branches directory on init,
2009-10-30) to get rid of the legacy "branches" directory.

We should probably get rid of its creation by removing the
"templates/branches--" file, but whatever we do with that we don't
need to be creating it in the templates that drive our own tests.

By removing this dependency it'll be more obvious what tests depend on
the existence of ".git/branches", and we can remove the first of the
three special-cases added to copy_template() in a preceding commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/init-db.c     | 1 -
 t/t5505-remote.sh     | 2 ++
 t/t5516-fetch-push.sh | 3 +++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 8dddb47bdc4..3700a6b854e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -193,7 +193,6 @@ static void copy_templates(int no_template, const char *template_dir,
 	len = path.len;
 
 	lazy_mkdir_strbuf_or_die_setlen(&path, len, "hooks");
-	lazy_mkdir_strbuf_or_die_setlen(&path, len, "branches");
 
 	strbuf_release(&path);
 }
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e6e3c8f552c..3bd8ec05802 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -929,6 +929,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 	(
 		cd six &&
 		git remote rm origin &&
+		mkdir .git/branches &&
 		echo "$origin_url#main" >.git/branches/origin &&
 		git remote rename origin origin &&
 		test_path_is_missing .git/branches/origin &&
@@ -943,6 +944,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches (2)'
 	(
 		cd seven &&
 		git remote rm origin &&
+		mkdir .git/branches &&
 		echo "quux#foom" > .git/branches/origin &&
 		git remote rename origin origin &&
 		test_path_is_missing .git/branches/origin &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7831a38ddef..eea191d042f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -921,6 +921,7 @@ test_expect_success 'fetch with branches' '
 	mk_empty testrepo &&
 	git branch second $the_first_commit &&
 	git checkout second &&
+	mkdir testrepo/.git/branches &&
 	echo ".." > testrepo/.git/branches/branch1 &&
 	(
 		cd testrepo &&
@@ -934,6 +935,7 @@ test_expect_success 'fetch with branches' '
 
 test_expect_success 'fetch with branches containing #' '
 	mk_empty testrepo &&
+	mkdir testrepo/.git/branches &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
 	(
 		cd testrepo &&
@@ -948,6 +950,7 @@ test_expect_success 'fetch with branches containing #' '
 test_expect_success 'push with branches' '
 	mk_empty testrepo &&
 	git checkout second &&
+	mkdir .git/branches &&
 	echo "testrepo" > .git/branches/branch1 &&
 	git push branch1 &&
 	(
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 09/13] t5540: don't rely on "hook/post-update.sample"
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (7 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 08/13] tests: don't depend on template-created .git/branches Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Change code added in a87679339c0 (test: rename http fetch and push
test files, 2014-02-06) to stop relying on the "exec git
update-server-info" in "templates/hooks--post-update.sample", let's
instead inline the expected hook in the test itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5540-http-push-webdav.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index 8b68bb38a44..8b1f76fb3b8 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -36,7 +36,9 @@ test_expect_success 'setup remote repository' '
 	git clone --bare test_repo test_repo.git &&
 	cd test_repo.git &&
 	git --bare update-server-info &&
-	mv hooks/post-update.sample hooks/post-update &&
+	write_script "hooks/post-update" <<-\EOF &&
+	exec git update-server-info
+	EOF
 	ORIG_HEAD=$(git rev-parse --verify HEAD) &&
 	cd - &&
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (8 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 09/13] t5540: don't rely on "hook/post-update.sample" Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-13 14:15   ` Eric Sunshine
  2021-12-12 20:13 ` [PATCH 11/13] tests: change "cat && chmod +x" to use "write_hook" Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Add a "write_hook" wrapper for the common case of "write_script
.git/hooks/<NAME>". This also accepts a "-C" option like
"test_commit". Let's convert various trivial cases of "write_script"
over to it.

For now this doesn't have much of an advantage, but in a subsequent
commit we'll implicitly create the leading "hooks" directory. This
will help us get rid of our --template dependency for "hooks".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1416-ref-transaction-hooks.sh    | 14 +++++++-------
 t/t5406-remote-rejects.sh           |  2 +-
 t/t5409-colorize-remote-messages.sh |  2 +-
 t/t5411-proc-receive-hook.sh        |  4 ++--
 t/t5510-fetch.sh                    |  6 +++---
 t/t5516-fetch-push.sh               |  4 ++--
 t/t5521-pull-options.sh             |  4 ++--
 t/t5547-push-quarantine.sh          |  4 ++--
 t/t5548-push-porcelain.sh           |  2 +-
 t/t6500-gc.sh                       |  4 ++--
 t/test-lib-functions.sh             | 26 ++++++++++++++++++++++++++
 11 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 6c941027a81..4532c90274b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -18,7 +18,7 @@ test_expect_success setup '
 test_expect_success 'hook allows updating ref if successful' '
 	test_when_finished "rm .git/hooks/reference-transaction" &&
 	git reset --hard PRE &&
-	write_script .git/hooks/reference-transaction <<-\EOF &&
+	write_hook reference-transaction <<-\EOF &&
 		echo "$*" >>actual
 	EOF
 	cat >expect <<-EOF &&
@@ -32,7 +32,7 @@ test_expect_success 'hook allows updating ref if successful' '
 test_expect_success 'hook aborts updating ref in prepared state' '
 	test_when_finished "rm .git/hooks/reference-transaction" &&
 	git reset --hard PRE &&
-	write_script .git/hooks/reference-transaction <<-\EOF &&
+	write_hook reference-transaction <<-\EOF &&
 		if test "$1" = prepared
 		then
 			exit 1
@@ -45,7 +45,7 @@ test_expect_success 'hook aborts updating ref in prepared state' '
 test_expect_success 'hook gets all queued updates in prepared state' '
 	test_when_finished "rm .git/hooks/reference-transaction actual" &&
 	git reset --hard PRE &&
-	write_script .git/hooks/reference-transaction <<-\EOF &&
+	write_hook reference-transaction <<-\EOF &&
 		if test "$1" = prepared
 		then
 			while read -r line
@@ -68,7 +68,7 @@ test_expect_success 'hook gets all queued updates in prepared state' '
 test_expect_success 'hook gets all queued updates in committed state' '
 	test_when_finished "rm .git/hooks/reference-transaction actual" &&
 	git reset --hard PRE &&
-	write_script .git/hooks/reference-transaction <<-\EOF &&
+	write_hook reference-transaction <<-\EOF &&
 		if test "$1" = committed
 		then
 			while read -r line
@@ -88,7 +88,7 @@ test_expect_success 'hook gets all queued updates in committed state' '
 test_expect_success 'hook gets all queued updates in aborted state' '
 	test_when_finished "rm .git/hooks/reference-transaction actual" &&
 	git reset --hard PRE &&
-	write_script .git/hooks/reference-transaction <<-\EOF &&
+	write_hook reference-transaction <<-\EOF &&
 		if test "$1" = aborted
 		then
 			while read -r line
@@ -115,11 +115,11 @@ test_expect_success 'interleaving hook calls succeed' '
 
 	git init --bare target-repo.git &&
 
-	write_script target-repo.git/hooks/reference-transaction <<-\EOF &&
+	write_hook -C target-repo.git reference-transaction <<-\EOF &&
 		echo $0 "$@" >>actual
 	EOF
 
-	write_script target-repo.git/hooks/update <<-\EOF &&
+	write_hook -C target-repo.git update <<-\EOF &&
 		echo $0 "$@" >>actual
 	EOF
 
diff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh
index 5c509db6fc3..c49aeea272c 100755
--- a/t/t5406-remote-rejects.sh
+++ b/t/t5406-remote-rejects.sh
@@ -5,7 +5,7 @@ test_description='remote push rejects are reported by client'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	write_script .git/hooks/update <<-\EOF &&
+	write_hook update <<-\EOF &&
 	exit 1
 	EOF
 	echo 1 >file &&
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 9f1a483f426..1f7cad31442 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -5,7 +5,7 @@ test_description='remote messages are colorized on the client'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	write_script .git/hooks/update <<-\EOF &&
+	write_hook update <<-\EOF &&
 	echo error: error
 	echo ERROR: also highlighted
 	echo hint: hint
diff --git a/t/t5411-proc-receive-hook.sh b/t/t5411-proc-receive-hook.sh
index 98b0e812082..054f4b2efb3 100755
--- a/t/t5411-proc-receive-hook.sh
+++ b/t/t5411-proc-receive-hook.sh
@@ -36,7 +36,7 @@ setup_upstream_and_workbench () {
 		TAG=$(git -C workbench rev-parse v123) &&
 
 		# setup pre-receive hook
-		write_script upstream.git/hooks/pre-receive <<-\EOF &&
+		write_hook -C upstream.git pre-receive <<-\EOF &&
 		exec >&2
 		echo "# pre-receive hook"
 		while read old new ref
@@ -46,7 +46,7 @@ setup_upstream_and_workbench () {
 		EOF
 
 		# setup post-receive hook
-		write_script upstream.git/hooks/post-receive <<-\EOF &&
+		write_hook -C upstream.git post-receive <<-\EOF &&
 		exec >&2
 		echo "# post-receive hook"
 		while read old new ref
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index a0faf0dd949..cdce39e6c76 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -265,7 +265,7 @@ test_expect_success 'fetch --atomic executes a single reference transaction only
 	EOF
 
 	rm -f atomic/actual &&
-	write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+	write_hook -C atomic reference-transaction <<-\EOF &&
 		( echo "$*" && cat ) >>actual
 	EOF
 
@@ -298,7 +298,7 @@ test_expect_success 'fetch --atomic aborts all reference updates if hook aborts'
 	EOF
 
 	rm -f atomic/actual &&
-	write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+	write_hook -C atomic/.git reference-transaction <<-\EOF &&
 		( echo "$*" && cat ) >>actual
 		exit 1
 	EOF
@@ -326,7 +326,7 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
 	test_line_count = 2 atomic/.git/FETCH_HEAD &&
 	cp atomic/.git/FETCH_HEAD expected &&
 
-	write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+	write_hook -C atomic reference-transaction <<-\EOF &&
 		exit 1
 	EOF
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index eea191d042f..11458052cb4 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1680,7 +1680,7 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 		git reset --hard HEAD^^ &&
 		git tag initial &&
 		git config receive.denyCurrentBranch updateInstead &&
-		write_script .git/hooks/push-to-checkout <<-\EOF
+		write_hook push-to-checkout <<-\EOF
 		echo >&2 updating from $(git rev-parse HEAD)
 		echo >&2 updating to "$1"
 
@@ -1739,7 +1739,7 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	(
 		cd void &&
 		git config receive.denyCurrentBranch updateInstead &&
-		write_script .git/hooks/push-to-checkout <<-\EOF
+		write_hook push-to-checkout <<-\EOF
 		if git rev-parse --quiet --verify HEAD
 		then
 			has_head=yes
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 66cfcb09c55..9b30e90383c 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -233,7 +233,7 @@ test_expect_success 'git pull --no-verify flag passed to merge' '
 	git init src &&
 	test_commit -C src one &&
 	git clone src dst &&
-	write_script dst/.git/hooks/commit-msg <<-\EOF &&
+	write_hook -C dst commit-msg <<-\EOF &&
 	false
 	EOF
 	test_commit -C src two &&
@@ -245,7 +245,7 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
 	git init src &&
 	test_commit -C src one &&
 	git clone src dst &&
-	write_script dst/.git/hooks/commit-msg <<-\EOF &&
+	write_hook -C dst commit-msg <<-\EOF &&
 	false
 	EOF
 	test_commit -C src two &&
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index faaa51ccc56..e0bd469b110 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -5,7 +5,7 @@ test_description='check quarantine of objects during push'
 
 test_expect_success 'create picky dest repo' '
 	git init --bare dest.git &&
-	write_script dest.git/hooks/pre-receive <<-\EOF
+	write_hook -C dest.git pre-receive <<-\EOF
 	while read old new ref; do
 		test "$(git log -1 --format=%s $new)" = reject && exit 1
 	done
@@ -60,7 +60,7 @@ test_expect_success 'push to repo path with path separator (colon)' '
 
 test_expect_success 'updating a ref from quarantine is forbidden' '
 	git init --bare update.git &&
-	write_script update.git/hooks/pre-receive <<-\EOF &&
+	write_hook -C update.git pre-receive <<-\EOF &&
 	read old new refname
 	git update-ref refs/heads/unrelated $new
 	exit 1
diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh
index f11ff57e549..8997433a992 100755
--- a/t/t5548-push-porcelain.sh
+++ b/t/t5548-push-porcelain.sh
@@ -168,7 +168,7 @@ run_git_push_porcelain_output_test() {
 	'
 
 	test_expect_success "prepare pre-receive hook ($PROTOCOL)" '
-		write_script "$upstream/hooks/pre-receive" <<-EOF
+		write_hook -C "$upstream" pre-receive <<-EOF
 		exit 1
 		EOF
 	'
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index c2021267f2c..98cf395806b 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -103,7 +103,7 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' '
 	git init pre-auto-gc-hook &&
 	(
 		cd pre-auto-gc-hook &&
-		write_script ".git/hooks/pre-auto-gc" <<-\EOF &&
+		write_hook pre-auto-gc <<-\EOF &&
 		echo >&2 no gc for you &&
 		exit 1
 		EOF
@@ -130,7 +130,7 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' '
 
 	(
 		cd pre-auto-gc-hook &&
-		write_script ".git/hooks/pre-auto-gc" <<-\EOF &&
+		write_hook pre-auto-gc <<-\EOF &&
 		echo >&2 will gc for you &&
 		exit 0
 		EOF
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index eef2262a360..3786d39ccab 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -551,6 +551,32 @@ write_script () {
 	chmod +x "$1"
 }
 
+## Usage: write-hook pre-receive
+## Usage: write-hook -C some-dir pre-receive
+write_hook () {
+	indir= &&
+	while test $# != 0
+	do
+		case "$1" in
+		-C)
+			indir="$2"
+			shift
+			;;
+		-*)
+			BUG "invalid write_hook: $1"
+			;;
+		*)
+			break
+			;;
+		esac &&
+		shift
+	done &&
+	git_dir=$(git -C "$indir" rev-parse --absolute-git-dir) &&
+	hook_dir="$git_dir/hooks" &&
+	hook_file="$hook_dir/$1"
+	write_script "$hook_file"
+}
+
 # Use test_set_prereq to tell that a particular prerequisite is available.
 # The prerequisite can later be checked for in two ways:
 #
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 11/13] tests: change "cat && chmod +x" to use "write_hook"
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (9 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 12/13] tests: migrate miscellaneous "write_script" to "write_hooks" Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 13/13] tests: don't depend on template-created .git/hooks Ævar Arnfjörð Bjarmason
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Refactor various test code to use the "write_hook()" helper. Let's
indent this code and add it to "test_expect_success" while we're at
it.

As in a preceding commit some of this code drops the explicit "mkdir
-p", but as noted we'll be having the "write_hook" wrapper handle that
soon anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3412-rebase-root.sh       | 18 ++++-------
 t/t3413-rebase-hook.sh       | 18 ++++-------
 t/t5401-update-hooks.sh      | 62 ++++++++++++++++--------------------
 t/t5402-post-merge-hook.sh   | 16 ++++++----
 t/t5407-post-rewrite-hook.sh | 14 ++++----
 t/t5516-fetch-push.sh        | 15 +++------
 t/t5541-http-push-smart.sh   |  4 +--
 t/t5601-clone.sh             |  4 +--
 8 files changed, 60 insertions(+), 91 deletions(-)

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 19c6f4acbf6..c2dfb562961 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -31,12 +31,9 @@ test_expect_success 'rebase --root fails with too many args' '
 '
 
 test_expect_success 'setup pre-rebase hook' '
-	mkdir -p .git/hooks &&
-	cat >.git/hooks/pre-rebase <<EOF &&
-#!$SHELL_PATH
-echo "\$1,\$2" >.git/PRE-REBASE-INPUT
-EOF
-	chmod +x .git/hooks/pre-rebase
+	write_hook pre-rebase <<-\EOF
+	echo "$1,$2" >.git/PRE-REBASE-INPUT
+	EOF
 '
 cat > expect <<EOF
 4
@@ -141,12 +138,9 @@ commit work7~5
 EOF
 
 test_expect_success 'setup pre-rebase hook that fails' '
-	mkdir -p .git/hooks &&
-	cat >.git/hooks/pre-rebase <<EOF &&
-#!$SHELL_PATH
-false
-EOF
-	chmod +x .git/hooks/pre-rebase
+	write_hook pre-rebase <<-\EOF
+	false
+	EOF
 '
 
 test_expect_success 'pre-rebase hook stops rebase' '
diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
index b4acb3be5cf..100157aa0ab 100755
--- a/t/t3413-rebase-hook.sh
+++ b/t/t3413-rebase-hook.sh
@@ -41,12 +41,9 @@ test_expect_success 'rebase -i' '
 '
 
 test_expect_success 'setup pre-rebase hook' '
-	mkdir -p .git/hooks &&
-	cat >.git/hooks/pre-rebase <<EOF &&
-#!$SHELL_PATH
-echo "\$1,\$2" >.git/PRE-REBASE-INPUT
-EOF
-	chmod +x .git/hooks/pre-rebase
+	write_hook pre-rebase <<-\EOF
+	echo "$1,$2" >.git/PRE-REBASE-INPUT
+	EOF
 '
 
 test_expect_success 'pre-rebase hook gets correct input (1)' '
@@ -102,12 +99,9 @@ test_expect_success 'pre-rebase hook gets correct input (6)' '
 '
 
 test_expect_success 'setup pre-rebase hook that fails' '
-	mkdir -p .git/hooks &&
-	cat >.git/hooks/pre-rebase <<EOF &&
-#!$SHELL_PATH
-false
-EOF
-	chmod +x .git/hooks/pre-rebase
+	write_hook pre-rebase <<-\EOF
+	false
+	EOF
 '
 
 test_expect_success 'pre-rebase hook stops rebase (1)' '
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 6012cc8172a..1bd656d35ce 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -20,45 +20,37 @@ test_expect_success setup '
 	git clone --bare ./. victim.git &&
 	GIT_DIR=victim.git git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/main $commit1 &&
-	git update-ref refs/heads/tofail $commit0
-'
+	git update-ref refs/heads/tofail $commit0 &&
 
-cat >victim.git/hooks/pre-receive <<'EOF'
-#!/bin/sh
-printf %s "$@" >>$GIT_DIR/pre-receive.args
-cat - >$GIT_DIR/pre-receive.stdin
-echo STDOUT pre-receive
-echo STDERR pre-receive >&2
-EOF
-chmod u+x victim.git/hooks/pre-receive
+	write_hook -C victim.git pre-receive <<-\EOF &&
+	printf %s "$@" >>$GIT_DIR/pre-receive.args
+	cat - >$GIT_DIR/pre-receive.stdin
+	echo STDOUT pre-receive
+	echo STDERR pre-receive >&2
+	EOF
 
-cat >victim.git/hooks/update <<'EOF'
-#!/bin/sh
-echo "$@" >>$GIT_DIR/update.args
-read x; printf %s "$x" >$GIT_DIR/update.stdin
-echo STDOUT update $1
-echo STDERR update $1 >&2
-test "$1" = refs/heads/main || exit
-EOF
-chmod u+x victim.git/hooks/update
+	write_hook -C victim.git update <<-\EOF &&
+	echo "$@" >>$GIT_DIR/update.args
+	read x; printf %s "$x" >$GIT_DIR/update.stdin
+	echo STDOUT update $1
+	echo STDERR update $1 >&2
+	test "$1" = refs/heads/main || exit
+	EOF
 
-cat >victim.git/hooks/post-receive <<'EOF'
-#!/bin/sh
-printf %s "$@" >>$GIT_DIR/post-receive.args
-cat - >$GIT_DIR/post-receive.stdin
-echo STDOUT post-receive
-echo STDERR post-receive >&2
-EOF
-chmod u+x victim.git/hooks/post-receive
+	write_hook -C victim.git post-receive <<-\EOF &&
+	printf %s "$@" >>$GIT_DIR/post-receive.args
+	cat - >$GIT_DIR/post-receive.stdin
+	echo STDOUT post-receive
+	echo STDERR post-receive >&2
+	EOF
 
-cat >victim.git/hooks/post-update <<'EOF'
-#!/bin/sh
-echo "$@" >>$GIT_DIR/post-update.args
-read x; printf %s "$x" >$GIT_DIR/post-update.stdin
-echo STDOUT post-update
-echo STDERR post-update >&2
-EOF
-chmod u+x victim.git/hooks/post-update
+	write_hook -C victim.git post-update <<-\EOF
+	echo "$@" >>$GIT_DIR/post-update.args
+	read x; printf %s "$x" >$GIT_DIR/post-update.stdin
+	echo STDOUT post-update
+	echo STDERR post-update >&2
+	EOF
+'
 
 test_expect_success push '
 	test_must_fail git send-pack --force ./victim.git \
diff --git a/t/t5402-post-merge-hook.sh b/t/t5402-post-merge-hook.sh
index 3e5e19c7191..c425a807efe 100755
--- a/t/t5402-post-merge-hook.sh
+++ b/t/t5402-post-merge-hook.sh
@@ -25,13 +25,15 @@ test_expect_success setup '
 	GIT_DIR=clone2/.git git update-index --add a
 '
 
-for clone in 1 2; do
-	cat >clone${clone}/.git/hooks/post-merge <<'EOF'
-#!/bin/sh
-echo $@ >> $GIT_DIR/post-merge.args
-EOF
-	chmod u+x clone${clone}/.git/hooks/post-merge
-done
+test_expect_success 'setup clone hooks' '
+	test_when_finished "rm -f hook" &&
+	cat >hook <<-\EOF &&
+	echo $@ >> $GIT_DIR/post-merge.args
+	EOF
+
+	write_hook -C clone1 post-merge <hook &&
+	write_hook -C clone2 post-merge <hook
+'
 
 test_expect_success 'post-merge does not run for up-to-date ' '
 	GIT_DIR=clone1/.git git merge $commit0 &&
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 6da8d760e28..7a2bf55d8e2 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -17,15 +17,13 @@ test_expect_success 'setup' '
 	git checkout A^0 &&
 	test_commit E bar E &&
 	test_commit F foo F &&
-	git checkout main
-'
+	git checkout main &&
 
-cat >.git/hooks/post-rewrite <<EOF
-#!/bin/sh
-echo \$@ > "$TRASH_DIRECTORY"/post-rewrite.args
-cat > "$TRASH_DIRECTORY"/post-rewrite.data
-EOF
-chmod u+x .git/hooks/post-rewrite
+	write_hook post-rewrite <<-EOF
+	echo \$@ > "$TRASH_DIRECTORY"/post-rewrite.args
+	cat > "$TRASH_DIRECTORY"/post-rewrite.data
+	EOF
+'
 
 clear_hook_input () {
 	rm -f post-rewrite.args post-rewrite.data
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 11458052cb4..fd355ae48c6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -62,32 +62,25 @@ mk_test_with_hooks() {
 	(
 		cd "$repo_name" &&
 		mkdir .git/hooks &&
-		cd .git/hooks &&
 
-		cat >pre-receive <<-'EOF' &&
-		#!/bin/sh
+		write_hook pre-receive <<-'EOF' &&
 		cat - >>pre-receive.actual
 		EOF
 
-		cat >update <<-'EOF' &&
-		#!/bin/sh
+		write_hook update <<-'EOF' &&
 		printf "%s %s %s\n" "$@" >>update.actual
 		EOF
 
-		cat >post-receive <<-'EOF' &&
-		#!/bin/sh
+		write_hook post-receive <<-'EOF' &&
 		cat - >>post-receive.actual
 		EOF
 
-		cat >post-update <<-'EOF' &&
-		#!/bin/sh
+		write_hook post-update <<-'EOF'
 		for ref in "$@"
 		do
 			printf "%s\n" "$ref" >>post-update.actual
 		done
 		EOF
-
-		chmod +x pre-receive update post-receive post-update
 	)
 }
 
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ca50f8b18c..11da2325c6d 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -96,11 +96,9 @@ test_expect_success 'create and delete remote branch' '
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '
 
-cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" <<EOF
-#!/bin/sh
+write_hook -C "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" update <<\EOF
 exit 1
 EOF
-chmod a+x "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
 
 cat >exp <<EOF
 remote: error: hook declined to update refs/heads/dev2
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 83c24fc97a7..6fd041e7d3e 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -79,12 +79,10 @@ test_expect_success 'clone from hooks' '
 	cd .. &&
 	git init r1 &&
 	cd r1 &&
-	cat >.git/hooks/pre-commit <<-\EOF &&
-	#!/bin/sh
+	write_hook pre-commit <<-\EOF &&
 	git clone ../r0 ../r2
 	exit 1
 	EOF
-	chmod u+x .git/hooks/pre-commit &&
 	: >file &&
 	git add file &&
 	test_must_fail git commit -m invoke-hook &&
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 12/13] tests: migrate miscellaneous "write_script" to "write_hooks"
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (10 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 11/13] tests: change "cat && chmod +x" to use "write_hook" Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  2021-12-12 20:13 ` [PATCH 13/13] tests: don't depend on template-created .git/hooks Ævar Arnfjörð Bjarmason
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3430-rebase-merges.sh    | 5 +++--
 t/t5540-http-push-webdav.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 43c82d9a33b..8003e41427c 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -292,9 +292,10 @@ test_expect_success 'post-rewrite hook and fixups work for merges' '
 	git commit --fixup HEAD same2.t &&
 	fixup="$(git rev-parse HEAD)" &&
 
-	mkdir -p .git/hooks &&
 	test_when_finished "rm .git/hooks/post-rewrite" &&
-	echo "cat >actual" | write_script .git/hooks/post-rewrite &&
+	write_hook post-rewrite <<-\EOF &&
+	cat >actual
+	EOF
 
 	test_tick &&
 	git rebase -i --autosquash -r HEAD^^^ &&
diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index 8b1f76fb3b8..31d13547b26 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -36,7 +36,7 @@ test_expect_success 'setup remote repository' '
 	git clone --bare test_repo test_repo.git &&
 	cd test_repo.git &&
 	git --bare update-server-info &&
-	write_script "hooks/post-update" <<-\EOF &&
+	write_hook post-update <<-\EOF &&
 	exec git update-server-info
 	EOF
 	ORIG_HEAD=$(git rev-parse --verify HEAD) &&
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 13/13] tests: don't depend on template-created .git/hooks
  2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
                   ` (11 preceding siblings ...)
  2021-12-12 20:13 ` [PATCH 12/13] tests: migrate miscellaneous "write_script" to "write_hooks" Ævar Arnfjörð Bjarmason
@ 2021-12-12 20:13 ` Ævar Arnfjörð Bjarmason
  12 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-12 20:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Change the "write_hook" wrapper to implicitly "mkdir -p
.git/hooks" (or equivalent), and stop having copy_templates() make the
"hooks" directory. One test in "t5516-fetch-push.sh" won't need to
move our hooks out of the way anymore.

As with a preceding change to drop the dependency on the
template-created "branches" we can now stop depending on the template
having created the "hooks" directory for us.

Since this was the last special-case handled by the
"lazy_mkdir_strbuf_or_die_setlen()" function added earlier in this
series we can remove that special-case and the
"GIT_TEST_BARE_TEMPLATE" handling.

The choice to not use "mkdir -p" in "write_hook" is deliberate. We're
being a bit stricter in not potentially creating N leading
directories, but also not failing on the second "write_hook"
invocation in a repository as a simple "mkdir" without "-p" would.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/init-db.c           | 39 +------------------------------------
 t/t5516-fetch-push.sh       |  4 +---
 t/t7450-bad-git-dotfiles.sh |  1 +
 t/test-lib-functions.sh     |  4 ++++
 t/test-lib.sh               |  3 +--
 5 files changed, 8 insertions(+), 43 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3700a6b854e..0301b8f613e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -93,36 +93,10 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
-static void lazy_mkdir_strbuf_or_die_setlen(struct strbuf *path, size_t oldlen,
-					    const char *dir)
-{
-	strbuf_addstr(path, dir);
-	if (mkdir(path->buf, 0777) < 0) {
-		int saved_errno = errno;
-		struct stat st;
-
-		/*
-		 * Unfortunately there's no EEXIST_{DIR,FILE}, and
-		 * we'd like to pass these only if the path is already
-		 * what we want it to be, not if it's a normal.
-		 */
-		if (lstat(path->buf, &st))
-			die_errno(_("cannot stat '%s'"), path->buf);
-		else if (S_ISDIR(st.st_mode))
-			goto cleanup;
-
-		errno = saved_errno;
-		die_errno(_("cannot mkdir '%s'"), path->buf);
-	}
-cleanup:
-	strbuf_setlen(path, oldlen);
-}
-
 static void copy_templates(int no_template, const char *template_dir,
 			   const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
-	size_t len;
 	struct strbuf template_path = STRBUF_INIT;
 	size_t template_len;
 	struct repository_format template_format = REPOSITORY_FORMAT_INIT;
@@ -134,7 +108,7 @@ static void copy_templates(int no_template, const char *template_dir,
 		return;
 	if (!template_dir && !init_template_dir &&
 	    git_env_bool(GIT_NO_TEMPLATE_DIR_ENVIRONMENT, 0))
-		goto no_template;
+		return;
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
@@ -184,17 +158,6 @@ static void copy_templates(int no_template, const char *template_dir,
 	strbuf_release(&template_path);
 	clear_repository_format(&template_format);
 	return;
-no_template:
-	if (!git_env_bool("GIT_TEST_BARE_TEMPLATE", 0))
-		return;
-
-	strbuf_addstr(&path, get_git_common_dir());
-	strbuf_complete(&path, '/');
-	len = path.len;
-
-	lazy_mkdir_strbuf_or_die_setlen(&path, len, "hooks");
-
-	strbuf_release(&path);
 }
 
 /*
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index fd355ae48c6..20677c84117 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -28,8 +28,7 @@ mk_empty () {
 	(
 		cd "$repo_name" &&
 		git init &&
-		git config receive.denyCurrentBranch warn &&
-		mv .git/hooks .git/hooks-disabled
+		git config receive.denyCurrentBranch warn
 	)
 }
 
@@ -61,7 +60,6 @@ mk_test_with_hooks() {
 	mk_test "$@" &&
 	(
 		cd "$repo_name" &&
-		mkdir .git/hooks &&
 
 		write_hook pre-receive <<-'EOF' &&
 		cat - >>pre-receive.actual
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index 41706c1c9ff..425440d40b7 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -54,6 +54,7 @@ test_expect_success 'add evil submodule' '
 
 	mkdir modules &&
 	cp -r .git/modules/evil modules &&
+	mkdir modules/evil/hooks &&
 	write_script modules/evil/hooks/post-checkout <<-\EOF &&
 	echo >&2 "RUNNING POST CHECKOUT"
 	EOF
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3786d39ccab..75fa312f3e9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -573,6 +573,10 @@ write_hook () {
 	done &&
 	git_dir=$(git -C "$indir" rev-parse --absolute-git-dir) &&
 	hook_dir="$git_dir/hooks" &&
+	if ! test -d "$hook_dir"
+	then
+		mkdir "$hook_dir"
+	fi &&
 	hook_file="$hook_dir/$1"
 	write_script "$hook_file"
 }
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bd09d691da3..3abd51464e6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1356,8 +1356,7 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:
 	fi
 fi
 GIT_NO_TEMPLATE_DIR=true
-GIT_TEST_BARE_TEMPLATE=true
-export GIT_NO_TEMPLATE_DIR GIT_TEST_BARE_TEMPLATE
+export GIT_NO_TEMPLATE_DIR
 GIT_CONFIG_NOSYSTEM=1
 GIT_ATTR_NOSYSTEM=1
 GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/.."
-- 
2.34.1.1020.gb1392dd1877


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-12 20:13 ` [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper Ævar Arnfjörð Bjarmason
@ 2021-12-13 14:15   ` Eric Sunshine
  2021-12-13 16:29     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Eric Sunshine @ 2021-12-13 14:15 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On Sun, Dec 12, 2021 at 4:24 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Add a "write_hook" wrapper for the common case of "write_script
> .git/hooks/<NAME>". This also accepts a "-C" option like
> "test_commit". Let's convert various trivial cases of "write_script"
> over to it.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -551,6 +551,32 @@ write_script () {
> +## Usage: write-hook pre-receive
> +## Usage: write-hook -C some-dir pre-receive
> +write_hook () {
> +       indir= &&
> +       while test $# != 0
> +       do

It's not clear whether the intention is to maintain the &&-chain in
this function...

> +               case "$1" in
> +               -C)
> +                       indir="$2"
> +                       shift
> +                       ;;

... or not care about it since it's broken here before `shift`...

> +               -*)
> +                       BUG "invalid write_hook: $1"
> +                       ;;
> +               *)
> +                       break
> +                       ;;
> +               esac &&
> +               shift
> +       done &&
> +       git_dir=$(git -C "$indir" rev-parse --absolute-git-dir) &&
> +       hook_dir="$git_dir/hooks" &&
> +       hook_file="$hook_dir/$1"
> +       write_script "$hook_file"

... and here before `write_script`.

> +}

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-13 14:15   ` Eric Sunshine
@ 2021-12-13 16:29     ` Ævar Arnfjörð Bjarmason
  2021-12-13 16:45       ` Eric Sunshine
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-13 16:29 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Git List, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 13 2021, Eric Sunshine wrote:

> On Sun, Dec 12, 2021 at 4:24 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Add a "write_hook" wrapper for the common case of "write_script
>> .git/hooks/<NAME>". This also accepts a "-C" option like
>> "test_commit". Let's convert various trivial cases of "write_script"
>> over to it.
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> @@ -551,6 +551,32 @@ write_script () {
>> +## Usage: write-hook pre-receive
>> +## Usage: write-hook -C some-dir pre-receive
>> +write_hook () {
>> +       indir= &&
>> +       while test $# != 0
>> +       do
>
> It's not clear whether the intention is to maintain the &&-chain in
> this function...
>
>> +               case "$1" in
>> +               -C)
>> +                       indir="$2"
>> +                       shift
>> +                       ;;
>
> ... or not care about it since it's broken here before `shift`...
>
>> +               -*)
>> +                       BUG "invalid write_hook: $1"
>> +                       ;;
>> +               *)
>> +                       break
>> +                       ;;
>> +               esac &&
>> +               shift
>> +       done &&
>> +       git_dir=$(git -C "$indir" rev-parse --absolute-git-dir) &&
>> +       hook_dir="$git_dir/hooks" &&
>> +       hook_file="$hook_dir/$1"
>> +       write_script "$hook_file"
>
> ... and here before `write_script`.

Thanks, those should all use &&-chaining. Will fix.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-13 16:29     ` Ævar Arnfjörð Bjarmason
@ 2021-12-13 16:45       ` Eric Sunshine
  2021-12-13 19:37         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Eric Sunshine @ 2021-12-13 16:45 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > It's not clear whether the intention is to maintain the &&-chain in
> > this function...
> > ... or not care about it since it's broken here before `shift`...
>
> Thanks, those should all use &&-chaining. Will fix.

By the way, the new chainlint could be made to catch broken &&-chains
(and missing `|| return 1`) in test script functions, as well; it
doesn't have to limit its checks only to tests. The reason I haven't
done so yet is that it's not clear how much we care about &&-chains in
functions, especially since we have _so many_ functions which don't
maintain the &&-chain. In the long run, I think it might be beneficial
to extend chainlint to check shell functions too, but fixing the
&&-chains in functions probably have to be done incrementally, thus
would likely require some sort of whitelisting or blacklisting
mechanism until all functions have been fixed. Anyhow, it's food for
thought.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-13 16:45       ` Eric Sunshine
@ 2021-12-13 19:37         ` Ævar Arnfjörð Bjarmason
  2021-12-13 21:33           ` Eric Sunshine
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-13 19:37 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Git List, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 13 2021, Eric Sunshine wrote:

> On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Dec 13 2021, Eric Sunshine wrote:
>> > It's not clear whether the intention is to maintain the &&-chain in
>> > this function...
>> > ... or not care about it since it's broken here before `shift`...
>>
>> Thanks, those should all use &&-chaining. Will fix.
>
> By the way, the new chainlint could be made to catch broken &&-chains
> (and missing `|| return 1`) in test script functions, as well; it
> doesn't have to limit its checks only to tests. The reason I haven't
> done so yet is that it's not clear how much we care about &&-chains in
> functions, especially since we have _so many_ functions which don't
> maintain the &&-chain. In the long run, I think it might be beneficial
> to extend chainlint to check shell functions too, but fixing the
> &&-chains in functions probably have to be done incrementally, thus
> would likely require some sort of whitelisting or blacklisting
> mechanism until all functions have been fixed. Anyhow, it's food for
> thought.

I think doing that & phasing it in would be very useful.

E.g. if you run the tests with SANITIZE=leak and
GIT_TEST_PASSING_SANITIZE_LEAK=true in "next" now you'll find it passes,
but we'll still (especially if you log them or --verbose-log) have
memory leaks in various places still.

Those aren't new issues in anything I've done in the leak testing mode,
although I'm hoping to eventually getting around to fixing them. They're
just cases where tests pass because some function is lacking a &&.

Although to be fair the SANITIZE=leak default is to die at the very end,
so if the program had something useful to do it probably got around to
doing it, but that doesn't apply when we invoke ourselves via
run-command.c (as that invocation will fail, and we'll usually abort).

But it would have been nice to have had those failures cascade up from
the functions up to the top-level.

We've also said we shouldn't use things like this, i.e. a pipe with git
on the LHS:

    git <cmd> | ... &&

But I've run into a few cases where a test succeeds, even if both
commands here die:

    test "$(git <cmd>)" = "$(git <cmd2>)"

Which, if we're adding more lints is maybe something to consider
too. I.e. it falls under the general umbrella of cases where we'd hide
failures in "git".


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper
  2021-12-13 19:37         ` Ævar Arnfjörð Bjarmason
@ 2021-12-13 21:33           ` Eric Sunshine
  0 siblings, 0 replies; 33+ messages in thread
From: Eric Sunshine @ 2021-12-13 21:33 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On Mon, Dec 13, 2021 at 2:42 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > By the way, the new chainlint could be made to catch broken &&-chains
> > (and missing `|| return 1`) in test script functions, as well; it
> > doesn't have to limit its checks only to tests. The reason I haven't
> > done so yet is that it's not clear how much we care about &&-chains in
> > functions, especially since we have _so many_ functions which don't
> > maintain the &&-chain. In the long run, I think it might be beneficial
> > to extend chainlint to check shell functions too, but fixing the
> > &&-chains in functions probably have to be done incrementally, thus
> > would likely require some sort of whitelisting or blacklisting
> > mechanism until all functions have been fixed. Anyhow, it's food for
> > thought.
>
> I think doing that & phasing it in would be very useful.

I forgot to mention another reason that I haven't really thought yet
about tackling the linting of shell functions, which is that some
shell functions drive tests:

    test_it () {
        foo=$1
        bar=$2
        test_expect_sucess "something $foo" '
            do_it "$bar"
        '
    }

in which an &&-chain within the function body isn't meaningful,
whereas other functions are called by tests:

    cmp_it () {
        a=$1 &&
        b=$2 &&
        test_cmp "$a" "$b"
    }

    test_expect_success 'something' '
        echo foo >foo &&
        echo bar >bar &&
        cmp_it foo bar
    '

in which the &&-chain within the function body is important.

It _may_ be possible to figure out automatically into which category a
function falls, but would probably be easier to have some sort of
annotation mechanism to distinguish one category of function from the
other, and only validate a function which falls into the latter
category.

> We've also said we shouldn't use things like this, i.e. a pipe with git
> on the LHS:
>
>     git <cmd> | ... &&
>
> But I've run into a few cases where a test succeeds, even if both
> commands here die:
>
>     test "$(git <cmd>)" = "$(git <cmd2>)"
>
> Which, if we're adding more lints is maybe something to consider
> too. I.e. it falls under the general umbrella of cases where we'd hide
> failures in "git".

Ya, this is a nice example, among others, of questionable code which a
linter might be able to detect. In fact, while working on the new
chainlint, I noted that it would be possible to replace
t/check-non-portable-shell.pl by adding a few more rules to the new
linter. A primary benefit of doing so is that
check-non-portable-shell.pl takes about 2.5 seconds to run on my
(admittedly 10+ year old) machine. However, I'm also hesitant to do so
since there is value in having those checks reside in a standalone
script like that; it's such a simple script that anyone can add new
checks without having to spend a lot of time studying how to do so.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-12 20:13 ` [PATCH 03/13] init: unconditionally create the "info" directory Ævar Arnfjörð Bjarmason
@ 2021-12-20 15:59   ` Derrick Stolee
  2021-12-20 16:13     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Derrick Stolee @ 2021-12-20 15:59 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
> In preceding commits the test suite has been taught to run without a
> template directory, but in doing so we needed to fix code that relied
> on the "hooks" and "branches" directories.
> 
> The "hooks" code was all specific to our own test suite. The
> "branches" directory is intentionally created, but has been "slightly
> deprecated" for a while, so it's not created when not using the
> default template.
> 
> However "info" is different. Trying to omit its creation would lead to
> a lot of test suite failures. Many of these we should arguably fix,
> the common pattern being to add an exclude to "info/excludes".

This would be painful to add because of the impact on the test suite.
That I understand.
 
> But we've also grown a hard dependency on this directory within git
> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
> subcommand, 2019-11-21) released with v2.25.0 the "git
> sparse-checkout" command has wanted to add exclusions to
> "info/sparse-checkout". It didn't check or create the leading
> directory, so if it's omitted the command will die.

> Even if that behavior were fixed we'd be left with older versions of
> "git" dying if that was attempted if they used a repository
> initialized without a template.

This, I don't understand. Why can't we add a
safe_create_leading_directories() to any place where we try to
create a sparse-checkout file?

This would fix situations where older versions were init'd with a
different template or if the user deleted the info dir. The change
you've made here doesn't fix those cases, which is what you are
claiming is the reason to not do the other fix that seems like it
would.

What am I misunderstanding here?

> So let's just bite the bullet and make the "info" directory mandatory,
> and document it as such. Let's also note that in the documentation
> that this doesn't apply to the "hooks" and "branches" directories.

I have no objection to this approach, but we should still do the
other thing.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 15:59   ` Derrick Stolee
@ 2021-12-20 16:13     ` Ævar Arnfjörð Bjarmason
  2021-12-20 17:39       ` Derrick Stolee
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-20 16:13 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Derrick Stolee wrote:

> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>> In preceding commits the test suite has been taught to run without a
>> template directory, but in doing so we needed to fix code that relied
>> on the "hooks" and "branches" directories.
>> 
>> The "hooks" code was all specific to our own test suite. The
>> "branches" directory is intentionally created, but has been "slightly
>> deprecated" for a while, so it's not created when not using the
>> default template.
>> 
>> However "info" is different. Trying to omit its creation would lead to
>> a lot of test suite failures. Many of these we should arguably fix,
>> the common pattern being to add an exclude to "info/excludes".
>
> This would be painful to add because of the impact on the test suite.
> That I understand.
>  
>> But we've also grown a hard dependency on this directory within git
>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>> subcommand, 2019-11-21) released with v2.25.0 the "git
>> sparse-checkout" command has wanted to add exclusions to
>> "info/sparse-checkout". It didn't check or create the leading
>> directory, so if it's omitted the command will die.
>
>> Even if that behavior were fixed we'd be left with older versions of
>> "git" dying if that was attempted if they used a repository
>> initialized without a template.
>
> This, I don't understand. Why can't we add a
> safe_create_leading_directories() to any place where we try to
> create a sparse-checkout file?
>
> This would fix situations where older versions were init'd with a
> different template or if the user deleted the info dir. The change
> you've made here doesn't fix those cases, which is what you are
> claiming is the reason to not do the other fix that seems like it
> would.
>
> What am I misunderstanding here?

I'll clarify that a bit in any re-roll.

Pedantically nothing changes, i.e. you can create a repository with an
empty template now, and it'll break on both the sparse-checkout on that
version, and any previous version that had that un-noticed issue.

But in practice I think it wouldn't have been a big deal, because while
you could omit or specify a custom template it was somewhat of a hassle,
and even somewhat undocumented.

Whereas with this series allowing you to easily configure it with
init.templateDir=false it becomes trivial. That was another motivation
of mine for adding this, I'd like to not have N copies of that template
crud all over my systems.

So I think in practice we need to be more conservative about
cross-version interaction here. It's not just a matter of "if", but also
of a new "how" making that "if" more common. I.e. needing to interact
with an empty-template .git directory.

We also have non-git.git code to worry about, e.g. us breaking any
user-custom script that might do:

    #!/bin/sh -e
    git init "$1"
    cd "$1"
    # *Boom* under init.templateDir=false, unless we keep ".git/info"
    echo <my ignores> >.git/info/excludes

So I just don't think it's worth it. Let's just create .git/info
unconditionally like we create .git/objects/{info,pack} now.

It's unrelated to this, but if this gets in I would eventually like to
submit a change to make some version of init.templateDir=false the
default. That's a bit more untandling, but I think ultimately
beneficial. E.g. the sample hooks are documented in "git help githooks"
along with general hook documentation. Such a change would ideally
involve splitting that out (maybe to a a
"gitrepository-sample-hooks(5)"). That's another reason for why I'd like
to make init.templateDir=false play nicely with existing in-tree and
out-of-tree expectations.

>> So let's just bite the bullet and make the "info" directory mandatory,
>> and document it as such. Let's also note that in the documentation
>> that this doesn't apply to the "hooks" and "branches" directories.
>
> I have no objection to this approach, but we should still do the
> other thing.

All that being said I don't really mind not creating a .git/info if the
consensus sways that way.

It's a bit more painful when it comes to the tests, but not *that*
painful. I had it mostly working before abandoning it for this approach.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 16:13     ` Ævar Arnfjörð Bjarmason
@ 2021-12-20 17:39       ` Derrick Stolee
  2021-12-20 18:16         ` Ævar Arnfjörð Bjarmason
                           ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Derrick Stolee @ 2021-12-20 17:39 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 20 2021, Derrick Stolee wrote:
> 
>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>> But we've also grown a hard dependency on this directory within git
>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>> sparse-checkout" command has wanted to add exclusions to
>>> "info/sparse-checkout". It didn't check or create the leading
>>> directory, so if it's omitted the command will die.
>>
>>> Even if that behavior were fixed we'd be left with older versions of
>>> "git" dying if that was attempted if they used a repository
>>> initialized without a template.
>>
>> This, I don't understand. Why can't we add a
>> safe_create_leading_directories() to any place where we try to
>> create a sparse-checkout file?
>>
>> This would fix situations where older versions were init'd with a
>> different template or if the user deleted the info dir. The change
>> you've made here doesn't fix those cases, which is what you are
>> claiming is the reason to not do the other fix that seems like it
>> would.
>>
>> What am I misunderstanding here?
> 
> I'll clarify that a bit in any re-roll.
> 
> Pedantically nothing changes, i.e. you can create a repository with an
> empty template now, and it'll break on both the sparse-checkout on that
> version, and any previous version that had that un-noticed issue.

You continue after this with more motivations for adding 'init' 
unconditionally, which I am not fighting.

What I _am_ saying is important is that if we are trying to write
a file to a known location and its parent directory doesn't exist,
then we should create it. Not doing so is a bug and should be
fixed, no matter how rare such a thing is to occur. As you've
shown, it is not required to have an info directory until we need
one (e.g. for sparse-checkout or an excludes file).

If you're not planning to add that to this series, then I'll add it
to my list. I do think it would fit well into this one, though.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 17:39       ` Derrick Stolee
@ 2021-12-20 18:16         ` Ævar Arnfjörð Bjarmason
  2021-12-20 19:06         ` Junio C Hamano
  2022-01-12 12:42         ` Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-20 18:16 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Derrick Stolee wrote:

> On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>> 
>>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>>> But we've also grown a hard dependency on this directory within git
>>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>>> sparse-checkout" command has wanted to add exclusions to
>>>> "info/sparse-checkout". It didn't check or create the leading
>>>> directory, so if it's omitted the command will die.
>>>
>>>> Even if that behavior were fixed we'd be left with older versions of
>>>> "git" dying if that was attempted if they used a repository
>>>> initialized without a template.
>>>
>>> This, I don't understand. Why can't we add a
>>> safe_create_leading_directories() to any place where we try to
>>> create a sparse-checkout file?
>>>
>>> This would fix situations where older versions were init'd with a
>>> different template or if the user deleted the info dir. The change
>>> you've made here doesn't fix those cases, which is what you are
>>> claiming is the reason to not do the other fix that seems like it
>>> would.
>>>
>>> What am I misunderstanding here?
>> 
>> I'll clarify that a bit in any re-roll.
>> 
>> Pedantically nothing changes, i.e. you can create a repository with an
>> empty template now, and it'll break on both the sparse-checkout on that
>> version, and any previous version that had that un-noticed issue.
>
> You continue after this with more motivations for adding 'init' 
> unconditionally, which I am not fighting.
>
> What I _am_ saying is important is that if we are trying to write
> a file to a known location and its parent directory doesn't exist,
> then we should create it. Not doing so is a bug and should be
> fixed, no matter how rare such a thing is to occur. As you've
> shown, it is not required to have an info directory until we need
> one (e.g. for sparse-checkout or an excludes file).
>
> If you're not planning to add that to this series, then I'll add it
> to my list. I do think it would fit well into this one, though.

Ah, you mean for the case where "git sparse-checkout" will fail because
.git/info/ doesn't exist now (e.g. because it's an existing repo with
--template=).

Yes that bug will not be fixed by this series. I'd welcome a patch to
fix it & could even integrate it with this series.

But I just found it rather "meh" and not that worth fixing. I.e. that it
hasn't been noticed or reported until now shows that this is a very rare
mode of operation. It seems most people just "git init" or "git clone",
or maybe almost nobody uses "sparse-checkout". I don't know.

I did try to fix it in an earlier version of this.

Maybe I went overboard with that, but I didn't just sprinkle
safe_create_leading_directories() to every caller as you suggest, but
rather wanted to change the API so that cases like:

        sparse_filename = get_sparse_checkout_filename();
        res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0);
        free(sparse_filename);

Would just call some function saying "add to sparse excludes", and not
care about the filename. Since having the API at that levels means you
end up with a lot of accounting work of getting the filename, freeing it
etc.

E.g. this as another example from add_patterns_cone_mode() (the function
makes no other use of sparse_filename):

	char *sparse_filename = get_sparse_checkout_filename();
        [...]
        if (add_patterns_from_file_to_list(sparse_filename, "", 0,
                                           &existing, NULL, 0))
                die(_("unable to load existing sparse-checkout patterns"));
        free(sparse_filename);

But per the upthread rationale I figured "meh" on that and that just
changing "git init" would fix the problem going forward in practice, so
I didn't pursue it.

Won't unconditionally adding a safe_create_leading_directories() also
close the door to having a core.sparseCheckoutFile similar to
core.excludesFile (but maybe it makes no sense). I.e. we'd be free to
"mkdir -p" the .git/info, but if the user is pointing it to some other
path then blindly creating that possibly nested path might be confusing,
as opposed to just immediately erroring out.

So...

All of which is to say that if you'd like to untangle it I'll review it,
and would be happy to include it in this series. But for the
--no-template change I thought I'd just try to sidestep that particular
aspect.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 17:39       ` Derrick Stolee
  2021-12-20 18:16         ` Ævar Arnfjörð Bjarmason
@ 2021-12-20 19:06         ` Junio C Hamano
  2021-12-21  1:15           ` Ævar Arnfjörð Bjarmason
  2022-01-12 12:42         ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2021-12-20 19:06 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Ævar Arnfjörð Bjarmason, git, Derrick Stolee,
	Adam Spiers, Jeff King, Johannes Schindelin

Derrick Stolee <stolee@gmail.com> writes:

> What I _am_ saying is important is that if we are trying to write
> a file to a known location and its parent directory doesn't exist,
> then we should create it. Not doing so is a bug and should be
> fixed, no matter how rare such a thing is to occur. As you've
> shown, it is not required to have an info directory until we need
> one (e.g. for sparse-checkout or an excludes file).
>
> If you're not planning to add that to this series, then I'll add it
> to my list. I do think it would fit well into this one, though.

Historically, "git init" relied on the templates to create necessary
directories, and the subcommands in turn learned to depend on the
presence of these directories.

At the same time we allowed that the templates can be customized by
the end users.  It was a bug, exactly for the reason you said above.

Before we talk about creating 'info' directory directly in "git
init" or anything done in this topic, we should fix the existing
bug, and the right fix is to use safe-create-leading-directories.

With that, it may become unnecessary to have this "create 'info' in
'init'".




^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 19:06         ` Junio C Hamano
@ 2021-12-21  1:15           ` Ævar Arnfjörð Bjarmason
  2021-12-21  2:10             ` Junio C Hamano
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-21  1:15 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Junio C Hamano wrote:

> Derrick Stolee <stolee@gmail.com> writes:
>
>> What I _am_ saying is important is that if we are trying to write
>> a file to a known location and its parent directory doesn't exist,
>> then we should create it. Not doing so is a bug and should be
>> fixed, no matter how rare such a thing is to occur. As you've
>> shown, it is not required to have an info directory until we need
>> one (e.g. for sparse-checkout or an excludes file).
>>
>> If you're not planning to add that to this series, then I'll add it
>> to my list. I do think it would fit well into this one, though.
>
> Historically, "git init" relied on the templates to create necessary
> directories, and the subcommands in turn learned to depend on the
> presence of these directories.
>
> At the same time we allowed that the templates can be customized by
> the end users.  It was a bug, exactly for the reason you said above.
>
> Before we talk about creating 'info' directory directly in "git
> init" or anything done in this topic, we should fix the existing
> bug, and the right fix is to use safe-create-leading-directories.
>
> With that, it may become unnecessary to have this "create 'info' in
> 'init'".

I don't see why we'd consider that as a worthwhile direction to go
in. The "git-init" documentation states:
    
    This command creates an empty Git repository - basically a `.git`
    directory with subdirectories for `objects`, `refs/heads`,
    `refs/tags`, and template files. 

I.e. we promise to create "objects", but not "objects/{info,pack}", even
though we've done so since f49fb35d0d5 (git-init-db: create "pack"
subdirectory under objects, 2005-06-27) and d57306c7945 (Create
objects/info/ directory in init-db., 2005-08-20).

Our test suite reveals our own assumptions, but it's also indicative of
assumptions others have made.

It's cheap to create .git/info unconditionally, and we create similar
empty subdirectories, so why not do it for .git/info? Why would we
needlessly break widely documented out-of-tree recipies like:

    some-user-excludes >.git/info/excludes

Which yes, rely on something you can't strictly rely on, but is true
enough most of the time that people do.

So I don't see what finding and fixing every instance of assuming
.git/info in the test suite buys us.

After doing that we'd be back to square one of having to decide if
exposing a mode that effectively did the same could be overly pedantic
in that case, or cheaply cater to out-of-tree code that made the same
assumption.

That's the question this topic is mainly trying to address. It's also
worthwhile to fix the in-tree dependency in sparse-checkout, but I don't
see why we'd insist that one can't be done without the other.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-21  1:15           ` Ævar Arnfjörð Bjarmason
@ 2021-12-21  2:10             ` Junio C Hamano
  2021-12-21  2:39               ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2021-12-21  2:10 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I don't see why we'd consider that as a worthwhile direction to go
> in. The "git-init" documentation states:
>     
>     This command creates an empty Git repository - basically a `.git`
>     directory with subdirectories for `objects`, `refs/heads`,
>     `refs/tags`, and template files. 
>
> I.e. we promise to create "objects", but not "objects/{info,pack}", even
> though we've done so since f49fb35d0d5 (git-init-db: create "pack"
> subdirectory under objects, 2005-06-27) and d57306c7945 (Create
> objects/info/ directory in init-db., 2005-08-20).

I view it as a documentation bug, though.

The only thing we need to promise is that (1) the directory prepared
by "git init" will be recognised as a repository by the current and
future versions of Git (and hopefully the existing ones, too), and
(2) versions of Git will not barf due to missing directory and stuff
due to end-user mischief around custom templates.  I do not think we
even need refs/heads/ directory and I do not see that, especially
with a presence with "basically" there, the sentence promises that
we will always create refs/tags/ directory.  For (1), only objects/,
refs/ and HEAD is necessary, as long as (2) is satisfied by lazy
creation of leading paths.

We would want to be able to cope with a repository that lost
.git/info directory due to loss of it in custom templates anyway.
Just like we create leading missing directories lazily for any
hierarchy under .git/, we should create .git/info on-demand.

Pre-creating .git/info might be nice, but becomes much less
important after it happens, and that is why I view it as a much
lower priority than what Derrick suggested.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-21  2:10             ` Junio C Hamano
@ 2021-12-21  2:39               ` Ævar Arnfjörð Bjarmason
  2021-12-21  6:38                 ` Junio C Hamano
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-21  2:39 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I don't see why we'd consider that as a worthwhile direction to go
>> in. The "git-init" documentation states:
>>     
>>     This command creates an empty Git repository - basically a `.git`
>>     directory with subdirectories for `objects`, `refs/heads`,
>>     `refs/tags`, and template files. 
>>
>> I.e. we promise to create "objects", but not "objects/{info,pack}", even
>> though we've done so since f49fb35d0d5 (git-init-db: create "pack"
>> subdirectory under objects, 2005-06-27) and d57306c7945 (Create
>> objects/info/ directory in init-db., 2005-08-20).
>
> I view it as a documentation bug, though.
>
> The only thing we need to promise is that (1) the directory prepared
> by "git init" will be recognised as a repository by the current and
> future versions of Git (and hopefully the existing ones, too), and
> (2) versions of Git will not barf due to missing directory and stuff
> due to end-user mischief around custom templates.  I do not think we
> even need refs/heads/ directory and I do not see that, especially
> with a presence with "basically" there, the sentence promises that
> we will always create refs/tags/ directory.  For (1), only objects/,
> refs/ and HEAD is necessary, as long as (2) is satisfied by lazy
> creation of leading paths.

Do you think that we should stop creating objects/{pack,info} then,
provided that the few test failures that assume them are cleand up
(which for at least one of them is much easier than the top-level
"info")?

> We would want to be able to cope with a repository that lost
> .git/info directory due to loss of it in custom templates anyway.
> Just like we create leading missing directories lazily for any
> hierarchy under .git/, we should create .git/info on-demand.
>
> Pre-creating .git/info might be nice, but becomes much less
> important after it happens, and that is why I view it as a much
> lower priority than what Derrick suggested.

Yes, we agree that we should deal with it not being created, that we
don't is a bug in sparse-checkout.

That's still a separate question from whether it's worthwhile investment
of both our and user time to skip creating them and deal with the
resulting churn in the tests and needlessly break code in the wild.

It seems as though you're saying that any fixes or changes in this area
would be incomplete without moving us towards the most pedantic and
minimalist interpretation possible when doing a "git init", is that
right?

I.e. to end up with this .git from "git init --template=" (the same as
now plus rmdir .git/{objects,refs}/*):
    
    $ tree -a -p
    .
    └── [drwxr-xr-x]  .git
        ├── [-rw-r--r--]  config
        ├── [-rw-r--r--]  HEAD
        ├── [drwxr-xr-x]  objects
        └── [drwxr-xr-x]  refs
    
    3 directories, 2 files

I can work towards that with these patches, it just seems to me to be
needlessly creating potential issues for little or no benefit.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-21  2:39               ` Ævar Arnfjörð Bjarmason
@ 2021-12-21  6:38                 ` Junio C Hamano
  2021-12-24 17:26                   ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2021-12-21  6:38 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> It seems as though you're saying that any fixes or changes in this area
> would be incomplete without moving us towards the most pedantic and
> minimalist interpretation possible when doing a "git init", is that
> right?

Not at all.  Belt and suspenders is a good way forward.  I am just
saying that on-demand creation is more important than the other one.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-21  6:38                 ` Junio C Hamano
@ 2021-12-24 17:26                   ` Ævar Arnfjörð Bjarmason
  2021-12-25  1:58                     ` Junio C Hamano
  0 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-24 17:26 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> It seems as though you're saying that any fixes or changes in this area
>> would be incomplete without moving us towards the most pedantic and
>> minimalist interpretation possible when doing a "git init", is that
>> right?
>
> Not at all.  Belt and suspenders is a good way forward.  I am just
> saying that on-demand creation is more important than the other one.

So you do agree that we should create .git/info whatever the template
says (as this series does), or not?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-24 17:26                   ` Ævar Arnfjörð Bjarmason
@ 2021-12-25  1:58                     ` Junio C Hamano
  0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2021-12-25  1:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Derrick Stolee, git, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Dec 20 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> It seems as though you're saying that any fixes or changes in this area
>>> would be incomplete without moving us towards the most pedantic and
>>> minimalist interpretation possible when doing a "git init", is that
>>> right?
>>
>> Not at all.  Belt and suspenders is a good way forward.  I am just
>> saying that on-demand creation is more important than the other one.
>
> So you do agree that we should create .git/info whatever the template
> says (as this series does), or not?

Probably "should" is too strong a word.  

I do not mind "mkdir -p" existing, but I think it is more important
to spend our engineering effort to make sure there are necessary
on-demand creation of leading paths.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2021-12-20 17:39       ` Derrick Stolee
  2021-12-20 18:16         ` Ævar Arnfjörð Bjarmason
  2021-12-20 19:06         ` Junio C Hamano
@ 2022-01-12 12:42         ` Ævar Arnfjörð Bjarmason
  2022-01-18 19:43           ` Derrick Stolee
  2 siblings, 1 reply; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:42 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Mon, Dec 20 2021, Derrick Stolee wrote:

> On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>> 
>>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>>> But we've also grown a hard dependency on this directory within git
>>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>>> sparse-checkout" command has wanted to add exclusions to
>>>> "info/sparse-checkout". It didn't check or create the leading
>>>> directory, so if it's omitted the command will die.
>>>
>>>> Even if that behavior were fixed we'd be left with older versions of
>>>> "git" dying if that was attempted if they used a repository
>>>> initialized without a template.
>>>
>>> This, I don't understand. Why can't we add a
>>> safe_create_leading_directories() to any place where we try to
>>> create a sparse-checkout file?
>>>
>>> This would fix situations where older versions were init'd with a
>>> different template or if the user deleted the info dir. The change
>>> you've made here doesn't fix those cases, which is what you are
>>> claiming is the reason to not do the other fix that seems like it
>>> would.
>>>
>>> What am I misunderstanding here?
>> 
>> I'll clarify that a bit in any re-roll.
>> 
>> Pedantically nothing changes, i.e. you can create a repository with an
>> empty template now, and it'll break on both the sparse-checkout on that
>> version, and any previous version that had that un-noticed issue.
>
> You continue after this with more motivations for adding 'init' 
> unconditionally, which I am not fighting.
>
> What I _am_ saying is important is that if we are trying to write
> a file to a known location and its parent directory doesn't exist,
> then we should create it. Not doing so is a bug and should be
> fixed, no matter how rare such a thing is to occur. As you've
> shown, it is not required to have an info directory until we need
> one (e.g. for sparse-checkout or an excludes file).
>
> If you're not planning to add that to this series, then I'll add it
> to my list. I do think it would fit well into this one, though.

Just so we'll avoid stepping on each other's toes, what's the status of
your plan/non-plan to work on that more isolated fix, perhaps you have
one that's unsubmitted?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2022-01-12 12:42         ` Ævar Arnfjörð Bjarmason
@ 2022-01-18 19:43           ` Derrick Stolee
  2022-01-19  1:00             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 33+ messages in thread
From: Derrick Stolee @ 2022-01-18 19:43 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin

On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 20 2021, Derrick Stolee wrote:
> 
>> On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>>>
>>>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>>>> But we've also grown a hard dependency on this directory within git
>>>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>>>> sparse-checkout" command has wanted to add exclusions to
>>>>> "info/sparse-checkout". It didn't check or create the leading
>>>>> directory, so if it's omitted the command will die.
>>>>
>>>>> Even if that behavior were fixed we'd be left with older versions of
>>>>> "git" dying if that was attempted if they used a repository
>>>>> initialized without a template.
>>>>
>>>> This, I don't understand. Why can't we add a
>>>> safe_create_leading_directories() to any place where we try to
>>>> create a sparse-checkout file?
>>>>
>>>> This would fix situations where older versions were init'd with a
>>>> different template or if the user deleted the info dir. The change
>>>> you've made here doesn't fix those cases, which is what you are
>>>> claiming is the reason to not do the other fix that seems like it
>>>> would.
>>>>
>>>> What am I misunderstanding here?
>>>
>>> I'll clarify that a bit in any re-roll.
>>>
>>> Pedantically nothing changes, i.e. you can create a repository with an
>>> empty template now, and it'll break on both the sparse-checkout on that
>>> version, and any previous version that had that un-noticed issue.
>>
>> You continue after this with more motivations for adding 'init' 
>> unconditionally, which I am not fighting.
>>
>> What I _am_ saying is important is that if we are trying to write
>> a file to a known location and its parent directory doesn't exist,
>> then we should create it. Not doing so is a bug and should be
>> fixed, no matter how rare such a thing is to occur. As you've
>> shown, it is not required to have an info directory until we need
>> one (e.g. for sparse-checkout or an excludes file).
>>
>> If you're not planning to add that to this series, then I'll add it
>> to my list. I do think it would fit well into this one, though.
> 
> Just so we'll avoid stepping on each other's toes, what's the status of
> your plan/non-plan to work on that more isolated fix, perhaps you have
> one that's unsubmitted?

I do not have one that is unsubmitted. I was hoping that you would
include it in a v2 to this series. I might have been quicker to
volunteer to create one had I not been sidelined for two weeks, but
right now I have a lot to catch up on so don't have the time.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/13] init: unconditionally create the "info" directory
  2022-01-18 19:43           ` Derrick Stolee
@ 2022-01-19  1:00             ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 33+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-19  1:00 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Junio C Hamano, Derrick Stolee, Adam Spiers, Jeff King,
	Johannes Schindelin


On Tue, Jan 18 2022, Derrick Stolee wrote:

> On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>> 
>>> On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>>>>
>>>>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>>>>> But we've also grown a hard dependency on this directory within git
>>>>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>>>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>>>>> sparse-checkout" command has wanted to add exclusions to
>>>>>> "info/sparse-checkout". It didn't check or create the leading
>>>>>> directory, so if it's omitted the command will die.
>>>>>
>>>>>> Even if that behavior were fixed we'd be left with older versions of
>>>>>> "git" dying if that was attempted if they used a repository
>>>>>> initialized without a template.
>>>>>
>>>>> This, I don't understand. Why can't we add a
>>>>> safe_create_leading_directories() to any place where we try to
>>>>> create a sparse-checkout file?
>>>>>
>>>>> This would fix situations where older versions were init'd with a
>>>>> different template or if the user deleted the info dir. The change
>>>>> you've made here doesn't fix those cases, which is what you are
>>>>> claiming is the reason to not do the other fix that seems like it
>>>>> would.
>>>>>
>>>>> What am I misunderstanding here?
>>>>
>>>> I'll clarify that a bit in any re-roll.
>>>>
>>>> Pedantically nothing changes, i.e. you can create a repository with an
>>>> empty template now, and it'll break on both the sparse-checkout on that
>>>> version, and any previous version that had that un-noticed issue.
>>>
>>> You continue after this with more motivations for adding 'init' 
>>> unconditionally, which I am not fighting.
>>>
>>> What I _am_ saying is important is that if we are trying to write
>>> a file to a known location and its parent directory doesn't exist,
>>> then we should create it. Not doing so is a bug and should be
>>> fixed, no matter how rare such a thing is to occur. As you've
>>> shown, it is not required to have an info directory until we need
>>> one (e.g. for sparse-checkout or an excludes file).
>>>
>>> If you're not planning to add that to this series, then I'll add it
>>> to my list. I do think it would fit well into this one, though.
>> 
>> Just so we'll avoid stepping on each other's toes, what's the status of
>> your plan/non-plan to work on that more isolated fix, perhaps you have
>> one that's unsubmitted?
>
> I do not have one that is unsubmitted. I was hoping that you would
> include it in a v2 to this series. I might have been quicker to
> volunteer to create one had I not been sidelined for two weeks, but
> right now I have a lot to catch up on so don't have the time.

Good to hear that you're better (or back?, not 100% sure what
"sidelined" here means).

I'll try to get to re-rolling it with a fix for that sparse-checkout
issue, hopefully sooner than later. Just wanted to avoid potential
duplicate work.

Thanks.



^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2022-01-19  1:02 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-12 20:13 [PATCH 00/13] tests + init: don't rely on templates & add --no-template + config Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 01/13] t0001: fix gaps in "TEMPLATE DIRECTORY" coverage Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 02/13] init: split out template population from create_default_files() Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 03/13] init: unconditionally create the "info" directory Ævar Arnfjörð Bjarmason
2021-12-20 15:59   ` Derrick Stolee
2021-12-20 16:13     ` Ævar Arnfjörð Bjarmason
2021-12-20 17:39       ` Derrick Stolee
2021-12-20 18:16         ` Ævar Arnfjörð Bjarmason
2021-12-20 19:06         ` Junio C Hamano
2021-12-21  1:15           ` Ævar Arnfjörð Bjarmason
2021-12-21  2:10             ` Junio C Hamano
2021-12-21  2:39               ` Ævar Arnfjörð Bjarmason
2021-12-21  6:38                 ` Junio C Hamano
2021-12-24 17:26                   ` Ævar Arnfjörð Bjarmason
2021-12-25  1:58                     ` Junio C Hamano
2022-01-12 12:42         ` Ævar Arnfjörð Bjarmason
2022-01-18 19:43           ` Derrick Stolee
2022-01-19  1:00             ` Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 04/13] t0008: don't rely on default ".git/info/exclude" Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 05/13] init & clone: add a --no-template option Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 06/13] init & clone: add init.templateDir=[bool] Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 07/13] test-lib: create test data with "git init --no-template" (almost) Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 08/13] tests: don't depend on template-created .git/branches Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 09/13] t5540: don't rely on "hook/post-update.sample" Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper Ævar Arnfjörð Bjarmason
2021-12-13 14:15   ` Eric Sunshine
2021-12-13 16:29     ` Ævar Arnfjörð Bjarmason
2021-12-13 16:45       ` Eric Sunshine
2021-12-13 19:37         ` Ævar Arnfjörð Bjarmason
2021-12-13 21:33           ` Eric Sunshine
2021-12-12 20:13 ` [PATCH 11/13] tests: change "cat && chmod +x" to use "write_hook" Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 12/13] tests: migrate miscellaneous "write_script" to "write_hooks" Ævar Arnfjörð Bjarmason
2021-12-12 20:13 ` [PATCH 13/13] tests: don't depend on template-created .git/hooks Ævar Arnfjörð Bjarmason

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).