setup: do not change to work tree prematurely

PATCH In review Clemens Buchacher 2 participants 3 messages Jun 15, 2016

Share & tools

Clemens Buchacher

author

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

PATCH 1/1 setup: do not change to work tree prematurely

If the work tree is known and a git command is invoked from within a git directory, git_setup_directory() will try to find the relative path from the work tree to the git dir. After doing so it changes directories to the work tree. It fails to update the relative path to the git directory, however.

Instead, do not change the working directory at this point and wait for git_setup_work_tree() to handle this correctly.

This fixes the following bug.

$ cd .git
$ git --work-tree=/tmp/git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref

Reported-by Frédéric Brière <[email protected]>

Signed-off-by Clemens Buchacher <[email protected]>

---

I am not 100% certain that my analysis is correct, since I still do not understand the setup code. But as far as setup_git_directory() is concerned, I think this is the intended behavior.

Clemens

 dir.c   |   33 +++++++++++++++++++--------------
 dir.h   |    1 +
 setup.c |   14 +++++++++-----
 3 files changed, 29 insertions(+), 19 deletions(-)

dir.c +19 ~~-14~~

index cb83332..7bf208d 100644
@@ -926,6 +926,24 @@ int file_exists(const char *f)
 	return lstat(f, &sb) == 0;
 }
 
+char *get_relative_path(char *cwd, const char *dir)
+{
+	if (!dir)
+		return NULL;
+	if (!is_absolute_path(dir))
+		dir = make_absolute_path(dir);
+
+	while (*dir && *dir == *cwd) {
+		dir++;
+		cwd++;
+	}
+	if (*dir)
+		return NULL;
+	if (*cwd == '/')
+		return cwd + 1;
+	return cwd;
+}
+
 /*
  * get_relative_cwd() gets the prefix of the current working directory
  * relative to 'dir'.  If we are not inside 'dir', it returns NULL.

@@ -942,25 +960,12 @@ int file_exists(const char *f)
  */
 char *get_relative_cwd(char *buffer, int size, const char *dir)
 {
-	char *cwd = buffer;
-
 	if (!dir)
 		return NULL;
 	if (!getcwd(buffer, size))
 		die_errno("can't find the current directory");
 
-	if (!is_absolute_path(dir))
-		dir = make_absolute_path(dir);
-
-	while (*dir && *dir == *cwd) {
-		dir++;
-		cwd++;
-	}
-	if (*dir)
-		return NULL;
-	if (*cwd == '/')
-		return cwd + 1;
-	return cwd;
+	return get_relative_path(buffer, dir);
 }
 
 int is_inside_dir(const char *dir)

dir.h +1 -0

index 3bead5f..3bcda1f 100644
@@ -79,6 +79,7 @@ extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *which);
 extern int file_exists(const char *);
 
+extern char *get_relative_path(char *cwd, const char *dir);
 extern char *get_relative_cwd(char *buffer, int size, const char *dir);
 extern int is_inside_dir(const char *dir);

setup.c +9 -5

index 5716d90..67b5122 100644
@@ -525,13 +525,17 @@ const char *setup_git_directory(void)
 
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
+		const char *work_tree = get_git_work_tree();
 		static char buffer[PATH_MAX + 1];
 		char *rel;
-		if (retval && chdir(retval))
-			die_errno ("Could not jump back into original cwd");
-		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
-		if (rel && *rel && chdir(get_git_work_tree()))
-			die_errno ("Could not jump to working directory");
+		if (retval) {
+			if (!is_absolute_path(retval))
+				retval = make_absolute_path(retval);
+			strncpy(buffer, retval, PATH_MAX);
+			buffer[PATH_MAX] = '\0';
+			rel = get_relative_path(buffer, work_tree);
+		} else
+			rel = get_relative_cwd(buffer, PATH_MAX, work_tree);
 		return rel && *rel ? strcat(rel, "/") : NULL;
 	}

Signature · 1 line

1.7.0.5.3.ga76e

Jonathan Nieder

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

Hi Clemens,

Quoted 3 lines · Clemens Buchacher

$ cd .git
$ git --work-tree=/tmp/git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref

Without your patch applied, I get

 $ cd .git
 $ git --work-tree=/tmp/git symbolic-ref HEAD
 refs/heads/hello

I should have done the following instead:

 $ worktree=$(pwd)
 $ cd .git
 $ git --work-tree="$worktree" symbolic-ref HEAD
 fatal: ref HEAD is not a symbolic ref

Quoted 1 line

+char *get_relative_path(char *cwd, const char *dir)

Looks reasonable. Should be const char *cwd.

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

Quoted 9 lines

setup.c +0 -0

index 5716d90..67b5122 100644
@@ -524,14 +524,18 @@ 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) {

If I understand correctly, you made two changes here:

- interpret GIT_WORK_TREE and friends relative to .git instead of the original cwd (by not calling chdir(retval) before get_git_work_tree())

- make setup_git_directory stay in the last directory searched for a .git directory instead of chdir-ing into the toplevel of the work tree.

Are these safe changes to make?

Jonathan

t/t1501-worktree.sh +9 -0

index 9df3012..93bf92c 100755
@@ -106,6 +106,15 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")
 '
 
+test -d work || cp -R repo.git/work .
+test_expect_success 'repo finds its .git dir with separate worktree' '
+	(unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
+	 worktree=$(pwd) &&
+	 cd repo.git &&
+	 git symbolic-ref HEAD &&
+	 git --work-tree="$worktree" symbolic-ref HEAD)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	(cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \

Clemens Buchacher

author

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

Quoted 6 lines · Jonathan Nieder

Without your patch applied, I get
 $ cd .git
 $ git --work-tree=/tmp/git symbolic-ref HEAD
 refs/heads/hello

Quoted 6 lines

I should have done the following instead:

 $ worktree=$(pwd)
 $ cd .git
 $ git --work-tree="$worktree" symbolic-ref HEAD
 fatal: ref HEAD is not a symbolic ref

Thanks

Quoted 3 lines

Quoted 1 line
+char *get_relative_path(char *cwd, const char *dir)
Looks reasonable. Should be const char *cwd.

That is not possible, because we return a pointer to cwd plus an offset, which will be subsequently modified by strcat().

Quoted 7 lines

setup.c +0 -0

index 5716d90..67b5122 100644
@@ -524,14 +524,18 @@ const char *setup_git_directory(void)
 	const char *retval = setup_git_directory_gently(NULL);

Quoted 8 lines

setup.c const char *setup_git_directory(void) · line 527
Quoted 2 lines
setup.c +0 -0
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
If I understand correctly, you made two changes here:
- interpret GIT_WORK_TREE and friends relative to .git instead of the original cwd (by not calling chdir(retval) before get_git_work_tree())

Oops

Quoted 4 lines

- make setup_git_directory stay in the last directory searched for a .git directory instead of chdir-ing into the toplevel of the work tree.
Are these safe changes to make?

Indeed, if you put it that way, this change does not look good. Maybe I'll have a look at Nguyen's patches.

t/t1501-worktree.sh

Quoted 4 lines

t/t1501-worktree.sh +0 -0
index 9df3012..93bf92c 100755

Thanks