[PATCH 1/2] builtins: do not commit pager choice early

[PATCH 1/2] builtins: do not commit pager choice early

[Nguyễn Thái Ngọc Duy]

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

diff --git a/git.c b/git.c
index 3798791..bd1d4bb 100644
--- a/git.c
+++ b/git.c
@@ -514,7 +514,6 @@ 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;
-- 
1.7.0.rc1.541.g2da82.dirty

[PATCH 2/2] builtins: setup repository before print unknown command error

[Nguyễn Thái Ngọc Duy]

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

diff --git a/git.c b/git.c
index bd1d4bb..d84996c 100644
--- a/git.c
+++ b/git.c
@@ -547,6 +547,10 @@ int main(int argc, const char **argv)
 			exit(1);
 		}
 		if (!done_help) {
+			if (!startup_info->have_run_setup_gitdir) {
+				int nongit_ok;
+				setup_git_directory_gently(&nongit_ok);
+			}
 			cmd = argv[0] = help_unknown_cmd(cmd);
 			done_help = 1;
 		} else
-- 
1.7.0.rc1.541.g2da82.dirty

Re: [PATCH 2/2] builtins: setup repository before print unknown command error

[Jonathan Nieder]

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

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

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?

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 6757734..792abe2 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -33,6 +33,21 @@ test_expect_success 'plain' '
 	check_config plain/.git false unset
 '
 
+test_expect_failure 'plain through aliased command' '
+	(
+		unset GIT_DIR GIT_WORK_TREE GIT_CONFIG_NOGLOBAL &&
+		HOME=$(pwd)/alias-config &&
+		export HOME &&
+		mkdir alias-config &&
+		echo "[alias] aliasedinit = init" >alias-config/.gitconfig &&
+		mkdir plain-aliased &&
+		cd plain-aliased &&
+		git aliasedinit
+	) &&
+	check_config plain-aliased/.git false unset
+'
+
 test_expect_success 'plain with GIT_WORK_TREE' '
 	if (
 		unset GIT_DIR

Re: [PATCH 1/2] builtins: do not commit pager choice early

[Jonathan Nieder]

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

> Committing pager choice may require setting up the pager, which
> will need access to repository.
[...]
> Signed-off-by: Duy Nguyen <pclouds@gmail.com>

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.

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

Makes sense.

Thanks,
Jonathan