[PATCH/RFC 0/9] Setup cleanup, chapter one

[PATCH/RFC 0/9] Setup cleanup, chapter one

[Jonathan Nieder]

These patches are designed to make people who want the pager.<cmd>
configuration to be more reliable a little happier.  More importantly,
they bring the setup procedure closer to doing the Right Thing™.

More precisely:

The pager configuration procedure does not always do as much as one
would like:

A. Unless one runs check_pager_config() first, the pager.<cmd>
   configuration is ignored.  The setup procedure does not run
   check_pager_config() for commands that do not use RUN_SETUP.

B. Unless one runs setup_repository_gently() first, functions that
   check configuration expect to find configuration in ./.git/config.
   This is wrong for two reasons: some commands should ignore the
   per-repository configuration, and most commands should be walking
   parent parent directories to find an appropriate .git or foo.git
   dir.  The upshot is that the per-repository configuration is
   ignored for pager.<cmd> and core.pager when git is run from the
   toplevel of a repository.

These patches are a modest attempt to start to ameliorate these
problems.  Patches 1, 2, and 4 introduce new tests to make sure
we are not making things worse.

Patch 3 delays startup of the pager.  By the time we actually need
to start a pager for commands with the RUN_SETUP option, the .git
directory will already have been found, addressing problem (B) for
those commands.

Patches 5 and 6 are something of a trojan horse: they introduce
infrastructure that I expect to be more widely useful after this
modest patch series.  That infrastructure is a collection of global
variables representing the state of the current repository, for
built-in commands only (for now).  The main benefit now is that it
lets us pass more data to builtins without changing the builtin API.

Patches 7 gives builtins the option to search for a repository
early even if they do not require a repository to work.  The benefits
of patch 3 will accrue to such commands just as though they used
RUN_SETUP.  Patch 8 addresses problem (A) for builtins using this new
RUN_SETUP_GENTLY option.

And patch 9 is an example command taking advantage of the opportunity.
I don’t imagine anyone was eagerly awaiting the ability to make their
‘git config’ automatically paginate.  The secondary benefits
(especially UI consistency) are in my opinion more important.

This series would not be possible without the hard work of Nguyễn Thái
Ngọc Duy.  It consists mostly of patches by him.  A footnote [1] contains
some context if you would like it.

Thoughts?

Jonathan Nieder (4):
  t7006: GIT_DIR/config should be honored in subdirs of toplevel
  t7006: test pager configuration for several git commands
  t7006: test pager.<cmd> configuration
  builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

Nguyễn Thái Ngọc Duy (5):
  builtins: do not commit pager choice early
  builtin: introduce startup_info struct
  builtin: remember whether repository was found
  builtin: Support RUN_SETUP_GENTLY to set up repository early if found
  config: run setup before commiting pager choice

 builtin/config.c |    6 +-
 cache.h          |    6 ++
 environment.c    |    1 +
 git.c            |   23 +++++--
 setup.c          |   12 ++++-
 t/t7006-pager.sh |  168 +++++++++++++++++++++++++++++++++++++++++-------------
 6 files changed, 165 insertions(+), 51 deletions(-)

[1] I warned that something like this was coming at
<http://thread.gmane.org/gmane.comp.version-control.git/144090>.

I will send a rebased version of the rest of the nd/setup series to
the list as a follow-up.  But beware: after perhaps introducing all
kinds of bugs through repeated rebasing, I didn’t give the later
patches of the series another glance.

[PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel

[Jonathan Nieder]

When git is passed the --paginate option, starting up a pager
requires deciding what pager to start, which requires access to the
core.pager configuration.

Unfortunately, the --paginate option is handled before git has a
chance to search for a git directory.  The effect is that with
--paginate and only with --paginate, a repository-local core.pager
setting does not take effect [*].

[*] unless the git directory is simply .git in the cwd or GIT_DIR
or GIT_CONFIG was set explicitly.

Add a test to demonstrate this counterintuitive behavior.

Noticed while reading over a patch by Nguyễn Thái Ngọc Duy that
fixes it.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d9202d5..4f804ed 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -164,6 +164,38 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	test -e core.pager_used
 '
 
+unset GIT_PAGER
+rm -f core.pager_used
+rm -fr sub
+test_expect_success TTY 'core.pager in subdir' '
+	PAGER=wc &&
+	stampname=$(pwd)/core.pager_used &&
+	export PAGER stampname &&
+	git config core.pager "wc > \"\$stampname\"" &&
+	mkdir sub &&
+	(
+		cd sub &&
+		test_terminal git log
+	) &&
+	test -e "$stampname"
+'
+
+unset GIT_PAGER
+rm -f core.pager_used
+rm -fr sub
+test_expect_failure TTY 'core.pager in subdir with --paginate' '
+	PAGER=wc &&
+	stampname=$(pwd)/core.pager_used &&
+	export PAGER stampname &&
+	git config core.pager "wc > \"\$stampname\"" &&
+	mkdir sub &&
+	(
+		cd sub &&
+		test_terminal git --paginate log
+	) &&
+	test -e "$stampname"
+'
+
 rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager wc &&
-- 
1.7.0.4

[PATCH 2/9] t7006: test pager configuration for several git commands

[Jonathan Nieder]

Test choice of pager at several stages of repository setup.  This
way we can have some (admittedly uninteresting) examples in mind when
considering future changes to the setup procedure.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |  151 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 80 insertions(+), 71 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f804ed..403c260 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -131,78 +131,87 @@ then
 	test_set_prereq SIMPLEPAGER
 fi
 
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
-	cat > $less <<-EOF &&
-	#!$SHELL_PATH
-	wc > default_pager_used
-	EOF
-	chmod +x $less &&
-	PATH=.:$PATH test_terminal git log &&
-	test -e default_pager_used
-'
-
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
-test_expect_success TTY 'PAGER overrides default pager' '
-	PAGER="wc > PAGER_used" &&
-	export PAGER &&
-	test_terminal git log &&
-	test -e PAGER_used
-'
-
-unset GIT_PAGER
-rm -f core.pager_used
-test_expect_success TTY 'core.pager overrides PAGER' '
-	PAGER=wc &&
-	export PAGER &&
-	git config core.pager "wc > core.pager_used" &&
-	test_terminal git log &&
-	test -e core.pager_used
-'
-
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_success TTY 'core.pager in subdir' '
-	PAGER=wc &&
-	stampname=$(pwd)/core.pager_used &&
-	export PAGER stampname &&
-	git config core.pager "wc > \"\$stampname\"" &&
-	mkdir sub &&
-	(
-		cd sub &&
-		test_terminal git log
-	) &&
-	test -e "$stampname"
-'
+test_pager_choice() {
+	if test $# = 1
+	then
+		cmd=$1
+		full_cmd="test_terminal $cmd"
+	else
+		cmd=$2
+		full_cmd="test_must_fail test_terminal $cmd"
+	fi
 
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_failure TTY 'core.pager in subdir with --paginate' '
-	PAGER=wc &&
-	stampname=$(pwd)/core.pager_used &&
-	export PAGER stampname &&
-	git config core.pager "wc > \"\$stampname\"" &&
-	mkdir sub &&
-	(
-		cd sub &&
-		test_terminal git --paginate log
-	) &&
-	test -e "$stampname"
-'
+	case "$cmd" in
+	*-p*)
+		test_expect_expected=test_expect_failure
+		;;
+	*)
+		test_expect_expected=test_expect_success
+		;;
+	esac
+
+	unset PAGER GIT_PAGER
+	git config --unset core.pager
+	rm -f default_pager_used
+	test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
+		cat > $less <<-\EOF &&
+		#!$SHELL_PATH
+		wc > default_pager_used
+		EOF
+		chmod +x $less &&
+		PATH=.:\$PATH $full_cmd &&
+		test -e default_pager_used
+	"
+
+	unset GIT_PAGER
+	git config --unset core.pager
+	rm -f PAGER_used
+	test_expect_success TTY "$cmd - PAGER overrides default pager" "
+		PAGER='wc > PAGER_used' &&
+		export PAGER &&
+		$full_cmd &&
+		test -e PAGER_used
+	"
+
+	unset GIT_PAGER
+	rm -f core.pager_used
+	test_expect_success TTY "$cmd - core.pager overrides PAGER" "
+		PAGER=wc &&
+		export PAGER &&
+		git config core.pager 'wc > core.pager_used' &&
+		$full_cmd &&
+		test -e core.pager_used
+	"
+
+	unset GIT_PAGER
+	rm -f core.pager_used
+	rm -fr sub
+	$test_expect_expected TTY "$cmd - core.pager in subdir" "
+		PAGER=wc &&
+		stampname=\$(pwd)/core.pager_used &&
+		export PAGER stampname &&
+		git config core.pager 'wc > \"\$stampname\"' &&
+		mkdir sub &&
+		(
+			cd sub &&
+			$full_cmd
+		) &&
+		test -e \"\$stampname\"
+	"
+
+	rm -f GIT_PAGER_used
+	test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
+		git config core.pager wc &&
+		GIT_PAGER='wc > GIT_PAGER_used' &&
+		export GIT_PAGER &&
+		$full_cmd &&
+		test -e GIT_PAGER_used
+	"
+}
 
-rm -f GIT_PAGER_used
-test_expect_success TTY 'GIT_PAGER overrides core.pager' '
-	git config core.pager wc &&
-	GIT_PAGER="wc > GIT_PAGER_used" &&
-	export GIT_PAGER &&
-	test_terminal git log &&
-	test -e GIT_PAGER_used
-'
+test_pager_choice 'git log'
+test_pager_choice 'git -p log'
+test_pager_choice test_must_fail 'git -p'
+test_pager_choice test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.0.4

[PATCH 3/9] builtins: do not commit pager choice early

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

If git is passed the --paginate option, committing the pager choice
will require setting up a pager, which requires access to repository
for the core.pager configuration.

After handle_options() is called, the repository has not been searched
for yet.  Unless GIT_DIR or GIT_CONFIG is set, attempting to access
the configuration at this point results in git_dir being set to .git,
which is almost certainly not what was wanted.  If a git command is
run from a subdirectory of the toplevel directory of a git repository
and the --paginate option is supplied, the core.pager setting from
that repository is not being respected.

There are several possible code paths after handle_options() and
commit_pager_choice() are called:

1. list_common_cmds_help() is a printout of 29 lines.  This can
   benefit from pagination, so do commit the pager choice before
   writing this output.

   Since ‘git’ without subcommand has no other reason to access the
   repository, ideally this would not involve accessing the
   repository-specific configuration file.  But for now, it is simpler
   to commit the pager choice using the funny code that would notice
   whatever repository happens to be at .git.  That this accesses a
   repository if it happens to be very convenient to is a bug but not
   a very important one.

2. run_argv() already commits pager choice inside run_builtin() if a
   command is found.  If the relevant git subcommand searches for a
   git directory, not commiting to a pager before then means the
   core.pager setting from the correct $GIT_DIR/config will be
   respected.

3. help_unknown_cmd() prints out a few lines to stderr.  It is not
   important to paginate this, so don’t.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Duy’s version did not allow paginating list_common_cmds_help() output.
I have an idea for fixing the setup procedure for that later, but one
thing at a time.  In other words, this is analogous to a lock pushdown
in a way: the immediate goal is not to eliminate problems but to reduce
their scope.

 git.c            |    2 +-
 t/t7006-pager.sh |   65 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/git.c b/git.c
index 6bae305..32720e5 100644
--- a/git.c
+++ b/git.c
@@ -502,12 +502,12 @@ int main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
-	commit_pager_choice();
 	if (argc > 0) {
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;
 	} else {
 		/* The user didn't specify a command; give them help */
+		commit_pager_choice();
 		printf("usage: %s\n\n", git_usage_string);
 		list_common_cmds_help();
 		printf("\n%s\n", git_more_info_string);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 403c260..944e830 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -132,23 +132,28 @@ then
 fi
 
 test_pager_choice() {
-	if test $# = 1
+	wrapper=test_terminal
+
+	if test "$1" = test_must_fail
 	then
-		cmd=$1
-		full_cmd="test_terminal $cmd"
-	else
-		cmd=$2
-		full_cmd="test_must_fail test_terminal $cmd"
+		wrapper="test_must_fail $wrapper"
+		shift
 	fi
 
-	case "$cmd" in
-	*-p*)
-		test_expect_expected=test_expect_failure
-		;;
-	*)
-		test_expect_expected=test_expect_success
-		;;
-	esac
+	cmd=$1
+	toplevel_expectation=${2:-success}
+	use_repository_config="${3:-yes}"
+
+	full_cmd="$wrapper $cmd"
+	test_expectation="test_expect_$toplevel_expectation"
+	if test "$use_repository_config" = no
+	then
+		if_want_local_config='! '
+		used_if_wanted='is not used'
+	else
+		if_want_local_config=
+		used_if_wanted='overrides PAGER'
+	fi
 
 	unset PAGER GIT_PAGER
 	git config --unset core.pager
@@ -175,18 +180,18 @@ test_pager_choice() {
 
 	unset GIT_PAGER
 	rm -f core.pager_used
-	test_expect_success TTY "$cmd - core.pager overrides PAGER" "
+	$test_expectation TTY "$cmd - core.pager ${used_if_wanted}" "
 		PAGER=wc &&
 		export PAGER &&
 		git config core.pager 'wc > core.pager_used' &&
 		$full_cmd &&
-		test -e core.pager_used
+		${if_want_local_config}test -e core.pager_used
 	"
 
 	unset GIT_PAGER
 	rm -f core.pager_used
 	rm -fr sub
-	$test_expect_expected TTY "$cmd - core.pager in subdir" "
+	test_expect_success TTY "$cmd - core.pager ${used_if_wanted} from subdirectory" "
 		PAGER=wc &&
 		stampname=\$(pwd)/core.pager_used &&
 		export PAGER stampname &&
@@ -196,7 +201,7 @@ test_pager_choice() {
 			cd sub &&
 			$full_cmd
 		) &&
-		test -e \"\$stampname\"
+		${if_want_local_config}test -e \"\$stampname\"
 	"
 
 	rm -f GIT_PAGER_used
@@ -209,9 +214,29 @@ test_pager_choice() {
 	"
 }
 
+doesnt_paginate() {
+	if test $# = 1
+	then
+		cmd=$1
+		full_cmd="test_terminal $cmd"
+	else
+		cmd=$2
+		full_cmd="test_must_fail test_terminal $cmd"
+	fi
+
+	rm -f GIT_PAGER_used
+	test_expect_success TTY "no pager for '$cmd'" "
+		GIT_PAGER='wc > GIT_PAGER_used' &&
+		export GIT_PAGER
+		$full_cmd &&
+		! test -e GIT_PAGER_used
+	"
+}
+
 test_pager_choice 'git log'
 test_pager_choice 'git -p log'
-test_pager_choice test_must_fail 'git -p'
-test_pager_choice test_must_fail 'git -p nonsense'
+test_pager_choice test_must_fail 'git -p' failure no
+
+doesnt_paginate test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.0.4

[PATCH 4/9] t7006: test pager.<cmd> configuration

[Jonathan Nieder]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 944e830..237a689 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -71,6 +71,22 @@ test_expect_success TTY 'git --paginate rev-list uses a pager' '
 	test -e paginated.out
 '
 
+rm -f paginated.out
+test_expect_success TTY 'git rev-list uses a pager if configured to' '
+	git config pager.rev-list true &&
+	test_terminal git rev-list HEAD &&
+	test -e paginated.out
+'
+git config --unset pager.rev-list
+
+rm -f paginated.out
+test_expect_failure TTY 'git config uses a pager if configured to' '
+	git config pager.config true &&
+	test_terminal git config --list &&
+	test -e paginated.out
+'
+git --no-pager config --unset pager.config
+
 rm -f file paginated.out
 test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 	git --paginate log | cat &&
@@ -239,4 +255,8 @@ test_pager_choice test_must_fail 'git -p' failure no
 
 doesnt_paginate test_must_fail 'git -p nonsense'
 
+doesnt_paginate 'git rev-list HEAD'
+git config pager.rev-list true
+test_pager_choice 'git rev-list HEAD'
+
 test_done
-- 
1.7.0.4

[PATCH 5/9] builtin: introduce startup_info struct

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

The startup_info struct will collect information managed by the git
setup code, such as the prefix for relative paths passed on the
command line (i.e., path to the starting cwd from the toplevel of
the work tree) and whether a git repository has been found.

In other words, startup_info is intended to be a collection of global
variables representing results that were previously returned from
setup functions.  Letting these values persist means there is more
flexibility in deciding when to run setup.

For now, the struct is empty.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Creation of struct split off from its population, mostly for
reviewability.  Feel free to merge this patch with the next one
if you prefer.

 cache.h       |    5 +++++
 environment.c |    1 +
 git.c         |    4 ++++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index 5eb0573..1479824 100644
--- a/cache.h
+++ b/cache.h
@@ -1058,6 +1058,11 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 char *alias_lookup(const char *alias);
 int split_cmdline(char *cmdline, const char ***argv);
 
+/* git.c */
+struct startup_info {
+};
+extern struct startup_info *startup_info;
+
 /* builtin/merge.c */
 int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
 
diff --git a/environment.c b/environment.c
index 876c5e5..c36c902 100644
--- a/environment.c
+++ b/environment.c
@@ -52,6 +52,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+struct startup_info *startup_info;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/git.c b/git.c
index 32720e5..b1d51c9 100644
--- a/git.c
+++ b/git.c
@@ -13,6 +13,7 @@ const char git_usage_string[] =
 const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
 
+static struct startup_info git_startup_info;
 static int use_pager = -1;
 struct pager_config {
 	const char *cmd;
@@ -477,6 +478,9 @@ int main(int argc, const char **argv)
 {
 	const char *cmd;
 
+	memset(&git_startup_info, 0, sizeof(git_startup_info));
+	startup_info = &git_startup_info;
+
 	cmd = git_extract_argv0_path(argv[0]);
 	if (!cmd)
 		cmd = "git-help";
-- 
1.7.0.4

[PATCH 6/9] builtin: remember whether repository was found

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Attempts to access gitdir are everywhere, even before cmd_*() is
called (in particular for alias handling, pager configuration, work
tree, and other core configuration).  Because the repository has not
been found, repository-specific information (GIT_DIR/config) can not
be read.  This can lead to obscure bugs.

The sooner we set up gitdir, the less trouble we may have to deal with.

Currently, the cmd_*() functions learn whether a repository was
found through the *nongit_ok return value to
setup_git_directory_gently().  If run_builtin() is to take care of
this sooner, then they will have to get it from somewhere else.
startup_info is a good place for this.

As a bonus, this information can be used to teach functions such as
git_config() to avoid trying to access a repository if none is
present.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 cache.h |    1 +
 setup.c |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index 1479824..030ba01 100644
--- a/cache.h
+++ b/cache.h
@@ -1060,6 +1060,7 @@ int split_cmdline(char *cmdline, const char ***argv);
 
 /* git.c */
 struct startup_info {
+	int have_repository;
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index 5716d90..3b4ff09 100644
--- a/setup.c
+++ b/setup.c
@@ -315,7 +315,7 @@ const char *read_gitfile_gently(const char *path)
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
-const char *setup_git_directory_gently(int *nongit_ok)
+static const char *setup_git_directory_gently_1(int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
@@ -443,6 +443,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	return cwd + offset;
 }
 
+const char *setup_git_directory_gently(int *nongit_ok)
+{
+	const char *prefix;
+
+	prefix = setup_git_directory_gently_1(nongit_ok);
+	if (startup_info)
+		startup_info->have_repository = !nongit_ok || !*nongit_ok;
+	return prefix;
+}
+
 int git_config_perm(const char *var, const char *value)
 {
 	int i;
-- 
1.7.0.4

[PATCH 7/9] builtin: Support RUN_SETUP_GENTLY to set up repository early if found

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Attempts to access gitdir are everywhere, even before cmd_*() is
called (examples: alias handling, pager configuration, work tree).
Because the repository has not been found, repository-specific
information (i.e., $GIT_DIR/config) can not be read.  This leads to
obscure bugs.

So the sooner we setup gitdir, the less trouble we may have to deal with.

This patch sets up the infrastructure.  Later patches will teach
particular commands to use it where applicable.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index b1d51c9..20e9bfc 100644
--- a/git.c
+++ b/git.c
@@ -220,13 +220,14 @@ static int handle_alias(int *argcp, const char ***argv)
 
 const char git_version_string[] = GIT_VERSION;
 
-#define RUN_SETUP	(1<<0)
-#define USE_PAGER	(1<<1)
+#define RUN_SETUP		(1<<0)
+#define USE_PAGER		(1<<1)
 /*
  * require working tree to be present -- anything uses this needs
  * RUN_SETUP for reading from the configuration file.
  */
-#define NEED_WORK_TREE	(1<<2)
+#define NEED_WORK_TREE		(1<<2)
+#define RUN_SETUP_GENTLY	(1<<3)
 
 struct cmd_struct {
 	const char *cmd;
@@ -245,6 +246,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	if (!help) {
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
+		if (p->option & RUN_SETUP_GENTLY) {
+			int nongit_ok;
+			prefix = setup_git_directory_gently(&nongit_ok);
+		}
 
 		if (use_pager == -1 && p->option & RUN_SETUP)
 			use_pager = check_pager_config(p->cmd);
-- 
1.7.0.4

[PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Jonathan Nieder]

Eventually, all git commands should check their configuration at
start-up.  For now, reading configuration before repository discovery
is dangerous because it could cause a pager.<cmd> setting from the
current repository to be neglected.

But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 20e9bfc..56e93cf 100644
--- a/git.c
+++ b/git.c
@@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & RUN_SETUP)
+		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
-- 
1.7.0.4

[PATCH 9/9] config: run setup before commiting pager choice

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

For the pager choice (and the choice to paginate in the first place)
to reflect the current repository configuration, the repository
needs to be searched for first.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That’s the end of the mini-series.  I hope you enjoyed reading it as
much as I did adapting it.

Looking forward to your thoughts,
Jonathan

 builtin/config.c |    6 ++----
 git.c            |    4 ++--
 t/t7006-pager.sh |   16 ++++++++++------
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 4bc46b1..ecc8f87 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -326,11 +326,9 @@ static int get_colorbool(int print)
 		return get_colorbool_found ? 0 : 1;
 }
 
-int cmd_config(int argc, const char **argv, const char *unused_prefix)
+int cmd_config(int argc, const char **argv, const char *prefix)
 {
-	int nongit;
 	char *value;
-	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
 
@@ -409,7 +407,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	}
 	else if (actions == ACTION_EDIT) {
 		check_argc(argc, 0, 0);
-		if (!config_exclusive_filename && nongit)
+		if (!config_exclusive_filename && !startup_info->have_repository)
 			die("not in a git directory");
 		git_config(git_default_config, NULL);
 		launch_editor(config_exclusive_filename ?
diff --git a/git.c b/git.c
index 56e93cf..0a0d9eb 100644
--- a/git.c
+++ b/git.c
@@ -309,7 +309,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-		{ "config", cmd_config },
+		{ "config", cmd_config, RUN_SETUP_GENTLY },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
@@ -366,7 +366,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "reflog", cmd_reflog, RUN_SETUP },
 		{ "remote", cmd_remote, RUN_SETUP },
 		{ "replace", cmd_replace, RUN_SETUP },
-		{ "repo-config", cmd_config },
+		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
 		{ "rerere", cmd_rerere, RUN_SETUP },
 		{ "reset", cmd_reset, RUN_SETUP },
 		{ "rev-list", cmd_rev_list, RUN_SETUP },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 237a689..e58b14d 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -80,7 +80,7 @@ test_expect_success TTY 'git rev-list uses a pager if configured to' '
 git config --unset pager.rev-list
 
 rm -f paginated.out
-test_expect_failure TTY 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
 	git config pager.config true &&
 	test_terminal git config --list &&
 	test -e paginated.out
@@ -172,7 +172,7 @@ test_pager_choice() {
 	fi
 
 	unset PAGER GIT_PAGER
-	git config --unset core.pager
+	git --no-pager config --unset core.pager
 	rm -f default_pager_used
 	test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
 		cat > $less <<-\EOF &&
@@ -185,7 +185,7 @@ test_pager_choice() {
 	"
 
 	unset GIT_PAGER
-	git config --unset core.pager
+	git --no-pager config --unset core.pager
 	rm -f PAGER_used
 	test_expect_success TTY "$cmd - PAGER overrides default pager" "
 		PAGER='wc > PAGER_used' &&
@@ -199,7 +199,7 @@ test_pager_choice() {
 	$test_expectation TTY "$cmd - core.pager ${used_if_wanted}" "
 		PAGER=wc &&
 		export PAGER &&
-		git config core.pager 'wc > core.pager_used' &&
+		git --no-pager config core.pager 'wc > core.pager_used' &&
 		$full_cmd &&
 		${if_want_local_config}test -e core.pager_used
 	"
@@ -211,7 +211,7 @@ test_pager_choice() {
 		PAGER=wc &&
 		stampname=\$(pwd)/core.pager_used &&
 		export PAGER stampname &&
-		git config core.pager 'wc > \"\$stampname\"' &&
+		git --no-pager config core.pager 'wc > \"\$stampname\"' &&
 		mkdir sub &&
 		(
 			cd sub &&
@@ -222,7 +222,7 @@ test_pager_choice() {
 
 	rm -f GIT_PAGER_used
 	test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
-		git config core.pager wc &&
+		git --no-pager config core.pager wc &&
 		GIT_PAGER='wc > GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_cmd &&
@@ -259,4 +259,8 @@ doesnt_paginate 'git rev-list HEAD'
 git config pager.rev-list true
 test_pager_choice 'git rev-list HEAD'
 
+doesnt_paginate 'git config -l'
+git config pager.config true
+test_pager_choice 'git config -l'
+
 test_done
-- 
1.7.0.4

[RFC/PATCH 00/46] nd/setup remainder for convenient reference

[Jonathan Nieder]

I was thinking of sending the remainder of nd/setup after this to save
others the trouble of rebasing, but I fear the traffic would be too
disruptive.  So I am putting it up by git.

Caveats: maybe these patches make no sense.  After I last rebased
them, I walked away without looking.  Still, maybe they can save
you some time.

Good night,
Jonathan

The following changes since commit e0cc2322c409c6cfa897f395f6feb058e2d53d4a:
  Nguyễn Thái Ngọc Duy (1):
        config: run setup before commiting pager choice

are available in the git repository at:

  git://repo.or.cz/git/jrn.git/ nd/setup

Jonathan Nieder (5):
      t0001: test git init when run via an alias
      t5512: test that ls-remote does not require a git repository
      t7002: test git grep --no-index from a bare repository
      t7006: expose test_terminal() for use by other tests
      help: note why it is appropriate for this command not to use RUN_SETUP_GENTLY

Nguyễn Thái Ngọc Duy (41):
      t1300: test that scripted commands do not respect stray .git/config
      setup: save prefix (original cwd relative to toplevel) in startup_info
      builtin: remove prefix variable from run_builtin()
      run_builtin(): save "-h" detection result for later use
      builtin: remember whether repository has been searched for
      builtin: USE_PAGER should not be used without RUN_SETUP*
      hash-object: use RUN_SETUP_GENTLY
      shortlog: use RUN_SETUP_GENTLY
      grep: use RUN_SETUP_GENTLY
      archive: use RUN_SETUP_GENTLY
      mailinfo: use RUN_SETUP_GENTLY
      check-ref-format: use RUN_SETUP_GENTLY
      verify-pack: use RUN_SETUP_GENTLY
      apply: use RUN_SETUP_GENTLY
      bundle: use RUN_SETUP_GENTLY
      diff: use RUN_SETUP_GENTLY
      ls-remote: use RUN_SETUP_GENTLY
      var: use RUN_SETUP_GENTLY
      merge-file: use RUN_SETUP_GENTLY
      worktree setup: calculate prefix even if no worktree is found
      index-pack: trust the prefix returned by setup_git_directory_gently()
      index-pack: use RUN_SETUP_GENTLY
      Move enter_repo() to setup.c
      enter_repo(): initialize other variables as setup_git_directory_gently() does
      rev-parse --git-dir: print relative gitdir correctly
      worktree setup: call set_git_dir explicitly
      Add git_config_early()
      Use git_config_early() instead of git_config() during repo setup
      worktree setup: restore original state when things go wrong
      init/clone: turn on startup->have_repository properly
      git_config(): do not read .git/config if there is no repository
      Do not read .git/info/exclude if there is no repository
      Do not read .git/info/attributes if there is no repository
      apply: do not check sha1 if there is no repository
      config: do not read .git/config if there is no repository
      builtins: utilize startup_info->help where possible
      builtins: check for startup_info->help, print and exit early
      Allow to undo setup_git_directory_gently() gracefully (and fix alias code)
      alias: keep repository found while collecting aliases as long as possible
      Guard unallowed access to repository when it's not set up
      builtins: setup repository before print unknown command error

 attr.c                                    |    5 +-
 builtin/apply.c                           |    9 +-
 builtin/archive.c                         |    2 +-
 builtin/branch.c                          |    3 +
 builtin/bundle.c                          |    6 +-
 builtin/check-ref-format.c                |    2 +-
 builtin/checkout-index.c                  |    3 +
 builtin/clone.c                           |    3 +-
 builtin/commit.c                          |    6 +
 builtin/config.c                          |    9 +-
 builtin/diff.c                            |    6 +-
 builtin/gc.c                              |    3 +
 builtin/grep.c                            |   11 +-
 builtin/hash-object.c                     |    9 +-
 builtin/help.c                            |    5 +
 builtin/index-pack.c                      |   21 +--
 builtin/init-db.c                         |   10 +-
 builtin/log.c                             |    6 +-
 builtin/ls-files.c                        |    3 +
 builtin/ls-remote.c                       |    3 -
 builtin/mailinfo.c                        |    3 -
 builtin/merge-file.c                      |    4 +-
 builtin/merge-ours.c                      |    2 +-
 builtin/merge.c                           |    3 +
 builtin/pack-redundant.c                  |    2 +-
 builtin/rev-parse.c                       |    8 +
 builtin/shortlog.c                        |    4 +-
 builtin/show-ref.c                        |    2 +-
 builtin/update-index.c                    |    3 +
 builtin/upload-archive.c                  |    7 +-
 builtin/var.c                             |    2 -
 cache.h                                   |    8 +-
 config.c                                  |   22 +++-
 dir.c                                     |    8 +-
 environment.c                             |   31 ++++-
 git.c                                     |   84 +++++++-----
 help.c                                    |    4 +
 path.c                                    |   91 ------------
 setup.c                                   |  213 ++++++++++++++++++++++++++---
 t/lib-pager.sh                            |   37 +++++
 t/{t7006 => lib-pager}/test-terminal.perl |    0
 t/t0001-init.sh                           |   56 ++++++++
 t/t1300-repo-config.sh                    |   15 ++
 t/t1302-repo-version.sh                   |    2 +-
 t/t1500-rev-parse.sh                      |    2 +-
 t/t1501-worktree.sh                       |    5 +
 t/t5512-ls-remote.sh                      |   14 ++
 t/t7002-grep.sh                           |  116 ++++++++++++++++
 t/t7006-pager.sh                          |   29 +----
 t/t7020-help.sh                           |   18 +++
 50 files changed, 660 insertions(+), 260 deletions(-)
 create mode 100644 t/lib-pager.sh
 rename t/{t7006 => lib-pager}/test-terminal.perl (100%)
 create mode 100755 t/t7020-help.sh

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Nguyen Thai Ngoc Duy]

2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> Eventually, all git commands should check their configuration at
> start-up.  For now, reading configuration before repository discovery
> is dangerous because it could cause a pager.<cmd> setting from the
> current repository to be neglected.
>
> But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  git.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git.c b/git.c
> index 20e9bfc..56e93cf 100644
> --- a/git.c
> +++ b/git.c
> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>                        prefix = setup_git_directory_gently(&nongit_ok);
>                }
>
> -               if (use_pager == -1 && p->option & RUN_SETUP)
> +               if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
>                        use_pager = check_pager_config(p->cmd);
>                if (use_pager == -1 && p->option & USE_PAGER)
>                        use_pager = 1;

This still leaves a chance of going wrong: when user explicitly gives
"--paginate", use_pager will be 1, but commands like "git init" does
not have RUN_SETUP*. So when setup_pager is called later on, it will
mess things up. This could be solved completely (indeed I have a patch
under testing), but it would require unset_git_directory(), making
this series a bit longer :(


-- 
Duy

Re: [PATCH 2/9] t7006: test pager configuration for several git commands

[Johannes Sixt]

Am 4/13/2010 4:17, schrieb Jonathan Nieder:
> +test_pager_choice() {
> +...
> +	test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
> +		cat > $less <<-\EOF &&
> +		#!$SHELL_PATH

This shell script is simple enough that you can use '#!/bin/sh' and avoid
$ expansion where one must ask whether it needs quoting or not.

> +		wc > default_pager_used
> +		EOF
> +...
> +	"
> +...
> +	test_expect_success TTY "$cmd - PAGER overrides default pager" "
> +...
> +	$test_expect_expected TTY "$cmd - core.pager in subdir" "
> +...
> +	test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
> +...
>  
> +test_pager_choice 'git log'
> +test_pager_choice 'git -p log'
> +test_pager_choice test_must_fail 'git -p'
> +test_pager_choice test_must_fail 'git -p nonsense'

The intents of the test cases would be much easier to follow and to review
if this were written as, for example:

test_default_pager        'git log'
test_PAGER_overrides      'git log'
test_core_pager_overrides 'git log'
test_core_pager_subdir    'git log'
test_GIT_PAGER_overrides  'git log'
test_default_pager        'git -p log'
test_PAGER_overrides      'git -p log'
test_core_pager_overrides 'git -p log'
test_core_pager_subdir    'git -p log' fails
test_GIT_PAGER_overrides  'git -p log'
test_default_pager        'test_must_fail git -p'
test_PAGER_overrides      'test_must_fail git -p'
test_core_pager_overrides 'test_must_fail git -p'
test_core_pager_subdir    'test_must_fail git -p' fails
test_GIT_PAGER_overrides  'test_must_fail git -p'
test_default_pager        'test_must_fail git -p nonsense'
test_PAGER_overrides      'test_must_fail git -p nonsense'
test_core_pager_overrides 'test_must_fail git -p nonsense'
test_core_pager_subdir    'test_must_fail git -p nonsense' fails
test_GIT_PAGER_overrides  'test_must_fail git -p nonsense'

the point being that it is less factorized (not the exact syntax).

-- Hannes

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Nguyen Thai Ngoc Duy]

2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> diff --git a/git.c b/git.c
> index 20e9bfc..56e93cf 100644
> --- a/git.c
> +++ b/git.c
> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>                        prefix = setup_git_directory_gently(&nongit_ok);
>                }
>
> -               if (use_pager == -1 && p->option & RUN_SETUP)
> +               if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
>                        use_pager = check_pager_config(p->cmd);
>                if (use_pager == -1 && p->option & USE_PAGER)
>                        use_pager = 1;

On the second look, there's another case, when RUN_SETUP_GENTLY is
set, but no repository is found. git_config() will pick
$(cwd)/.git/config if it exists. I guess it's OK for this series
because the true fix will require more changes.
-- 
Duy

Re: [PATCH 2/9] t7006: test pager configuration for several git commands

[Jonathan Nieder]

Johannes Sixt wrote:

> The intents of the test cases would be much easier to follow and to review
> if this were written as, for example:
> 
> test_default_pager        'git log'
> test_PAGER_overrides      'git log'
> test_core_pager_overrides 'git log'
> test_core_pager_subdir    'git log'
> test_GIT_PAGER_overrides  'git log'
> test_default_pager        'git -p log'
[...]

Good idea.  This would also make the expected result much easier to
change test-by-test.  Will do, thanks.

Jonathan

[PATCH 2/9 v2] t7006: test pager configuration for several git commands

[Jonathan Nieder]

Test choice of pager at several stages of repository setup.  Some
patches under consideration will produce different results for
different commands, so it should be useful to have examples like this
in mind to illustrate their effect.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Johannes Sixt <j6t@kdbg.org>
---
Changes from v1:
 - Use /bin/sh instead of $SHELL_PATH to simplify quoting issues
 - Split the test_pager_choice function into single-test pieces to
   make the test easier to understand.

I’ll resend the patches 3 and 9 with corresponding adjustments in a
moment.

Thanks to Hannes for the feedback.

 t/t7006-pager.sh |  192 +++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 124 insertions(+), 68 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f804ed..d5f8a18 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -131,78 +131,134 @@ then
 	test_set_prereq SIMPLEPAGER
 fi
 
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
-	cat > $less <<-EOF &&
-	#!$SHELL_PATH
-	wc > default_pager_used
-	EOF
-	chmod +x $less &&
-	PATH=.:$PATH test_terminal git log &&
-	test -e default_pager_used
-'
+# Tests for pager choice.
 
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
-test_expect_success TTY 'PAGER overrides default pager' '
-	PAGER="wc > PAGER_used" &&
-	export PAGER &&
-	test_terminal git log &&
-	test -e PAGER_used
-'
+# Usage:
+#	parse_args expect_(success|failure) [test_must_fail] git whatever
+#	$test_expectation "$cmd - behaves well" "
+#		...
+#		$full_command &&
+#		...
+#	"
+parse_args() {
+	test_expectation="test_$1"
+	shift
+	if test "$1" = test_must_fail
+	then
+		full_command="test_must_fail test_terminal "
+		shift
+	else
+		full_command="test_terminal "
+	fi
+	cmd=$1
+	full_command="$full_command $1"
+}
 
-unset GIT_PAGER
-rm -f core.pager_used
-test_expect_success TTY 'core.pager overrides PAGER' '
-	PAGER=wc &&
-	export PAGER &&
-	git config core.pager "wc > core.pager_used" &&
-	test_terminal git log &&
-	test -e core.pager_used
-'
+# usage: test_default_pager expect_(success|failure) [test_must_fail] command
+test_default_pager() {
+	parse_args "$@"
 
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_success TTY 'core.pager in subdir' '
-	PAGER=wc &&
-	stampname=$(pwd)/core.pager_used &&
-	export PAGER stampname &&
-	git config core.pager "wc > \"\$stampname\"" &&
-	mkdir sub &&
-	(
-		cd sub &&
-		test_terminal git log
-	) &&
-	test -e "$stampname"
-'
+	unset PAGER GIT_PAGER
+	git config --unset core.pager
+	rm -f default_pager_used
+	$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
+		cat >$less <<-\EOF &&
+			#!/bin/sh
+			wc > default_pager_used
+		EOF
+		chmod +x $less &&
+		(
+			PATH=.:$PATH &&
+			export PATH &&
+			$full_command
+		) &&
+		test -e default_pager_used
+	"
+}
 
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_failure TTY 'core.pager in subdir with --paginate' '
-	PAGER=wc &&
-	stampname=$(pwd)/core.pager_used &&
-	export PAGER stampname &&
-	git config core.pager "wc > \"\$stampname\"" &&
-	mkdir sub &&
-	(
-		cd sub &&
-		test_terminal git --paginate log
-	) &&
-	test -e "$stampname"
-'
+test_PAGER_overrides() {
+	parse_args "$@"
 
-rm -f GIT_PAGER_used
-test_expect_success TTY 'GIT_PAGER overrides core.pager' '
-	git config core.pager wc &&
-	GIT_PAGER="wc > GIT_PAGER_used" &&
-	export GIT_PAGER &&
-	test_terminal git log &&
-	test -e GIT_PAGER_used
-'
+	unset GIT_PAGER
+	git config --unset core.pager
+	rm -f PAGER_used
+	$test_expectation TTY "$cmd - PAGER overrides default pager" "
+		PAGER='wc > PAGER_used' &&
+		export PAGER &&
+		$full_command &&
+		test -e PAGER_used
+	"
+}
+
+test_core_pager_overrides() {
+	parse_args "$@"
+
+	unset GIT_PAGER
+	rm -f core.pager_used
+	$test_expectation TTY "$cmd - core.pager overrides PAGER" "
+		PAGER=wc &&
+		export PAGER &&
+		git config core.pager 'wc > core.pager_used' &&
+		$full_command &&
+		test -e core.pager_used
+	"
+}
+
+test_core_pager_subdir() {
+	parse_args "$@"
+
+	unset GIT_PAGER
+	rm -f core.pager_used
+	rm -fr sub
+	$test_expectation TTY "$cmd - core.pager from subdirectory" "
+		stampname=\$(pwd)/core.pager_used &&
+		PAGER=wc &&
+		export PAGER stampname &&
+		git config core.pager 'wc > \"\$stampname\"' &&
+		mkdir sub &&
+		(
+			cd sub &&
+			$full_command
+		) &&
+		test -e \"\$stampname\"
+	"
+}
+
+test_GIT_PAGER_overrides() {
+	parse_args "$@"
+
+	rm -f GIT_PAGER_used
+	$test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
+		git config core.pager wc &&
+		GIT_PAGER='wc > GIT_PAGER_used' &&
+		export GIT_PAGER &&
+		$full_command &&
+		test -e GIT_PAGER_used
+	"
+}
+
+test_default_pager        expect_success 'git log'
+test_PAGER_overrides      expect_success 'git log'
+test_core_pager_overrides expect_success 'git log'
+test_core_pager_subdir    expect_success 'git log'
+test_GIT_PAGER_overrides  expect_success 'git log'
+
+test_default_pager        expect_success 'git -p log'
+test_PAGER_overrides      expect_success 'git -p log'
+test_core_pager_overrides expect_success 'git -p log'
+test_core_pager_subdir    expect_failure 'git -p log'
+test_GIT_PAGER_overrides  expect_success 'git -p log'
+
+test_default_pager        expect_success test_must_fail 'git -p'
+test_PAGER_overrides      expect_success test_must_fail 'git -p'
+test_core_pager_overrides expect_success test_must_fail 'git -p'
+test_core_pager_subdir    expect_failure test_must_fail 'git -p'
+test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
+
+test_default_pager        expect_success test_must_fail 'git -p nonsense'
+test_PAGER_overrides      expect_success test_must_fail 'git -p nonsense'
+test_core_pager_overrides expect_success test_must_fail 'git -p nonsense'
+test_core_pager_subdir    expect_failure test_must_fail 'git -p nonsense'
+test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.0.4

[PATCH 3/9 v2] builtins: do not commit pager choice early

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

If git is passed the --paginate option, committing the pager choice
will require setting up a pager, which requires access to repository
for the core.pager configuration.

After handle_options() is called, the repository has not been searched
for yet.  Unless GIT_DIR or GIT_CONFIG is set, attempting to access
the configuration at this point results in git_dir being set to .git,
which is almost certainly not what was wanted.  If a git command is
run from a subdirectory of the toplevel directory of a git repository
and the --paginate option is supplied, the core.pager setting from
that repository is not being respected.

There are several possible code paths after handle_options() and
commit_pager_choice() are called:

1. list_common_cmds_help() is a printout of 29 lines.  This can
   benefit from pagination, so do commit the pager choice before
   writing this output.

   Since ‘git’ without subcommand has no other reason to access the
   repository, ideally this would not involve accessing the
   repository-specific configuration file.  But for now, it is simpler
   to commit the pager choice using the funny code that would notice
   whatever repository happens to be at .git.  That this accesses a
   repository if it happens to be very convenient is a bug but not
   a very important one.

2. run_argv() already commits pager choice inside run_builtin() if a
   command is found.  If the relevant git subcommand searches for a
   git directory, not commiting to a pager before then means the
   core.pager setting from the correct $GIT_DIR/config will be
   respected.

3. help_unknown_cmd() prints out a few lines to stderr.  It is not
   important to paginate this, so don’t.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased on top of patch 2 v2

 git.c            |    2 +-
 t/t7006-pager.sh |   63 +++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/git.c b/git.c
index 6bae305..32720e5 100644
--- a/git.c
+++ b/git.c
@@ -502,12 +502,12 @@ int main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
-	commit_pager_choice();
 	if (argc > 0) {
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;
 	} else {
 		/* The user didn't specify a command; give them help */
+		commit_pager_choice();
 		printf("usage: %s\n\n", git_usage_string);
 		list_common_cmds_help();
 		printf("\n%s\n", git_more_info_string);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d5f8a18..a240b15 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -190,27 +190,58 @@ test_PAGER_overrides() {
 	"
 }
 
+parse_ifwanted_arg() {
+	if test "$1" = yes
+	then
+		if_want_local_config=
+		used_if_wanted='overrides PAGER'
+	else
+		if_want_local_config='! '
+		used_if_wanted='is not used'
+	fi
+}
+
 test_core_pager_overrides() {
+	test_core_pager yes "$@"
+}
+
+test_local_config_ignored() {
+	test_core_pager no "$@"
+}
+
+test_core_pager() {
+	parse_ifwanted_arg "$1"
+	shift
 	parse_args "$@"
 
 	unset GIT_PAGER
 	rm -f core.pager_used
-	$test_expectation TTY "$cmd - core.pager overrides PAGER" "
+	$test_expectation TTY "$cmd - core.pager $used_if_wanted" "
 		PAGER=wc &&
 		export PAGER &&
 		git config core.pager 'wc > core.pager_used' &&
 		$full_command &&
-		test -e core.pager_used
+		${if_want_local_config}test -e core.pager_used
 	"
 }
 
 test_core_pager_subdir() {
+	test_pager_subdir_helper yes "$@"
+}
+
+test_no_local_config_subdir() {
+	test_pager_subdir_helper no "$@"
+}
+
+test_pager_subdir_helper() {
+	parse_ifwanted_arg "$1"
+	shift
 	parse_args "$@"
 
 	unset GIT_PAGER
 	rm -f core.pager_used
 	rm -fr sub
-	$test_expectation TTY "$cmd - core.pager from subdirectory" "
+	$test_expectation TTY "$cmd - core.pager $used_if_wanted from subdirectory" "
 		stampname=\$(pwd)/core.pager_used &&
 		PAGER=wc &&
 		export PAGER stampname &&
@@ -220,7 +251,7 @@ test_core_pager_subdir() {
 			cd sub &&
 			$full_command
 		) &&
-		test -e \"\$stampname\"
+		${if_want_local_config}test -e \"\$stampname\"
 	"
 }
 
@@ -237,6 +268,18 @@ test_GIT_PAGER_overrides() {
 	"
 }
 
+test_doesnt_paginate() {
+	parse_args "$@"
+
+	rm -f GIT_PAGER_used
+	$test_expectation TTY "no pager for '$cmd'" "
+		GIT_PAGER='wc > GIT_PAGER_used' &&
+		export GIT_PAGER &&
+		$full_command &&
+		! test -e GIT_PAGER_used
+	"
+}
+
 test_default_pager        expect_success 'git log'
 test_PAGER_overrides      expect_success 'git log'
 test_core_pager_overrides expect_success 'git log'
@@ -246,19 +289,15 @@ test_GIT_PAGER_overrides  expect_success 'git log'
 test_default_pager        expect_success 'git -p log'
 test_PAGER_overrides      expect_success 'git -p log'
 test_core_pager_overrides expect_success 'git -p log'
-test_core_pager_subdir    expect_failure 'git -p log'
+test_core_pager_subdir    expect_success 'git -p log'
 test_GIT_PAGER_overrides  expect_success 'git -p log'
 
 test_default_pager        expect_success test_must_fail 'git -p'
 test_PAGER_overrides      expect_success test_must_fail 'git -p'
-test_core_pager_overrides expect_success test_must_fail 'git -p'
-test_core_pager_subdir    expect_failure test_must_fail 'git -p'
+test_local_config_ignored expect_failure test_must_fail 'git -p'
+test_no_local_config_subdir expect_success test_must_fail 'git -p'
 test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
-test_default_pager        expect_success test_must_fail 'git -p nonsense'
-test_PAGER_overrides      expect_success test_must_fail 'git -p nonsense'
-test_core_pager_overrides expect_success test_must_fail 'git -p nonsense'
-test_core_pager_subdir    expect_failure test_must_fail 'git -p nonsense'
-test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p nonsense'
+test_doesnt_paginate expect_success test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.0.4

[PATCH 4/9 v2] t7006: test pager.<cmd> configuration

[Jonathan Nieder]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased against patch 3 v2.

 t/t7006-pager.sh |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a240b15..97ca5d6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -71,6 +71,22 @@ test_expect_success TTY 'git --paginate rev-list uses a pager' '
 	test -e paginated.out
 '
 
+rm -f paginated.out
+test_expect_success TTY 'git rev-list uses a pager if configured to' '
+	git config pager.rev-list true &&
+	test_terminal git rev-list HEAD &&
+	test -e paginated.out
+'
+git config --unset pager.rev-list
+
+rm -f paginated.out
+test_expect_failure TTY 'git config uses a pager if configured to' '
+	git config pager.config true &&
+	test_terminal git config --list &&
+	test -e paginated.out
+'
+git --no-pager config --unset pager.config
+
 rm -f file paginated.out
 test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 	git --paginate log | cat &&
@@ -300,4 +316,14 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
 test_doesnt_paginate expect_success test_must_fail 'git -p nonsense'
 
+test_doesnt_paginate expect_success 'git rev-list HEAD'
+test_expect_success 'set up rev-list to use a pager' '
+	git config pager.rev-list true
+'
+test_default_pager        expect_success 'git rev-list HEAD'
+test_PAGER_overrides      expect_success 'git rev-list HEAD'
+test_core_pager_overrides expect_success 'git rev-list HEAD'
+test_core_pager_subdir    expect_success 'git rev-list HEAD'
+test_GIT_PAGER_overrides  expect_success 'git rev-list HEAD'
+
 test_done
-- 
1.7.0.4

[PATCH 9/9 v2] config: run setup before commiting pager choice

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

For the pager choice (and the choice to paginate in the first place)
to reflect the current repository configuration, the repository
needs to be searched for first.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased for changes to t7006 in patches 2, 3, 4 v2.

 builtin/config.c |    6 ++----
 git.c            |    4 ++--
 t/t7006-pager.sh |   22 ++++++++++++++++------
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 4bc46b1..ecc8f87 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -326,11 +326,9 @@ static int get_colorbool(int print)
 		return get_colorbool_found ? 0 : 1;
 }
 
-int cmd_config(int argc, const char **argv, const char *unused_prefix)
+int cmd_config(int argc, const char **argv, const char *prefix)
 {
-	int nongit;
 	char *value;
-	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
 
@@ -409,7 +407,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	}
 	else if (actions == ACTION_EDIT) {
 		check_argc(argc, 0, 0);
-		if (!config_exclusive_filename && nongit)
+		if (!config_exclusive_filename && !startup_info->have_repository)
 			die("not in a git directory");
 		git_config(git_default_config, NULL);
 		launch_editor(config_exclusive_filename ?
diff --git a/git.c b/git.c
index 56e93cf..0a0d9eb 100644
--- a/git.c
+++ b/git.c
@@ -309,7 +309,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-		{ "config", cmd_config },
+		{ "config", cmd_config, RUN_SETUP_GENTLY },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
@@ -366,7 +366,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "reflog", cmd_reflog, RUN_SETUP },
 		{ "remote", cmd_remote, RUN_SETUP },
 		{ "replace", cmd_replace, RUN_SETUP },
-		{ "repo-config", cmd_config },
+		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
 		{ "rerere", cmd_rerere, RUN_SETUP },
 		{ "reset", cmd_reset, RUN_SETUP },
 		{ "rev-list", cmd_rev_list, RUN_SETUP },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 97ca5d6..d649f55 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -80,7 +80,7 @@ test_expect_success TTY 'git rev-list uses a pager if configured to' '
 git config --unset pager.rev-list
 
 rm -f paginated.out
-test_expect_failure TTY 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
 	git config pager.config true &&
 	test_terminal git config --list &&
 	test -e paginated.out
@@ -175,7 +175,7 @@ test_default_pager() {
 	parse_args "$@"
 
 	unset PAGER GIT_PAGER
-	git config --unset core.pager
+	git --no-pager config --unset core.pager
 	rm -f default_pager_used
 	$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
 		cat >$less <<-\EOF &&
@@ -196,7 +196,7 @@ test_PAGER_overrides() {
 	parse_args "$@"
 
 	unset GIT_PAGER
-	git config --unset core.pager
+	git --no-pager config --unset core.pager
 	rm -f PAGER_used
 	$test_expectation TTY "$cmd - PAGER overrides default pager" "
 		PAGER='wc > PAGER_used' &&
@@ -235,7 +235,7 @@ test_core_pager() {
 	$test_expectation TTY "$cmd - core.pager $used_if_wanted" "
 		PAGER=wc &&
 		export PAGER &&
-		git config core.pager 'wc > core.pager_used' &&
+		git --no-pager config core.pager 'wc > core.pager_used' &&
 		$full_command &&
 		${if_want_local_config}test -e core.pager_used
 	"
@@ -261,7 +261,7 @@ test_pager_subdir_helper() {
 		stampname=\$(pwd)/core.pager_used &&
 		PAGER=wc &&
 		export PAGER stampname &&
-		git config core.pager 'wc > \"\$stampname\"' &&
+		git --no-pager config core.pager 'wc > \"\$stampname\"' &&
 		mkdir sub &&
 		(
 			cd sub &&
@@ -276,7 +276,7 @@ test_GIT_PAGER_overrides() {
 
 	rm -f GIT_PAGER_used
 	$test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
-		git config core.pager wc &&
+		git --no-pager config core.pager wc &&
 		GIT_PAGER='wc > GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
@@ -326,4 +326,14 @@ test_core_pager_overrides expect_success 'git rev-list HEAD'
 test_core_pager_subdir    expect_success 'git rev-list HEAD'
 test_GIT_PAGER_overrides  expect_success 'git rev-list HEAD'
 
+test_doesnt_paginate expect_success 'git config -l'
+test_expect_success "set up 'git config' to use a pager" '
+	git config pager.config true
+'
+test_default_pager        expect_success 'git config -l'
+test_PAGER_overrides      expect_success 'git config -l'
+test_core_pager_overrides expect_success 'git config -l'
+test_core_pager_subdir    expect_success 'git config -l'
+test_GIT_PAGER_overrides  expect_success 'git config -l'
+
 test_done
-- 
1.7.0.4

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:
> 2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:

>> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
[...]
>> -		if (use_pager == -1 && p->option & RUN_SETUP)
>> +		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
>>			use_pager = check_pager_config(p->cmd);
>>		if (use_pager == -1 && p->option & USE_PAGER)
>>			use_pager = 1;
>
> This still leaves a chance of going wrong: when user explicitly gives
> "--paginate", use_pager will be 1, but commands like "git init" does
> not have RUN_SETUP*. So when setup_pager is called later on, it will
> mess things up.

I decided not to check_pager_config() unconditionally to avoid
breaking ‘git diff’ (especially ‘git diff --exit-code’).  Maybe that
is too conservative; not sure.  The commit message should be more
explicit, something like:

   Eventually, all git commands should check their configuration at
   start-up.  For now, reading configuration before repository discovery
   is dangerous because it could cause a pager.<cmd> setting from the
   current repository to be neglected.

   But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.

   This will not affect commands like "git init" that cannot have
   RUN_SETUP* set; they will have to be helped separately later.  In
   particular, commands such as "git diff" that use command-specific
   options to decide whether to search for a repository and whether to
   paginate output are outside the scope of this change.

Guiding principle: better to leave some bugs for later than risk
regressions.

> This could be solved completely (indeed I have a patch
> under testing), but it would require unset_git_directory(), making
> this series a bit longer :(

I agree; that’s the right way to do it.  run_builtin() ought to be
relatively sure about whether a repo is going to be searched for.  For
commands like diff and grep, I think it should be okay to search for a
repo unless --no-index is the first argument.

Thanks for your thoughtfulness.
Jonathan

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> there's another case, when RUN_SETUP_GENTLY is
> set, but no repository is found. git_config() will pick
> $(cwd)/.git/config if it exists. I guess it's OK for this series
> because the true fix will require more changes.

I have claimed before that it’s better to fix a few bugs now and
provide a solid foundation to build on than to try to do everything
right away.  Sounds good in principle, but in practice we have to look
to the future to decide whether the foundation is strong enough.

So here are some ideas for future work (not necessarily in order).

 - Teach remaining commands that need to search for a repository to
   use RUN_SETUP_GENTLY, with appropriate exceptions where needed
   (for --no-index, for example).

 - Introduce unset_git_directory and the RUN_UNSETUP option (yes,
   this needs a better name).  Teach commands that work without a
   git directory to use it (this should fix the init poisoned by
   parent repository and aliased init problems).

 - Teach git_config() to ignore the repository-specific configuration
   if have_run_setup is true but have_repository is false.

 - Teach git_attr() to ignore .git/info/attributes if have_run_setup
   is true but have_repository is false.

 - Teach git_config() to optionally die if have_run_setup is not true
   and the setup_git_dir* to optionally die if have_run_setup is true.
   test-lib.sh would enable this option.

Sane?
Jonathan

Re: [RFC/PATCH 00/46] nd/setup remainder for convenient reference

[Nguyen Thai Ngoc Duy]

2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> I was thinking of sending the remainder of nd/setup after this to save
> others the trouble of rebasing, but I fear the traffic would be too
> disruptive.  So I am putting it up by git.
>
> Caveats: maybe these patches make no sense.  After I last rebased
> them, I walked away without looking.  Still, maybe they can save
> you some time.

Will go through them, but not now.

> are available in the git repository at:
>
>  git://repo.or.cz/git/jrn.git/ nd/setup

I took your tree (5d46d95), dropped "builtin: USE_PAGER should not be
used without RUN_SETUP*" (it's useless), lightly modified "alias: keep
repository found while collecting aliases as long as possible" and
replaced "Guard unallowed access to repository when it's not set up".
Available at:

git://repo.or.cz/git/pclouds.git tp/setup

I will split the "Guard..." patch for easier review, later today. I
couldn't because it was a die() patch. Now all it does is warning(),
so it's safe to split up. One more good point of warning().
-- 
Duy

Re: [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +unset GIT_PAGER
> +rm -f core.pager_used
> +rm -fr sub

Why do you have these outside the test itself?

> +test_expect_success TTY 'core.pager in subdir' '
> +	PAGER=wc &&
> +	stampname=$(pwd)/core.pager_used &&
> +	export PAGER stampname &&
> +	git config core.pager "wc > \"\$stampname\"" &&
> +	mkdir sub &&
> +	(
> +		cd sub &&
> +		test_terminal git log
> +	) &&
> +	test -e "$stampname"
> +'

They are reasonable clean-up steps to start the real test (starting from
setting and exporting the pager) in a known good state, and as long as you
write them not to fail I don't see any reason to have them outside the
test.  You _might_ want to "unset GIT_PAGER" immediately after the test
that does "export PAGER", so that it won't contaminate the environment the
test script itself runs, but even for that purpose, it would probably be
better to run the parts that depend on the exported value in a subshell.

Re: [PATCH/RFC 0/9] Setup cleanup, chapter one

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> These patches are designed to make people who want the pager.<cmd>
> configuration to be more reliable a little happier.  More importantly,
> they bring the setup procedure closer to doing the Right Thing™.

Thanks for looking into this.

By the way, the other day I had an interesting experience after running:

	git -p help -m cat-file

If you try this yourself, you probably need "reset" or "stty sane" to
recover.

Does this series address it?

Re: [PATCH/RFC 0/9] Setup cleanup, chapter one

[Jonathan Nieder]

Junio C Hamano wrote:

> By the way, the other day I had an interesting experience after running:
> 
> 	git -p help -m cat-file
> 
> If you try this yourself, you probably need "reset" or "stty sane" to
> recover.
> 
> Does this series address it?

No, it doesn’t.

The bug seems to be timing-related.  The two copies of ‘less’ fight over
initialization of the terminal and nobody wins.

What is the right thing to do in this case?  Can we assume ‘man’ comes
with a pager and suppress our own?  Should we ask ‘man’ to suppress its
pager, losing the formatting and status line in the process, but setting
MANPAGER=cat?  Probably the right thing to do would be to suppress our
pager and set MANPAGER according to git_pager() so ‘man’ can use it.

Maybe something roughly like this (on top of the series)?  This does
not help backends that do not know how to use MANPAGER.

diff --git a/builtin/help.c b/builtin/help.c
index 3182a2b..d9a0307 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -316,6 +316,40 @@ static const char *cmd_to_page(const char *git_cmd)
 		return prepend("git", git_cmd);
 }
 
+static void commit_pager_choice(void) {
+	switch (startup_info->use_pager) {
+	case 0:
+		setenv("GIT_PAGER", "cat", 1);
+		break;
+	case 1:
+		setup_pager();
+		break;
+	default:
+		break;
+	}
+}
+
+static void setup_man_pager(void) {
+	const char *pager;
+
+	switch (startup_info->use_pager) {
+	case 0:
+		setenv("MANPAGER", "cat", 1);
+		break;
+	case 1:
+		pager = git_pager(isatty(1));
+		if (!pager) {
+			setenv("MANPAGER", "cat", 1);
+		} else {
+			setenv("LESS", "FRSX", 0);
+			setenv("MANPAGER", pager, 1);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
 static void setup_man_path(void)
 {
 	struct strbuf new_path = STRBUF_INIT;
@@ -358,6 +392,7 @@ static void show_man_page(const char *git_cmd)
 	const char *fallback = getenv("GIT_MAN_VIEWER");
 
 	setup_man_path();
+	setup_man_pager();
 	for (viewer = man_viewer_list; viewer; viewer = viewer->next)
 	{
 		exec_viewer(viewer->name, page); /* will return when unable */
@@ -423,20 +458,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	if (show_all) {
+	if (show_all || !argv[0]) {
+		commit_pager_choice();
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git commands", &main_cmds, &other_cmds);
+		if (show_all)
+			list_commands("git commands", &main_cmds, &other_cmds);
+		else
+			list_common_cmds_help();
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
 
-	if (!argv[0]) {
-		printf("usage: %s\n\n", git_usage_string);
-		list_common_cmds_help();
-		printf("\n%s\n", git_more_info_string);
-		return 0;
-	}
-
 	setup_git_directory_gently(&nongit);
 	git_config(git_help_config, NULL);
 
diff --git a/cache.h b/cache.h
index 030ba01..8ba5727 100644
--- a/cache.h
+++ b/cache.h
@@ -1061,6 +1061,7 @@ int split_cmdline(char *cmdline, const char ***argv);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	int use_pager;
 };
 extern struct startup_info *startup_info;
 
diff --git a/git.c b/git.c
index 0a0d9eb..a81726e 100644
--- a/git.c
+++ b/git.c
@@ -14,7 +14,6 @@ const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
 
 static struct startup_info git_startup_info;
-static int use_pager = -1;
 struct pager_config {
 	const char *cmd;
 	int val;
@@ -39,7 +38,7 @@ int check_pager_config(const char *cmd)
 }
 
 static void commit_pager_choice(void) {
-	switch (use_pager) {
+	switch (startup_info->use_pager) {
 	case 0:
 		setenv("GIT_PAGER", "cat", 1);
 		break;
@@ -86,9 +85,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			puts(system_path(GIT_HTML_PATH));
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			use_pager = 1;
+			startup_info->use_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
-			use_pager = 0;
+			startup_info->use_pager = 0;
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--no-replace-objects")) {
@@ -221,13 +220,15 @@ static int handle_alias(int *argcp, const char ***argv)
 const char git_version_string[] = GIT_VERSION;
 
 #define RUN_SETUP		(1<<0)
-#define USE_PAGER		(1<<1)
 /*
  * require working tree to be present -- anything uses this needs
  * RUN_SETUP for reading from the configuration file.
  */
-#define NEED_WORK_TREE		(1<<2)
-#define RUN_SETUP_GENTLY	(1<<3)
+#define NEED_WORK_TREE		(1<<1)
+#define RUN_SETUP_GENTLY	(1<<2)
+#define USE_PAGER		(1<<3)
+/* do not commit_pager_choice() -- the builtin will take care of it */
+#define NO_PAGER_SETUP		(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -244,6 +245,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
+		int use_pager = startup_info->use_pager;
+
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
 		if (p->option & RUN_SETUP_GENTLY) {
@@ -255,8 +258,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
+		startup_info->use_pager = use_pager;
 	}
-	commit_pager_choice();
+	if (!(p->option & NO_PAGER_SETUP))
+		commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
@@ -328,7 +333,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
 		{ "grep", cmd_grep, USE_PAGER },
 		{ "hash-object", cmd_hash_object },
-		{ "help", cmd_help },
+		{ "help", cmd_help, NO_PAGER_SETUP },
 		{ "index-pack", cmd_index_pack },
 		{ "init", cmd_init_db },
 		{ "init-db", cmd_init_db },
@@ -484,6 +489,7 @@ int main(int argc, const char **argv)
 	const char *cmd;
 
 	memset(&git_startup_info, 0, sizeof(git_startup_info));
+	git_startup_info.use_pager = -1;
 	startup_info = &git_startup_info;
 
 	cmd = git_extract_argv0_path(argv[0]);

[PATCH] t7006: guard cleanup with test_expect_success

[Jonathan Nieder]

Most of these tests are removing files, environment variables, and
configuration that might interfere outside the test.  Putting these
clean-up commands in the test (in the same spirit as v1.7.1-rc0~59,
2010-03-20) means that errors during setup will be caught quickly and
non-error text will be suppressed without -v.

While at it, apply some other minor fixes:

 - do not rely on the shell to export variables defined with the same
   command as a function call

 - avoid whitespace immediately after the > redirection operator, for
   consistency with the style of other tests

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> They are reasonable clean-up steps to start the real test (starting from
> setting and exporting the pager) in a known good state, and as long as you
> write them not to fail I don't see any reason to have them outside the
> test.

Good catch, thanks.  Here’s a patch for the existing test (against master).

 t/t7006-pager.sh |  149 ++++++++++++++++++++++++++++++++++++-----------------
 t/test-lib.sh    |   16 ++++++
 2 files changed, 117 insertions(+), 48 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d9202d5..62595ab 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -4,17 +4,24 @@ test_description='Test automatic use of a pager.'
 
 . ./test-lib.sh
 
-rm -f stdout_is_tty
+cleanup_fail() {
+	echo >&2 cleanup failed
+	exit 1
+}
+
 test_expect_success 'set up terminal for tests' '
+	rm -f stdout_is_tty ||
+	cleanup_fail &&
+
 	if test -t 1
 	then
-		: > stdout_is_tty
+		>stdout_is_tty
 	elif
 		test_have_prereq PERL &&
 		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
 			sh -c "test -t 1"
 	then
-		: > test_terminal_works
+		>test_terminal_works
 	fi
 '
 
@@ -32,53 +39,68 @@ else
 	say no usable terminal, so skipping some tests
 fi
 
-unset GIT_PAGER GIT_PAGER_IN_USE
-git config --unset core.pager
-PAGER='cat > paginated.out'
-export PAGER
-
 test_expect_success 'setup' '
+	unset GIT_PAGER GIT_PAGER_IN_USE &&
+	test_might_fail git config --unset core.pager &&
+
+	PAGER="cat >paginated.out" &&
+	export PAGER &&
+
 	test_commit initial
 '
 
-rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
+	rm -f paginated.out ||
+	cleanup_fail &&
+
 	test_terminal git log &&
 	test -e paginated.out
 '
 
-rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
+	rm -f paginated.out ||
+	cleanup_fail &&
+
 	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '
 
-rm -f paginated.out
 test_expect_success 'no pager when stdout is a pipe' '
+	rm -f paginated.out ||
+	cleanup_fail &&
+
 	git log | cat &&
 	! test -e paginated.out
 '
 
-rm -f paginated.out
 test_expect_success 'no pager when stdout is a regular file' '
-	git log > file &&
+	rm -f paginated.out ||
+	cleanup_fail &&
+
+	git log >file &&
 	! test -e paginated.out
 '
 
-rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
+	rm -f paginated.out ||
+	cleanup_fail &&
+
 	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '
 
-rm -f file paginated.out
 test_expect_success 'no pager even with --paginate when stdout is a pipe' '
+	rm -f file paginated.out ||
+	cleanup_fail &&
+
 	git --paginate log | cat &&
 	! test -e paginated.out
 '
 
-rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
+	rm -f paginated.out ||
+	cleanup_fail &&
+
 	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '
@@ -86,88 +108,119 @@ test_expect_success TTY 'no pager with --no-pager' '
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-	read firstline < $1
+	read firstline <$1
 	! expr "$firstline" : "^[a-zA-Z]" >/dev/null
 }
 
-rm -f colorful.log colorless.log
 test_expect_success 'tests can detect color' '
-	git log --no-color > colorless.log &&
-	git log --color > colorful.log &&
+	rm -f colorful.log colorless.log ||
+	cleanup_fail &&
+
+	git log --no-color >colorless.log &&
+	git log --color >colorful.log &&
 	! colorful colorless.log &&
 	colorful colorful.log
 '
 
-rm -f colorless.log
-git config color.ui auto
 test_expect_success 'no color when stdout is a regular file' '
-	git log > colorless.log &&
+	rm -f colorless.log &&
+	git config color.ui auto ||
+	cleanup_fail &&
+
+	git log >colorless.log &&
 	! colorful colorless.log
 '
 
-rm -f paginated.out
-git config color.ui auto
 test_expect_success TTY 'color when writing to a pager' '
-	TERM=vt100 test_terminal git log &&
+	rm -f paginated.out &&
+	git config color.ui auto ||
+	cleanup_fail &&
+
+	(
+		TERM=vt100 &&
+		export TERM &&
+		test_terminal git log
+	) &&
 	colorful paginated.out
 '
 
-rm -f colorful.log
-git config color.ui auto
 test_expect_success 'color when writing to a file intended for a pager' '
-	TERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&
+	rm -f colorful.log &&
+	git config color.ui auto ||
+	cleanup_fail &&
+
+	(
+		TERM=vt100 &&
+		GIT_PAGER_IN_USE=true &&
+		export TERM GIT_PAGER_IN_USE &&
+		git log >colorful.log
+	) &&
 	colorful colorful.log
 '
 
-unset PAGER GIT_PAGER
-git config --unset core.pager
 test_expect_success 'determine default pager' '
+	unset PAGER GIT_PAGER &&
+	test_might_fail git config --unset core.pager ||
+	cleanup_fail &&
+
 	less=$(git var GIT_PAGER) &&
 	test -n "$less"
 '
 
-if expr "$less" : '^[a-z]*$' > /dev/null && test_have_prereq TTY
+if expr "$less" : '^[a-z][a-z]*$' >/dev/null && test_have_prereq TTY
 then
 	test_set_prereq SIMPLEPAGER
 fi
 
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
 test_expect_success SIMPLEPAGER 'default pager is used by default' '
-	cat > $less <<-EOF &&
-	#!$SHELL_PATH
-	wc > default_pager_used
+	unset PAGER GIT_PAGER &&
+	test_might_fail git config --unset core.pager &&
+	rm -f default_pager_used ||
+	cleanup_fail &&
+
+	cat >$less <<-\EOF &&
+	#!/bin/sh
+	wc >default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH test_terminal git log &&
+	(
+		PATH=.:$PATH &&
+		export PATH &&
+		test_terminal git log
+	) &&
 	test -e default_pager_used
 '
 
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
-	PAGER="wc > PAGER_used" &&
+	unset GIT_PAGER &&
+	test_might_fail git config --unset core.pager &&
+	rm -f PAGER_used ||
+	cleanup_fail &&
+
+	PAGER="wc >PAGER_used" &&
 	export PAGER &&
 	test_terminal git log &&
 	test -e PAGER_used
 '
 
-unset GIT_PAGER
-rm -f core.pager_used
 test_expect_success TTY 'core.pager overrides PAGER' '
+	unset GIT_PAGER &&
+	rm -f core.pager_used ||
+	cleanup_fail &&
+
 	PAGER=wc &&
 	export PAGER &&
-	git config core.pager "wc > core.pager_used" &&
+	git config core.pager "wc >core.pager_used" &&
 	test_terminal git log &&
 	test -e core.pager_used
 '
 
-rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+	rm -f GIT_PAGER_used ||
+	cleanup_fail &&
+
 	git config core.pager wc &&
-	GIT_PAGER="wc > GIT_PAGER_used" &&
+	GIT_PAGER="wc >GIT_PAGER_used" &&
 	export GIT_PAGER &&
 	test_terminal git log &&
 	test -e GIT_PAGER_used
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c582964..f807625 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -516,6 +516,22 @@ test_must_fail () {
 	test $? -gt 0 -a $? -le 129 -o $? -gt 192
 }
 
+# Similar to test_must_fail, but tolerates success, too.  This is
+# meant to be used in contexts like:
+#
+#	test_expect_success 'some command works without configuration' '
+#		test_might_fail git config --unset all.configuration &&
+#		do something
+#	'
+#
+# Writing "git config --unset all.configuration || :" would be wrong,
+# because we want to notice if it fails due to segv.
+
+test_might_fail () {
+	"$@"
+	test $? -ge 0 -a $? -le 129 -o $? -gt 192
+}
+
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
 #
-- 
1.7.1.rc1.18.g08771.dirty

Re: [PATCH] t7006: guard cleanup with test_expect_success

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index d9202d5..62595ab 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -4,17 +4,24 @@ test_description='Test automatic use of a pager.'
>  
>  . ./test-lib.sh
>  
> -rm -f stdout_is_tty
> +cleanup_fail() {
> +	echo >&2 cleanup failed
> +	exit 1
> +}

I think you meant to say "false" or "(exit 1)" here.  To see why...

>  test_expect_success 'set up terminal for tests' '
> +	rm -f stdout_is_tty ||
> +	cleanup_fail &&
> +

... try your patch with "rm -f stdout_is_tty" replaced with "(exit 1)" to
see how your cleanup_fail behaves.

Other than that I like the general idea, especially "might-fail" is cute.

Re: [PATCH] t7006: guard cleanup with test_expect_success

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

[...]
>> +cleanup_fail() {
>> +	echo >&2 cleanup failed
>> +	exit 1
>> +}
>
> I think you meant to say "false" or "(exit 1)" here.  To see why...
[...]
> ... try your patch with "rm -f stdout_is_tty" replaced with "(exit 1)" to
> see how your cleanup_fail behaves.

Good catch; thanks.  Here’s a patch for squashing in case you would
like one.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 62595ab..a3d0210 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -6,7 +6,7 @@ test_description='Test automatic use of a pager.'
 
 cleanup_fail() {
 	echo >&2 cleanup failed
-	exit 1
+	return 1
 }
 
 test_expect_success 'set up terminal for tests' '

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Nguyen Thai Ngoc Duy]

On Wed, Apr 14, 2010 at 7:06 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> So here are some ideas for future work (not necessarily in order).
>
>  - Teach remaining commands that need to search for a repository to
>   use RUN_SETUP_GENTLY, with appropriate exceptions where needed
>   (for --no-index, for example).

 - Two patches "worktree setup: calculate prefix.." and "index-pack:
trust the prefix" may form a separate series. That would unblock
RUN_SETUP_GENTLY series.

>  - Introduce unset_git_directory and the RUN_UNSETUP option (yes,
>   this needs a better name).  Teach commands that work without a
>   git directory to use it (this should fix the init poisoned by
>   parent repository and aliased init problems).

 - Two patches that move enter_repo() to setup.c and fix it up can go
first. They should have no impact on system.

>
>  - Teach git_config() to ignore the repository-specific configuration
>   if have_run_setup is true but have_repository is false.
>
>  - Teach git_attr() to ignore .git/info/attributes if have_run_setup
>   is true but have_repository is false.
>
>  - Teach git_config() to optionally die if have_run_setup is not true
>   and the setup_git_dir* to optionally die if have_run_setup is true.
>   test-lib.sh would enable this option.
>

 - Introduce startup_info->prefix. I think that's a good change, but
it's independent to this series. The reason is, prefix can change
after setup_git_dir is called (i.e. setup_work_tree()).
 - The fix for "git cmd -h" can make a separate series (4 patches)
 - Finally the soft guard patch and assorted fixups to avoid warnings.

I'll reorder my tree and see if it looks good. Patches of this 9-patch
series will be untouched.
-- 
Duy

Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Nguyen Thai Ngoc Duy]

On Thu, Apr 15, 2010 at 10:49 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> FWIW, my latest is at
>
>  git://repo.or.cz/git/jrn nd/setup/chapter-one
>
> Changes since the last series I sent to the list:
>
>  - test cleanups, after Junio’s feedback
>  - clarified commit messages, I hope
>
> If you’ve already started cleaning up or if something in the diff looks
> iffy, feel free to carry on with what you have.  I don’t want the test
> changes to interfere with more productive work.
>
> Looking forward to watching this evolve,

OK. Here is the topic breakdown into 11 sub-topics, after rebasing on
setup/chapter-one. Patches 1, 2-3, 4-5, 8-9, 10-12, 13-18 can go as
separate independent sub-topics, given that chapter-one is accepted.
The rest must go in sequence because they will need some of previous
groups. Commit messages aren't cleaned up yet, just the idea of
grouping.

Patches available at git://repo.or.cz/git/pclouds.git tp/setup

     1	rev-parse --git-dir: print relative gitdir correctly

This will become a problem when patches 37-43 (let's name it
"unset_git_directory") is merged. Other than that, it's can go alone.

     2	worktree setup: calculate prefix even if no worktree is found
     3	index-pack: trust the prefix returned by setup_git_directory_gently()

This is debatable, as you pointed out, because it changes "git
rev-parse --show-prefix" behavior. This is a prerequisite for 19-36
(run-setup-gently)

     4	Move enter_repo() to setup.c
     5	enter_repo(): initialize other variables as
setup_git_directory_gently() does

Good thing. Quite simple. Prerequisite for 6-7 (have_run_setup)

     6	builtin: remember whether repository has been searched for
     7	builtins: setup repository before print unknown command error

Introduce startup_info->have_run_setup_gitdir, and a guard in command
error printing.

     8	setup: save prefix (original cwd relative to toplevel) in startup_info
     9	builtin: remove prefix variable from run_builtin()

Introduce startup_info->prefix. Will be needed for 47-48 (alias fix)
because setup_git_dir may be called in one place, and unset_git_dir is
called in another place.

    10	run_builtin(): save "-h" detection result for later use
    11	builtins: utilize startup_info->help where possible
    12	builtins: check for startup_info->help, print and exit early

Fix "git cmd -h" shortcut. There will be warnings from git_config()
once the old "guard unallowed access" patch is in. It will be fixed in
patch 45.

    13	init/clone: turn on startup->have_repository properly
    14	t7002: test git grep --no-index from a bare repository
    15	Do not read .git/info/exclude if there is no repository
    16	Do not read .git/info/attributes if there is no repository
    17	apply: do not check sha1 if there is no repository
    18	config: do not read .git/config if there is no repository

Assorted fixes wrt unconditional access to .git/. All these patches
simple add "if (have_repository)" or so.

    19	t0001: test git init when run via an alias
    20	t5512: test that ls-remote does not require a git repository
    21	t7006: expose test_terminal() for use by other tests
    22	help: note why it is appropriate for this command not to use
RUN_SETUP_GENTLY
    23	hash-object: use RUN_SETUP_GENTLY
    24	shortlog: use RUN_SETUP_GENTLY
    25	grep: use RUN_SETUP_GENTLY
    26	archive: use RUN_SETUP_GENTLY
    27	mailinfo: use RUN_SETUP_GENTLY
    28	check-ref-format: use RUN_SETUP_GENTLY
    29	verify-pack: use RUN_SETUP_GENTLY
    30	apply: use RUN_SETUP_GENTLY
    31	bundle: use RUN_SETUP_GENTLY
    32	diff: use RUN_SETUP_GENTLY
    33	ls-remote: use RUN_SETUP_GENTLY
    34	var: use RUN_SETUP_GENTLY
    35	merge-file: use RUN_SETUP_GENTLY
    36	index-pack: use RUN_SETUP_GENTLY

The RUN_SETUP_GENTLY series. Of course these will need to be reviewed,
some commands might need finer-grain setup.

    37	Add git_config_early()
    38	t1300: test that scripted commands do not respect stray .git/config
    39	git_config(): do not read .git/config if there is no repository
    40	Use git_config_early() instead of git_config() during repo setup
    41	worktree setup: call set_git_dir explicitly
    42	worktree setup: restore original state when things go wrong
    43	Allow to undo setup_git_directory_gently() gracefully (and fix
alias code)

The unset_git_directory series. Make setup_git_directory() work right.

    44	Guard unallowed access to repository when it's not set up
    45	run_builtin: do now raise alarms
    46	setup_work_tree: do not raise alarm

Put unallowed access warnings in place. Also fixes false alarms.

    47	alias: keep repository found while collecting aliases as long as possible
    48	run_builtin: respect have_run_setup_gitdir

Fix alias related setup problem.

Insane?
-- 
Duy