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

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

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

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 a note about this.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 git.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/git.c b/git.c
index bd1d4bb..7ebfea5 100644
--- a/git.c
+++ b/git.c
@@ -547,6 +547,13 @@ int main(int argc, const char **argv)
 			exit(1);
 		}
 		if (!done_help) {
+			/*
+			 * help_unknown_cmd() requires that a repository has been
+			 * 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");
 			cmd = argv[0] = help_unknown_cmd(cmd);
 			done_help = 1;
 		} else
-- 
1.7.0.rc1.541.g2da82.dirty

[PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository

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

startup_info->have_run_setup_gitdir is used to guard unallowed access
to repository. When a repository has been set up and the real command
does not expect any setup, we should revert to the original "fresh"
state, including startup_info->have_run_setup_gitdir. Otherwise, the
next attempt to set up repository will fail.

While at it, also reset repository_format_version to zero. When
omitted, the format should be understood as the last supported
version, i.e. zero. This is probably only used by "git init" or "git
clone".

Fault caught by Jonathan Nieder (and demonstrated with a beautiful test case)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Junio, I can split this up and put it to the original patches where the faults
 are introduced if you want.

 git.c           |   13 ++++++++-----
 setup.c         |    2 +-
 t/t0001-init.sh |   14 ++++++++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index 7ebfea5..5c249fd 100644
--- a/git.c
+++ b/git.c
@@ -242,11 +242,14 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			int nongit_ok;
 			setup_git_directory_gently(&nongit_ok);
 		}
-		else if (startup_info->have_repository) {
-			if (p->option & (RUN_SETUP_GENTLY | RUN_SETUP))
-				; /* done already */
-			else
-				unset_git_directory(startup_info->prefix);
+		else if (startup_info->have_run_setup_gitdir) {
+			if (startup_info->have_repository) {
+				if (p->option & (RUN_SETUP_GENTLY | RUN_SETUP))
+					; /* done already */
+				else
+					unset_git_directory(startup_info->prefix);
+			}
+			startup_info->have_run_setup_gitdir = 0;
 		}
 
 		if (use_pager == -1 && p->option & RUN_SETUP)
diff --git a/setup.c b/setup.c
index d9bb616..43b529e 100644
--- a/setup.c
+++ b/setup.c
@@ -358,7 +358,7 @@ void unset_git_directory(const char *prefix)
 	inside_git_dir = -1;
 
 	/* Initialized in check_repository_format_version() */
-	repository_format_version = 0xFF;
+	repository_format_version = 0;
 	shared_repository = PERM_UMASK;
 	is_bare_repository_cfg = -1;
 	git_work_tree_cfg = NULL;
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 5386504..f93856b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -33,6 +33,20 @@ test_expect_success 'plain' '
 	check_config plain/.git false unset
 '
 
+test_expect_success '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
-- 
1.7.0.rc1.541.g2da82.dirty

Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository

[Jonathan Nieder]

Hi again,

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

> While at it, also reset repository_format_version to zero. When
> omitted, the format should be understood as the last supported
> version, i.e. zero. This is probably only used by "git init" or "git
> clone".
[...]
>  	/* Initialized in check_repository_format_version() */
> -	repository_format_version = 0xFF;
> +	repository_format_version = 0;

Good change, but wrong justification in my opinion.

v0.99.9l^2~54 (init-db: check template and repository format.,
2005-11-25) taught ‘git init-db’ to make sure that

  repository_format_version <= GIT_REPO_VERSION

before initializing a new repository.  repository_format_version was
being explicitly initialized globally to 0 at the time, presumably to
ensure tests like this always succeed when no repository format version
is declared (or in other words, the repository format for repositories
initialized before git v0.99.9l^2~56 is zero by convention).  As a happy
side effect, that default takes care of the “no pre-existing repository”
case here.

The explicit initialization was removed in commit v1.4.3-rc1~230 (remove
unnecessary initializations, 2006-08-15), since according to ANSI C it
is redundant.

So I think the convention is not “if in doubt, the repository has
version GIT_REPO_VERSION” but “if the repository lacks a
core.repositoryversion setting, it must be really old”.

Jonathan

Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository

[Jonathan Nieder]

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

>  - 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 a note about this.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Makes sense, thanks.

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

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

> startup_info->have_run_setup_gitdir is used to guard unallowed access
> to repository. When a repository has been set up and the real command
> does not expect any setup, we should revert to the original "fresh"
> state, including startup_info->have_run_setup_gitdir. Otherwise, the
> next attempt to set up repository will fail.
[...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

The test this adds fails for current master, too.

The cause: core.worktree gets set to point to the trash directory.
This results from a facet of the test I was not paying attention to ---
the trash directory is a git repository itself, which poisons git with
some extra information it hadn’t learned to forget yet.

The third and fourth tests below fail in master for this reason.

The first test passes everywhere; it is just checking basic behavior.

The second test passes in master and fails in nd/setup without the
git.c hunk of your patch.  It is a more targeted test, meant to check
only for the problem your patch solves.

Of course, with your patch applied, all four tests pass.  Thanks.

Reviewed-and-tested-by: Jonathan Nieder <jrnieder@gmail.com>

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

Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository

[Nguyen Thai Ngoc Duy]

2010/3/27 Jonathan Nieder <jrnieder@gmail.com>:
> Hi again,
>
> Nguyễn Thái Ngọc Duy wrote:
>
>> While at it, also reset repository_format_version to zero. When
>> omitted, the format should be understood as the last supported
>> version, i.e. zero. This is probably only used by "git init" or "git
>> clone".
> [...]
>>       /* Initialized in check_repository_format_version() */
>> -     repository_format_version = 0xFF;
>> +     repository_format_version = 0;
>
> Good change, but wrong justification in my opinion.
>
> v0.99.9l^2~54 (init-db: check template and repository format.,
> 2005-11-25) taught ‘git init-db’ to make sure that
>
>  repository_format_version <= GIT_REPO_VERSION
>
> before initializing a new repository.  repository_format_version was
> being explicitly initialized globally to 0 at the time, presumably to
> ensure tests like this always succeed when no repository format version
> is declared (or in other words, the repository format for repositories
> initialized before git v0.99.9l^2~56 is zero by convention).  As a happy
> side effect, that default takes care of the “no pre-existing repository”
> case here.
>
> The explicit initialization was removed in commit v1.4.3-rc1~230 (remove
> unnecessary initializations, 2006-08-15), since according to ANSI C it
> is redundant.
>
> So I think the convention is not “if in doubt, the repository has
> version GIT_REPO_VERSION” but “if the repository lacks a
> core.repositoryversion setting, it must be really old”.

Makes sense. Thanks. Will leave a note at the initialization to reduce
history digging time for other people.
-- 
Duy