[PATCH 1/9] "git read-tree -m" and the like require worktree

[PATCH 1/9] "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 e78cf69..95318e3 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -219,6 +219,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)
+		setup_work_tree();
+
 	if (opts.prefix) {
 		int pfxlen = strlen(opts.prefix);
 		int pos;
-- 
1.5.4.2.281.g28d0e

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> 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 e78cf69..95318e3 100644
> --- a/builtin-read-tree.c
> +++ b/builtin-read-tree.c
> @@ -219,6 +219,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)
> +		setup_work_tree();
> +
>  	if (opts.prefix) {
>  		int pfxlen = strlen(opts.prefix);
>  		int pos;

How would this interact with "read-tree -m -i"?

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

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 7:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>  > 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 e78cf69..95318e3 100644
>  > --- a/builtin-read-tree.c
>  > +++ b/builtin-read-tree.c
>  > @@ -219,6 +219,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)
>  > +             setup_work_tree();
>  > +
>  >       if (opts.prefix) {
>  >               int pfxlen = strlen(opts.prefix);
>  >               int pos;
>
>  How would this interact with "read-tree -m -i"?
>

It setups worktree anyway. Bad. The following would be better

+     if (opts.merge && !opts.index_only)
+             setup_work_tree();
+
-- 
Duy

[PATCH 2/9] 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 9fd6982..b4c2310 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -703,7 +703,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 3/9] 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 b9af1a5..651b5e6 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 4/9] 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 95318e3..0a398f9 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -220,7 +220,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)
-		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 651b5e6..e82cac2 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 a5b9ffd..dc768db 100644
--- a/cache.h
+++ b/cache.h
@@ -282,7 +282,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 4f804a2..90451ee 100644
--- a/git.c
+++ b/git.c
@@ -252,7 +252,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;
-	}
-
-	return retval;
+	return setup_git_directory_gently(NULL);
 }
-- 
1.5.4.2.281.g28d0e

Re: [PATCH 4/9] Make setup_work_tree() return new prefix

[Johannes Schindelin]

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

Hi,

On Wed, 27 Feb 2008, Nguyễn Thái Ngọc Duy wrote:

> @@ -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;

What about the situation where you are in a subdirectory of core.worktree?  
You return prefix NULL?  That's wrong.

I am sorry, but with all these intrusive changes, I get a very uneasy 
feeling.  As uneasy as with the original series, which I tried to fix up, 
not really succeeding.

Ciao,
Dscho

Re: [PATCH 4/9] Make setup_work_tree() return new prefix

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 6:30 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>
>  On Wed, 27 Feb 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  > @@ -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;
>
>  What about the situation where you are in a subdirectory of core.worktree?
>  You return prefix NULL?  That's wrong.

This is an intermediate patch. At the end of the series, you will see
that _gently() does not return prefix. I split it for easier review.

>  I am sorry, but with all these intrusive changes, I get a very uneasy
>  feeling.  As uneasy as with the original series, which I tried to fix up,
>  not really succeeding.

In constrast I feel good :) To me setup_git_directory*() at the end
look really nice. No chdir() all over the place
(temporary chdir() for detecting .git does not count). It also gives a
chance to get rid of worktree to commands that do not care about
worktree at all.

>  Ciao,
>  Dscho
>



-- 
Duy

Re: [PATCH 4/9] Make setup_work_tree() return new prefix

[Johannes Schindelin]

Hi,

On Thu, 28 Feb 2008, Nguyen Thai Ngoc Duy wrote:

> On Thu, Feb 28, 2008 at 6:30 PM, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> wrote:
>
> > I am sorry, but with all these intrusive changes, I get a very uneasy 
> > feeling.  As uneasy as with the original series, which I tried to fix 
> > up, not really succeeding.
> 
> In constrast I feel good :) To me setup_git_directory*() at the end look 
> really nice. No chdir() all over the place (temporary chdir() for 
> detecting .git does not count). It also gives a chance to get rid of 
> worktree to commands that do not care about worktree at all.

Well, I'll have to review your patches in depth, then.  Which will take 
some days, because I wanted to submit builtin-remote first.

Ciao,
Dscho

[PATCH 5/9] 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 406270f..1dd78e8 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2173,7 +2173,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);
 
@@ -2375,7 +2375,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

Re: [PATCH 5/9] http-push: Avoid calling setup_git_directory() twice

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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

Calling setup more than once is now an error?  I do not mind the
new restriction but is it documented clearly somewhere?

Re: [PATCH 5/9] http-push: Avoid calling setup_git_directory() twice

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 7:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>  > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>  > ---
>  >  http-push.c |    4 ++--
>  >  1 files changed, 2 insertions(+), 2 deletions(-)
>
>  Calling setup more than once is now an error?  I do not mind the
>  new restriction but is it documented clearly somewhere?
>

This part was left from my attempt to make setup_git_directory()
return no prefix. The attempt failed but I thought it was a still good
cleanup. Now think it again. I agree calling setup_git_directory()
twice is an error as it may call setup_work_tree(NULL) twice. Yes it
needs to be documented. Will amend the documentation patch.
-- 
Duy

[PATCH 6/9] 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     |   10 +++++-----
 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, 45 insertions(+), 61 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 12e85a4..c50ae32 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -3103,8 +3103,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..2529900 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -13,21 +13,21 @@ 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);
+		rev.prefix = setup_work_tree(NULL);
 		argc = setup_revisions(argc, argv, &rev, NULL);
 	}
 	if (!rev.diffopt.output_format)
diff --git a/builtin-diff.c b/builtin-diff.c
index 254e5a0..61b30e6 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);
+		rev.prefix = 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 dc768db..a5952db 100644
--- a/cache.h
+++ b/cache.h
@@ -283,7 +283,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 90451ee..ffdf6b9 100644
--- a/git.c
+++ b/git.c
@@ -156,11 +156,10 @@ 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;
 
-	subdir = setup_git_directory_gently(&nongit);
+	setup_git_directory_gently(&nongit);
 
 	alias_command = (*argv)[0];
 	git_config(git_alias_config);
@@ -216,9 +215,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;
-
-	/* Make "offset" point to past the '/', and add a '/' at the end */
-	offset++;
-	cwd[len++] = '/';
-	cwd[len] = 0;
-	return cwd + offset;
+	cwd[offset] = '/';
+	strcpy(cwd+offset+1, DEFAULT_GIT_DIR_ENVIRONMENT);
+	setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+	check_repository_format_gently(nongit_ok);
 }
 
 int git_config_perm(const char *var, const char *value)
@@ -444,5 +443,6 @@ int check_repository_format(void)
 
 const char *setup_git_directory(void)
 {
-	return setup_git_directory_gently(NULL);
+	setup_git_directory_gently(NULL);
+	return is_inside_work_tree() ? setup_work_tree(NULL) : NULL;
 }
-- 
1.5.4.2.281.g28d0e

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> -	prefix = setup_git_directory_gently(&nongit);
> -	init_revisions(&rev, prefix);
> +	setup_git_directory_gently(&nongit);
> +	init_revisions(&rev, NULL);


> @@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> ...
> -	init_revisions(&rev, prefix);
> +	init_revisions(&rev, NULL);

Hmm.  How is the effect of this change compensated later to give
proper prefix value to rev.diffopt.prefix?

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 9:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>  > -     prefix = setup_git_directory_gently(&nongit);
>  > -     init_revisions(&rev, prefix);
>  > +     setup_git_directory_gently(&nongit);
>  > +     init_revisions(&rev, NULL);
>
>
>
> > @@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  > ...
>
> > -     init_revisions(&rev, prefix);
>  > +     init_revisions(&rev, NULL);
>
>  Hmm.  How is the effect of this change compensated later to give
>  proper prefix value to rev.diffopt.prefix?
>

I assume you meant rev.prefix? rev.prefix is set right before
setup_revisions(). (grr.. I think I left an redundant
rev.diffopt.skip_stat_unmatch assignment)
-- 
Duy

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Junio C Hamano]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On Thu, Feb 28, 2008 at 9:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>  > -     prefix = setup_git_directory_gently(&nongit);
>>  > -     init_revisions(&rev, prefix);
>>  > +     setup_git_directory_gently(&nongit);
>>  > +     init_revisions(&rev, NULL);
>>
>>
>>
>> > @@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>  > ...
>>
>> > -     init_revisions(&rev, prefix);
>>  > +     init_revisions(&rev, NULL);
>>
>>  Hmm.  How is the effect of this change compensated later to give
>>  proper prefix value to rev.diffopt.prefix?
>>
>
> I assume you meant rev.prefix? rev.prefix is set right before
> setup_revisions(). (grr.. I think I left an redundant
> rev.diffopt.skip_stat_unmatch assignment)

I did mean rev.diffopt.prefix that is initialized by the last
four lines in init_revisions() from the value of prefix you
pass.

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 10:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
>  > On Thu, Feb 28, 2008 at 9:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  >> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>  >>
>  >>  > -     prefix = setup_git_directory_gently(&nongit);
>  >>  > -     init_revisions(&rev, prefix);
>  >>  > +     setup_git_directory_gently(&nongit);
>  >>  > +     init_revisions(&rev, NULL);
>  >>
>  >>
>  >>
>  >> > @@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  >>  > ...
>  >>
>  >> > -     init_revisions(&rev, prefix);
>  >>  > +     init_revisions(&rev, NULL);
>  >>
>  >>  Hmm.  How is the effect of this change compensated later to give
>  >>  proper prefix value to rev.diffopt.prefix?
>  >>
>  >
>  > I assume you meant rev.prefix? rev.prefix is set right before
>  > setup_revisions(). (grr.. I think I left an redundant
>  > rev.diffopt.skip_stat_unmatch assignment)
>
>  I did mean rev.diffopt.prefix that is initialized by the last
>  four lines in init_revisions() from the value of prefix you
>  pass.

Um.. I missed commit "diff --relative". Thanks for noticing.
-- 
Duy

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Johannes Schindelin]

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

Hi,

On Wed, 27 Feb 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().

I do see it, though.  Why make the users work harder?  If you want to get 
the git directory, chances are that you want to work with a worktree, too.

And you really cannot properly separate worktree detection from git 
directory detection: in most of the cases, you will find them at the 
_same_ time (if .git/ is the git directory, the working directory is .).

So I am mildly negative on the thrust of your patch series.

Ciao,
Dscho

Re: [PATCH 6/9] Completely move out worktree setup from setup_git_directory_gently()

[Nguyen Thai Ngoc Duy]

On Thu, Feb 28, 2008 at 6:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>
>  On Wed, 27 Feb 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().
>
>  I do see it, though.  Why make the users work harder?

To make clear separation of worktree usage? I think it's good to add
setup_work_tree() where you really need it. Right now in commands that
use setup_git_directory_gently(), it is (to me) really hard to tell
whether an option require worktree.

Another point of removing worktree setup from _gently(): worktree
setup can die() while _gently()'s purpose is, well, do it gently. If
it's not safe, let the caller decide.

Also having to deal with prefix while you don't need worktree at all
is a bit of work that can be eliminated with this series.

>  If you want to get
>  the git directory, chances are that you want to work with a worktree, too.

Yes. That's why I keep worktree setup in setup_git_directory().

>  And you really cannot properly separate worktree detection from git
>  directory detection: in most of the cases, you will find them at the
>  _same_ time (if .git/ is the git directory, the working directory is .).

No I don't separate worktree detection. I only avoid moving to
worktree unless necessary. Right after _gently() you can tell if you
have a worktree and where it is.

>  So I am mildly negative on the thrust of your patch series.
>
>  Ciao,
>  Dscho
>



-- 
Duy

[PATCH 7/9] 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 8/9] 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 e82cac2..384e23f 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);
-
 	pathspec = get_pathspec(prefix, argv);
+	if (!pathspec)
+		die("No valid pathspec");
 	seen = NULL;
 	for (i = 0; pathspec[i] ; i++)
 		/* nothing */;
diff --git a/setup.c b/setup.c
index 78ae2f9..f0de42f 100644
--- a/setup.c
+++ b/setup.c
@@ -444,5 +444,5 @@ int check_repository_format(void)
 const char *setup_git_directory(void)
 {
 	setup_git_directory_gently(NULL);
-	return is_inside_work_tree() ? setup_work_tree(NULL) : NULL;
+	return get_git_work_tree() ? setup_work_tree(NULL) : NULL;
 }
-- 
1.5.4.2.281.g28d0e

[PATCH 9/9] 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   |   70 +++++++++++++++++++++++++++----
 2 files changed, 72 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..43e5a81 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -1,13 +1,67 @@
 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.
+
+. 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()`
-- 
1.5.4.2.281.g28d0e

Re: [PATCH 9/9] Documentation: update api-builtin and api-setup

[Jonathan Nieder]

Hi,

On 2008-02-27, Nguyễn Thái Ngọc Duy wrote:

>  Documentation/technical/api-builtin.txt |   10 ++++
>  Documentation/technical/api-setup.txt   |   70 +++++++++++++++++++++++++++----
>  2 files changed, 72 insertions(+), 8 deletions(-)

I'm curious --- did anything ever come of this patch?

> --- a/Documentation/technical/api-setup.txt
> +++ b/Documentation/technical/api-setup.txt
> @@ -1,13 +1,67 @@
>  setup API
>  =========
>  
> +End-user point-of-view how the setup works
> +------------------------------------------

I suppose this is scattered through git(1) and git-config(1) now.
Maybe it would make sense to centralized in git-repository-layout(5)
or gitcli(7) and put cross-references elsewhere.

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

E.g., like this (but simpler somehow):

	Locating the repository
	-----------------------
	Typically git is run from within a work tree with a `.git`
	subdirectory at its top level or within a `<project>.git` directory
	associated to a bare repository.  To find the toplevel directory,
	git will repeatedly chdir("..") and at each level check for (in
	order of preference):

	 a) a subdirectory named ".git" with readable "objects" and
	    "refs" subdirectories and a readable "HEAD" file;
	 b) a file named ".git";
	 c) readable "objects" and "refs" subdirectories and a
	    readable "HEAD" file.

	Each is interpreted differently:

	 a) a .git subdirectory is the repository and the directory
	    containing it is the top of the work tree.
	 b) a .git file is used to locate the repository (see
	    "Platform-independent .git symlink" below) and the
	    directory containing a .git file is the top of the
	    work tree.
	 c) an ancestory of the starting cwd containing objects/,
	    refs/, and HEAD is treated as a bare repository.
	    There is no work tree in that case.

	Limiting the repository search
	------------------------------
	Git will not chdir("..") into a directory named in the
	GIT_CEILING_DIRECTORIES environment variable, nor will it
	chdir("..") into a directory on a different filesystem unless
	the GIT_DISCOVERY_ACROSS_FILESYSTEM variable is set.

	Unusual repository layouts
	--------------------------
	If the GIT_DIR environment variable is set, it will be used
	and no repository search performed.  The work tree defaults
	to ".".

	The GIT_WORK_TREE environment variable can be used to
	specify the top level of the work tree.  It can even be used
	from within a bare repository, to set up a temporary work
	tree.

	The "core.worktree" configuration variable in a repository
	can be used to specify the top level of the work tree,
	relative to the repository root.  This can be useful in
	situations in which the work tree must be completely pristine
	(so that it is not possible to put a .git symlink or .git file
	in the top level).

	Some files usually found in the repository can be placed
	elsewhere if requested through the environment.

	 * GIT_CONFIG - .git/config
	 * GIT_OBJECT_DIRECTORY - .git/objects
	 * GIT_INDEX_FILE - .git/index
	 * GIT_GRAFT_FILE - .git/info/grafts

> +
> +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.
> +
> +. 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)`.

Simpler advice might be:

 . If the command is builtin and always needs a repository,
   use the RUN_SETUP flag in the builtin table.
 . If the command is builtin and can benefit from a repository,
   use RUN_SETUP_GENTLY.
 . If you have to run a repository search later, call
   "setup_git_directory_gently" or the shortcut "setup_git_directory"
   (which means "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.

 . If you try to access the git repository (even indirectly like
   `git_config()`) before calling setup_git_directory_gently then git
   will look in the wrong place.
 . When changing the value of the GIT_DIR environment variable, call
   set_git_dir.  setup_git_directory_gently does this already.

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

Did we ever figure out what happens/should happen when the requested
worktree is not an ancestor of cwd?

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

What do prefix_* do when there is no worktree?

> -Talk about
> -
> +
> -* setup_git_directory()
> -* setup_git_directory_gently()
> -* is_inside_git_dir()
> -* is_inside_work_tree()
> -* setup_work_tree()
> -* get_pathspec()
> -
> -(Dscho)
> -- 
> 1.5.4.2.281.g28d0e
> 

Re: [PATCH 9/9] Documentation: update api-builtin and api-setup

[Nguyen Thai Ngoc Duy]

2011/4/5 Jonathan Nieder <jrnieder@gmail.com>:
> Hi,
>
> On 2008-02-27, Nguyễn Thái Ngọc Duy wrote:
>
>>  Documentation/technical/api-builtin.txt |   10 ++++
>>  Documentation/technical/api-setup.txt   |   70 +++++++++++++++++++++++++++----
>>  2 files changed, 72 insertions(+), 8 deletions(-)
>
> I'm curious --- did anything ever come of this patch?

No idea. Perhaps nothing :) You should submit a new patch. Your
description looks very good. Some comments..

> Simpler advice might be:
>
>  . If the command is builtin and always needs a repository,
>   use the RUN_SETUP flag in the builtin table.
>  . If the command is builtin and can benefit from a repository,
>   use RUN_SETUP_GENTLY.
>  . If you have to run a repository search later, call
>   "setup_git_directory_gently" or the shortcut "setup_git_directory"
>   (which means "setup_git_directory_gently(NULL)").

I would discourage people from calling setup_git_directory_* directly,
at least for builtins. It complicates setup handling because there
will be a short time since main() until that function where we are not
sure if we can access repo. There's also alias handling which may set
things up while the command may not want it

I'd rather explicitly mark (RUN_SETUP_NO_THANKS?) commands that want
to setup repo manually (few of them, init/clone and server commands).
Then main() just goes ahead and calls setup_gently() at the very
beginning. Setup hack around alias handling would be gone. If it ends
up with a RUN_SETUP_NO_THANKS one, it undoes the setup. Hmm... I need
to get back to this.

>> +
>> +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.
>
>  . If you try to access the git repository (even indirectly like
>   `git_config()`) before calling setup_git_directory_gently then git
>   will look in the wrong place.
>  . When changing the value of the GIT_DIR environment variable, call
>   set_git_dir.  setup_git_directory_gently does this already.

You can also check startup_info.have_repository before you access repo
because setup*gently may find no repo.

> Did we ever figure out what happens/should happen when the requested
> worktree is not an ancestor of cwd?

prefix is NULL, cwd is unchanged. Repo-wide commands should work fine.
cwd-sensitive ones may cry.

>> +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()`
>
> What do prefix_* do when there is no worktree?

These functions are purely string manipulation. If there is no
worktree, prefix would be NULL and be passed to the functions as such.
-- 
Duy