[PATCH 01/10] "git read-tree -m" and the like require worktree

[PATCH 01/10] "git read-tree -m" and the like require worktree

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

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

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 0138f5a..a69bac4 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -220,6 +220,9 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	if ((opts.dir && !opts.update))
 		die("--exclude-per-directory is meaningless unless -u");
 
+	if (opts.merge && !opts.index_only)
+		setup_work_tree();
+
 	if (opts.prefix) {
 		int pfxlen = strlen(opts.prefix);
 		int pos;
-- 
1.5.4.2.281.g28d0e

[PATCH 02/10] Make sure setup_git_directory is called before accessing repository

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

Right now setup_git_env is called automatically when needed and
will not die out if $GIT_DIR is invalid. The next patch no longer
guarantee that, so make sure all commands have environment properly
setup before using.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 fast-import.c |    1 +
 hash-object.c |   10 +++++-----
 index-pack.c  |    2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 0d3449f..7f197d5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2377,6 +2377,7 @@ int main(int argc, const char **argv)
 {
 	unsigned int i, show_stats = 1;
 
+	setup_git_directory();
 	git_config(git_pack_config);
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
diff --git a/hash-object.c b/hash-object.c
index 61e7160..46d57ad 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -42,7 +42,10 @@ int main(int argc, char **argv)
 	int prefix_length = -1;
 	int no_more_flags = 0;
 	int hashstdin = 0;
+	int nongit = 0;
 
+	prefix = setup_git_directory_gently(&nongit);
+	prefix_length = prefix ? strlen(prefix) : 0;
 	git_config(git_default_config);
 
 	for (i = 1 ; i < argc; i++) {
@@ -53,11 +56,8 @@ int main(int argc, char **argv)
 				type = argv[i];
 			}
 			else if (!strcmp(argv[i], "-w")) {
-				if (prefix_length < 0) {
-					prefix = setup_git_directory();
-					prefix_length =
-						prefix ? strlen(prefix) : 0;
-				}
+				if (nongit)
+					die("Not a git repository");
 				write_object = 1;
 			}
 			else if (!strcmp(argv[i], "--")) {
diff --git a/index-pack.c b/index-pack.c
index 9c0c278..1dbe88d 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -785,7 +785,9 @@ int main(int argc, char **argv)
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
 	struct pack_idx_entry **idx_objects;
 	unsigned char sha1[20];
+	int nongit = 0;
 
+	setup_git_directory_gently(&nongit);
 	git_config(git_index_pack_config);
 
 	for (i = 1; i < argc; i++) {
-- 
1.5.4.2.281.g28d0e

[PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-rev-parse.c    |   14 +-------------
 environment.c          |    1 +
 t/t1300-repo-config.sh |    2 +-
 t/t1400-update-ref.sh  |    4 ++--
 t/t9300-fast-import.sh |    2 +-
 5 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 90dbb9d..13412b4 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -496,19 +496,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--git-dir")) {
-				const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
-				static char cwd[PATH_MAX];
-				if (gitdir) {
-					puts(gitdir);
-					continue;
-				}
-				if (!prefix) {
-					puts(".git");
-					continue;
-				}
-				if (!getcwd(cwd, PATH_MAX))
-					die("unable to get current working directory");
-				printf("%s/.git\n", cwd);
+				puts(get_git_dir());
 				continue;
 			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
diff --git a/environment.c b/environment.c
index 6739a3f..ab7e54c 100644
--- a/environment.c
+++ b/environment.c
@@ -51,6 +51,7 @@ static void setup_git_env(void)
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	git_dir = xstrdup(make_absolute_path(git_dir));
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {
 		git_object_dir = xmalloc(strlen(git_dir) + 9);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4928a57..45b26d5 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -497,7 +497,7 @@ test_expect_success numbers '
 '
 
 cat > expect <<EOF
-fatal: bad config value for 'aninvalid.unit' in .git/config
+fatal: bad config value for 'aninvalid.unit' in $(pwd)/.git/config
 EOF
 
 test_expect_success 'invalid unit' '
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 78cd412..4099c66 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -169,7 +169,7 @@ test_expect_success \
 	'rm -f o e
 	 git rev-parse --verify "master@{2005-05-26 23:33:01}" >o 2>e &&
 	 test '"$B"' = $(cat o) &&
-	 test "warning: Log .git/logs/'"$m has gap after $gd"'." = "$(cat e)"'
+	 test "warning: Log '"$(pwd)"'/.git/logs/'"$m has gap after $gd"'." = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-26 23:38:00}" (middle of history)' \
 	'rm -f o e
@@ -187,7 +187,7 @@ test_expect_success \
 	'rm -f o e
 	 git rev-parse --verify "master@{2005-05-28}" >o 2>e &&
 	 test '"$D"' = $(cat o) &&
-	 test "warning: Log .git/logs/'"$m unexpectedly ended on $ld"'." = "$(cat e)"'
+	 test "warning: Log '"$(pwd)"'/.git/logs/'"$m unexpectedly ended on $ld"'." = "$(cat e)"'
 
 
 rm -f .git/$m .git/logs/$m expect
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index cceedbb..98a0cdf 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -510,7 +510,7 @@ test_expect_success \
     'git-fast-import --export-pack-edges=edges.list <input'
 
 cat >expect <<EOF
-.git/objects/pack/pack-.pack: `git rev-parse --verify export-boundary`
+$(pwd)/.git/objects/pack/pack-.pack: `git rev-parse --verify export-boundary`
 EOF
 test_expect_success \
 	'I: verify edge list' \
-- 
1.5.4.2.281.g28d0e

[PATCH 04/10] Make setup_work_tree() return new prefix

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

worktree setup inside setup_git_directory*() is not perfect. It does
not take core.worktree into account. So when you do setup_work_tree(),
the real work tree may be not the one setup_git_directory*() gives you.
You need a new prefix in that case.

This also effectively reverts dd5c8af (Fix
setup_git_directory_gently() with relative GIT_DIR & GIT_WORK_TREE).
The problem is IMHO that git-diff does not setup worktree when it
needs it, so setting up worktree from setup_git_directory_gently() is
a fix in a wrong place.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-blame.c      |    4 +-
 builtin-diff-files.c |    4 ++-
 builtin-diff.c       |    4 ++-
 builtin-ls-files.c   |    8 ++--
 builtin-read-tree.c  |    2 +-
 builtin-rev-parse.c  |    2 +
 builtin-rm.c         |    2 +-
 cache.h              |    2 +-
 git.c                |    2 +-
 setup.c              |   84 ++++++++++++++++----------------------------------
 10 files changed, 45 insertions(+), 69 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index bfd562d..f5a878b 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2291,6 +2291,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			/* (3) */
 			if (argc <= i)
 				usage(blame_usage);
+			prefix = setup_work_tree(prefix);
 			path = add_prefix(prefix, argv[i]);
 			if (i + 1 == argc - 1) {
 				final_commit_name = argv[i + 1];
@@ -2306,7 +2307,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			else if (i != argc - 1)
 				usage(blame_usage); /* garbage at end */
 
-			setup_work_tree();
 			if (!has_path_in_work_tree(path))
 				die("cannot stat path %s: %s",
 				    path, strerror(errno));
@@ -2354,7 +2354,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 * do not default to HEAD, but use the working tree
 		 * or "--contents".
 		 */
-		setup_work_tree();
+		setup_work_tree(prefix);
 		sb.final = fake_working_tree_commit(path, contents_from);
 		add_pending_object(&revs, &(sb.final->object), ":");
 	}
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 4abe3c2..11671a8 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -26,8 +26,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
 		argc = 0;
-	else
+	else {
+		rev.prefix = setup_work_tree(prefix);
 		argc = setup_revisions(argc, argv, &rev, NULL);
+	}
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	result = run_diff_files_cmd(&rev, argc, argv);
diff --git a/builtin-diff.c b/builtin-diff.c
index 444ff2f..254e5a0 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -244,8 +244,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
 		argc = 0;
-	else
+	else {
+		rev.prefix = setup_work_tree(prefix);
 		argc = setup_revisions(argc, argv, &rev, NULL);
+	}
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 		if (diff_setup_done(&rev.diffopt) < 0)
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 25dbfb4..cf4c492 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -23,7 +23,7 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 
 static int prefix_len;
-static int prefix_offset;
+static int prefix_offset = -1;
 static const char **pathspec;
 static int error_unmatch;
 static char *ps_matched;
@@ -435,8 +435,6 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	struct dir_struct dir;
 
 	memset(&dir, 0, sizeof(dir));
-	if (prefix)
-		prefix_offset = strlen(prefix);
 	git_config(git_default_config);
 
 	for (i = 1; i < argc; i++) {
@@ -569,7 +567,9 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	}
 
 	if (require_work_tree && !is_inside_work_tree())
-		setup_work_tree();
+		prefix = setup_work_tree(prefix);
+	if (prefix_offset == -1)
+		prefix_offset = prefix ? strlen(prefix) : 0;
 
 	pathspec = get_pathspec(prefix, argv + i);
 
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index a69bac4..851f24d 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -221,7 +221,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		die("--exclude-per-directory is meaningless unless -u");
 
 	if (opts.merge && !opts.index_only)
-		setup_work_tree();
+		setup_work_tree(NULL);
 
 	if (opts.prefix) {
 		int pfxlen = strlen(opts.prefix);
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 13412b4..3d3a309 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -472,6 +472,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--show-prefix")) {
+				if (is_inside_work_tree())
+					prefix = setup_work_tree(prefix);
 				if (prefix)
 					puts(prefix);
 				continue;
diff --git a/builtin-rm.c b/builtin-rm.c
index c0a8bb6..2c15d66 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -156,7 +156,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_rm_usage, builtin_rm_options);
 
 	if (!index_only)
-		setup_work_tree();
+		prefix = setup_work_tree(prefix);
 
 	pathspec = get_pathspec(prefix, argv);
 	seen = NULL;
diff --git a/cache.h b/cache.h
index bdefcc9..b98f7e8 100644
--- a/cache.h
+++ b/cache.h
@@ -315,7 +315,7 @@ extern const char *get_git_work_tree(void);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
-extern void setup_work_tree(void);
+extern const char *setup_work_tree(const char *prefix);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern const char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/git.c b/git.c
index 1e3eb10..9e53a2e 100644
--- a/git.c
+++ b/git.c
@@ -241,7 +241,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	if (p->option & USE_PAGER)
 		setup_pager();
 	if (p->option & NEED_WORK_TREE)
-		setup_work_tree();
+		prefix = setup_work_tree(prefix);
 
 	trace_argv_printf(argv, "trace: built-in: git");
 
diff --git a/setup.c b/setup.c
index 89c81e5..6c908a5 100644
--- a/setup.c
+++ b/setup.c
@@ -262,38 +262,23 @@ int is_inside_work_tree(void)
 	return inside_work_tree;
 }
 
-/*
- * set_work_tree() is only ever called if you set GIT_DIR explicitely.
- * The old behaviour (which we retain here) is to set the work tree root
- * to the cwd, unless overridden by the config, the command line, or
- * GIT_WORK_TREE.
- */
-static const char *set_work_tree(const char *dir)
-{
-	char buffer[PATH_MAX + 1];
-
-	if (!getcwd(buffer, sizeof(buffer)))
-		die ("Could not get the current working directory");
-	git_work_tree_cfg = xstrdup(buffer);
-	inside_work_tree = 1;
-
-	return NULL;
-}
-
-void setup_work_tree(void)
+const char *setup_work_tree(const char *prefix)
 {
-	const char *work_tree, *git_dir;
-	static int initialized = 0;
+	const char *work_tree;
+	static char buffer[PATH_MAX + 1];
+	char *rel;
 
-	if (initialized)
-		return;
+	get_git_dir(); /* ensure git_dir has been initialized as cwd will change */
 	work_tree = get_git_work_tree();
-	git_dir = get_git_dir();
-	if (!is_absolute_path(git_dir))
-		set_git_dir(make_absolute_path(git_dir));
-	if (!work_tree || chdir(work_tree))
+	if (!work_tree)
 		die("This operation must be run in a work tree");
-	initialized = 1;
+	if (prefix && chdir(prefix))
+		die ("Could not jump back into original cwd");
+	rel = get_relative_cwd(buffer, PATH_MAX, work_tree);
+	trace_printf("test: worktree = %s\n", rel ? rel : "NULL");
+	if (chdir(work_tree))
+		die("This operation must be run in a work tree");
+	return rel && *rel ? strcat(rel, "/") : NULL;
 }
 
 static int check_repository_format_gently(int *nongit_ok)
@@ -336,24 +321,21 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			static char buffer[1024 + 1];
 			const char *retval;
 
+			/*
+			 * The old behaviour (which we retain here) is to set
+			 * the work tree root to the cwd, unless overridden by
+			 * the config, the command line, or GIT_WORK_TREE.
+			 */
 			if (!work_tree_env) {
-				retval = set_work_tree(gitdirenv);
-				/* config may override worktree */
-				if (check_repository_format_gently(nongit_ok))
-					return NULL;
-				return retval;
+				char buffer[PATH_MAX + 1];
+
+				if (!getcwd(buffer, sizeof(buffer)))
+					die ("Could not get the current working directory");
+				git_work_tree_cfg = xstrdup(buffer);
+				inside_work_tree = 1;
 			}
-			if (check_repository_format_gently(nongit_ok))
-				return NULL;
-			retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
-					get_git_work_tree());
-			if (!retval || !*retval)
-				return NULL;
-			set_git_dir(make_absolute_path(gitdirenv));
-			if (chdir(work_tree_env) < 0)
-				die ("Could not chdir to %s", work_tree_env);
-			strcat(buffer, "/");
-			return retval;
+			check_repository_format_gently(nongit_ok);
+			return NULL;
 		}
 		if (nongit_ok) {
 			*nongit_ok = 1;
@@ -462,17 +444,5 @@ int check_repository_format(void)
 
 const char *setup_git_directory(void)
 {
-	const char *retval = setup_git_directory_gently(NULL);
-
-	/* If the work tree is not the default one, recompute prefix */
-	if (inside_work_tree < 0) {
-		static char buffer[PATH_MAX + 1];
-		char *rel;
-		if (retval && chdir(retval))
-			die ("Could not jump back into original cwd");
-		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
-		return rel && *rel ? strcat(rel, "/") : NULL;
-	}

[PATCH 05/10] http-push: Avoid calling setup_git_directory() twice

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

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

diff --git a/http-push.c b/http-push.c
index 5b23038..c4b28f2 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2175,7 +2175,7 @@ int main(int argc, char **argv)
 	struct ref *ref;
 	char *rewritten_url = NULL;
 
-	setup_git_directory();
+	const char *prefix = setup_git_directory();
 
 	remote = xcalloc(sizeof(*remote), 1);
 
@@ -2382,7 +2382,7 @@ int main(int argc, char **argv)
 			commit_argv[3] = old_sha1_hex;
 			commit_argc++;
 		}
-		init_revisions(&revs, setup_git_directory());
+		init_revisions(&revs, prefix);
 		setup_revisions(commit_argc, commit_argv, &revs, NULL);
 		free(new_sha1_hex);
 		if (old_sha1_hex) {
-- 
1.5.4.2.281.g28d0e

[PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()

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

This was impossible earlier because git_dir can be relative. Now that
git_dir is absolute, I see no reason for worktree setup inside
setup_git_directory_gently(). The semantic is now clearer: if you need
worktree, call setup_work_tree yourself (well, I will clean up
setup_git_directory() part later)

This patch will free some commands from prefix handling if they don't
ever need worktree.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-apply.c          |    7 +++++--
 builtin-bundle.c         |    9 ++-------
 builtin-config.c         |   15 +++++----------
 builtin-diff-files.c     |   11 ++++++-----
 builtin-diff.c           |   11 ++++++-----
 builtin-upload-archive.c |    4 ++--
 cache.h                  |    2 +-
 git.c                    |    6 +-----
 hash-object.c            |    8 +-------
 setup.c                  |   34 +++++++++++++++++-----------------
 10 files changed, 46 insertions(+), 61 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index a3f075d..b77e84e 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2998,8 +2998,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 
 	const char *whitespace_option = NULL;
 
-	prefix = setup_git_directory_gently(&is_not_gitdir);
-	prefix_length = prefix ? strlen(prefix) : 0;
+	setup_git_directory_gently(&is_not_gitdir);
+	if (!is_not_gitdir && is_inside_work_tree()) {
+		prefix = setup_work_tree(NULL);
+		prefix_length = prefix ? strlen(prefix) : 0;
+	}
 	git_config(git_apply_config);
 	if (apply_default_whitespace)
 		parse_whitespace_option(apply_default_whitespace);
diff --git a/builtin-bundle.c b/builtin-bundle.c
index 9f38e21..2a7687e 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -11,7 +11,7 @@
 
 static const char *bundle_usage="git-bundle (create <bundle> <git-rev-list args> | verify <bundle> | list-heads <bundle> [refname]... | unbundle <bundle> [refname]... )";
 
-int cmd_bundle(int argc, const char **argv, const char *prefix)
+int cmd_bundle(int argc, const char **argv, const char *unused_prefix)
 {
 	struct bundle_header header;
 	int nongit = 0;
@@ -27,12 +27,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	argc -= 2;
 	argv += 2;
 
-	prefix = setup_git_directory_gently(&nongit);
-	if (prefix && bundle_file[0] != '/') {
-		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-		bundle_file = buffer;
-	}
-
+	setup_git_directory_gently(&nongit);
 	memset(&header, 0, sizeof(header));
 	if (strcmp(cmd, "create") && (bundle_fd =
 				read_bundle_header(bundle_file, &header)) < 0)
diff --git a/builtin-config.c b/builtin-config.c
index 2b9a426..ae6be9b 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -262,11 +262,11 @@ static int get_colorbool(int argc, const char **argv)
 	}
 }
 
-int cmd_config(int argc, const char **argv, const char *prefix)
+int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit = 0;
 	char* value;
-	const char *file = setup_git_directory_gently(&nongit);
+	setup_git_directory_gently(&nongit);
 
 	while (1 < argc) {
 		if (!strcmp(argv[1], "--int"))
@@ -276,8 +276,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
 			if (argc != 2)
 				usage(git_config_set_usage);
-			if (git_config(show_all_config) < 0 && file && errno)
-				die("unable to read config file %s: %s", file,
+			if (git_config(show_all_config) < 0 && errno)
+				die("unable to read config file: %s",
 				    strerror(errno));
 			return 0;
 		}
@@ -296,12 +296,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) {
 			if (argc < 3)
 				usage(git_config_set_usage);
-			if (!is_absolute_path(argv[2]) && file)
-				file = prefix_filename(file, strlen(file),
-						       argv[2]);
-			else
-				file = argv[2];
-			setenv(CONFIG_ENVIRONMENT, file, 1);
+			setenv(CONFIG_ENVIRONMENT, argv[2], 1);
 			argc--;
 			argv++;
 		}
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 11671a8..826f052 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -13,21 +13,22 @@ static const char diff_files_usage[] =
 "git-diff-files [-q] [-0/-1/2/3 |-c|--cc|--no-index] [<common diff options>] [<path>...]"
 COMMON_DIFF_OPTIONS_HELP;
 
-int cmd_diff_files(int argc, const char **argv, const char *prefix)
+int cmd_diff_files(int argc, const char **argv, const char *unused_prefix)
 {
 	struct rev_info rev;
 	int nongit = 0;
 	int result;
 
-	prefix = setup_git_directory_gently(&nongit);
-	init_revisions(&rev, prefix);
+	setup_git_directory_gently(&nongit);
+	init_revisions(&rev, NULL);
 	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
-	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
+	if (!setup_diff_no_index(&rev, argc, argv, nongit, NULL))
 		argc = 0;
 	else {
-		rev.prefix = setup_work_tree(prefix);
+		init_revisions(&rev, setup_work_tree(NULL));
+		rev.abbrev = 0;
 		argc = setup_revisions(argc, argv, &rev, NULL);
 	}
 	if (!rev.diffopt.output_format)
diff --git a/builtin-diff.c b/builtin-diff.c
index 254e5a0..a92ab0e 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -202,7 +202,7 @@ static void refresh_index_quietly(void)
 	rollback_lock_file(lock_file);
 }
 
-int cmd_diff(int argc, const char **argv, const char *prefix)
+int cmd_diff(int argc, const char **argv, const char *unused_prefix)
 {
 	int i;
 	struct rev_info rev;
@@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	 * Other cases are errors.
 	 */
 
-	prefix = setup_git_directory_gently(&nongit);
+	setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config);
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, NULL);
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
-	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
+	if (!setup_diff_no_index(&rev, argc, argv, nongit, NULL))
 		argc = 0;
 	else {
-		rev.prefix = setup_work_tree(prefix);
+		init_revisions(&rev, setup_work_tree(NULL));
+		rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 		argc = setup_revisions(argc, argv, &rev, NULL);
 	}
 	if (!rev.diffopt.output_format) {
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 48ae09e..4f7fa35 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -17,7 +17,7 @@ static const char lostchild[] =
 "git-upload-archive: archiver process was lost";
 
 
-static int run_upload_archive(int argc, const char **argv, const char *prefix)
+static int run_upload_archive(int argc, const char **argv, const char *unused_prefix)
 {
 	struct archiver ar;
 	const char *sent_argv[MAX_ARGS];
@@ -67,7 +67,7 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 	/* parse all options sent by the client */
 	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar);
 
-	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, prefix);
+	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, NULL);
 	parse_pathspec_arg(sent_argv + treeish_idx + 1, &ar.args);
 
 	return ar.write_archive(&ar.args);
diff --git a/cache.h b/cache.h
index b98f7e8..33dcfb1 100644
--- a/cache.h
+++ b/cache.h
@@ -316,7 +316,7 @@ extern const char *get_git_work_tree(void);
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern const char *setup_work_tree(const char *prefix);
-extern const char *setup_git_directory_gently(int *);
+extern void setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 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);
diff --git a/git.c b/git.c
index 9e53a2e..d87c33e 100644
--- a/git.c
+++ b/git.c
@@ -143,13 +143,12 @@ static int split_cmdline(char *cmdline, const char ***argv)
 static int handle_alias(int *argcp, const char ***argv)
 {
 	int nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;
-	const char *subdir;
 	int count, option_count;
 	const char** new_argv;
 	const char *alias_command;
 	char *alias_string;
 
-	subdir = setup_git_directory_gently(&nongit);
+	setup_git_directory_gently(&nongit);
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
@@ -205,9 +204,6 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	if (subdir)
-		chdir(subdir);
-
 	errno = saved_errno;
 
 	return ret;
diff --git a/hash-object.c b/hash-object.c
index 46d57ad..5986701 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -38,14 +38,11 @@ int main(int argc, char **argv)
 	int i;
 	const char *type = blob_type;
 	int write_object = 0;
-	const char *prefix = NULL;
-	int prefix_length = -1;
 	int no_more_flags = 0;
 	int hashstdin = 0;
 	int nongit = 0;
 
-	prefix = setup_git_directory_gently(&nongit);
-	prefix_length = prefix ? strlen(prefix) : 0;
+	setup_git_directory_gently(&nongit);
 	git_config(git_default_config);
 
 	for (i = 1 ; i < argc; i++) {
@@ -80,9 +77,6 @@ int main(int argc, char **argv)
 				hash_stdin(type, write_object);
 				hashstdin = 0;
 			}
-			if (0 <= prefix_length)
-				arg = prefix_filename(prefix, prefix_length,
-						      arg);
 			hash_object(arg, type_from_string(type), write_object);
 			no_more_flags = 1;
 		}
diff --git a/setup.c b/setup.c
index 6c908a5..78ae2f9 100644
--- a/setup.c
+++ b/setup.c
@@ -301,7 +301,7 @@ static int check_repository_format_gently(int *nongit_ok)
  * 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.
  */
-const char *setup_git_directory_gently(int *nongit_ok)
+void setup_git_directory_gently(int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
 	static char cwd[PATH_MAX+1];
@@ -335,11 +335,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				inside_work_tree = 1;
 			}
 			check_repository_format_gently(nongit_ok);
-			return NULL;
+			return;
 		}
 		if (nongit_ok) {
 			*nongit_ok = 1;
-			return NULL;
+			return;
 		}
 		die("Not a git repository: '%s'", gitdirenv);
 	}
@@ -364,9 +364,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			if (chdir(cwd))
+				die("Cannot come back to cwd");
+			cwd[offset] = '\0';
+			setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
 			check_repository_format_gently(nongit_ok);
-			return NULL;
+			return;
 		}
 		chdir("..");
 		do {
@@ -375,27 +378,23 @@ const char *setup_git_directory_gently(int *nongit_ok)
 					if (chdir(cwd))
 						die("Cannot come back to cwd");
 					*nongit_ok = 1;
-					return NULL;
+					return;
 				}
 				die("Not a git repository");
 			}
 		} while (cwd[--offset] != '/');
 	}
 
+	if (chdir(cwd))
+		die("Cannot come back to cwd");
 	inside_git_dir = 0;
 	if (!work_tree_env)
 		inside_work_tree = 1;
 	git_work_tree_cfg = xstrndup(cwd, offset);
-	if (check_repository_format_gently(nongit_ok))
-		return NULL;
-	if (offset == len)
-		return NULL;

[PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"

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

cmd_archive() is registered without RUN_SETUP so its prefix
will be NULL forever. Let's make that clear.

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

diff --git a/builtin-archive.c b/builtin-archive.c
index c2e0c1e..84405df 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -236,11 +236,12 @@ static const char *extract_remote_arg(int *ac, const char **av)
 	return remote;
 }
 
-int cmd_archive(int argc, const char **argv, const char *prefix)
+int cmd_archive(int argc, const char **argv, const char *unused_prefix)
 {
 	struct archiver ar;
 	int tree_idx;
 	const char *remote = NULL;
+	const char *prefix;
 
 	remote = extract_remote_arg(&argc, argv);
 	if (remote)
@@ -250,9 +251,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	memset(&ar, 0, sizeof(ar));
 	tree_idx = parse_archive_args(argc, argv, &ar);
-	if (prefix == NULL)
-		prefix = setup_git_directory();
 
+	prefix = setup_git_directory();
 	argv += tree_idx;
 	parse_treeish_arg(argv, &ar.args, prefix);
 	parse_pathspec_arg(argv + 1, &ar.args);
-- 
1.5.4.2.281.g28d0e

[PATCH 08/10] Make setup_git_directory() auto-setup worktree if found

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

The semantic is simpler: if worktree is found, use it or die.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-ls-files.c  |    4 ++--
 builtin-rev-parse.c |    7 +++++--
 builtin-rm.c        |    5 ++---
 setup.c             |    2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index cf4c492..a53881d 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -566,8 +566,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (require_work_tree && !is_inside_work_tree())
-		prefix = setup_work_tree(prefix);
+	if (require_work_tree && !get_git_work_tree())
+		die("This operation must be run in a work tree");
 	if (prefix_offset == -1)
 		prefix_offset = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 3d3a309..5016890 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -359,15 +359,16 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-int cmd_rev_parse(int argc, const char **argv, const char *prefix)
+int cmd_rev_parse(int argc, const char **argv, const char *unused_prefix)
 {
 	int i, as_is = 0, verify = 0;
 	unsigned char sha1[20];
+	const char *prefix = NULL;
 
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
 
-	prefix = setup_git_directory();
+	setup_git_directory_gently(NULL);
 	git_config(git_default_config);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -487,6 +488,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 						printf("%s\n", work_tree);
 					continue;
 				}
+				else
+					pfx = prefix = setup_work_tree(prefix);
 				while (pfx) {
 					pfx = strchr(pfx, '/');
 					if (pfx) {
diff --git a/builtin-rm.c b/builtin-rm.c
index 2c15d66..dedef9f 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -155,10 +155,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (!argc)
 		usage_with_options(builtin_rm_usage, builtin_rm_options);
 
-	if (!index_only)
-		prefix = setup_work_tree(prefix);

[PATCH 09/10] Documentation: update api-builtin and api-setup

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/api-builtin.txt |   10 ++++
 Documentation/technical/api-setup.txt   |   91 ++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
index 52cdb4c..f0d988b 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -33,6 +33,10 @@ git:
 	If the standard output is connected to a tty, spawn a pager and
 	feed our output to it.
 
+`NEED_WORK_TREE`::
+
+	Make sure there is a work tree to work on.
+
 . Add `builtin-foo.o` to `BUILTIN_OBJS` in `Makefile`.
 
 Additionally, if `foo` is a new command, there are 3 more things to do:
@@ -59,5 +63,11 @@ to the subdirectory the command started from.  This allows you to
 convert a user-supplied pathname (typically relative to that directory)
 to a pathname relative to the top of the work tree.
 
+`NEED_WORK_TREE` also set `prefix` like `RUN_SETUP`. But it will `die()`
+if there is no work tree.
+
+If neither `NEED_WORK_TREE` nor `RUN_SETUP` is set, `prefix` is always `NULL`.
+No chdir(2) will be done.
+
 The return value from `cmd_foo()` becomes the exit status of the
 command.
diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
index 4f63a04..6f2defa 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -1,13 +1,88 @@
 setup API
 =========
 
-Talk about
+End-user point-of-view how the setup works
+------------------------------------------
 
-* setup_git_directory()
-* setup_git_directory_gently()
-* is_inside_git_dir()
-* is_inside_work_tree()
-* setup_work_tree()
-* get_pathspec()
+. If you have `GIT_DIR` exported, then no discovery is attempted.
+  We use the `GIT_DIR` you set it, and the repository lives
+  there.  `$GIT_DIR/config` is the repository config.
 
-(Dscho)
+. Otherwise we do the usual discovery going up to find the
+  repository.
+
+. If you have `GIT_WORK_TREE` exported, or otherwise if the
+  config has `core.worktree`, that's where your worktree is.
+  If these variables point to an invalid place, you have no worktree.
+
+. Otherwise, if you have `GIT_DIR` exported, you do not have a
+  worktree.  Else one level above your `$GIT_DIR` is the toplevel
+  of your worktree.
+
+Repository setup
+----------------
+
+At startup:
+
+. If the command always need a repository, call
+  `setup_git_directory()`. It will ensure you have a valid
+  repository. It will `die()` otherwise. If a worktree is detected,
+  it will be setup automatically. Note that `setup_git_directory()`
+  can only be called once.
+
+. If the command can optionally run in a repository, use
+  `setup_git_directory_gently(&nongit_ok)`,which is similar
+  to `setup_git_directory()` except it won't `die()`
+  but sets `nongit_ok` to true if run outside a repository.
+  No chdir(2) is done.
+
+. If you don't want worktree setup at all, but always need a git repository,
+  you can use `setup_git_directory_gently(NULL)`.
+
+Do not access git repository (even indirectly like `git_config()`) before
+calling one of these functions. Otherwise you may encounter `die()` if git
+fails to automatically find/setup a repository.
+
+Working directory setup
+-----------------------
+
+If `setup_git_directory()` is used, worktree can be optionally setup already.
+To check if work tree has been setup, use `get_git_work_tree()`. The return
+value is `NULL` if no work tree is setup or work tree directory otherwise.
+
+If you need a working directory, use `setup_work_tree()`. It will
+move current directory to top-level working directory and return
+a prefix. It will `die()` if unable to setup working directory.
+
+Miscellanous functions
+----------------------
+
+To know where `$GIT_DIR` is, use `get_git_dir()`. It will always return
+an absolute path. To know where `$GIT_WORK_TREE` is, use
+`get_git_work_tree()`. To check if you are inside a worktree or a git
+repository, use `is_inside_work_tree()` or `is_inside_git_dir()` respectively.
+There functions may be not valid until `setup_git_directory*()` is called.
+
+When working with pathspecs and prefix, you can use `get_pathspec()`
+to auto prepend a given prefix to pathspecs. Other helpful functions
+are `prefix_path()`, `prefix_filename()`
+
+Shell-based setup
+-----------------
+
+At startup, you need to `source git-sh-setup`. If your command does not always
+require a repository, set variable `NONGIT_OK` before sourcing git-sh-setup.
+
+If the command requires worktree, call `require_work_tree` (after
+git-sh-setup).
+
+Two variables `USAGE` and `LONG_USAGE` can be optionally set before sourcing
+git-sh-setup. They will be printed as help usage.
+
+To check if a repository is a bare repository, `is_bare_repository` can be
+used.
+
+There are several `git rev-parse` options to help shell scripts initialize
+their environment. Those are `--show-prefix`, `--show-cdup`, `--git-dir`,
+`--is-inside-git-dir`, `--is-inside-work-tree`, `--is-bare-repository` and
+`--parseopt`. Please refer to `git rev-parse` man page for more information.
-- 
1.5.4.2.281.g28d0e

[PATCH 10/10] Additional tests to capture worktree special cases

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

Most of them are for setup_git_directory_gently() commands
as those are likely to break.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1501-worktree.sh |   57 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 7ee3820..a53c1f5 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -47,7 +47,19 @@ export GIT_CONFIG="$(pwd)"/$GIT_DIR/config
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
-say "core.worktree = absolute path"
+say "core.worktree = absolute path with GIT_DIR unset"
+mkdir -p $(pwd)/repo.git/work/sub/dir || exit 1
+unset GIT_DIR
+export GIT_CONFIG=$(pwd)/repo.git/config
+git config core.worktree "$(pwd)/repo.git/work"
+test_rev_parse 'outside'      false false false
+cd repo.git/work || exit 1
+test_rev_parse 'inside'       false true true ''
+cd sub/dir || exit 1
+test_rev_parse 'subdirectory' false true true sub/dir/
+cd ../../../.. || exit 1
+
+say "core.worktree = absolute path with GIT_DIR set"
 export GIT_DIR=$(pwd)/repo.git
 export GIT_CONFIG=$GIT_DIR/config
 git config core.worktree "$(pwd)/work"
@@ -58,6 +70,47 @@ cd sub/dir || exit 1
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+test_expect_success '"git ls-files -o" gets correct prefix' '
+	(cd work/sub && touch untracked &&
+	test "$(git ls-files -o)" = untracked)'
+
+rm work/sub/untracked || exit 1
+
+cat <<EOF >expected
+:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	sub/tracked
+EOF
+
+cat <<EOF >expected2
+:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	tracked
+EOF
+
+test_expect_success '"git diff-files" gets correct prefix' '
+	(cd work/sub  && touch tracked &&
+	git add tracked && echo modified > tracked &&
+	git diff-files > ../../result &&
+	git diff-files --relative > ../../result2 &&
+	git rm -f tracked) &&
+	cmp result expected &&
+	cmp result2 expected2'
+
+cat <<EOF >expected
+:100644 100644 e69de29... 0000000... M	sub/tracked
+EOF
+
+cat <<EOF >expected2
+:100644 100644 e69de29... 0000000... M	tracked
+EOF
+
+
+test_expect_success '"git diff" gets correct prefix' '
+	(cd work/sub  && touch tracked &&
+	git add tracked && echo modified > tracked &&
+	git diff --raw > ../../result &&
+	git diff --raw --relative > ../../result2 &&
+	git rm -f tracked) &&
+	cmp result expected &&
+	cmp result2 expected2'
+
 say "GIT_WORK_TREE=relative path (override core.worktree)"
 export GIT_DIR=$(pwd)/repo.git
 export GIT_CONFIG=$GIT_DIR/config
@@ -103,7 +156,7 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")
 '
 
-test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
+test_expect_success '"git diff" setup worktree properly' '
 	cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
 		git diff --exit-code tracked &&
-- 
1.5.4.2.281.g28d0e

Re: [PATCH 01/10] "git read-tree -m" and the like require worktree

[Johannes Schindelin]

Hi,

How about

	git read-tree -m without -i requires work tree

Hmm?

Other than that, it looks like an obvious bugfix which is independent of 
the rest.

Ciao,
Dscho

Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 231 bytes --]

Hi,

On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

I do not like this change.  It is IMO completely unnecessary, and might 
break assumptions.

Ciao,
Dscho

Re: [PATCH 04/10] Make setup_work_tree() return new prefix

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 393 bytes --]

Hi,

On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:

> worktree setup inside setup_git_directory*() is not perfect. It does not 
> take core.worktree into account. So when you do setup_work_tree(), the 
> real work tree may be not the one setup_git_directory*() gives you. You 
> need a new prefix in that case.

Probably the real fix would be to setup_git_directory(), no?

Ciao,
Dscho

Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 201 bytes --]

Hi,

On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This fix is completely independent of the rest of your series.

Ciao,
Dscho

Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 809 bytes --]

Hi,

On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:

> This was impossible earlier because git_dir can be relative. Now that
> git_dir is absolute, I see no reason for worktree setup inside
> setup_git_directory_gently(). The semantic is now clearer: if you need
> worktree, call setup_work_tree yourself (well, I will clean up
> setup_git_directory() part later)

As I said earlier, the work for getting the prefix as most likely be done 
already in the search for .git/.  I mean, it _is_ the common case to have 
a working tree with a .git/ in it, and that's it.

So I am quite certain that it is not worth the complicated and intrusive 
patch to separate the logic.

Particularly since working tree has a bad reputation already, as Junio 
pointed out: whenever we touch it, we get burnt.

Ciao,
Dscho

Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 350 bytes --]

Hi,

On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:

> cmd_archive() is registered without RUN_SETUP so its prefix
> will be NULL forever. Let's make that clear.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

AFAICT this patch is -- like two others -- an independent fix, and should 
be submitted as such.

Ciao,
Dscho

Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:33 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  > cmd_archive() is registered without RUN_SETUP so its prefix
>  > will be NULL forever. Let's make that clear.
>  >
>  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>  > ---
>
>  AFAICT this patch is -- like two others -- an independent fix, and should
>  be submitted as such.

It's not really independent. Before the series, setup_dir_gently()
_can_ change the prefix. After the series it does not (semantics
change). The first patch of the series ("git read-tree -m") is
independent and can be cherry picked if necessary.
-- 
Duy

Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:28 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  >
>  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
>  This fix is completely independent of the rest of your series.

Not really. It would have no impact (good or bad) if sent separately.
But it is required by the series (otherwise it would die() under
certain condition because prefix cannot be recomputed properly).
-- 
Duy

Re: [PATCH 01/10] "git read-tree -m" and the like require worktree

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:02 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>  How about
>
>         git read-tree -m without -i requires work tree
>
>  Hmm?

I thought that was the patch was about. Did I write the patch so bad? :(

>
>  Other than that, it looks like an obvious bugfix which is independent of
>  the rest.

Agreed completely. As said in the other mail, this patch can be cherry
picked well without the rest of the series (should I resend it?).

>
>  Ciao,
>  Dscho
>
>



-- 
Duy

Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:20 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  >
>  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
>  I do not like this change.  It is IMO completely unnecessary, and might
>  break assumptions.

It could. And I tried my best to make sure it did not break anything.
It is also documented (if not clearly, I can correct the document).
While it's not really necessary, it would be IMHO a good change as you
would not have to rely on `cwd` anymore (that would mean whether
moving cwd by prefix or not would no longer matter).

>  Ciao,
>  Dscho
>
-- 
Duy

Re: [PATCH 01/10] "git read-tree -m" and the like require worktree

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:

> On Tue, Mar 11, 2008 at 8:02 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> >  How about
> >
> >         git read-tree -m without -i requires work tree
> >
> >  Hmm?
> 
> I thought that was the patch was about. Did I write the patch so bad? :(

I meant as a replacement for your commit subject (which I found pretty 
hard to understand, harder than the patch itself).

Ciao,
Dscho

Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1147 bytes --]

Hi,

On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:

> On Tue, Mar 11, 2008 at 8:28 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> >  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
> >
> >  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >
> >  This fix is completely independent of the rest of your series.
> 
> Not really. It would have no impact (good or bad) if sent separately. 
> But it is required by the series (otherwise it would die() under certain 
> condition because prefix cannot be recomputed properly).

Well, it _is_ independent.  Either you say "we want to have this as an 
example where you can call setup_git_directory() twice" (which I would not 
find all that bad), or you say "this was calling setup_git_directory() 
twice, fix that".

In any case, it should be separate from this series.

Back to the subject of this series: as I stated already, I do not like the 
separation of work-tree and git directory, and I especially do not like 
how much you had to work outside setup.c.  I think changing the semantics 
for all callers was not necessary, and in fact, not desirable.

Ciao,
Dscho

Re: [PATCH 04/10] Make setup_work_tree() return new prefix

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:27 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  > worktree setup inside setup_git_directory*() is not perfect. It does not
>  > take core.worktree into account. So when you do setup_work_tree(), the
>  > real work tree may be not the one setup_git_directory*() gives you. You
>  > need a new prefix in that case.
>
>  Probably the real fix would be to setup_git_directory(), no?

setup_work_tree() does the job fine. Why duplicate the code one more?
In the old days, setup_git_directory() was just a simple wrapper
around setup_git_dir_gently(). I wanted to move it back to those days.
And setup_git_directory() is fixed  in "Completely move out worktree
setup from setup_git_directory_gently()" so setup_git_directory() does
contain the real fix in the end.

>  Ciao,
>  Dscho
-- 
Duy

Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1079 bytes --]

Hi,

On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:

> On Tue, Mar 11, 2008 at 8:20 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> >  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
> >
> >  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >
> >  I do not like this change.  It is IMO completely unnecessary, and 
> >  might break assumptions.
> 
> It could. And I tried my best to make sure it did not break anything.

That is contradicting.  I think that whatever you tried, once you let 
rev-parse --git-dir return only absolute paths, you already risked 
breakage.

Besides, I remember that Junio specifically requested that I would _not_ 
do something like that (this was for --show-cdup, but I think it really 
applies here, too).

> While it's not really necessary, it would be IMHO a good change as you 
> would not have to rely on `cwd` anymore (that would mean whether moving 
> cwd by prefix or not would no longer matter).

I do not see the problem.  And therefore I do not see that this patch 
solves anything.

Ciao,
Dscho

Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 8:31 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  > This was impossible earlier because git_dir can be relative. Now that
>  > git_dir is absolute, I see no reason for worktree setup inside
>  > setup_git_directory_gently(). The semantic is now clearer: if you need
>  > worktree, call setup_work_tree yourself (well, I will clean up
>  > setup_git_directory() part later)
>
>  As I said earlier, the work for getting the prefix as most likely be done
>  already in the search for .git/.  I mean, it _is_ the common case to have
>  a working tree with a .git/ in it, and that's it.
>
>  So I am quite certain that it is not worth the complicated and intrusive
>  patch to separate the logic.

It does for two purposes:

 - Leave a chance for commands that does not care about worktree at
all. Those commands won't have to care about prefix anymore.
 - Clean up the code. Now only setup_work_tree() does worktree setup
things (like chdir())

>  Particularly since working tree has a bad reputation already, as Junio
>  pointed out: whenever we touch it, we get burnt.

I'd rather get burnt now than later (which might be probably worse) :)
In case we are to be burnt, we should not have it in 1.5.5 :D

>  Ciao,
>  Dscho



-- 
Duy

Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"

[Jay Soffian]

I hate to make this request, but Gmail seems unable to decode messages
which are "Content-Disposition: inline" + "Content-Transfer-Encoding:
base64". The message body shows up completely blank in the Gmail web
interface. (I wonder if it is due to the footer that Majordomo appends?)

I've reported this is a bug to Gmail, but since I imagine I'm not the
only one using Gmail to read this list, and since so far your messages
seem to be the only ones sent this way, can I ask that you configure
your MUA to not send messages this way, if possible?

Much appreciated,

j.

Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 10:18 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:
>
>  > On Tue, Mar 11, 2008 at 8:20 PM, Johannes Schindelin
>  > <Johannes.Schindelin@gmx.de> wrote:
>  >
>
> > >  On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
>  > >
>  > >  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>  > >
>  > >  I do not like this change.  It is IMO completely unnecessary, and
>  > >  might break assumptions.
>  >
>  > It could. And I tried my best to make sure it did not break anything.
>
>  That is contradicting.  I think that whatever you tried, once you let
>  rev-parse --git-dir return only absolute paths, you already risked
>  breakage.
>
>  Besides, I remember that Junio specifically requested that I would _not_
>  do something like that (this was for --show-cdup, but I think it really
>  applies here, too).

I might have lost track. Could you provide me the link for reference? Thanks.

>  > While it's not really necessary, it would be IMHO a good change as you
>  > would not have to rely on `cwd` anymore (that would mean whether moving
>  > cwd by prefix or not would no longer matter).
>
>  I do not see the problem.  And therefore I do not see that this patch
>  solves anything.

It can simplify things (in other patches, such as builtin-config.c
changes). Though without it things just run fine (with a little more
complication in prefix handling). Recent mails about git-am running in
subdirectory (Message-ID:
20080301062255.GA27538@coredump.intra.peff.net) made me think there
was room for this.

It is also prerequisite for later changes in the series (IIRC,
removing prefix from setup*_gently() needs it).

>  Ciao,
>  Dscho
>



-- 
Duy

Re: [PATCH 01/10] "git read-tree -m" and the like require worktree

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 10:06 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:
>
>  > On Tue, Mar 11, 2008 at 8:02 PM, Johannes Schindelin
>  > <Johannes.Schindelin@gmx.de> wrote:
>  >
>
> > >  How about
>  > >
>  > >         git read-tree -m without -i requires work tree
>  > >
>  > >  Hmm?
>  >
>  > I thought that was the patch was about. Did I write the patch so bad? :(
>
>  I meant as a replacement for your commit subject (which I found pretty
>  hard to understand, harder than the patch itself).

Yes it is absolutely a better subject. Thanks.
-- 
Duy

Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 10:08 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:
>
>  > On Tue, Mar 11, 2008 at 8:28 PM, Johannes Schindelin
>  > <Johannes.Schindelin@gmx.de> wrote:
>  >
>  > >  On Sun, 2 Mar 2008, Nguy­n Thái Ng÷c Duy wrote:
>  > >
>  > >  > Signed-off-by: Nguy­n Thái Ng÷c Duy <pclouds@gmail.com>
>
> > >
>  > >  This fix is completely independent of the rest of your series.
>  >
>  > Not really. It would have no impact (good or bad) if sent separately.
>  > But it is required by the series (otherwise it would die() under certain
>  > condition because prefix cannot be recomputed properly).
>
>  Well, it _is_ independent.  Either you say "we want to have this as an
>  example where you can call setup_git_directory() twice" (which I would not
>  find all that bad), or you say "this was calling setup_git_directory()
>  twice, fix that".
>
>  In any case, it should be separate from this series.

My point stands still. It is merely cleanup without harm or goodness
if seperated from the series. This patch does not fix the real
problem.

>  Back to the subject of this series: as I stated already, I do not like the
>  separation of work-tree and git directory, and I especially do not like
>  how much you had to work outside setup.c.  I think changing the semantics
>  for all callers was not necessary, and in fact, not desirable.

For one, setup_work_tree() prototype change is necessary to deal with
(gitdir autodetection + core.worktree + setup_work_tree()).

For setup_*gently() changes, I think I can make
setup_git_directory_gently() retain its current semantics by removing
06/10 and 07/10. But I do think it's nice to have setup_*_gently()
does things gently _and_ easily-predictable (current _gently() setups
worktree under certain conditions and not otherwise, which I consider
uneasily-predictable). Patch 06 and 07 are mainly cleanup as a result
from _gently() changes. Are those most intrusive to you?

>  Ciao,
>  Dscho
>
-- 
Duy

Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:

> On Tue, Mar 11, 2008 at 10:18 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >  Besides, I remember that Junio specifically requested that I would 
> >  _not_ do something like that (this was for --show-cdup, but I think 
> >  it really applies here, too).
> 
> I might have lost track. Could you provide me the link for reference? 
> Thanks.

http://article.gmane.org/gmane.comp.version-control.git/53993/match=show+cdup

This is how you can find it yourself next time (and how I found it):

Go to gmane, search for "show-cdup", then select Junio as writer.  It is 
already the second hit, which was obvious by the email's title.

> >  I do not see the problem.  And therefore I do not see that this patch 
> >  solves anything.
> 
> It can simplify things (in other patches, such as builtin-config.c 
> changes). Though without it things just run fine (with a little more 
> complication in prefix handling). Recent mails about git-am running in 
> subdirectory (Message-ID: 
> 20080301062255.GA27538@coredump.intra.peff.net) made me think there was 
> room for this.

Okay, I see that you want to simplify things.  However, I am not at all 
convinced that your patch series does that.

However, instead of continuing this thread, I will try to find some time 
later this week to look into simplifying the work-tree logic again.

If I find things, I will discuss them on the list.

Ciao,
Dscho

Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Nguyen Thai Ngoc Duy wrote:

> On Tue, Mar 11, 2008 at 8:31 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >  Particularly since working tree has a bad reputation already, as 
> >  Junio pointed out: whenever we touch it, we get burnt.
> 
> I'd rather get burnt now than later (which might be probably worse) :) 
> In case we are to be burnt, we should not have it in 1.5.5 :D

I'd rather be not burnt at all.  That's why I am opposed to this huge 
(relative to what it accomplishes) patch series.

Ciao,
Dscho

Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"

[Nguyen Thai Ngoc Duy]

On Tue, Mar 11, 2008 at 10:38 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I hate to make this request, but Gmail seems unable to decode messages
>  which are "Content-Disposition: inline" + "Content-Transfer-Encoding:
>  base64". The message body shows up completely blank in the Gmail web
>  interface. (I wonder if it is due to the footer that Majordomo appends?)

Funny that meesage was created by gmail.

>  I've reported this is a bug to Gmail, but since I imagine I'm not the
>  only one using Gmail to read this list, and since so far your messages
>  seem to be the only ones sent this way, can I ask that you configure
>  your MUA to not send messages this way, if possible?

If I does not misunderstand it, non-ascii characters make gmail encode
messages base64. I'll be careful next time removing all non-ascii (or
switch back to mutt).

>  Much appreciated,
>
>  j.
>
-- 
Duy