Move worktree setup out of setup_git_directory*

PATCH In review Nguyễn Thái Ngọc Duy 2 participants 4 messages Jun 15, 2016

Share & tools

Nguyễn Thái Ngọc Duy

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 Move worktree setup out of setup_git_directory*

setup_git_directory* now work as if there is no worktree. It may set some worktree-related variables but its prefix (and current directory) should not be affected by worktree settings.

Previously only setup_git_directory() takes worktree into account when calculating prefix (with an exception). This behaviour possibly gives commands that do only call setup_git_directory_gently() wrong prefix. The behavior does not expose often as setup_git_directory_gently() tries to make correct prefix if both GIT_DIR and GIT_WORK_TREE are set (the mentioned exception). Only if core.worktree is used, the defect is summoned.

setup_work_tree() will now take the role of recalculating prefix when worktree is required.

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.

While at it, fix builtin-diff-files.c as well with the same problem.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
setup_git_directory() is now just a thin wrapper around _gently(). Sweet.

 builtin-blame.c      |    4 +-
 builtin-diff-files.c |    4 ++-
 builtin-diff.c       |    5 ++-
 builtin-ls-files.c   |   10 +++---
 builtin-rev-parse.c  |    2 +
 builtin-rm.c         |    2 +-
 cache.h              |    2 +-
 git.c                |    2 +-
 setup.c              |   80 ++++++++++++++++---------------------------------
 t/t1501-worktree.sh  |   30 ++++++++++++++++++-
 10 files changed, 74 insertions(+), 67 deletions(-)

builtin-blame.c +2 -2

index bfd562d..f5a878b 100644
@@ -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), ":");
 	}

builtin-diff-files.c +3 -1

index 4abe3c2..e3cc5cc 100644
@@ -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 = 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);

builtin-diff.c +4 -1

index 444ff2f..93221a0 100644
@@ -244,8 +244,11 @@ 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 = prefix = setup_work_tree(prefix);
+		rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 		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)

builtin-ls-files.c +5 -5

index 25dbfb4..4d833e1 100644
@@ -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++) {

@@ -568,8 +566,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (require_work_tree && !is_inside_work_tree())
-		setup_work_tree();
+	if (require_work_tree)
+		prefix = setup_work_tree(prefix);
+	if (prefix_offset == -1)
+		prefix_offset = prefix ? strlen(prefix) : 0;
 
 	pathspec = get_pathspec(prefix, argv + i);

builtin-rev-parse.c +2 -0

index b9af1a5..9f05cb3 100644
@@ -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;

builtin-rm.c +1 -1

index c0a8bb6..2c15d66 100644
@@ -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;

cache.h +1 -1

index a5b9ffd..dc768db 100644
@@ -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);

git.c +1 -1

index 4f804a2..90451ee 100644
@@ -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");

setup.c +26 ~~-54~~

105 lines not on this page · raw message

t/t1501-worktree.sh +29 -1

index 7ee3820..2b1629b 100755
@@ -58,6 +58,34 @@ 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
+
+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 rm -f tracked) &&
+	cmp result expected'
+
+cat <<EOF >expected
+:100644 100644 e69de29... 0000000... M	sub/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 rm -f tracked) &&
+	cmp result expected'
+
 say "GIT_WORK_TREE=relative path (override core.worktree)"
 export GIT_DIR=$(pwd)/repo.git
 export GIT_CONFIG=$GIT_DIR/config

@@ -103,7 +131,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 &&

Signature · 1 line

1.5.4.2.281.g28d0e

Junio C Hamano

2016-06-15 22:44 UTC #1 lore raw

Quoted 6 lines · Nguyễn Thái Ngọc Duy

setup_git_directory* now work as if there is no worktree. It may set some worktree-related variables but its prefix (and current directory) should not be affected by worktree settings.
...
setup_work_tree() will now take the role of recalculating prefix when worktree is required.

This definitely needs documented in-tree, Documentation/technical/.

I think we have a good understanding from the end-user point-of-view how these things work:

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

We would need a set of rules for application writers, written like this (here is just an example of _how_ the rules might be written, I do not think the contents are correct):

- At startup:

- If the command always need to run from a repository, call setup_git_directory(), which would complain and die if you are outside.

- If the command can optionally run in a repository, use setup_git_directory_gently(&nongit_ok); it won't complain but sets nongit_ok to true if run outside a repository.

In either case, they return prefix string to be used for setup_work_tree() later.

In either case, there is no chdir() done at this point.

- After calling setup_git_directory(), or you made sure you are in a repository after callilng setup_git_directory_gently(), call setup_work_tree(). This will chdir to the top of the work tree.

- When you need to know where $GIT_DIR is, call get_git_dir(). When you need to know where the work tree is, call get_git_work_tree(). However, it is an error to call these functions before calling setup_git_directory and setup_work_tree.

Nguyen Thai Ngoc Duy

author

2016-06-15 22:44 UTC #2 lore raw

Quoted 6 lines · Junio C Hamano

- At startup:
- If the command always need to run from a repository, call setup_git_directory(), which would complain and die if you are outside.

Quoted 8 lines

- If the command can optionally run in a repository, use setup_git_directory_gently(&nongit_ok); it won't complain but sets nongit_ok to true if run outside a repository.
In either case, they return prefix string to be used for setup_work_tree() later.
In either case, there is no chdir() done at this point.

Interesting. It currently does chdir(). But it is pointless now as worktree should not be accessed until setup_work_tree() is called. setup_git_directory* should not return a prefix at all.

Quoted 10 lines

- After calling setup_git_directory(), or you made sure you are in a repository after callilng setup_git_directory_gently(), call setup_work_tree(). This will chdir to the top of the work tree.
- When you need to know where $GIT_DIR is, call get_git_dir(). When you need to know where the work tree is, call get_git_work_tree(). However, it is an error to call these functions before calling setup_git_directory and setup_work_tree.

I guess I just need to verify your statements and put them to Doc/technical/api-setup.txt ;)

Signature · 1 line

Duy

Junio C Hamano

2016-06-15 22:44 UTC #3 lore raw

Quoted 6 lines · Nguyen Thai Ngoc Duy

Interesting. It currently does chdir(). But it is pointless now as worktree should not be accessed until setup_work_tree() is called. setup_git_directory* should not return a prefix at all.
...
I guess I just need to verify your statements and put them to Doc/technical/api-setup.txt ;)

The above, as I said, was written out of thin air without re-reading the current code nor carefully reading what your patch does, so I am afraid there needs a lot more than that to happen before we have something to validate the callers against, but I think you got why I think it is a good idea to have that documented.