threads / patch / 24205

patch, 4 partsgit --paginate: do not commit pager choice too early

Subject: [PATCH 0/4] git --paginate: do not commit pager choice too early

## tl;dr

42 messages between Jun 26, 2010 and Aug 16, 2010. Diffs are folded; open one to read it.

replies: 41people: 6as markdown or json

Jonathan Nieder· Jun 26, 2010, 19:22 UTC · lore
Here’s a fix from the famous setup cleanup series[1].

The problem it fixes is somewhat obscure: ‘git -p foo’ is not paying attention to the repository-local ‘[core] pager’ configuration when run from a subdirectory. But it is a real bug, and the fix gives an example of how to deal with repository setup and should be safe.

Patch is against master. There is a small semantic conflict with jn/grep-open: SIMPLEPAGER should be changed to SIMPLEPAGERTTY in the prerequisites for the test_default_pager function. Please let me know if I should push a merge commit to help resolve that.

Thoughts?
Jonathan Nieder (3):
  t7006 (pager): introduce helper for parameterized tests
  t7006: test pager configuration for several git commands
  tests: local config file should be honored from subdirs of toplevel
Nguyễn Thái Ngọc Duy (1):
  git --paginate: do not commit pager choice too early
 git.c            |    2 +-
 t/t7006-pager.sh |  234 +++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 182 insertions(+), 54 deletions(-)
[1] http://thread.gmane.org/gmane.comp.version-control.git/144778
Jonathan Nieder· Jun 26, 2010, 19:23 UTC · re: Jonathan Nieder · lore

[PATCH 1/4] t7006 (pager): introduce helper for parameterized tests

The current tests test pager configuration for ‘git log’, but other commands use a different setup procedure and should therefore be tested separately. Add a helper to make this easier.

This patch introduces the helper and changes some existing tests to use it. The only functional change should be the introduction of ‘git log - ’ to a few test descriptions.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   72 ++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 54 insertions(+), 18 deletions(-)
Show changes to t/t7006-pager.sh +54 −18
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 9a83241..b117ebb 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -172,58 +172,94 @@ then
 	test_set_prereq SIMPLEPAGER
 fi
 
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
+# Use this helper to make it easy for the caller of your
+# terminal-using function to specify whether it should fail.
+# If you write
+#
+#	your_test() {
+#		parse_args "$@"
+#
+#		$test_expectation "$cmd - behaves well" "
+#			...
+#			$full_command &&
+#			...
+#		"
+#	}
+#
+# then your test can be used like this:
+#
+#	your_test expect_(success|failure) [test_must_fail] 'git foo'
+#
+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"
+}
+
+parse_args expect_success 'git log'
+$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
 	unset PAGER GIT_PAGER;
 	test_might_fail git config --unset core.pager &&
 	rm -f default_pager_used ||
 	cleanup_fail &&
 
-	cat >$less <<-\EOF &&
+	cat >\$less <<-\EOF &&
 	#!/bin/sh
 	wc >default_pager_used
 	EOF
-	chmod +x $less &&
+	chmod +x \$less &&
 	(
-		PATH=.:$PATH &&
+		PATH=.:\$PATH &&
 		export PATH &&
-		test_terminal git log
+		$full_command
 	) &&
 	test -e default_pager_used
-'
+"
 
-test_expect_success TTY 'PAGER overrides default pager' '
+parse_args expect_success 'git log'
+$test_expectation TTY "$cmd - PAGER overrides default pager" "
 	unset GIT_PAGER;
 	test_might_fail git config --unset core.pager &&
 	rm -f PAGER_used ||
 	cleanup_fail &&
 
-	PAGER="wc >PAGER_used" &&
+	PAGER='wc >PAGER_used' &&
 	export PAGER &&
-	test_terminal git log &&
+	$full_command &&
 	test -e PAGER_used
-'
+"
 
-test_expect_success TTY 'core.pager overrides PAGER' '
+parse_args expect_success 'git log'
+$test_expectation TTY "$cmd - 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" &&
-	test_terminal git log &&
+	git config core.pager 'wc >core.pager_used' &&
+	$full_command &&
 	test -e core.pager_used
-'
+"
 
-test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+parse_args expect_success 'git log'
+$test_expectation TTY "$cmd - 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 &&
+	$full_command &&
 	test -e GIT_PAGER_used
-'
+"
 
 test_done
-- 
1.7.1.579.ge2549
Jonathan Nieder· Jun 26, 2010, 19:24 UTC · re: Jonathan Nieder · lore

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

Test choice of pager at several stages of repository setup. This provides some (admittedly uninteresting) examples to keep in mind when considering changes to the setup procedure.

Improved-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The patch from ‘diff -w’ should be easier to review.  I’ll send it as
a reply.
Thanks to both Junio and Hannes for improvements.
 t/t7006-pager.sh |  146 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 89 insertions(+), 57 deletions(-)
Show changes to t/t7006-pager.sh +89 −57
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b117ebb..4420f91 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -204,62 +204,94 @@ parse_args() {
 	full_command="$full_command $1"
 }
 
-parse_args expect_success 'git log'
-$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
-	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 &&
-		export PATH &&
-		$full_command
-	) &&
-	test -e default_pager_used
-"
-
-parse_args expect_success 'git log'
-$test_expectation TTY "$cmd - PAGER overrides default pager" "
-	unset GIT_PAGER;
-	test_might_fail git config --unset core.pager &&
-	rm -f PAGER_used ||
-	cleanup_fail &&
-
-	PAGER='wc >PAGER_used' &&
-	export PAGER &&
-	$full_command &&
-	test -e PAGER_used
-"
-
-parse_args expect_success 'git log'
-$test_expectation TTY "$cmd - 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' &&
-	$full_command &&
-	test -e core.pager_used
-"
-
-parse_args expect_success 'git log'
-$test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
-	rm -f GIT_PAGER_used ||
-	cleanup_fail &&
-
-	git config core.pager wc &&
-	GIT_PAGER='wc >GIT_PAGER_used' &&
-	export GIT_PAGER &&
-	$full_command &&
-	test -e GIT_PAGER_used
-"
+test_default_pager() {
+	parse_args "$@"
+
+	$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
+		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 &&
+			export PATH &&
+			$full_command
+		) &&
+		test -e default_pager_used
+	"
+}
+
+test_PAGER_overrides() {
+	parse_args "$@"
+
+	$test_expectation TTY "$cmd - PAGER overrides default pager" "
+		unset GIT_PAGER;
+		test_might_fail git config --unset core.pager &&
+		rm -f PAGER_used ||
+		cleanup_fail &&
+
+		PAGER='wc >PAGER_used' &&
+		export PAGER &&
+		$full_command &&
+		test -e PAGER_used
+	"
+}
+
+test_core_pager_overrides() {
+	parse_args "$@"
+
+	$test_expectation TTY "$cmd - 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' &&
+		$full_command &&
+		test -e core.pager_used
+	"
+}
+
+test_GIT_PAGER_overrides() {
+	parse_args "$@"
+
+	$test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
+		rm -f GIT_PAGER_used ||
+		cleanup_fail &&
+
+		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_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_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_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_GIT_PAGER_overrides  expect_success test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.1.579.ge2549
Jonathan Nieder· Jun 26, 2010, 19:28 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Test choice of pager at several stages of repository setup.
Here it is again, without whitespace changes. :)
 t/t7006-pager.sh |   40 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 36 insertions(+), 4 deletions(-)
Show changes to t/t7006-pager.sh +36 −4
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b117ebb..4420f91 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -204,7 +204,9 @@ parse_args() {
 	full_command="$full_command $1"
 }
 
-parse_args expect_success 'git log'
+test_default_pager() {
+	parse_args "$@"
+
 $test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
 	unset PAGER GIT_PAGER;
 	test_might_fail git config --unset core.pager &&
@@ -223,8 +225,11 @@ $test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
 	) &&
 	test -e default_pager_used
 "
+}
+
+test_PAGER_overrides() {
+	parse_args "$@"
 
-parse_args expect_success 'git log'
 $test_expectation TTY "$cmd - PAGER overrides default pager" "
 	unset GIT_PAGER;
 	test_might_fail git config --unset core.pager &&
@@ -236,8 +241,11 @@ $test_expectation TTY "$cmd - PAGER overrides default pager" "
 	$full_command &&
 	test -e PAGER_used
 "
+}
+
+test_core_pager_overrides() {
+	parse_args "$@"
 
-parse_args expect_success 'git log'
 $test_expectation TTY "$cmd - core.pager overrides PAGER" "
 	unset GIT_PAGER;
 	rm -f core.pager_used ||
@@ -249,8 +257,11 @@ $test_expectation TTY "$cmd - core.pager overrides PAGER" "
 	$full_command &&
 	test -e core.pager_used
 "
+}
+
+test_GIT_PAGER_overrides() {
+	parse_args "$@"
 
-parse_args expect_success 'git log'
 $test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
 	rm -f GIT_PAGER_used ||
 	cleanup_fail &&
@@ -261,5 +272,26 @@ $test_expectation TTY "$cmd - GIT_PAGER overrides core.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_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_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_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_GIT_PAGER_overrides  expect_success test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.1.579.ge2549
Jonathan Nieder· Jun 26, 2010, 19:25 UTC · re: Jonathan Nieder · lore

[PATCH 3/4] tests: local config file should be honored from subdirs of toplevel

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. If --paginate is handled before searching for the git dir, this configuration will be missed.

In other words, with --paginate and only with --paginate, any repository-local core.pager setting is being ignored [*].

[*] unless the git directory is ./.git or GIT_DIR or GIT_CONFIG was set explicitly.

Add a test to demonstrate this counterintuitive behavior. Noticed while reading over a patch by Duy that fixes it.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Improved-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Show changes to t/t7006-pager.sh +26 −0
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4420f91..2b106be 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -259,6 +259,28 @@ test_core_pager_overrides() {
 	"
 }
 
+test_core_pager_subdir() {
+	parse_args "$@"
+
+	$test_expectation TTY "$cmd - core.pager from subdirectory" "
+		unset GIT_PAGER;
+		rm -f core.pager_used &&
+		rm -fr sub ||
+		cleanup_fail &&
+
+		PAGER=wc &&
+		stampname=\$(pwd)/core.pager_used &&
+		export PAGER stampname &&
+		git config core.pager 'wc >\"\$stampname\"' &&
+		mkdir sub &&
+		(
+			cd sub &&
+			$full_command
+		) &&
+		test -e core.pager_used
+	"
+}
+
 test_GIT_PAGER_overrides() {
 	parse_args "$@"
 
@@ -277,21 +299,25 @@ test_GIT_PAGER_overrides() {
 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.1.579.ge2549
Jonathan Nieder· Jun 26, 2010, 19:26 UTC · re: Jonathan Nieder · lore

[PATCH 4/4] git --paginate: do not commit pager choice too early

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

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.

At the relevant moment, the repository has not been searched for yet. Attempting to access the configuration at this point results in git_dir being set to .git [*], which is almost certainly not what was wanted. In particular, when run from a subdirectory of the toplevel, git --paginate does not respect the core.pager setting from the current repository.

[*] unless GIT_DIR or GIT_CONFIG is set
So delay the pager startup when possible:
1. run_argv() already commits pager choice inside run_builtin() if a
   command is found.  For commands that use RUN_SETUP, waiting until
   then fixes the problem described above: once git knows where to
   look, it happily respects the core.pager setting.
2. list_common_cmds_help() prints out 29 lines and exits.  This can
   benefit from pagination, so we need to commit the pager choice
   before writing this output.
   Luckily ‘git’ without subcommand has no other reason to access a
   repository, so it would be intuitive to ignore repository-local
   configuration in this case.  Simpler for now to choose a pager
   using the funny code that notices a repository that happens to be
   at .git.  That this accesses a repository when it is very
   convenient to is a bug but not an important one.
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>
---
Thanks for reading.
 git.c            |    2 +-
 t/t7006-pager.sh |   58 ++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 13 deletions(-)
Show changes to 2 files +47 −13

git.c, t/t7006-pager.sh

diff --git a/git.c b/git.c
index 99f0363..25e7693 100644
--- a/git.c
+++ b/git.c
@@ -511,12 +511,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 2b106be..eefef45 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -244,9 +244,21 @@ test_PAGER_overrides() {
 }
 
 test_core_pager_overrides() {
+	if_local_config=
+	used_if_wanted='overrides PAGER'
+	test_core_pager "$@"
+}
+
+test_local_config_ignored() {
+	if_local_config='! '
+	used_if_wanted='is not used'
+	test_core_pager "$@"
+}
+
+test_core_pager() {
 	parse_args "$@"
 
-	$test_expectation TTY "$cmd - core.pager overrides PAGER" "
+	$test_expectation TTY "$cmd - repository-local core.pager setting $used_if_wanted" "
 		unset GIT_PAGER;
 		rm -f core.pager_used ||
 		cleanup_fail &&
@@ -255,14 +267,26 @@ test_core_pager_overrides() {
 		export PAGER &&
 		git config core.pager 'wc >core.pager_used' &&
 		$full_command &&
-		test -e core.pager_used
+		${if_local_config}test -e core.pager_used
 	"
 }
 
 test_core_pager_subdir() {
+	if_local_config=
+	used_if_wanted='overrides PAGER'
+	test_pager_subdir_helper "$@"
+}
+
+test_no_local_config_subdir() {
+	if_local_config='! '
+	used_if_wanted='is not used'
+	test_pager_subdir_helper "$@"
+}
+
+test_pager_subdir_helper() {
 	parse_args "$@"
 
-	$test_expectation TTY "$cmd - core.pager from subdirectory" "
+	$test_expectation TTY "$cmd - core.pager $used_if_wanted from subdirectory" "
 		unset GIT_PAGER;
 		rm -f core.pager_used &&
 		rm -fr sub ||
@@ -277,7 +301,7 @@ test_core_pager_subdir() {
 			cd sub &&
 			$full_command
 		) &&
-		test -e core.pager_used
+		${if_local_config}test -e core.pager_used
 	"
 }
 
@@ -296,6 +320,20 @@ test_GIT_PAGER_overrides() {
 	"
 }
 
+test_doesnt_paginate() {
+	parse_args "$@"
+
+	$test_expectation TTY "no pager for '$cmd'" "
+		rm -f GIT_PAGER_used ||
+		cleanup_fail &&
+
+		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'
@@ -305,19 +343,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.1.579.ge2549
Jeff King· Jun 28, 2010, 09:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

On Sat, Jun 26, 2010 at 02:22:03PM -0500, Jonathan Nieder wrote:
Show 11 quoted lines
> Here’s a fix from the famous setup cleanup series[1].
> 
> The problem it fixes is somewhat obscure: ‘git -p foo’ is not paying
> attention to the repository-local ‘[core] pager’ configuration when
> run from a subdirectory.  But it is a real bug, and the fix gives
> an example of how to deal with repository setup and should be safe.
> 
> Patch is against master.  There is a small semantic conflict with
> jn/grep-open: SIMPLEPAGER should be changed to SIMPLEPAGERTTY in the
> prerequisites for the test_default_pager function.  Please let me
> know if I should push a merge commit to help resolve that.

I just read through the patches, and it seems like a reasonable fix. So in that sense, ACK.

But reading the message for patch 4/4, I can't help but wonder if the right way forward is for the git wrapper to _always_ find the repository as the very first thing. I.e., make finding the repository a non-destructive thing (especially don't end in a different directory), and just do it before we run any other code.

For a few commands like "init" or "clone", we would end up just ignoring this information, but the worst case should be a little bit of wasted effort.

And then all these silly setup and timing issues could just go away, as we would know that GIT_DIR and GIT_WORK_TREE would be set properly.

-Peff
Jonathan Nieder· Jun 28, 2010, 10:13 UTC · re: Jeff King · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

Jeff King wrote:
> But reading the message for patch 4/4, I can't help but wonder if the
> right way forward is for the git wrapper to _always_ find the repository
> as the very first thing.
[...]
> the worst case should be a little bit of
> wasted effort.

This is a very useful thought. Git without the chdir() to toplevel would be pleasanter, I think. (Yes, I realize you weren’t necessarily suggesting suppressing a chdir_to_toplevel() in place of setup_git_repository() and friends.)

Regarding the repository search: automounters can cause pain if DISCOVERY_ACROSS_FILESYSTEM is set and GIT_CEILING_DIRECTORIES is not set appropriately. An important script that does

  tmpdir=$(mktemp -d)
  cd "$tmpdir"
  ...
  git diff --no-index a b
  ...

could hang indefinitely if some nut has created a named pipe at /tmp/.git. I haven’t come up with any non far-fetched reason to mind the repository search, though.

Will think about this.
Jeff King· Jun 28, 2010, 10:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

On Mon, Jun 28, 2010 at 05:13:35AM -0500, Jonathan Nieder wrote:
Show 11 quoted lines
> > But reading the message for patch 4/4, I can't help but wonder if the
> > right way forward is for the git wrapper to _always_ find the repository
> > as the very first thing.
> [...]
> > the worst case should be a little bit of
> > wasted effort.
> 
> This is a very useful thought.  Git without the chdir() to toplevel
> would be pleasanter, I think.  (Yes, I realize you weren’t necessarily
> suggesting suppressing a chdir_to_toplevel() in place of
> setup_git_repository() and friends.)

I had just meant that we would not do the chdir() initially, but would do so right before running the actual command which wanted repository setup (and commands like init which do not do that setup would never chdir to the toplevel). But we must always run at least aliases and external sub-commands from the toplevel to keep backwards compatibility.

Show 16 quoted lines
> Regarding the repository search: automounters can cause pain if
> DISCOVERY_ACROSS_FILESYSTEM is set and GIT_CEILING_DIRECTORIES is not
> set appropriately.  An important script that does
> 
>   tmpdir=$(mktemp -d)
>   cd "$tmpdir"
>   ...
> 
>   git diff --no-index a b
>   ...
> 
> could hang indefinitely if some nut has created a named pipe at
> /tmp/.git.  I haven’t come up with any non far-fetched reason to mind
> the repository search, though.
> 
> Will think about this.

Yeah, I considered that, too. But I doubt it is a big problem. If it were, then just running "git log" when you are not in a repository would be similarly painful. The people who have setups like that are already having to deal with GIT_CEILING_DIRECTORIES to make things less painful (though I imagine the new cross-filesystem discovery code will prevent most of the pain without them having to actually do anything).

So yes, we are adding the extra lookup for commands like "git clone", but I suspect in practice nobody will care. If it is a big deal, we can do something like:

  if (!strcmp(cmd, "clone") || !strcmp(cmd, "init"))
     ... don't do setup ...

which I admit is a terrible hack, but it is an optimization, not a correctness thing. So we are not shutting out external user-defined commands that might not care about the repo. We are just making our common built-in ones a bit more efficient.

-Peff
Nguyen Thai Ngoc Duy· Jun 28, 2010, 12:45 UTC · re: Jeff King · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

2010/6/28 Jeff King <peff@peff.net>:
Show 19 quoted lines
> On Mon, Jun 28, 2010 at 05:13:35AM -0500, Jonathan Nieder wrote:
>
>> > But reading the message for patch 4/4, I can't help but wonder if the
>> > right way forward is for the git wrapper to _always_ find the repository
>> > as the very first thing.
>> [...]
>> > the worst case should be a little bit of
>> > wasted effort.
>>
>> This is a very useful thought.  Git without the chdir() to toplevel
>> would be pleasanter, I think.  (Yes, I realize you weren’t necessarily
>> suggesting suppressing a chdir_to_toplevel() in place of
>> setup_git_repository() and friends.)
>
> I had just meant that we would not do the chdir() initially, but would
> do so right before running the actual command which wanted repository
> setup (and commands like init which do not do that setup would never
> chdir to the toplevel). But we must always run at least aliases and
> external sub-commands from the toplevel to keep backwards compatibility.

I think I'm missing something here. Looking up aliases means repository search must be done in git wrapper anyway. Even if we don't have to do repo search in git wrapper, the very first thing a sub command does is likely git_config(), which will need repo again. As long as repo search does not have any side effects, everything should be fine, IMO.

Show 7 quoted lines
> So yes, we are adding the extra lookup for commands like "git clone",
> but I suspect in practice nobody will care. If it is a big deal, we can
> do something like:
>
>  if (!strcmp(cmd, "clone") || !strcmp(cmd, "init"))
>     ... don't do setup ...
>

If git wrapper does not need setup (i.e. aliases) at all, that should happen without hacks. "clone" and "init" do not have RUN_SETUP (nor RUN_SETUP_GENTLY in my series), so there should not be any setup before cmd_{init,clone} is run.

PS. I haven't forgotten my cleanup series. Day job has been taking too much of my energy unfortunately.

-- 
Duy
Junio C Hamano· Jun 29, 2010, 05:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

Jonathan Nieder <jrnieder@gmail.com> writes:
> Patch is against master.  There is a small semantic conflict with
> jn/grep-open: SIMPLEPAGER should be changed to SIMPLEPAGERTTY in the
> prerequisites for the test_default_pager function.  Please let me
> know if I should push a merge commit to help resolve that.

Thanks for advance warning; please double check the merge result in 'pu' when I push it out...

Junio C Hamano· Jul 14, 2010, 20:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Patch is against master.  There is a small semantic conflict with
>> jn/grep-open: SIMPLEPAGER should be changed to SIMPLEPAGERTTY in the
>> prerequisites for the test_default_pager function.  Please let me
>> know if I should push a merge commit to help resolve that.
>
> Thanks for advance warning; please double check the merge result in 'pu'
> when I push it out...
I hate an enumeration that pretends to be exhausitive but is not.
    So delay the pager startup when possible:
    
    1. run_argv() already commits pager choice inside run_builtin() if a
       command is found.  For commands that use RUN_SETUP, waiting until
       then fixes the problem described above: once git knows where to
       look, it happily respects the core.pager setting.
... and for commands that do not use RUN_SETUP, what happens?
    2. list_common_cmds_help() prints out 29 lines and exits....
    3. help_unknown_cmd() prints out a few lines to stderr.  It is not
       important to paginate this, so don’t.

Missing from the above enumeration are are external commands. They depend on commit_pager_choice() to be called before execv_dashed_external() gets called. For example, "git -p request-pull $args" no longer works with this patch.

Sigh..
Jonathan Nieder· Jul 14, 2010, 21:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

Junio C Hamano wrote:
> ... and for commands that do not use RUN_SETUP, what happens?

run_builtin() still commits pager choice. The bug is neither fixed nor made worse for them.

> Missing from the above enumeration are are external commands.  They depend
> on commit_pager_choice() to be called before execv_dashed_external() gets
> called.  For example, "git -p request-pull $args" no longer works with
> this patch.
Something like the following may help.

This does not protect against calling setup_pager() more than once. Once the first pager has been set up, isatty(1) is false, preventing additional pagers from being spawned and competing with it.

As futureproofing against --paginate=always, setup_pager() should probably be taught to check pager_in_use(), but that should probably wait for a separate patch.

Show changes to git.c +2 −0
diff --git a/git.c b/git.c
index 25e7693..2dcd952 100644
--- a/git.c
+++ b/git.c
@@ -162,6 +162,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	int unused_nongit;
 
 	subdir = setup_git_directory_gently(&unused_nongit);
+	commit_pager_choice();
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
@@ -433,6 +434,7 @@ static void execv_dashed_external(const char **argv)
 	int status;
 
 	strbuf_addf(&cmd, "git-%s", argv[0]);
+	commit_pager_choice();
 
 	/*
 	 * argv[0] must be the git command, but the argv array
Jonathan Nieder· Jul 14, 2010, 22:55 UTC · re: Junio C Hamano · lore

[PATCH] git --paginate: paginate external commands again

73e25e7c (git --paginate: do not commit pager choice too early, 2010-06-26) failed to take some cases into account.

1b. Builtins that do not use RUN_SETUP (like git config) do
    not find GIT_DIR set correctly when the pager is launched
    from run_builtin().  So the core.pager configuration is
    not honored from subdirectories of the toplevel for them.
4a. External git commands (like git request-pull) relied on the
    early pager launch to take care of handling the -p option.
    Ever since 73e25e7c, they do not honor the -p option at all.
4b. Commands invoked through ! aliases (like ls) were also relying
    on the early pager launch.

Fix (4a) by launching the pager (if requested) before running such a “dashed external”. For simplicity, this still does not search for a .git directory before running the external command; when run from a subdirectory of the toplevel, therefore, the “[core] pager” configuration is still not honored.

Fix (4b) by launching pager if requested before carrying out such an alias. Actually doing this has no effect, since the pager (if any) would have already been launched in a failed attempt to try a dashed external first. The choice-of-pager-not-honored-from- subdirectory bug still applies here, too.

(1b) is not a regression.  There is no need to fix it yet.
Noticed by Junio.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> I hate an enumeration that pretends to be exhausitive but is not.
Sorry about that.
Here’s a patch that could be squashed in or applied on top.
 git.c            |    3 +++
 t/t7006-pager.sh |   47 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 12 deletions(-)
Show changes to 2 files +38 −12

git.c, t/t7006-pager.sh

diff --git a/git.c b/git.c
index 25e7693..99517dd 100644
--- a/git.c
+++ b/git.c
@@ -167,6 +167,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	alias_string = alias_lookup(alias_command);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
+			commit_pager_choice();
 			if (*argcp > 1) {
 				struct strbuf buf;
 
@@ -432,6 +433,8 @@ static void execv_dashed_external(const char **argv)
 	const char *tmp;
 	int status;
 
+	commit_pager_choice();
+
 	strbuf_addf(&cmd, "git-%s", argv[0]);
 
 	/*
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index eefef45..9215c2f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -334,17 +334,40 @@ test_doesnt_paginate() {
 	"
 }
 
-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_success 'git -p log'
-test_GIT_PAGER_overrides  expect_success 'git -p log'
+test_pager_choices() {
+	test_default_pager        expect_success "$@"
+	test_PAGER_overrides      expect_success "$@"
+	test_core_pager_overrides expect_success "$@"
+	test_core_pager_subdir    expect_success "$@"
+	test_GIT_PAGER_overrides  expect_success "$@"
+}
+
+test_expect_success 'setup: some aliases' '
+	git config alias.aliasedlog log &&
+	git config alias.true "!true"
+'
+
+test_pager_choices                       'git log'
+test_pager_choices                       'git -p log'
+test_pager_choices                       'git aliasedlog'
+
+test_default_pager        expect_success 'git -p aliasedlog'
+test_PAGER_overrides      expect_success 'git -p aliasedlog'
+test_core_pager_overrides expect_success 'git -p aliasedlog'
+test_core_pager_subdir    expect_failure 'git -p aliasedlog'
+test_GIT_PAGER_overrides  expect_success 'git -p aliasedlog'
+
+test_default_pager        expect_success 'git -p true'
+test_PAGER_overrides      expect_success 'git -p true'
+test_core_pager_overrides expect_success 'git -p true'
+test_core_pager_subdir    expect_failure 'git -p true'
+test_GIT_PAGER_overrides  expect_success 'git -p true'
+
+test_default_pager        expect_success test_must_fail 'git -p request-pull'
+test_PAGER_overrides      expect_success test_must_fail 'git -p request-pull'
+test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
+test_core_pager_subdir    expect_failure test_must_fail 'git -p request-pull'
+test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p request-pull'
 
 test_default_pager        expect_success test_must_fail 'git -p'
 test_PAGER_overrides      expect_success test_must_fail 'git -p'
@@ -352,6 +375,6 @@ 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_doesnt_paginate      expect_success test_must_fail 'git -p nonsense'
+test_doesnt_paginate      expect_failure test_must_fail 'git -p nonsense'
 
 test_done
-- 
1.7.2.rc2.532.g65d9b.dirty
Nguyen Thai Ngoc Duy· Jul 18, 2010, 12:27 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/4] git --paginate: do not commit pager choice too early

2010/7/15 Junio C Hamano <gitster@pobox.com>:
Show 6 quoted lines
> Missing from the above enumeration are are external commands.  They depend
> on commit_pager_choice() to be called before execv_dashed_external() gets
> called.  For example, "git -p request-pull $args" no longer works with
> this patch.
>
> Sigh..
I'm sorry. I missed those paths..
-- 
Duy
Jonathan Nieder· Aug 6, 2010, 02:35 UTC · re: Junio C Hamano · lore

[PATCH jn/paginate-fix 0/12] Re: git --paginate: do not commit pager choice too early

Junio C Hamano wrote:
Show 8 quoted lines
>     So delay the pager startup when possible:
>     
>     1. run_argv() already commits pager choice inside run_builtin() if a
>        command is found.  For commands that use RUN_SETUP, waiting until
>        then fixes the problem described above: once git knows where to
>        look, it happily respects the core.pager setting.
> 
> ... and for commands that do not use RUN_SETUP, what happens?
Let’s help some of them out a little.

These patches teach a few more built-ins to do something like RUN_SETUP; more precisely, a RUN_SETUP_GENTLY facility is introduced to run setup_git_directory_gently() early just like NEEDS_PREFIX has always caused setup_git_directory() to be run early.

This series wouldn’t be possible without Duy’s recent efforts to roll out other fixes from the famous nd/setup topic --- thanks! The patches should be familiar[1] and I think they are better justified now.

The first 8 patches apply on top to jn/paginate-fix from pu. Between patch 8 and 9 I have a merge of jn/maint-setup-fix^, with the following explanation:

	Merge branch 'jn/maint-setup-fix' (early part) into jn/paginate-fix
	That topic includes a fix for “git index-pack” (which used
	to save the cwd before the repository search to work around
	a bug) which is needed before the repository search can be
	safely run earlier.

That merge involves a trivial conflict between adding the RUN_SETUP_GENTLY flag for this topic and removing USE_PAGER for grep -O. The conflict is only nominal --- the two changes do not interfere with each other.

One final note: this series is deliberately very conservative, in that it does not touch commands like diff (which has a --no-index mode suppressing the repository search). Such commands are neither helped nor hindered by this series, and I think that during this transition time we should be opportunistic rather than rigid: the rule is to run the repository search as soon as can be easily justified but no sooner.

Thoughts welcome, as always.
Nguyễn Thái Ngọc Duy (12):
  git wrapper: introduce startup_info struct
  setup: remember whether repository was found
  git wrapper: allow setup_git_directory_gently() be called earlier
  shortlog: run setup_git_directory_gently() sooner
  grep: run setup_git_directory_gently() sooner
  apply: run setup_git_directory_gently() sooner
  bundle: run setup_git_directory_gently() sooner
  config: run setup_git_directory_gently() sooner
  index-pack: run setup_git_directory_gently() sooner
  ls-remote: run setup_git_directory_gently() sooner
  var: run setup_git_directory_gently() sooner
  merge-file: run setup_git_directory_gently() sooner
[1] http://thread.gmane.org/gmane.comp.version-control.git/144000/focus=144110
Jonathan Nieder· Aug 6, 2010, 02:40 UTC · re: Jonathan Nieder · lore

[PATCH 01/12] git wrapper: introduce startup_info struct

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 with results that were previously returned from setup functions. This state is global anyway (since the cwd is), even if it is not currently tracked that way. 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>
---
Changes from last round:
 - commit message
 - remove unnecessary memset (since statics are already automatically
   zero-initialized)
 cache.h       |    5 +++++
 environment.c |    1 +
 git.c         |    3 +++
 3 files changed, 9 insertions(+), 0 deletions(-)
Show changes to 3 files +9 −0

cache.h, environment.c, git.c

diff --git a/cache.h b/cache.h
index c9fa3df..0822c3b 100644
--- a/cache.h
+++ b/cache.h
@@ -1097,6 +1097,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 83d38d3..eeb2687 100644
--- a/environment.c
+++ b/environment.c
@@ -53,6 +53,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 99517dd..8a8d2b2 100644
--- a/git.c
+++ b/git.c
@@ -14,6 +14,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;
@@ -489,6 +490,8 @@ int main(int argc, const char **argv)
 {
 	const char *cmd;
 
+	startup_info = &git_startup_info;
+
 	cmd = git_extract_argv0_path(argv[0]);
 	if (!cmd)
 		cmd = "git-help";
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 02:46 UTC · re: Jonathan Nieder · lore

[PATCH 02/12] setup: remember whether repository was found

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

As v1.7.2~16^2 (git --paginate: paginate external commands again, 2010-07-14) explains, builtins (like git config) that do not use RUN_SETUP are not finding GIT_DIR set correctly when it is time to launch the pager from run_builtin(). If they were to search for a repository sooner, then the outcome of such early repository accesses would be more predictable and reliable.

The cmd_*() functions learn whether a repository was found through the *nongit_ok return value from setup_git_directory_gently(). If run_builtin() is to take care of the repository search itself, that datum needs to be retrievable from somewhere else. Use the startup_info struct for this.

As a bonus, this information becomes available to functions such as git_config() which might want to avoid trying to access a repository when 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>
---
Tweaked the log message a bit.
 cache.h |    1 +
 setup.c |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)
Show changes to 2 files +12 −1

cache.h, setup.c

diff --git a/cache.h b/cache.h
index 0822c3b..f9e82a8 100644
--- a/cache.h
+++ b/cache.h
@@ -1099,6 +1099,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 7e04602..1e32fb7 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);
@@ -470,6 +470,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.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 02:52 UTC · re: Jonathan Nieder · lore

[PATCH 03/12] git wrapper: allow setup_git_directory_gently() be called earlier

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

In the spirit of v1.4.2-rc3~34^2^2 (Call setup_git_directory() much earlier, 2006-07-28), let run_builtin() take care of searching for a repository for built-ins that want to make use of one if present.

So now you can mark your command with RUN_SETUP_GENTLY and use
	nongit = !startup_info->have_repository;
in place of
	prefix = setup_git_directory_gently(&nongit);

and everything will be the same, except the repository is discovered a little sooner.

As v1.7.2~16^2 (2010-07-14) explains, this should allow more commands to robustly use features like "git --paginate" that look at local configuration before the command is actually run.

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

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>
---
 - reordered setup option bits (RUN_SETUP_GENTLY lives next to RUN_SETUP now)
 - squashed in "builtin: check pager.<cmd> configuration if
   RUN_SETUP_GENTLY is used"
 git.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
Show changes to git.c +9 −4
diff --git a/git.c b/git.c
index 8a8d2b2..c46c996 100644
--- a/git.c
+++ b/git.c
@@ -230,13 +230,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 RUN_SETUP_GENTLY	(1<<1)
+#define USE_PAGER		(1<<2)
 /*
  * 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<<3)
 
 struct cmd_struct {
 	const char *cmd;
@@ -255,8 +256,12 @@ 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)
+		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.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:01 UTC · re: Jonathan Nieder · lore

[PATCH 04/12] shortlog: run setup_git_directory_gently() sooner

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

shortlog already runs a repository search unconditionally; running such a search earlier is not very risky.

Without this change, the “[pager] shortlog” configuration is not respected at all: “git shortlog” unconditionally paginates.

The tests are a bit slow. Running the full battery like this for all built-in commands would be counterproductive; the intent is rather to test shortlog as a representative example command using ..._gently().

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>
---
Clarified commit message; simplified diff by keeping the nongit
variable; added tests.
 builtin/shortlog.c |    3 +--
 git.c              |    2 +-
 t/t7006-pager.sh   |    9 +++++++++
 3 files changed, 11 insertions(+), 3 deletions(-)
Show changes to 3 files +11 −3

builtin/shortlog.c, git.c, t/t7006-pager.sh

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 5089502..36cb24b 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -249,7 +249,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 {
 	static struct shortlog log;
 	static struct rev_info rev;
-	int nongit;
+	int nongit = !startup_info->have_repository;
 
 	static const struct option options[] = {
 		OPT_BOOLEAN('n', "numbered", &log.sort_by_number,
@@ -265,7 +265,6 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 
 	struct parse_opt_ctx_t ctx;
 
-	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_default_config, NULL);
 	shortlog_init(&log);
 	init_revisions(&rev, prefix);
diff --git a/git.c b/git.c
index c46c996..b821058 100644
--- a/git.c
+++ b/git.c
@@ -384,7 +384,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
 		{ "rm", cmd_rm, RUN_SETUP },
 		{ "send-pack", cmd_send_pack, RUN_SETUP },
-		{ "shortlog", cmd_shortlog, USE_PAGER },
+		{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
 		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 9215c2f..084dfdb 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -377,4 +377,13 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
 test_doesnt_paginate      expect_failure test_must_fail 'git -p nonsense'
 
+test_pager_choices                       'git shortlog'
+test_expect_success 'setup: configure shortlog not to paginate' '
+	git config pager.shortlog false
+'
+test_doesnt_paginate      expect_success 'git shortlog'
+test_no_local_config_subdir expect_success 'git shortlog'
+test_default_pager        expect_success 'git -p shortlog'
+test_core_pager_subdir    expect_success 'git -p shortlog'
+
 test_done
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:06 UTC · re: Jonathan Nieder · lore

[PATCH 05/12] grep: run setup_git_directory_gently() sooner

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

git grep already runs a repository search unconditionally, even when the --no-index option is supplied; running such a search earlier is not very risky.

Just like with shortlog, without this change, the “[pager] grep” configuration is not respected at all.

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>
---
Aside from rewriting the commit message and adding tests, this
drops the interesting
 -		/* die the same way as if we did it at the beginning */
 -		setup_git_directory();
 +		die("No git repository found");

hunk. That change might be a good idea but it does not fit the theme of this chapter.

 builtin/grep.c   |    6 ++----
 git.c            |    2 +-
 t/t7006-pager.sh |   13 +++++++++++++
 3 files changed, 16 insertions(+), 5 deletions(-)
Show changes to 3 files +16 −5

builtin/grep.c, git.c, t/t7006-pager.sh

diff --git a/builtin/grep.c b/builtin/grep.c
index d0a73da..cd44926 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -791,7 +791,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	const char **paths = NULL;
 	int i;
 	int dummy;
-	int nongit = 0, use_index = 1;
+	int use_index = 1;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
 			"search in index instead of in the work tree"),
@@ -879,8 +879,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	prefix = setup_git_directory_gently(&nongit);
-
 	/*
 	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
 	 * to show usage information and exit.
@@ -925,7 +923,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_STOP_AT_NON_OPTION |
 			     PARSE_OPT_NO_INTERNAL_HELP);
 
-	if (use_index && nongit)
+	if (use_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
 
diff --git a/git.c b/git.c
index b821058..0240179 100644
--- a/git.c
+++ b/git.c
@@ -336,7 +336,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "fsck-objects", cmd_fsck, RUN_SETUP },
 		{ "gc", cmd_gc, RUN_SETUP },
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
-		{ "grep", cmd_grep, USE_PAGER },
+		{ "grep", cmd_grep, RUN_SETUP_GENTLY | USE_PAGER },
 		{ "hash-object", cmd_hash_object },
 		{ "help", cmd_help },
 		{ "index-pack", cmd_index_pack },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 084dfdb..fd7f77b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -105,6 +105,19 @@ test_expect_success TTY 'no pager with --no-pager' '
 	! test -e paginated.out
 '
 
+test_expect_success TTY 'configuration can disable pager' '
+	rm -f paginated.out &&
+	test_might_fail git config --unset pager.grep &&
+	test_terminal git grep initial &&
+	test -e paginated.out &&
+
+	rm -f paginated.out &&
+	git config pager.grep false &&
+	test_when_finished "git config --unset pager.grep" &&
+	test_terminal git grep initial &&
+	! test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:08 UTC · re: Jonathan Nieder · lore

[PATCH 06/12] apply: run setup_git_directory_gently() sooner

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

As v1.7.2~16^2 (2010-07-14) explains, without this change, “git --paginate apply” can ignore the repository-local “[core] pager” configuration.

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>
---
New explanation and test; simplified patch by keeping the
is_not_gitdir local.
 builtin/apply.c  |    3 +--
 git.c            |    2 +-
 t/t7006-pager.sh |    3 +++
 3 files changed, 5 insertions(+), 3 deletions(-)
Show changes to 3 files +5 −3

builtin/apply.c, git.c, t/t7006-pager.sh

diff --git a/builtin/apply.c b/builtin/apply.c
index 562e534..08f3586 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3611,7 +3611,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 {
 	int i;
 	int errs = 0;
-	int is_not_gitdir;
+	int is_not_gitdir = !startup_info->have_repository;
 	int binary;
 	int force_apply = 0;
 
@@ -3684,7 +3684,6 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		OPT_END()
 	};
 
-	prefix = setup_git_directory_gently(&is_not_gitdir);
 	prefix_length = prefix ? strlen(prefix) : 0;
 	git_config(git_apply_config, NULL);
 	if (apply_default_whitespace)
diff --git a/git.c b/git.c
index 0240179..82b390b 100644
--- a/git.c
+++ b/git.c
@@ -301,7 +301,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 		{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 		{ "annotate", cmd_annotate, RUN_SETUP },
-		{ "apply", cmd_apply },
+		{ "apply", cmd_apply, RUN_SETUP_GENTLY },
 		{ "archive", cmd_archive },
 		{ "bisect--helper", cmd_bisect__helper, RUN_SETUP | NEED_WORK_TREE },
 		{ "blame", cmd_blame, RUN_SETUP },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fd7f77b..96fbb0f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -399,4 +399,7 @@ test_no_local_config_subdir expect_success 'git shortlog'
 test_default_pager        expect_success 'git -p shortlog'
 test_core_pager_subdir    expect_success 'git -p shortlog'
 
+test_core_pager_subdir    expect_success test_must_fail \
+                                         'git -p apply </dev/null'
+
 test_done
-- 
1.7.2.1.544.ga752d.dirty
Ævar Arnfjörð Bjarmason· Aug 15, 2010, 20:13 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/12] apply: run setup_git_directory_gently() sooner

2010/8/6 Jonathan Nieder <jrnieder@gmail.com>:
Show 5 quoted lines
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> As v1.7.2~16^2 (2010-07-14) explains, without this change,
> “git --paginate apply” can ignore the repository-local
> “[core] pager” configuration.
Applying this patch broke the following tests:
    ./t4119-apply-config.sh ./t4111-apply-subdir.sh
./t4131-apply-fake-ancestor.sh

I didn't look into why, they're breaking in pu now at 6ea3604. I didn't look into why, see the smoke report at http://smoke.git.nix.is/app/projects/report_details/33

Nguyen Thai Ngoc Duy· Aug 15, 2010, 22:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 06/12] apply: run setup_git_directory_gently() sooner

On Mon, Aug 16, 2010 at 6:13 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 15 quoted lines
> 2010/8/6 Jonathan Nieder <jrnieder@gmail.com>:
>> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> As v1.7.2~16^2 (2010-07-14) explains, without this change,
>> “git --paginate apply” can ignore the repository-local
>> “[core] pager” configuration.
>
> Applying this patch broke the following tests:
>
>    ./t4119-apply-config.sh ./t4111-apply-subdir.sh
> ./t4131-apply-fake-ancestor.sh
>
> I didn't look into why, they're breaking in pu now at 6ea3604. I
> didn't look into why, see the smoke report at
> http://smoke.git.nix.is/app/projects/report_details/33

The patch loses prefix and git-apply couldn't find files on disk. This patch may fix it. I'm running tests now.

Show changes to builtin/apply.c +2 −1
diff --git a/builtin/apply.c b/builtin/apply.c
index 6c4b747..29f4194 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3779,7 +3779,7 @@ static int option_parse_directory(const struct
option *opt,
 	return 0;
 }

-int cmd_apply(int argc, const char **argv, const char *unused_prefix)
+int cmd_apply(int argc, const char **argv, const char *prefix_)
 {
 	int i;
 	int errs = 0;
@@ -3856,6 +3856,7 @@ int cmd_apply(int argc, const char **argv, const
char *unused_prefix)
 		OPT_END()
 	};

+	prefix = prefix_;
 	prefix_length = prefix ? strlen(prefix) : 0;
 	git_config(git_apply_config, NULL);
 	if (apply_default_whitespace)
-- 
Duy
Jonathan Nieder· Aug 15, 2010, 23:11 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 06/12] apply: run setup_git_directory_gently() sooner

Nguyen Thai Ngoc Duy wrote:
> The patch loses prefix and git-apply couldn't find files on disk. This
> patch may fix it. I'm running tests now.
Oh, good catch.  Thanks!
Show 20 quoted lines
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 6c4b747..29f4194 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3779,7 +3779,7 @@ static int option_parse_directory(const struct
> option *opt,
>  	return 0;
>  }
> 
> -int cmd_apply(int argc, const char **argv, const char *unused_prefix)
> +int cmd_apply(int argc, const char **argv, const char *prefix_)
>  {
>  	int i;
>  	int errs = 0;
> @@ -3856,6 +3856,7 @@ int cmd_apply(int argc, const char **argv, const
> char *unused_prefix)
>  		OPT_END()
>  	};
> 
> +	prefix = prefix_;
This is why we shouldn’t be using globals so cavalierly. :)
Nguyễn Thái Ngọc Duy· Aug 16, 2010, 00:36 UTC · re: Jonathan Nieder · lore

[PATCH 06/12 v2] apply: run setup_git_directory_gently() sooner

As v1.7.2~16^2 (2010-07-14) explains, without this change, “git --paginate apply” can ignore the repository-local “[core] pager” configuration.

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 This is a replacement for 4ce097c (apply: run
 setup_git_directory_gently() sooner) in pu, branch jn/paginate-fix,
 which fixes prefix loss in 4ce097c.
 All tests seem to run ok for me.
 builtin/apply.c  |    6 +++---
 git.c            |    2 +-
 t/t7006-pager.sh |    3 +++
 3 files changed, 7 insertions(+), 4 deletions(-)
Show changes to 3 files +7 −4

builtin/apply.c, git.c, t/t7006-pager.sh

diff --git a/builtin/apply.c b/builtin/apply.c
index 12ef9ea..f005ba1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3607,11 +3607,11 @@ static int option_parse_directory(const struct option *opt,
 	return 0;
 }
 
-int cmd_apply(int argc, const char **argv, const char *unused_prefix)
+int cmd_apply(int argc, const char **argv, const char *prefix_)
 {
 	int i;
 	int errs = 0;
-	int is_not_gitdir;
+	int is_not_gitdir = !startup_info->have_repository;
 	int binary;
 	int force_apply = 0;
 
@@ -3684,7 +3684,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		OPT_END()
 	};
 
-	prefix = setup_git_directory_gently(&is_not_gitdir);
+	prefix = prefix_;
 	prefix_length = prefix ? strlen(prefix) : 0;
 	git_config(git_apply_config, NULL);
 	if (apply_default_whitespace)
diff --git a/git.c b/git.c
index 5a47bfb..38dbe70 100644
--- a/git.c
+++ b/git.c
@@ -301,7 +301,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 		{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 		{ "annotate", cmd_annotate, RUN_SETUP },
-		{ "apply", cmd_apply },
+		{ "apply", cmd_apply, RUN_SETUP_GENTLY },
 		{ "archive", cmd_archive },
 		{ "bisect--helper", cmd_bisect__helper, RUN_SETUP | NEED_WORK_TREE },
 		{ "blame", cmd_blame, RUN_SETUP },
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index bb95335..6c86d70 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -391,4 +391,7 @@ test_no_local_config_subdir expect_success 'git shortlog'
 test_default_pager        expect_success 'git -p shortlog'
 test_core_pager_subdir    expect_success 'git -p shortlog'
 
+test_core_pager_subdir    expect_success test_must_fail \
+					 'git -p apply </dev/null'
+
 test_done
-- 
1.7.1.rc1.69.g24c2f7
Junio C Hamano· Aug 16, 2010, 03:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 06/12 v2] apply: run setup_git_directory_gently() sooner

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 12 quoted lines
> As v1.7.2~16^2 (2010-07-14) explains, without this change,
> “git --paginate apply” can ignore the repository-local
> “[core] pager” configuration.
>
> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  This is a replacement for 4ce097c (apply: run
>  setup_git_directory_gently() sooner) in pu, branch jn/paginate-fix,
>  which fixes prefix loss in 4ce097c.
Thanks for a quick fix.
Jonathan Nieder· Aug 6, 2010, 03:12 UTC · re: Jonathan Nieder · lore

[PATCH 07/12] bundle: run setup_git_directory_gently() sooner

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

Without this change, “git -p bundle” does not always respect the repository-local “[core] pager” setting.

It is hard to notice because subcommands other than “git bundle unbundle” do not produce much output.

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>
---
New tests and explanation.

The first new test perhaps deserves some explanation. Its only purpose is to let us use the

	test -e paginated.out || test -e subdir/paginated.out
hedge in other tests with good conscience.
 builtin/bundle.c |    6 ++----
 git.c            |    2 +-
 t/t7006-pager.sh |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)
Show changes to 3 files +36 −5

builtin/bundle.c, git.c, t/t7006-pager.sh

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 2006cc5..80649ba 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -18,7 +18,6 @@ static const char builtin_bundle_usage[] =
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
 	struct bundle_header header;
-	int nongit;
 	const char *cmd, *bundle_file;
 	int bundle_fd = -1;
 	char buffer[PATH_MAX];
@@ -31,7 +30,6 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	argc -= 2;
 	argv += 2;
 
-	prefix = setup_git_directory_gently(&nongit);
 	if (prefix && bundle_file[0] != '/') {
 		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
 		bundle_file = buffer;
@@ -54,11 +52,11 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 		return !!list_bundle_refs(&header, argc, argv);
 	}
 	if (!strcmp(cmd, "create")) {
-		if (nongit)
+		if (!startup_info->have_repository)
 			die("Need a repository to create a bundle.");
 		return !!create_bundle(&header, bundle_file, argc, argv);
 	} else if (!strcmp(cmd, "unbundle")) {
-		if (nongit)
+		if (!startup_info->have_repository)
 			die("Need a repository to unbundle.");
 		return !!unbundle(&header, bundle_fd) ||
 			list_bundle_refs(&header, argc, argv);
diff --git a/git.c b/git.c
index 82b390b..94bb1e3 100644
--- a/git.c
+++ b/git.c
@@ -306,7 +306,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "bisect--helper", cmd_bisect__helper, RUN_SETUP | NEED_WORK_TREE },
 		{ "blame", cmd_blame, RUN_SETUP },
 		{ "branch", cmd_branch, RUN_SETUP },
-		{ "bundle", cmd_bundle },
+		{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
 		{ "checkout-index", cmd_checkout_index,
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 96fbb0f..6680668 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -57,6 +57,21 @@ test_expect_success TTY 'some commands use a pager' '
 	test -e paginated.out
 '
 
+test_expect_failure TTY 'pager runs from subdir' '
+	echo subdir/paginated.out >expected &&
+	mkdir -p subdir &&
+	rm -f paginated.out subdir/paginated.out &&
+	(
+		cd subdir &&
+		test_terminal git log
+	) &&
+	{
+		ls paginated.out subdir/paginated.out ||
+		:
+	} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success TTY 'some commands do not use a pager' '
 	rm -f paginated.out ||
 	cleanup_fail &&
@@ -118,6 +133,24 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
+test_expect_success 'configuration can enable pager (from subdir)' '
+	rm -f paginated.out &&
+	mkdir -p subdir &&
+	git config pager.bundle true &&
+	test_when_finished "git config --unset pager.bundle" &&
+
+	git bundle create test.bundle --all &&
+	rm -f paginated.out subdir/paginated.out &&
+	(
+		cd subdir &&
+		test_terminal git bundle unbundle ../test.bundle
+	) &&
+	{
+		test -e paginated.out ||
+		test -e subdir/paginated.out
+	}
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
1.7.2.1.544.ga752d.dirty
Thomas Rast· Aug 16, 2010, 07:21 UTC · re: Jonathan Nieder · lore

Re: [PATCH 07/12] bundle: run setup_git_directory_gently() sooner

Jonathan Nieder wrote:
>  builtin/bundle.c |    6 ++----
>  git.c            |    2 +-
>  t/t7006-pager.sh |   33 +++++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+), 5 deletions(-)
[...]
Show 21 quoted lines
> +test_expect_success 'configuration can enable pager (from subdir)' '
> +	rm -f paginated.out &&
> +	mkdir -p subdir &&
> +	git config pager.bundle true &&
> +	test_when_finished "git config --unset pager.bundle" &&
> +
> +	git bundle create test.bundle --all &&
> +	rm -f paginated.out subdir/paginated.out &&
> +	(
> +		cd subdir &&
> +		test_terminal git bundle unbundle ../test.bundle
> +	) &&
> +	{
> +		test -e paginated.out ||
> +		test -e subdir/paginated.out
> +	}
> +'
> +
>  # A colored commit log will begin with an appropriate ANSI escape
>  # for the first color; the text "commit" comes later.
>  colorful() {

On my valgrind test setup, this never worked (i.e., fails and bisects to this commit).

Oddly, I am seeing this error message from the second test (second in t7006, not in this patch):

  expecting success: 
          rm -f stdout_is_tty ||
          cleanup_fail &&
          if test -t 1
          then
                  >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
          fi
  Can't locate IO/Pty.pm in @INC (@INC contains: <snip>) at /local/home/trast/git/t/t7006/test-terminal.perl line 4.
  BEGIN failed--compilation aborted at /local/home/trast/git/t/t7006/test-terminal.perl line 4.
  ok 2 - set up terminal for tests

Which raises a few questions: Why was this never an issue before? Am I supposed to have IO::Pty with a perl install (it's a perl 5.8.8) or does the test need a prerequisite other than HAVE_PERL?4

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Jonathan Nieder· Aug 16, 2010, 08:07 UTC · re: Thomas Rast · lore

Re: [PATCH 07/12] bundle: run setup_git_directory_gently() sooner

On Mon, Aug 16, 2010 at 09:21:32AM +0200, Thomas Rast wrote:
> Jonathan Nieder wrote:
>> +test_expect_success 'configuration can enable pager (from subdir)' '
[...]
>> +		test_terminal git bundle unbundle ../test.bundle
[...]
> On my valgrind test setup, this never worked (i.e., fails and bisects
> to this commit).
Agh, sloppy me...  Fixes below.
> Oddly, I am seeing this error message from the second test (second in
> t7006, not in this patch):
[...]
Show 5 quoted lines
>   Can't locate IO/Pty.pm in @INC (@INC contains: <snip>) at /local/home/trast/git/t/t7006/test-terminal.perl line 4.
>   BEGIN failed--compilation aborted at /local/home/trast/git/t/t7006/test-terminal.perl line 4.
>   ok 2 - set up terminal for tests
> 
> Which raises a few questions: Why was this never an issue before?

That’s expected behavior, marked with “ok”. :) That test is checking if IO::Pty is available and works; if not, the relevant tests should be skipped.

> does the test need a prerequisite other than HAVE_PERL?4
Exactly.
Jonathan Nieder (2):
  t7006 (pager): add missing TTY prerequisite
  t7006 (pager): add missing TTY prerequisite
 t/t7006-pager.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Jonathan Nieder· Aug 16, 2010, 08:11 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] t7006 (pager): add missing TTY prerequisite

The config pagination test should not run if there is not a tty available to force pagination on.

Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This one fixes for “config: run setup_git_directory_gently()
sooner”.

Thanks again for the report. I’ll think more about how to make problems like this easily reproducible (a test_mentions_prereq() function?).

 t/t7006-pager.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to t/t7006-pager.sh +1 −1
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fcd846a..fb744e3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,7 +134,7 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
-test_expect_success 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
 	rm -f paginated.out &&
 	git config pager.config true &&
 	test_when_finished "git config --unset pager.config" &&
-- 
1.7.2.1.544.ga752d.dirty
Junio C Hamano· Aug 16, 2010, 16:41 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] t7006 (pager): add missing TTY prerequisite

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
> The config pagination test should not run if there is not a tty
> available to force pagination on.
>
> Reported-by: Thomas Rast <trast@student.ethz.ch>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This one fixes for “config: run setup_git_directory_gently()
> sooner”.
Somehow we didn't seem to have got 1/2, but here is what I'll queue.

By the way, could you stop using those "pretty quotes" and stick to the good old "ASCII 0x22" double-quotes? I find them ugly in the git log output.

-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mon, 16 Aug 2010 03:08:34 -0500
Subject: [PATCH] t7006 (pager): add missing TTY prerequisites

The "git bundle unbundle" and "git config" pagination tests are not supposed to run when stdout is not a terminal and IO::Pty not available to make one on the fly.

Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7006-pager.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7006-pager.sh +2 −2
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 017565f..fb744e3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,7 +134,7 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
-test_expect_success 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
 	rm -f paginated.out &&
 	git config pager.config true &&
 	test_when_finished "git config --unset pager.config" &&
@@ -142,7 +142,7 @@ test_expect_success 'git config uses a pager if configured to' '
 	test -e paginated.out
 '
 
-test_expect_success 'configuration can enable pager (from subdir)' '
+test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	rm -f paginated.out &&
 	mkdir -p subdir &&
 	git config pager.bundle true &&
-- 
1.7.2.1.224.g2f41ea
Jonathan Nieder· Aug 6, 2010, 03:15 UTC · re: Jonathan Nieder · lore

[PATCH 08/12] config: run setup_git_directory_gently() sooner

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

For the pager choice (and the choice to paginate) to reflect the current repository configuration, the repository needs to be located 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>
---
Relative to the previous round, this simplifies the patch by retaining
the nongit var; squashes in a test; and adds some words of explanation
to the log message.
 builtin/config.c |    5 ++---
 git.c            |    4 ++--
 t/t7006-pager.sh |    8 ++++++++
 3 files changed, 12 insertions(+), 5 deletions(-)
Show changes to 3 files +12 −5

builtin/config.c, git.c, t/t7006-pager.sh

diff --git a/builtin/config.c b/builtin/config.c
index f3d1660..3f8ef91 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -330,11 +330,10 @@ 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;
+	int nongit = !startup_info->have_repository;
 	char *value;
-	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
 
diff --git a/git.c b/git.c
index 94bb1e3..f57003d 100644
--- a/git.c
+++ b/git.c
@@ -319,7 +319,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 },
@@ -376,7 +376,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 6680668..dbbe8d9 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -133,6 +133,14 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
+test_expect_success 'git config uses a pager if configured to' '
+	rm -f paginated.out &&
+	git config pager.config true &&
+	test_when_finished "git config --unset pager.config" &&
+	test_terminal git config --list &&
+	test -e paginated.out
+'
+
 test_expect_success 'configuration can enable pager (from subdir)' '
 	rm -f paginated.out &&
 	mkdir -p subdir &&
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:18 UTC · re: Jonathan Nieder · lore

[PATCH 09/12] index-pack: run setup_git_directory_gently() sooner

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

index-pack already runs a repository search unconditionally; running such a search earlier is not risky and ensures GIT_DIR will be set correctly if the configuration needs to be accessed from run_builtin().

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>
---
Requires the jn/maint-setup-fix topic to apply.  See the cover
letter "[PATCH jn/paginate-fix 0/12] Re: ..." for more on that.
Just like last round, except for the new log message.
 builtin/index-pack.c |    2 --
 git.c                |    2 +-
 2 files changed, 1 insertions(+), 3 deletions(-)
Show changes to 2 files +1 −3

builtin/index-pack.c, git.c

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 89a1f12..e852890 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -880,12 +880,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
-	int nongit;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(index_pack_usage);
 
-	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_index_pack_config, NULL);
 	if (prefix && chdir(prefix))
 		die("Cannot come back to cwd");
diff --git a/git.c b/git.c
index e89c010..92ec5d3 100644
--- a/git.c
+++ b/git.c
@@ -339,7 +339,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "grep", cmd_grep, RUN_SETUP_GENTLY },
 		{ "hash-object", cmd_hash_object },
 		{ "help", cmd_help },
-		{ "index-pack", cmd_index_pack },
+		{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
 		{ "init", cmd_init_db },
 		{ "init-db", cmd_init_db },
 		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:20 UTC · re: Jonathan Nieder · lore

[PATCH 10/12] ls-remote: run setup_git_directory_gently() sooner

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

ls-remote already runs a repository search unconditionally to learn about remote nicknames and "[url] insteadof" shortcuts. Run that search a little sooner, and now one can try

	[pager]
		ls-remote
to automatically paginate ls-remote output, or use repository-local
	[core]
		pager = whatever
with "git --paginate ls-remote <url>".
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>
---
As before, aside from the commit message.
 builtin/ls-remote.c |    3 ---
 git.c               |    4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)
Show changes to 2 files +2 −5

builtin/ls-remote.c, git.c

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 34480cf..97eed40 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -32,7 +32,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	const char *dest = NULL;
-	int nongit;
 	unsigned flags = 0;
 	int quiet = 0;
 	const char *uploadpack = NULL;
@@ -42,8 +41,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct transport *transport;
 	const struct ref *ref;
 
-	setup_git_directory_gently(&nongit);
-
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/git.c b/git.c
index 92ec5d3..63630e7 100644
--- a/git.c
+++ b/git.c
@@ -345,7 +345,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
 		{ "ls-files", cmd_ls_files, RUN_SETUP },
 		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-		{ "ls-remote", cmd_ls_remote },
+		{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 		{ "mailinfo", cmd_mailinfo },
 		{ "mailsplit", cmd_mailsplit },
 		{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
@@ -366,7 +366,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
 		{ "pack-redundant", cmd_pack_redundant, RUN_SETUP },
 		{ "patch-id", cmd_patch_id },
-		{ "peek-remote", cmd_ls_remote },
+		{ "peek-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 		{ "pickaxe", cmd_blame, RUN_SETUP },
 		{ "prune", cmd_prune, RUN_SETUP },
 		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:21 UTC · re: Jonathan Nieder · lore

[PATCH 11/12] var: run setup_git_directory_gently() sooner

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

Part of a campaign to make repository-local configuration available early (simplifying the startup sequence for built-in commands).

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>
---
snuck in a little vertical compression while at it ;-)
 builtin/var.c |    9 ++-------
 git.c         |    2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)
Show changes to 2 files +3 −8

builtin/var.c, git.c

diff --git a/builtin/var.c b/builtin/var.c
index 70fdb4d..0744bb8 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -74,14 +74,9 @@ static int show_config(const char *var, const char *value, void *cb)
 
 int cmd_var(int argc, const char **argv, const char *prefix)
 {
-	const char *val;
-	int nongit;
-	if (argc != 2) {
+	const char *val = NULL;
+	if (argc != 2)
 		usage(var_usage);
-	}
-
-	setup_git_directory_gently(&nongit);
-	val = NULL;
 
 	if (strcmp(argv[1], "-l") == 0) {
 		git_config(show_config, NULL);
diff --git a/git.c b/git.c
index 63630e7..84bef76 100644
--- a/git.c
+++ b/git.c
@@ -398,7 +398,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "update-ref", cmd_update_ref, RUN_SETUP },
 		{ "update-server-info", cmd_update_server_info, RUN_SETUP },
 		{ "upload-archive", cmd_upload_archive },
-		{ "var", cmd_var },
+		{ "var", cmd_var, RUN_SETUP_GENTLY },
 		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 		{ "version", cmd_version },
 		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:27 UTC · re: Jonathan Nieder · lore

[PATCH 12/12] merge-file: run setup_git_directory_gently() sooner

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

Part of a campaign to make repository-local configuration available early (simplifying the startup sequence for built-in commands).

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>
---
As before (except the commit message).

Well, that’s it. There’s also a separate fix for "check-ref-format --branch", which I’ll send under separate cover.

Still to be considered are various commands that currently do not do an unconditional repository search, especially low-level commands that are not about the current repository: verify-pack, mailinfo, hash-object, archive --remote, check-ref-format, diff --no-index, help, and all the others. Let’s bite off one thing at a time. :)

Thanks for reading; I hope the patches were not too dull. Thoughts (especially improvements) welcome, as always.

 builtin/merge-file.c |    4 +---
 git.c                |    2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)
Show changes to 2 files +2 −4

builtin/merge-file.c, git.c

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index b8e9e5b..b6664d4 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -28,7 +28,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmparam_t xmp = {{0}};
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
-	int nongit;
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, "send results to standard output"),
 		OPT_SET_INT(0, "diff3", &xmp.style, "use a diff3 based merge", XDL_MERGE_DIFF3),
@@ -50,8 +49,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmp.style = 0;
 	xmp.favor = 0;
 
-	prefix = setup_git_directory_gently(&nongit);
-	if (!nongit) {
+	if (startup_info->have_repository) {
 		/* Read the configuration file */
 		git_config(git_xmerge_config, NULL);
 		if (0 <= git_xmerge_style)
diff --git a/git.c b/git.c
index 84bef76..5433875 100644
--- a/git.c
+++ b/git.c
@@ -350,7 +350,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "mailsplit", cmd_mailsplit },
 		{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-base", cmd_merge_base, RUN_SETUP },
-		{ "merge-file", cmd_merge_file },
+		{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 		{ "merge-index", cmd_merge_index, RUN_SETUP },
 		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
 		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:34 UTC · re: Jonathan Nieder · lore

[PATCH master 0/2] fix "check-ref-format --branch" from subdir of toplevel

Here’s a quick fix from the nd/setup series. Tested against master, though it probably should be against maint.

Jonathan Nieder (2):
  check-ref-format: split off functions for subcommands
  check-ref-format --branch: run repository search
 builtin/check-ref-format.c  |   44 ++++++++++++++++++++++++++----------------
 t/t1402-check-ref-format.sh |   17 ++++++++++++++++
 2 files changed, 44 insertions(+), 17 deletions(-)
Jonathan Nieder· Aug 6, 2010, 03:36 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] check-ref-format: handle subcommands in separate functions

The code for each subcommand should be easier to read and manipulate this way.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/check-ref-format.c |   42 +++++++++++++++++++++++++-----------------
 1 files changed, 25 insertions(+), 17 deletions(-)
Show changes to builtin/check-ref-format.c +25 −17
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index b106c65..8707ee9 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -33,28 +33,36 @@ static void collapse_slashes(char *dst, const char *src)
 	*dst = '\0';
 }
 
+static int check_ref_format_branch(const char *arg)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (strbuf_check_branch_ref(&sb, arg))
+		die("'%s' is not a valid branch name", arg);
+	printf("%s\n", sb.buf + 11);
+	return 0;
+}
+
+static int check_ref_format_print(const char *arg)
+{
+	char *refname = xmalloc(strlen(arg) + 1);
+
+	if (check_ref_format(arg))
+		return 1;
+	collapse_slashes(refname, arg);
+	printf("%s\n", refname);
+	return 0;
+}
+
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(builtin_check_ref_format_usage);
 
-	if (argc == 3 && !strcmp(argv[1], "--branch")) {
-		struct strbuf sb = STRBUF_INIT;
-
-		if (strbuf_check_branch_ref(&sb, argv[2]))
-			die("'%s' is not a valid branch name", argv[2]);
-		printf("%s\n", sb.buf + 11);
-		exit(0);
-	}
-	if (argc == 3 && !strcmp(argv[1], "--print")) {
-		char *refname = xmalloc(strlen(argv[2]) + 1);
-
-		if (check_ref_format(argv[2]))
-			exit(1);
-		collapse_slashes(refname, argv[2]);
-		printf("%s\n", refname);
-		exit(0);
-	}
+	if (argc == 3 && !strcmp(argv[1], "--branch"))
+		return check_ref_format_branch(argv[2]);
+	if (argc == 3 && !strcmp(argv[1], "--print"))
+		return check_ref_format_print(argv[2]);
 	if (argc != 2)
 		usage(builtin_check_ref_format_usage);
 	return !!check_ref_format(argv[1]);
-- 
1.7.2.1.544.ga752d.dirty
Jonathan Nieder· Aug 6, 2010, 03:39 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] Allow "check-ref-format --branch" from subdirectory

check-ref-format --branch requires access to the repository to resolve refs like @{-1}.

Noticed by Duy.
Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The original used the RUN_SETUP_GENTLY flag to run setup
unconditionally.  That might still be a good idea, but I am
more comfortable running setup just for this subcommand
for now so commands like "git check-ref-format refs/foo" are not
affected.
 builtin/check-ref-format.c  |    2 ++
 t/t1402-check-ref-format.sh |   17 +++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
Show changes to 2 files +19 −0

builtin/check-ref-format.c, t/t1402-check-ref-format.sh

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 8707ee9..ae3f281 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -36,7 +36,9 @@ static void collapse_slashes(char *dst, const char *src)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
+	int nongit;
 
+	setup_git_directory_gently(&nongit);
 	if (strbuf_check_branch_ref(&sb, arg))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", sb.buf + 11);
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index eb45afb..782e75d 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -41,6 +41,23 @@ test_expect_success "check-ref-format --branch @{-1}" '
 	refname2=$(git check-ref-format --branch @{-2}) &&
 	test "$refname2" = master'
 
+test_expect_success 'check-ref-format --branch from subdir' '
+	mkdir subdir &&
+
+	T=$(git write-tree) &&
+	sha1=$(echo A | git commit-tree $T) &&
+	git update-ref refs/heads/master $sha1 &&
+	git update-ref refs/remotes/origin/master $sha1
+	git checkout master &&
+	git checkout origin/master &&
+	git checkout master &&
+	refname=$(
+		cd subdir &&
+		git check-ref-format --branch @{-1}
+	) &&
+	test "$refname" = "$sha1"
+'
+
 valid_ref_normalized() {
 	test_expect_success "ref name '$1' simplifies to '$2'" "
 		refname=\$(git check-ref-format --print '$1') &&
-- 
1.7.2.1.544.ga752d.dirty
Junio C Hamano· Aug 6, 2010, 19:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] Allow "check-ref-format --branch" from subdirectory

Jonathan Nieder <jrnieder@gmail.com> writes:
> check-ref-format --branch requires access to the repository
> to resolve refs like @{-1}.
Thanks; the patch makes sense to me.
Nguyen Thai Ngoc Duy· Aug 6, 2010, 04:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH jn/paginate-fix 0/12] Re: git --paginate: do not commit pager choice too early

2010/8/6 Jonathan Nieder <jrnieder@gmail.com>:
Show 16 quoted lines
> Nguyễn Thái Ngọc Duy (12):
>  git wrapper: introduce startup_info struct
>  setup: remember whether repository was found
>  git wrapper: allow setup_git_directory_gently() be called earlier
>  shortlog: run setup_git_directory_gently() sooner
>  grep: run setup_git_directory_gently() sooner
>  apply: run setup_git_directory_gently() sooner
>  bundle: run setup_git_directory_gently() sooner
>  config: run setup_git_directory_gently() sooner
>  index-pack: run setup_git_directory_gently() sooner
>  ls-remote: run setup_git_directory_gently() sooner
>  var: run setup_git_directory_gently() sooner
>  merge-file: run setup_git_directory_gently() sooner
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/144000/focus=144110
>

I was waiting for jn/maint-setup-fix to graduate before pushing out some more patches then I got side tracked by the subtree clone. Anyway, thanks!

-- 
Duy

← back to recent threads