patch, 2 partsbuiltins: do not commit pager choice early

From: Duy Nguyen <pclouds@gmail.com>

Committing pager choice may require setting up the pager, which will need access to repository.

At the time after handle_options() is called, the repository has not been found yet. As a result, unallowed access to repository may happen.

There are several possible code path after handle_options()/commit_pager_choice() is called:

1. list_common_cmds_help()
2. run_argv()
3. help_unknown_cmd()

Case 2. will have commit_pager_choice() called inside run_builtin() if a command is found. Case 1. and 3. won't need a pager, it's short printout and should be fitted within a screen. So, removing commit_pager_choice() call after handle_options() is safe.

Signed-off-by: Duy Nguyen <pclouds@gmail.com>
---
 On top of nd/setup. This may help fix the breakage in t7006. And forget t9100
 breakage I mentioned elsewhere. My system is broken, git-svn dies when it "exit 0;"
 and even git one year ago does not fix it.

 git.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

From: Duy Nguyen <pclouds@gmail.com>

help_unknown_cmd() will need to look into repository's config, in order to collect all possible commands/aliases and give a suggestion. So, repository must be set up before this function is called.

As it is now, because
 - alias handling will always be done before help_unknown_cmd()
 - alias handling code will search and set up repository if found
 - alias handline code will not undo repository setup

These ensure that repository will always be set up (or attempted to set up) before help_unknown_cmd(), so there is no issue. But the setup dependency here is subtle. It may break some day if someone reorders the loop, for example.

Make the repository setup explicit, to express it clearer, although this code will never be run (It does not mean the code is not tested, I removed alias handling code to reproduce the problem, and this code helped fix it)

Signed-off-by: Duy Nguyen <pclouds@gmail.com>
---
 git.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Nguyễn Thái Ngọc Duy wrote:

It took me a while to figure out what was going on here. I think it is too complicated. Could it make sense to use

	/*
	 * help_unknown_cmd() requires that the repository has searched
	 * for and set up if found.
	 * Luckily, the alias handling code already took care of this.
	 */
	if (!startup_info->have_run_setup_gitdir)
		die("internal error handling unknown command");

instead, to document the current assumption?

Alternatively, one might add a new ensure_git_directory_is_set_up() function that only runs setup_git_repository_gently if it has not already been run. I do not like that so much --- too easy to hide bugs.

Better to run setup_git_repository_gently once unconditionally, before handle_argv. Unfortunately, that breaks git-init. Actually, the current series breaks git-init when run through an alias.

Thoughts?

Nguyễn Thái Ngọc Duy wrote:

[...]

Tested-by: Jonathan Nieder <jrnieder@gmail.com>

Without this patch, I get

| * expecting success: 
|         test_terminal git --paginate rev-list HEAD &&
|         test -e paginated.out
| 
| fatal: internal error: access to .git/config without repo setup
| * FAIL 7: git --paginate rev-list uses a pager

and with it, t7006 passes again. I assume on your system that test was being skipped when running tests without -v (confusing, yes) because that test needs a tty but IO::Pty from CPAN is not present.

Makes sense.

Thanks, Jonathan