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

Junio C Hamano [off-list ref] writes:

Jonathan Nieder [off-list ref] writes:

quoted

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..

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.

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

-- 

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 <redacted>
---
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(-)

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

2010/7/15 Junio C Hamano [off-list ref]:

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

Junio C Hamano wrote:

    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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Tweaked the log message a bit.

 cache.h |    1 +
 setup.c |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 - 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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
As before, aside from the commit message.

 builtin/ls-remote.c |    3 ---
 git.c               |    4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
snuck in a little vertical compression while at it ;-)

 builtin/var.c |    9 ++-------
 git.c         |    2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

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

From: Nguyễn Thái Ngọc Duy <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

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(-)

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

Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/check-ref-format.c |   42 +++++++++++++++++++++++++-----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

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

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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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

2010/8/6 Jonathan Nieder [off-list ref]:

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

Jonathan Nieder [off-list ref] writes:

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

Thanks; the patch makes sense to me.

2010/8/6 Jonathan Nieder [off-list ref]:

From: Nguyễn Thái Ngọc Duy <redacted>

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

On Mon, Aug 16, 2010 at 6:13 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

2010/8/6 Jonathan Nieder [off-list ref]:

quoted

From: Nguyễn Thái Ngọc Duy <redacted>

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.

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

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!

This is why we shouldn’t be using globals so cavalierly. :)

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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(-)

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

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 wrote:

 builtin/bundle.c |    6 ++----
 git.c            |    2 +-
 t/t7006-pager.sh |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

[...]

+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

On Mon, Aug 16, 2010 at 09:21:32AM +0200, Thomas Rast wrote:

Jonathan Nieder wrote:

quoted

+test_expect_success 'configuration can enable pager (from subdir)' '

[...]

quoted

+		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):

[...]

  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(-)

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

Reported-by: Thomas Rast <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

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