threads / patch / 23202

patchbuiltins: reset startup_info->have_run_setup_gitdir when unsetting up repository

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

## tl;dr

5 messages between Mar 27, 2010 and Mar 28, 2010. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Mar 27, 2010, 09:13 UTC · lore

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

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(-)
Show changes to git.c +7 −0
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
Nguyễn Thái Ngọc Duy· Mar 27, 2010, 09:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

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(-)
Show changes to 3 files +23 −6

git.c, setup.c, t/t0001-init.sh

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
Jonathan Nieder· Mar 27, 2010, 22:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Nguyen Thai Ngoc Duy· Mar 28, 2010, 07:11 UTC · re: Jonathan Nieder · lore

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

2010/3/27 Jonathan Nieder <jrnieder@gmail.com>:
Show 35 quoted lines
> 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
Jonathan Nieder· Mar 28, 2010, 00:34 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy wrote:
Show 12 quoted lines
>  - 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:
Show 5 quoted lines
> 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>
Show changes to t/t0001-init.sh +53 −0
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

← back to recent threads