Re: nd/setup

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

Junio,

I wanted to update nd/setup topic, but found nd/root-git was merged in
between its two parts, which resulted in non-trivial conflicts in the
end. Changes are minor, mainly to clean up the topic. So it's up to
you to either leave the topic as is (plus Jonathan's "help revert"
patch on top) or replace with some patches I attach, because if I send
the whole topic again, you would need to resolve conflicts manually.
I'll make proper patch messages next time once I figure out how to
send conflict resolutions through mail.

- 717b885 help: use RUN_SETUP_GENTLY
  replaced by 0001 (needs to resolve conflict in the next patch in
topic, but it's quite simple)
- 450476a worktree setup: restore original state when things go wrong
  replaced by 0002
- bc7cd1c Guard unallowed access to repository when it's not set up
  replaced by 0003 and 0004
- a55fdda builtins: reset startup_info->have_run_setup_gitdir when
unsetting up repository
  dropped (splitted as part of 0002-0004)
--
Duy

[-- Attachment #2: 0001-help-take-note-why-this-command-is-not-applicable-fo.patch --]
[-- Type: application/octet-stream, Size: 2380 bytes --]

From 4c5d6bc49e262d3331069dd7d8fb8fc6785f5d4b Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 4 Apr 2010 17:49:38 +0200
Subject: [PATCH] help: take note why this command is not applicable for RUN_SETUP_GENTLY
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

"git help" calls setup_git_directory_gently(), thus a candidate for
RUN_SETUP_GENTLY. However RUN_SETUP_GENTLY should not be used for
performance reason, as follows.

In automount setups like that which prompted v1.6.0-rc0~121^2~1 (Add
support for GIT_CEILING_DIRECTORIES, 2008-05-19), if
GIT_CEILING_DIRECTORIES is unset, then probing for the Git directory
can take a long time.  Thus unnecessarily searching for a git
directory can slow down 'git help -a' (and thus bash completion).

'git help' does not use RUN_SETUP or USE_PAGER, and neither option
parsing nor producing output for plain 'git help' or 'git help -a'
requires access to the git configuration.  Therefore it is safe to not
search for the git directory early in this case.

Also add some comments to document the requirements this places on
list_commands() and list_common_cmds_help().

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/help.c |    5 +++++
 help.c         |    4 ++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 3182a2b..1626251 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -271,6 +271,11 @@ static int git_help_config(const char *var, const char *value, void *cb)
 
 static struct cmdnames main_cmds, other_cmds;
 
+/*
+ * Used for plain 'git' and 'git help'.
+ * Therefore this should not use git_config(), nor any other function
+ * that requires searching for a repository.
+ */
 void list_common_cmds_help(void)
 {
 	int i, longest = 0;
diff --git a/help.c b/help.c
index 7f4928e..d4c3165 100644
--- a/help.c
+++ b/help.c
@@ -221,6 +221,10 @@ void load_command_list(const char *prefix,
 	exclude_cmds(other_cmds, main_cmds);
 }
 
+/*
+ * Used for 'git help -a'.  Therefore this should not use git_config(),
+ * nor any other function that requires searching for a repository.
+ */
 void list_commands(const char *title, struct cmdnames *main_cmds,
 		   struct cmdnames *other_cmds)
 {
-- 
1.7.0.rc1.541.g2da82.dirty


[-- Attachment #3: 0002-worktree-setup-restore-original-state-when-things-go.patch --]
[-- Type: application/octet-stream, Size: 6002 bytes --]

From 092b20337ac1f1824a6656ba5249b409626ca52e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= <pclouds@gmail.com>
Date: Sun, 21 Mar 2010 17:30:34 +0700
Subject: [PATCH] worktree setup: restore original state when things go wrong
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In current state of setup_git_directory_gently(), when a repository is
not found, many things will not be the same as when setup procedure is
started (while they should be). For one thing, current working
directory will not be the same in certain cases.

This patch reorders actions in clear steps and rolls back if one step
is wrong. (Actually the last step, setting git_dir, can't be rolled
back by this patch).

The steps and their state change are:

 1. Looking in working directory for repository candidates (or $GIT_DIR
    if it is set)

    This step may move $(cwd) to somewhere.

 2. Set internal variables to record repository features, based only
    on repository location and environment variables.

    This step sets variables:
    - inside_git_dir
    - inside_work_tree

 3. Inspect $GIT_DIR/config to see if it's a valid repository.

    This step may change variables:
    - repository_format_version
    - shared_repository
    - is_bare_repository_cfg
    - git_work_tree_cfg

 4. Save the repository location for later use.

    This step calls setup_git_dir() to set many variables in environment.c

 5. Calculate prefix (relative path to the original current working
    directory)

Setup procedure is completed at step 5. Stopping at any other steps
is considered a setup failure.

In case of setup failure, if it's not fatal and nongit_ok is not NULL,
prefix must be calculated before returning to caller. In other words:

  if (!fatal && gently)
    goto step_5;

Things that go wrong before step 4 will be cleaned up by
unset_git_directory(). Step 4 is irreversible, for now.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h |    1 +
 setup.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 57cd79e..4debeaa 100644
--- a/cache.h
+++ b/cache.h
@@ -409,6 +409,7 @@ extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
+extern void unset_git_directory(const char *prefix);
 extern const char *prefix_path(const char *prefix, int len, const char *path);
 extern const char *prefix_filename(const char *prefix, int len, const char *path);
 extern int check_filename(const char *prefix, const char *name);
diff --git a/setup.c b/setup.c
index 19fe3e3..908cfff 100644
--- a/setup.c
+++ b/setup.c
@@ -322,6 +322,42 @@ const char *read_gitfile_gently(const char *path)
 	return path;
 }
 
+void unset_git_directory(const char *prefix)
+{
+	/*
+	 * FIXME:
+	 * chdir(prefix) may be enough for most of cases,
+	 * if original cwd is outside worktree, prefix
+	 * will always be set NULL, thus impossible to move
+	 * back to orignal cwd
+	 */
+	if (prefix && chdir(prefix))
+		die("Cannot change to '%s'", prefix);
+
+	if (startup_info) {
+		startup_info->prefix = NULL;
+		startup_info->have_repository = 0;
+	}
+
+	/* Initialized in setup_git_directory_gently_1() */
+	inside_work_tree = -1;
+	inside_git_dir = -1;
+
+	/* Initialized in check_repository_format_version() */
+	/*
+	 * repository_format_version is zero for two reasons:
+	 * - For really old repos, there won't be core.repositoryformatversion
+	 *   we treat it as supported version zero.
+	 * - For repo creating commands like "git init" or "git clone"
+	 *   $GIT_DIR/config may not be there, just let it pass, the
+	 *   repository will be properly initialized later.
+	 * For history of this setting see commits 4f62953 and 96f1e58
+	 */
+	repository_format_version = 0;
+	shared_repository = PERM_UMASK;
+	is_bare_repository_cfg = -1;
+	git_work_tree_cfg = NULL;
+}
 /*
  * 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.
@@ -402,6 +438,13 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	 * - ../ (bare)
 	 * - ../../.git/
 	 *   etc.
+	 *
+	 * When a repository is found:
+	 * - inside_git_dir/inside_work_tree are set
+	 * - check_repository_format_gently() is called
+	 *   if repo version is not supported, restore cwd
+	 * - set_git_dir
+	 * - calculate and return prefix
 	 */
 	offset = len = strlen(cwd);
 	for (;;) {
@@ -409,13 +452,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		if (!gitfile_dir && is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 			gitfile_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 		if (gitfile_dir) {
-			if (set_git_dir(gitfile_dir))
-				die("Repository setup failed");
 			inside_git_dir = 0;
 			if (!work_tree_env)
 				inside_work_tree = 1;
-			if (check_repository_format_gently(gitfile_dir, nongit_ok))
+			if (check_repository_format_gently(gitfile_dir, nongit_ok)) {
+				unset_git_directory(offset != len ? cwd + offset + 1: NULL);
 				return NULL;
+			}
+			if (set_git_dir(gitfile_dir))
+				die("Repository setup failed");
 			root_len = offset_1st_component(cwd);
 			git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
 			break;
@@ -424,8 +469,10 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			if (check_repository_format_gently(".", nongit_ok))
+			if (check_repository_format_gently(".", nongit_ok)) {
+				unset_git_directory(offset != len ? cwd + offset + 1: NULL);
 				return NULL;
+			}
 			if (offset != len) {
 				cwd[offset] = '\0';
 				set_git_dir(cwd);
-- 
1.7.0.rc1.541.g2da82.dirty


[-- Attachment #4: 0003-Guard-unallowed-access-to-repository-when-it-s-not-s.patch --]
[-- Type: application/octet-stream, Size: 5581 bytes --]

From 1c1e4af05262b57f5779e7f825e25945c8c21754 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= <pclouds@gmail.com>
Date: Sun, 21 Mar 2010 17:30:46 +0700
Subject: [PATCH] Guard unallowed access to repository when it's not set up
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Many code path will skip repo access if startup_info->have_repository
is false. This may be a fault if startup_info->have_repository has not
been properly initialized.

So the rule is one of the following commands must be run before any
repo access. And none of them can be called twice.

 - setup_git_directory*
 - enter_repo
 - init_db

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c |    1 +
 cache.h           |    1 +
 config.c          |    2 ++
 environment.c     |   13 +++++++++++--
 git.c             |   16 +++++++++++-----
 setup.c           |   13 +++++++++++++
 6 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 5b2113b..8242fb9 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -288,6 +288,7 @@ int init_db(const char *git_dir, const char *template_dir, unsigned int flags)
 
 	set_git_dir(make_absolute_path(git_dir));
 	startup_info->have_repository = 1;
+	startup_info->have_run_setup_gitdir = 1;
 
 	safe_create_dir(get_git_dir(), 0);
 
diff --git a/cache.h b/cache.h
index 1b55ad7..c118935 100644
--- a/cache.h
+++ b/cache.h
@@ -1046,6 +1046,7 @@ int split_cmdline(char *cmdline, const char ***argv);
 /* git.c */
 struct startup_info {
 	const char *prefix;
+	int have_run_setup_gitdir;
 	int have_repository;
 	int help;
 };
diff --git a/config.c b/config.c
index 07d854a..9981b09 100644
--- a/config.c
+++ b/config.c
@@ -737,6 +737,8 @@ int git_config(config_fn_t fn, void *data)
 	char *repo_config = NULL;
 	int ret;
 
+	if (startup_info && !startup_info->have_run_setup_gitdir)
+		die("internal error: access to .git/config without repo setup");
 	if (!startup_info || startup_info->have_repository)
 		repo_config = git_pathdup("config");
 	ret = git_config_early(fn, data, repo_config);
diff --git a/environment.c b/environment.c
index ead00a3..ca34a7b 100644
--- a/environment.c
+++ b/environment.c
@@ -81,9 +81,18 @@ void unset_git_env(void)
 
 static void setup_git_env(void)
 {
+	if (startup_info && startup_info->have_run_setup_gitdir)
+		die("internal error: setup_git_env can't be called twice");
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
-		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (!git_dir) {
+		/*
+		 * Repo detection should be done by setup_git_directory*
+		 * or enter_repo, not by this function
+		 */
+		 if (startup_info)
+			 die("internal error: $GIT_DIR is empty");
+		 git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+	}
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	git_object_dir = getenv(DB_ENVIRONMENT);
diff --git a/git.c b/git.c
index deba49a..21272d8 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)
@@ -257,6 +260,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			use_pager = 1;
 		}
 	}
+	else
+		/* Stop git_config() from complaining that no repository found. */
+		startup_info->have_run_setup_gitdir = 1;
 	commit_pager_choice();
 
 	if (!startup_info->help && p->option & NEED_WORK_TREE)
diff --git a/setup.c b/setup.c
index 7938481..3bb3a92 100644
--- a/setup.c
+++ b/setup.c
@@ -236,7 +236,17 @@ void setup_work_tree(void)
 		git_dir = make_absolute_path(git_dir);
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+
+	/*
+	 * have_run_setup_gitdir is unset in order to avoid die()ing
+	 * inside set_git_env(). We don't actually initialize
+	 * repo twice, we're just relative-izing gitdir
+	 */
+	if (startup_info)
+		startup_info->have_run_setup_gitdir = 0;
 	set_git_dir(make_relative_path(git_dir, work_tree));
+	if (startup_info)
+		startup_info->have_run_setup_gitdir = 1;
 	initialized = 1;
 }
 
@@ -339,6 +349,7 @@ void unset_git_directory(const char *prefix)
 			unset_git_env();
 		startup_info->prefix = NULL;
 		startup_info->have_repository = 0;
+		startup_info->have_run_setup_gitdir = 0;
 	}
 
 	/* Initialized in setup_git_directory_gently_1() */
@@ -513,6 +524,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	prefix = setup_git_directory_gently_1(nongit_ok);
 	if (startup_info) {
 		startup_info->prefix = prefix;
+		startup_info->have_run_setup_gitdir = 1;
 		startup_info->have_repository = !nongit_ok || !*nongit_ok;
 	}
 	return prefix;
@@ -607,6 +619,7 @@ char *enter_repo(char *path, int strict)
 		set_git_dir(".");
 		if (startup_info) {
 			startup_info->prefix = NULL;
+			startup_info->have_run_setup_gitdir = 1;
 			startup_info->have_repository = 1;
 		}
 		return path;
-- 
1.7.0.rc1.541.g2da82.dirty


[-- Attachment #5: 0004-t0001-Add-test-cases-for-git-init-with-aliases.patch --]
[-- Type: application/octet-stream, Size: 2297 bytes --]

From 7c50eebbefc1a1afe665a0315b1416b35c2021e0 Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 4 Apr 2010 18:16:53 +0200
Subject: [PATCH] t0001: Add test cases for "git init" with aliases
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t0001-init.sh |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 5386504..245c01a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -33,6 +33,58 @@ 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' '
+	(
+		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 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
-- 
1.7.0.rc1.541.g2da82.dirty