[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
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 <redacted>
---
t/t0001-init.sh | 85 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 82 insertions(+), 3 deletions(-)
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 <redacted>
---
builtin/init-db.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
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 <redacted>
---
Documentation/gitrepository-layout.txt | 17 ++++++++++++++++-
builtin/init-db.c | 6 ++++++
2 files changed, 22 insertions(+), 1 deletion(-)
@@ -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
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 <redacted>
---
t/t0008-ignores.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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 <redacted>
---
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(-)
@@ -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].)
@@ -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;
@@ -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);
@@ -540,14 +547,18 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)constchar*git_dir;constchar*real_git_dir=NULL;constchar*work_tree;+intno_template=0;constchar*template_dir=NULL;unsignedintflags=0;constchar*object_format=NULL;constchar*initial_branch=NULL;inthash_algo=GIT_HASH_UNKNOWN;conststructoptioninit_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);
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 <redacted>
---
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(-)
@@ -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`.
@@ -2251,6 +2263,16 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const chareturn1;}+intgit_configset_get_bool_or_pathname(structconfig_set*cs,constchar*key,+int*is_bool,constchar**dest)+{+constchar*value;+if(!git_configset_get_value(cs,key,&value))+returngit_config_bool_or_pathname(dest,key,value,is_bool);+else+return1;+}+/* Functions use to read configuration from a repository */staticvoidrepo_read_config(structrepository*repo){
@@ -2384,6 +2406,13 @@ int repo_config_get_pathname(struct repository *repo,returnret;}+intrepo_config_get_bool_or_pathname(structrepository*repo,+constchar*key,int*is_bool,constchar**dest)+{+git_config_check_init(repo);+returngit_configset_get_bool_or_pathname(repo->config,key,is_bool,dest);+}+/* Functions used historically to read configuration from 'the_repository' */voidgit_config(config_fn_tfn,void*data){
@@ -493,6 +502,8 @@ int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);intgit_configset_get_bool_or_int(structconfig_set*cs,constchar*key,int*is_bool,int*dest);intgit_configset_get_maybe_bool(structconfig_set*cs,constchar*key,int*dest);intgit_configset_get_pathname(structconfig_set*cs,constchar*key,constchar**dest);+intgit_configset_get_bool_or_pathname(structconfig_set*cs,constchar*key,+int*is_bool,constchar**dest);/* Functions for reading a repository's config */structrepository;
@@ -517,6 +528,9 @@ int repo_config_get_maybe_bool(struct repository *repo,constchar*key,int*dest);intrepo_config_get_pathname(structrepository*repo,constchar*key,constchar**dest);+intrepo_config_get_bool_or_pathname(structrepository*repo,+constchar*key,int*is_bool,+constchar**dest);/***QueryingForSpecificVariables
@@ -607,6 +621,9 @@ int git_config_get_maybe_bool(const char *key, int *dest);*/intgit_config_get_pathname(constchar*key,constchar**dest);+intgit_config_get_bool_or_pathname(constchar*key,int*is_bool,+constchar**dest);+intgit_config_get_index_threads(int*dest);intgit_config_get_split_index(void);intgit_config_get_max_percent_split_change(void);
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 <redacted>
---
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(-)
@@ -267,7 +260,7 @@ test_expect_success 'init with init.templatedir using ~ expansion' 'echoContent>templatedir-source/file&&test_config_globalinit.templatedir"~/templatedir-source"&&-init_no_templatedir_envtemplatedir-expansion&&+gitinittemplatedir-expansion&&test_cmptemplatedir-source/filetemplatedir-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''-gitinitmain-worktree&&(+gitinitmain-worktree&&+cdmain-worktree&&test_commitfirst&&gitworktreeadd../linked-worktree&&+>empty&&+cpempty.git/info/exclude&&mv.git/info/excludeexpected-exclude&&cp.git/configexpected-config&&find.git/worktrees-print|sort>expected&&git-C../linked-worktreeinit&&+cpempty.git/info/exclude&&test_cmpexpected-exclude.git/info/exclude&&test_cmpexpected-config.git/config&&find.git/worktrees-print|sort>actual&&
@@ -1355,11 +1355,13 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:PATH="$GIT_BUILD_DIR:$GIT_BUILD_DIR/t/helper:$PATH"fifi-GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt+GIT_NO_TEMPLATE_DIR=true+GIT_TEST_BARE_TEMPLATE=true+exportGIT_NO_TEMPLATE_DIRGIT_TEST_BARE_TEMPLATEGIT_CONFIG_NOSYSTEM=1GIT_ATTR_NOSYSTEM=1GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/.."-exportPATHGIT_EXEC_PATHGIT_TEMPLATE_DIRGIT_CONFIG_NOSYSTEMGIT_ATTR_NOSYSTEMGIT_CEILING_DIRECTORIES+exportPATHGIT_EXEC_PATHGIT_CONFIG_NOSYSTEMGIT_ATTR_NOSYSTEMGIT_CEILING_DIRECTORIESiftest-z"$GIT_TEST_CMP"then
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 <redacted>
---
builtin/init-db.c | 1 -
t/t5505-remote.sh | 2 ++
t/t5516-fetch-push.sh | 3 +++
3 files changed, 5 insertions(+), 1 deletion(-)
@@ -929,6 +929,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '(cdsix&&gitremotermorigin&&+mkdir.git/branches&&echo"$origin_url#main">.git/branches/origin&&gitremoterenameoriginorigin&&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)'(cdseven&&gitremotermorigin&&+mkdir.git/branches&&echo"quux#foom">.git/branches/origin&&gitremoterenameoriginorigin&&test_path_is_missing.git/branches/origin&&
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 <redacted>
---
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(-)
@@ -25,13 +25,15 @@ test_expect_success setup 'GIT_DIR=clone2/.gitgitupdate-index--adda'-forclonein12;do-cat>clone${clone}/.git/hooks/post-merge<<'EOF'-#!/bin/sh-echo$@>>$GIT_DIR/post-merge.args-EOF-chmodu+xclone${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-Cclone1post-merge<hook&&+write_hook-Cclone2post-merge<hook+' test_expect_success'post-merge does not run for up-to-date ''GIT_DIR=clone1/.gitgitmerge$commit0&&
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 <redacted>
---
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(-)
@@ -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_hookupdate<<-\EOF&&echoerror:errorechoERROR:alsohighlightedechohint:hint
@@ -5,7 +5,7 @@ test_description='check quarantine of objects during push' test_expect_success'create picky dest repo''gitinit--baredest.git&&-write_scriptdest.git/hooks/pre-receive<<-\EOF+write_hook-Cdest.gitpre-receive<<-\EOFwhilereadoldnewref;dotest"$(gitlog-1--format=%s$new)"=reject&&exit1done
@@ -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''gitinit--bareupdate.git&&-write_scriptupdate.git/hooks/pre-receive<<-\EOF&&+write_hook-Cupdate.gitpre-receive<<-\EOF&&readoldnewrefnamegitupdate-refrefs/heads/unrelated$newexit1
@@ -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=&&+whiletest$#!=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:#
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 <redacted>
---
t/t5540-http-push-webdav.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -292,9 +292,10 @@ test_expect_success 'post-rewrite hook and fixups work for merges' 'gitcommit--fixupHEADsame2.t&&fixup="$(gitrev-parseHEAD)"&&-mkdir-p.git/hooks&&test_when_finished"rm .git/hooks/post-rewrite"&&-echo"cat >actual"|write_script.git/hooks/post-rewrite&&+write_hookpost-rewrite<<-\EOF&&+cat>actual+EOFtest_tick&&gitrebase-i--autosquash-rHEAD^^^&&
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 <redacted>
---
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(-)
@@ -1356,8 +1356,7 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:fifiGIT_NO_TEMPLATE_DIR=true-GIT_TEST_BARE_TEMPLATE=true-exportGIT_NO_TEMPLATE_DIRGIT_TEST_BARE_TEMPLATE+exportGIT_NO_TEMPLATE_DIRGIT_CONFIG_NOSYSTEM=1GIT_ATTR_NOSYSTEM=1GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/.."
From: Eric Sunshine <hidden> Date: 2021-12-13 14:16:11
On Sun, Dec 12, 2021 at 4:24 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
On Sun, Dec 12, 2021 at 4:24 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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 <redacted>
---
From: Eric Sunshine <hidden> Date: 2021-12-13 16:45:54
On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Mon, Dec 13 2021, Eric Sunshine wrote:
quoted
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.
On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Mon, Dec 13 2021, Eric Sunshine wrote:
quoted
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".
From: Eric Sunshine <hidden> Date: 2021-12-13 21:33:34
On Mon, Dec 13, 2021 at 2:42 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Mon, Dec 13 2021, Eric Sunshine wrote:
quoted
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.
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
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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.
quoted
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.
quoted
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.
On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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
On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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.
On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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?
On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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
On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Mon, Dec 20 2021, Derrick Stolee wrote:
quoted
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
quoted
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.
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 <redacted>
---
t/t0008-ignores.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
This reduced-in-scope re-roll of [1] changes those tests that
implicitly relied on the existence of the default "git init" template
to no longer do so. This makes the intent of the tests more explicit,
e.g. in the case of the .git/branch directory.
Parts of the v1 were already split up, re-rolled and merged as
a5bf611cc4f (Merge branch 'ab/hook-tests-updates', 2022-03-30).
Future goals: I then have a local topic on top of this to add the
"--no-template" option to "git init", and to have the test suite run
in that mode by default, that's currently one of the blockers for
making the test run without the bin-wrappers[2], which as noted in [3]
leaves us with some blind spots in our tests.
But for now this is all relatively straightforward conversion of
existing tests to make them not rely on "git init" giving them a
template, which they opt-in to with a new TEST_CREATE_REPO_NO_TEMPLATE
variable.
Junio: There's a trivial conflict in one of the tests with "next",
i.e.:
diff --cc t/t6101-rev-parse-parents.sh
index 5f55ab98d35,a3a41c7a3e4..00000000000
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@@ -8,7 -8,7 +8,11 @@@ test_description='Test git rev-parse wi
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
++<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< HEAD
+TEST_CREATE_REPO_NO_TEMPLATE=1
++================================
+ TEST_PASSES_SANITIZE_LEAK=true
++>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> origin/seen
The resolution is to keep both sides, i.e. to end up with:
TEST_CREATE_REPO_NO_TEMPLATE=1
TEST_PASSES_SANITIZE_LEAK=true
1. https://lore.kernel.org/git/cover-00.13-00000000000-20211212T201308Z-avarab@gmail.com/
2. https://lore.kernel.org/git/220405.86k0c3lt2l.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/211124.86sfvld4cl.gmgdl@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (7):
t0008: don't rely on default ".git/info/exclude"
tests: don't depend on template-created .git/branches
tests: don't assume a .git/info for .git/info/grafts
tests: don't assume a .git/info for .git/info/attributes
tests: don't assume a .git/info for .git/info/refs
tests: don't assume a .git/info for .git/info/exclude
tests: don't assume a .git/info for .git/info/sparse-checkout
t/annotate-tests.sh | 3 ++-
t/lib-submodule-update.sh | 7 +++++--
t/t0003-attributes.sh | 5 ++++-
t/t0008-ignores.sh | 12 +++++++-----
t/t0028-working-tree-encoding.sh | 2 ++
t/t1011-read-tree-sparse-checkout.sh | 2 ++
t/t1090-sparse-checkout-scope.sh | 5 ++++-
t/t1301-shared-repo.sh | 3 ++-
t/t2018-checkout-branch.sh | 4 +++-
t/t2400-worktree-add.sh | 4 ++++
t/t3426-rebase-submodule.sh | 1 +
t/t3507-cherry-pick-conflict.sh | 2 ++
t/t5000-tar-tree.sh | 5 ++++-
t/t5001-archive-attr.sh | 5 ++++-
t/t5002-archive-attr-pattern.sh | 5 ++++-
t/t5003-archive-zip.sh | 5 ++++-
t/t5318-commit-graph.sh | 3 ++-
t/t5505-remote.sh | 6 ++++--
t/t5516-fetch-push.sh | 16 ++++++++++++++--
t/t5550-http-fetch-dumb.sh | 3 ++-
t/t6001-rev-list-graft.sh | 1 +
t/t6101-rev-parse-parents.sh | 2 ++
t/t6435-merge-sparse.sh | 2 ++
t/t7063-status-untracked-cache.sh | 3 ++-
t/t7418-submodule-sparse-gitmodules.sh | 3 ++-
t/t7814-grep-recurse-submodules.sh | 8 ++++++--
t/t8001-annotate.sh | 1 +
t/t8002-blame.sh | 1 +
t/t8012-blame-colors.sh | 1 +
t/test-lib.sh | 4 +++-
30 files changed, 97 insertions(+), 27 deletions(-)
Range-diff against v1:
1: 5526d3dc838 < -: ----------- t0001: fix gaps in "TEMPLATE DIRECTORY" coverage
2: ef2b67768cf < -: ----------- init: split out template population from create_default_files()
3: 784b7947512 < -: ----------- init: unconditionally create the "info" directory
4: 3d4ea5c5d30 = 1: 21927e21832 t0008: don't rely on default ".git/info/exclude"
5: 6bbb39f13fc < -: ----------- init & clone: add a --no-template option
6: 2f478f7ba4a < -: ----------- init & clone: add init.templateDir=[bool]
7: 9402fb23b40 < -: ----------- test-lib: create test data with "git init --no-template" (almost)
8: 0c9b953dd43 < -: ----------- tests: don't depend on template-created .git/branches
9: d97122f5fd8 < -: ----------- t5540: don't rely on "hook/post-update.sample"
10: ca55471d134 < -: ----------- test-lib-functions: add and use a "write_hook" wrapper
11: 6f805f7ebac < -: ----------- tests: change "cat && chmod +x" to use "write_hook"
12: 2acbaa77f8d < -: ----------- tests: migrate miscellaneous "write_script" to "write_hooks"
13: d021a5981a1 < -: ----------- tests: don't depend on template-created .git/hooks
-: ----------- > 2: c4d0e6c684b tests: don't depend on template-created .git/branches
-: ----------- > 3: 967762b4670 tests: don't assume a .git/info for .git/info/grafts
-: ----------- > 4: 64292c20c84 tests: don't assume a .git/info for .git/info/attributes
-: ----------- > 5: b9f667bf292 tests: don't assume a .git/info for .git/info/refs
-: ----------- > 6: 29da5a3d431 tests: don't assume a .git/info for .git/info/exclude
-: ----------- > 7: 0149c80f09c tests: don't assume a .git/info for .git/info/sparse-checkout
--
2.36.1.1119.g5a713b43bd4
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 our default behavior, our
tests should be tightened up to explicitly create the .git/branches
directory if they rely on our default templates, to make the
dependency on those templates clear.
So let's amend the two tests that would fail if .git/branches wasn't
created. To do this introduce a new "TEST_CREATE_REPO_NO_TEMPLATE"
variable, which we'll set before sourcing test-lib.sh, and change the
"git clone" and "git init" commands in the tests themselves to
explicitly pass "--template=".
This way they won't get a .git/branches in either their top-level
.git, or in the ones they create. We can then amend the tests that
rely on the ".git/branches" directory existing to create it
explicitly, and to remove it after its creation.
This new "TEST_CREATE_REPO_NO_TEMPLATE" variable is a less
heavy-handed version of the "NO_SET_GIT_TEMPLATE_DIR" variable. See
a94d305bf80 (t/t0001-init.sh: add test for 'init with init.templatedir
set', 2010-02-26) for its implementation.
Unlike "TEST_CREATE_REPO_NO_TEMPLATE", this new
"TEST_CREATE_REPO_NO_TEMPLATE" variable is narrowly scoped to what the
"git init" in test-lib.sh does, as opposed to the global effect of
"NO_SET_GIT_TEMPLATE_DIR" and the setting of "GIT_TEMPLATE_DIR" in
wrap-for-bin.sh.
I experimented with adding a new "GIT_WRAP_FOR_BIN_VIA_TEST_LIB"
variable set in test-lib.sh, which would cause wrap-for-bin.sh to not
set GIT_TEMPLATE_DIR, GITPERLLIB etc, as we set those in
test-lib.sh. I think that's a viable approach, but it would interact
e.g. with the appending feature of GITPERLLIB added in
8bade1e12e2 (wrap-for-bin: make bin-wrappers chainable, 2013-07-04).
Doing so would allow us to convert the tests in t0001-init.sh that now
use "NO_SET_GIT_TEMPLATE_DIR" to simply unset "GIT_TEMPLATE_DIR" in a
sub-shell before invoking "git init" or "git clone". I think that
approach is worth pursuing, but let's table it for now. Some future
wrap-for-bin.sh refactoring can try to address it.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t5505-remote.sh | 6 ++++--
t/t5516-fetch-push.sh | 16 ++++++++++++++--
t/test-lib.sh | 4 +++-
3 files changed, 21 insertions(+), 5 deletions(-)
@@ -957,11 +957,12 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '' test_expect_success'migrate a remote from named file in $GIT_DIR/branches''-gitcloneonesix&&+gitclone--template=onesix&&origin_url=$(pwd)/one&&(cdsix&&gitremotermorigin&&+mkdir.git/branches&&echo"$origin_url#main">.git/branches/origin&&gitremoterenameoriginorigin&&test_path_is_missing.git/branches/origin&&
@@ -972,10 +973,11 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '' test_expect_success'migrate a remote from named file in $GIT_DIR/branches (2)''-gitcloneoneseven&&+gitclone--template=oneseven&&(cdseven&&gitremotermorigin&&+mkdir.git/branches&&echo"quux#foom">.git/branches/origin&&gitremoterenameoriginorigin&&test_path_is_missing.git/branches/origin&&
@@ -17,6 +17,7 @@ This test checks the following functionality:GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=mainexportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME+TEST_CREATE_REPO_NO_TEMPLATE=1 ../test-lib.shD=$(pwd)
Change those tests that assumed that a .git/info directory would be
created for them when writing .git/info/grafts to explicitly create
the directory.
Do this using the new "TEST_CREATE_REPO_NO_TEMPLATE" facility, and use
"mkdir" instead of "mkdir -p" to assert that we don't have the
.git/info already. An exception to this is the "with grafts" test in
"t6001-rev-list-graft.sh". There we're modifying our ".git" state in a
for-loop, in lieu of refactoring that more extensively let's use
"mkdir -p" there.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/annotate-tests.sh | 3 ++-
t/t2400-worktree-add.sh | 4 ++++
t/t5318-commit-graph.sh | 3 ++-
t/t6001-rev-list-graft.sh | 1 +
t/t6101-rev-parse-parents.sh | 2 ++
t/t8001-annotate.sh | 1 +
t/t8002-blame.sh | 1 +
t/t8012-blame-colors.sh | 1 +
8 files changed, 14 insertions(+), 2 deletions(-)
Change those tests that assumed that a .git/info directory would be
created for them when writing .git/info/attributes to explicitly
create the directory by setting "TEST_CREATE_REPO_NO_TEMPLATE=1"
before sourcing test-lib.sh, and using the "--template=" argument to
"git clone".
The change here in here in t7814-grep-recurse-submodules.sh would
continue "succeeding" with only the "TEST_CREATE_REPO_NO_TEMPLATE=1"
part of this change. That's because those tests use
"test_expect_failure", so they'd "pass" without this change, as
"test_expect_failure" by design isn't discerning about what failure
conditions it'll accept.
But as we're fixing these sorts of issues across the test suite let's
fix this one too. This issue was spotted with a local merge with
another topic of mine[1], which introduces a stricter alternative to
"test_expect_failure".
1. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t0003-attributes.sh | 5 ++++-
t/t0028-working-tree-encoding.sh | 2 ++
t/t5000-tar-tree.sh | 5 ++++-
t/t5001-archive-attr.sh | 5 ++++-
t/t5002-archive-attr-pattern.sh | 5 ++++-
t/t5003-archive-zip.sh | 5 ++++-
t/t7814-grep-recurse-submodules.sh | 8 ++++++--
7 files changed, 28 insertions(+), 7 deletions(-)
@@ -139,7 +141,8 @@ test_expect_success 'setup export-subst and diff attributes' '' test_expect_success'create bare clone''-gitclone--bare.bare.git&&+gitclone--template=--bare.bare.git&&+mkdirbare.git/info&&cp.git/info/attributesbare.git/info/attributes&&# Recreate our changes to .git/config rather than just copying it, as# we do not want to clobber core.bare or other settings.
@@ -6,6 +6,7 @@ This test verifies the recurse-submodules feature correctly greps across submodules.'+TEST_CREATE_REPO_NO_TEMPLATE=1 ../test-lib.shGIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
@@ -471,8 +472,10 @@ test_expect_failure 'grep --textconv: superproject .gitattributes (from index) d test_expect_failure'grep --textconv: superproject .git/info/attributes does not affect submodules''reset_and_clean&&test_config_globaldiff.d2x.textconv"sed -e \"s/d/x/\""&&-super_attr="$(gitrev-parse--git-pathinfo/attributes)"&&+super_info="$(gitrev-parse--git-pathinfo)"&&+super_attr="$super_info/attributes"&&test_when_finished"rm -f \"$super_attr\""&&+mkdir"$super_info"&&echo"a diff=d2x">"$super_attr"&&cat>expect<<-\EOF&&
Change those tests that assumed that a .git/info directory would be
created for them when writing .git/info/refs to explicitly create the
directory by using the "--template=" argument to "git init".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1301-shared-repo.sh | 3 ++-
t/t5550-http-fetch-dumb.sh | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
@@ -422,7 +422,8 @@ test_expect_success 'set up evil alternates scheme' 'sha1=$(git-C"$victim"rev-parseHEAD)&&evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git&&-gitinit--bare"$evil"&&+gitinit--template=--bare"$evil"&&+mkdir"$evil/info"&&# do this by hand to avoid object existence checkprintf"%s\\t%s\\n"$sha1refs/heads/main>"$evil/info/refs"'
Change those tests that assumed that a .git/info directory would be
created for them when writing .git/info/exclude to explicitly create
the directory by setting "TEST_CREATE_REPO_NO_TEMPLATE=1" before
sourcing test-lib.sh, and using the "--template=" argument to "git
clone" and "git init".
In the case of ".git/modules/sub1/info" we deviate from the
established pattern in this and preceding commits of passing a
"--template=" and doing a "mkdir .git/info".
In that case "git checkout" will run the "submodule--helper clone",
and both e.g. "git submodule update --init" and "git checkout" do not
have a way to pass down options to the eventual "git init" or "git
clone". Let's instead assume that the submodule was populated with our
default templates, remove them, and then run the "mkdir".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/lib-submodule-update.sh | 7 +++++--
t/t0008-ignores.sh | 2 ++
t/t3426-rebase-submodule.sh | 1 +
t/t7063-status-untracked-cache.sh | 3 ++-
4 files changed, 10 insertions(+), 3 deletions(-)
@@ -207,7 +207,7 @@ prolog () {# should be updated to an existing commit. reset_work_tree_to(){rm-rfsubmodule_update&&-gitclonesubmodule_update_reposubmodule_update&&+gitclone--template=submodule_update_reposubmodule_update&&(cdsubmodule_update&&rm-rfsub1&&
@@ -902,13 +902,14 @@ test_submodule_switch_recursing_with_args () {'# ... but an ignored file is fine.test_expect_$RESULTOI"$command: added submodule removes an untracked ignored file"'-test_when_finished"rm submodule_update/.git/info/exclude"&&+test_when_finished"rm -rf submodule_update/.git/info"&&prolog&&reset_work_tree_to_interestedno_submodule&&(cdsubmodule_update&&gitbranch-tadd_sub1origin/add_sub1&&:>sub1&&+mkdir.git/info&&echosub1>.git/info/exclude&&$commandadd_sub1&&test_superproject_contentorigin/add_sub1&&
Change those tests that assumed that a .git/info directory would be
created for them when writing .git/info/sparse-checkout to explicitly
create the directory by setting "TEST_CREATE_REPO_NO_TEMPLATE=1"
before sourcing test-lib.sh, and using the "--template=" argument to
"git clone".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1011-read-tree-sparse-checkout.sh | 2 ++
t/t1090-sparse-checkout-scope.sh | 5 ++++-
t/t2018-checkout-branch.sh | 4 +++-
t/t3507-cherry-pick-conflict.sh | 2 ++
t/t6435-merge-sparse.sh | 2 ++
t/t7418-submodule-sparse-gitmodules.sh | 3 ++-
6 files changed, 15 insertions(+), 3 deletions(-)
@@ -53,6 +54,7 @@ test_expect_success 'read-tree without .git/info/sparse-checkout' '' test_expect_success'read-tree with .git/info/sparse-checkout but disabled''+mkdir.git/info&&echo>.git/info/sparse-checkout&&read_tree_u_must_succeed-m-uHEAD&&gitls-files-t>result&&