setup_git_directory: Setup cwd properly if worktree is found

PATCH In review Nguyễn Thái Ngọc Duy 3 participants 6 messages Jun 15, 2016 1 message not in the archive

Share & tools

Nguyễn Thái Ngọc Duy

author

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

PATCH 1/1 setup_git_directory: Setup cwd properly if worktree is found

This makes sure (if possible) the current working directory is at root worktree. "git rev-parse --show-cwd" is added to aid the tests. Also the first test is for "Add missing inside_work_tree" commit.

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

---
 builtin-rev-parse.c |    6 ++++++
 setup.c             |    7 ++++++-
 t/t1501-worktree.sh |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletions(-)

builtin-rev-parse.c +6 -0

index 8d78b69..933875c 100644
@@ -319,6 +319,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(prefix);
 				continue;
 			}
+			if (!strcmp(arg, "--show-cwd")) {
+				char cwd[PATH_MAX+1];
+				getcwd(cwd, sizeof(cwd));
+				printf("%s\n", cwd);
+				continue;
+			}
 			if (!strcmp(arg, "--show-cdup")) {
 				const char *pfx = prefix;
 				if (!is_inside_work_tree()) {

setup.c +6 -1

index 6f8f769..d90f65e 100644
@@ -360,7 +360,12 @@ const char *setup_git_directory(void)
 		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;
+		if (rel && *rel) {
+			if (chdir(get_git_work_tree()))
+				die ("Could not chdir to %s", get_git_work_tree());
+			return strcat(rel, "/");
+		}
+		return NULL;
 	}
 
 	return retval;

t/t1501-worktree.sh +33 -0

index 7ee3820..a4e1f72 100755
@@ -30,6 +30,18 @@ test_rev_parse() {
 
 mkdir -p work/sub/dir || exit 1
 mv .git repo.git || exit 1
+export SAVED_WORK_DIR=$(pwd)/work
+
+say "GIT_WORK_TREE without core.worktree"
+
+export GIT_DIR="$(pwd)"/repo.git
+export GIT_CONFIG="$GIT_DIR"/config
+export GIT_WORK_TREE="$(pwd)"/work
+
+test_expect_success 'cwd is set properly' '
+	(cd work/sub &&
+	 test "$(git rev-parse --show-cwd)" = "$SAVED_WORK_DIR")
+'
 
 say "core.worktree = relative path"
 export GIT_DIR=repo.git

@@ -47,6 +59,11 @@ export GIT_CONFIG="$(pwd)"/$GIT_DIR/config
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+test_expect_success 'cwd is set properly' '
+	(cd work/sub &&
+	 test "$(GIT_DIR=../../repo.git git rev-parse --show-cwd)" = "$SAVED_WORK_DIR")
+'
+
 say "core.worktree = absolute path"
 export GIT_DIR=$(pwd)/repo.git
 export GIT_CONFIG=$GIT_DIR/config

@@ -58,6 +75,11 @@ cd sub/dir || exit 1
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+test_expect_success 'cwd is set properly' '
+	(cd work/sub &&
+	 test "$(git rev-parse --show-cwd)" = "$SAVED_WORK_DIR")
+'
+
 say "GIT_WORK_TREE=relative path (override core.worktree)"
 export GIT_DIR=$(pwd)/repo.git
 export GIT_CONFIG=$GIT_DIR/config

@@ -72,7 +94,13 @@ export GIT_WORK_TREE=../..
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+test_expect_success 'cwd is set properly' '
+	(cd work/sub &&
+	 test "$(GIT_WORK_TREE=.. git rev-parse --show-cwd)" = "$SAVED_WORK_DIR")
+'
+
 mv work repo.git/work
+export SAVED_WORK_DIR=$(pwd)/repo.git/work
 
 say "GIT_WORK_TREE=absolute path, work tree below git dir"
 export GIT_DIR=$(pwd)/repo.git

@@ -89,6 +117,11 @@ cd sub/dir || exit 1
 test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/
 cd ../../../.. || exit 1
 
+test_expect_success 'cwd is set properly' '
+	(cd repo.git/work/sub &&
+	 test "$(git rev-parse --show-cwd)" = "$SAVED_WORK_DIR")
+'
+
 test_expect_success 'repo finds its work tree' '
 	(cd repo.git &&
 	 : > work/sub/dir/untracked &&

Signature · 1 line

1.5.3.5.475.gd7a30

Johannes Sixt

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

t/t1501-worktree.sh test_rev_parse() { · line 39

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

t/t1501-worktree.sh +4 -0

+export SAVED_WORK_DIR=$(pwd)/work
+export GIT_DIR="$(pwd)"/repo.git
+export GIT_CONFIG="$GIT_DIR"/config
+export GIT_WORK_TREE="$(pwd)"/work

These are not very portable; use:

   GIT_DIR="$(pwd)"/repo.git
   GIT_CONFIG="$GIT_DIR"/config
   GIT_WORK_TREE="$(pwd)"/work
   export GIT_DIR GIT_CONFIG GIT_WORK_TREE

The first one also needs $(pwd) quoted.

-- Hannes

Johannes Schindelin

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

In reply to Nguyễn Thái Ngọc Duy

Hi,

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

setup.c +0 -0

index 6f8f769..d90f65e 100644

setup.c const char *setup_git_directory(void) · line 368

Quoted 10 lines

setup.c +6 -1

@@ -360,7 +360,12 @@ const char *setup_git_directory(void)
 		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;
+		if (rel && *rel) {
+			if (chdir(get_git_work_tree()))
+				die ("Could not chdir to %s", get_git_work_tree());
+			return strcat(rel, "/");
+		}
+		return NULL;

Hmm. Maybe this needs a bit more clarification?

When setup_git_directory() returns, the cwd is supposed to be the current working tree's root. Your patch fixes that nicely when the worktree setting was overridden by the config (which is read in check_repository_format()).

But what about setup_git_directory_gently()? If the working tree is overridden by the config, this function is still bogus, right?

As far as I see, setup_git_directory_gently() only works correctly when core.worktree is _not_ set, unless GIT_WORK_TREE is set (which is supposed to override the config setting). Note: I treat GIT_WORK_TREE the same as --work-tree, since at that time they are identical.

Maybe the config stuff has to move into _gently()?

Ciao,
Dscho

P.S.: Thanks for picking up the ball. At some stage I got so fed up with the work-tree stuff that I had to take a long break from it. It is a messy concept, and the implementation is messy, partly because of it, and partly because I wrote it.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johannes Schindelin

2016-06-15 22:43 UTC #4 lore raw

Hi,

Quoted 10 lines · Nguyen Thai Ngoc Duy

Quoted 2 lines · Johannes Schindelin
But what about setup_git_directory_gently()? If the working tree is overridden by the config, this function is still bogus, right?
Hmm.. thinking a little bit more. I guess you're right because GIT_WORK_TREE takes precedence over core.worktree. Maybe some more bits for check_repository_format_version(). Tough decision because, from the value of inside_work_tree, we don't know if we can safely skip overriding inside_work_tree.

I was thinking about adding check_repository_format_version() and a check for inside_work_tree < 0 with obvious handling in two places, probably as a function: first, when we have a gitdirenv but no work_tree_env, and second, at the end of _gently() when we found a git dir but only if work_tree_env was not set.

Quoted 10 lines

Quoted 6 lines
As far as I see, setup_git_directory_gently() only works correctly when core.worktree is _not_ set, unless GIT_WORK_TREE is set (which is supposed to override the config setting). Note: I treat GIT_WORK_TREE the same as --work-tree, since at that time they are identical.
Maybe the config stuff has to move into _gently()?
Well, it could be a bit more complicated because you need to know GIT_DIR first before reading config. I'd rather not move as _gently() is complicated already.

AFAICT it is not a question of complexity, but of correctness. Wouldn't you agree that the prefix _gently() returns is wrong if we don't fix it?

Besides, it might be needed anyway if we are serious about the version check. This check, however, would have to be done _whenever_ we found a git directory, not only when work_tree_env is NULL.

But we could break down _gently() quite nicely. ATM it both handles the cases when gitdirenv was set, and when it was unset. Even if those are really two pretty different situations.

Ciao,
Dscho

Nguyen Thai Ngoc Duy

author

2016-06-15 22:43 UTC #5 lore raw

Quoted 32 lines · Johannes Schindelin

Hi,
Quoted 10 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Johannes Schindelin
But what about setup_git_directory_gently()? If the working tree is overridden by the config, this function is still bogus, right?
Hmm.. thinking a little bit more. I guess you're right because GIT_WORK_TREE takes precedence over core.worktree. Maybe some more bits for check_repository_format_version(). Tough decision because, from the value of inside_work_tree, we don't know if we can safely skip overriding inside_work_tree.
I was thinking about adding check_repository_format_version() and a check for inside_work_tree < 0 with obvious handling in two places, probably as a function: first, when we have a gitdirenv but no work_tree_env, and second, at the end of _gently() when we found a git dir but only if work_tree_env was not set.
Quoted 10 lines
Quoted 6 lines
As far as I see, setup_git_directory_gently() only works correctly when core.worktree is _not_ set, unless GIT_WORK_TREE is set (which is supposed to override the config setting). Note: I treat GIT_WORK_TREE the same as --work-tree, since at that time they are identical.
Maybe the config stuff has to move into _gently()?
Well, it could be a bit more complicated because you need to know GIT_DIR first before reading config. I'd rather not move as _gently() is complicated already.

Quoted 6 lines

AFAICT it is not a question of complexity, but of correctness. Wouldn't you agree that the prefix _gently() returns is wrong if we don't fix it?
Besides, it might be needed anyway if we are serious about the version check. This check, however, would have to be done _whenever_ we found a git directory, not only when work_tree_env is NULL.

Question time. setup_git_directory_gently() can be happy even if there is no repository. Now if we move version check into setup_..._gently and it finds git program is too old to handle the repository, what would we do? die() like in check_repository_format() or tell the caller there is no repository?

Signature · 1 line

Duy

Johannes Schindelin

2016-06-15 22:43 UTC #6 lore raw

Hi,

Quoted 5 lines · Nguyen Thai Ngoc Duy

Question time. setup_git_directory_gently() can be happy even if there is no repository. Now if we move version check into setup_..._gently and it finds git program is too old to handle the repository, what would we do? die() like in check_repository_format() or tell the caller there is no repository?

In the interest of least surprise, die().

Ciao,
Dscho