Preserve cwd in setup_git_directory()

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

Share & tools

Nguyễn Thái Ngọc Duy

author

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

PATCH 1/1 Preserve cwd in setup_git_directory()

When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().

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

---
 setup.c             |    4 ++++
 t/t1501-worktree.sh |   13 ++++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

setup.c +4 -0

index 6cf9094..ef58761 100644
@@ -577,10 +577,14 @@ const char *setup_git_directory(void)
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
 		static char buffer[PATH_MAX + 1];
+		static char cwd[PATH_MAX + 1];
 		char *rel;
+		getcwd(cwd, PATH_MAX);
 		if (retval && chdir(retval))
 			die ("Could not jump back into original cwd");
 		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
+		if (retval && chdir(cwd))
+			die ("Could not jump back into original cwd");
 		return rel && *rel ? strcat(rel, "/") : NULL;
 	}

t/t1501-worktree.sh +12 -1

index 2ee88d8..64f8dea 100755
@@ -29,7 +29,18 @@ test_rev_parse() {
 }
 
 mkdir -p work/sub/dir || exit 1
-mv .git repo.git || exit 1
+
+git config core.worktree "$(pwd)"/work
+mv .git work || exit 1
+test_expect_success '--git-dir with relative .git' '
+	(
+	MYPWD="$(pwd)"
+	cd work/sub/dir &&
+	test "$MYPWD"/work/.git = "$(git rev-parse --git-dir)"
+	)
+'
+
+mv work/.git repo.git || exit 1
 
 say "core.worktree = relative path"
 GIT_DIR=repo.git

Signature · 1 line

1.5.5.GIT

Johannes Sixt

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

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

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

setup.c +2 -0

@@ -577,10 +577,14 @@ const char *setup_git_directory(void)
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
 		static char buffer[PATH_MAX + 1];
+		static char cwd[PATH_MAX + 1];
 		char *rel;
+		getcwd(cwd, PATH_MAX);

This needs an error check.

-- Hannes

Nguyen Thai Ngoc Duy

author

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

PATCH 1/1 Preserve cwd in setup_git_directory()

When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to setup worktree, this leads to incorrect .git location later in setup_work_tree().

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

---

Quoted 12 lines · Johannes Sixt

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

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

setup.c +2 -0

@@ -577,10 +577,14 @@ const char *setup_git_directory(void)
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
 		static char buffer[PATH_MAX + 1];
+		static char cwd[PATH_MAX + 1];
 		char *rel;
+		getcwd(cwd, PATH_MAX);

This needs an error check.

Check added.

 setup.c             |    5 +++++
 t/t1501-worktree.sh |   13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

setup.c +5 -0

index 6cf9094..fa1d696 100644
@@ -577,10 +577,15 @@ const char *setup_git_directory(void)
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
 		static char buffer[PATH_MAX + 1];
+		static char cwd[PATH_MAX + 1];
 		char *rel;
+		if (!getcwd(cwd, PATH_MAX))
+			die ("Could not get the current working directory");
 		if (retval && chdir(retval))
 			die ("Could not jump back into original cwd");
 		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
+		if (retval && chdir(cwd))
+			die ("Could not jump back into original cwd");
 		return rel && *rel ? strcat(rel, "/") : NULL;
 	}

t/t1501-worktree.sh +12 -1

index 2ee88d8..d53d66a 100755
@@ -29,7 +29,18 @@ test_rev_parse() {
 }
 
 mkdir -p work/sub/dir || exit 1
-mv .git repo.git || exit 1
+
+git config core.worktree "$(pwd)"/work
+mv .git work || exit 1
+test_expect_success '--git-dir with relative .git' '
+	(
+	MYPWD="$(pwd)"
+	cd work/sub/dir &&
+	test "$MYPWD"/work/.git = "$(git rev-parse --git-dir)"
+	)
+'
+
+mv work/.git repo.git || exit 1
 
 say "core.worktree = relative path"
 GIT_DIR=repo.git

Signature · 1 line

1.5.5.GIT

Johannes Schindelin

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

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

Hi,

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

When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().

I do not understand. core.worktree is either absolute, in which case there is no problem. Or it is relative to where the config lives, no?

Besides, this touches a _very_ delicate part of Git. I'd rather not touch it during the -rc cycle.

I remember I was opposed to the whole worktree crap, and judging by the sheer amount of bug reports, next to nobody uses it anyway.

It was implemented in a really ugly manner, too, and my attempt to fix it was still messy. That is why we have _only_ problems with it.

Just thinking of worktree makes me uneasy,
Dscho

Nguyen Thai Ngoc Duy

author

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

Quoted 5 lines · Johannes Schindelin

Hi,
On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:

Quoted 8 lines

Quoted 4 lines
When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().
I do not understand. core.worktree is either absolute, in which case
there is no problem. Or it is relative to where the config lives, no?

The problem is GIT_DIR is not absolute in this case. So cwd must stay where git dir is until it is absolute-ized by setup_work_tree().

Signature · 1 line

Duy

Johannes Schindelin

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

Hi,

Quoted 9 lines · Nguyen Thai Ngoc Duy

Quoted 7 lines · Johannes Schindelin
Quoted 4 lines · Nguyen Thai Ngoc Duy
When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().

Quoted 5 lines

Quoted 2 lines
I do not understand. core.worktree is either absolute, in which case there is no problem. Or it is relative to where the config lives, no?
The problem is GIT_DIR is not absolute in this case. So cwd must stay where git dir is until it is absolute-ized by setup_work_tree().

I do not see GIT_DIR being set in your test case at all.

I do not see how get_git_work_tree() ro get_relative_cwd() should ever be allowed to chdir().

_If_ they were (which I strongly doubt), they should chdir() back themselves.

I now wasted easily 30 minutes just trying to make sense of your patch and your response. And I am still puzzled.

Your commit message was of no help.

This patch is definitely the opposite of "obviously correct".

Ciao,
Dscho

Nguyen Thai Ngoc Duy

author

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

Quoted 26 lines · Johannes Schindelin

Hi,
Quoted 10 lines · Nguyen Thai Ngoc Duy
Quoted 7 lines · Johannes Schindelin
Quoted 4 lines · Nguyen Thai Ngoc Duy
When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().
Quoted 5 lines
Quoted 2 lines
I do not understand. core.worktree is either absolute, in which case there is no problem. Or it is relative to where the config lives, no?
The problem is GIT_DIR is not absolute in this case. So cwd must stay where git dir is until it is absolute-ized by setup_work_tree().
I do not see GIT_DIR being set in your test case at all.
I do not see how get_git_work_tree() ro get_relative_cwd() should ever be allowed to chdir().

Quoted 7 lines

_If_ they were (which I strongly doubt), they should chdir() back themselves.
I now wasted easily 30 minutes just trying to make sense of your patch and your response. And I am still puzzled.
Your commit message was of no help.

Alright, let's look at the code.

1. cwd is moved to toplevel working directory by setup_git_directory_gently()
2. setup_git_env() by default will set git_dir variable as ".git" as
part of check_repository_format_gently()
3. now in setup_git_directory() finds out core.worktree, it chdir()
to get relative prefix
4. setup_work_tree() sees that git_dir is not absolute path, it makes
git_dir absolute

If in step 3, it does not chdir(), step 4 will be right. In this case, step 3 does chdir() and not going back, access to git repository will fail as Geoff Russell discovered.

Signature · 1 line

Duy

Geoff Russell

2016-06-15 22:45 UTC #7 lore raw

Hi,

Quoted 46 lines · Nguyen Thai Ngoc Duy

Quoted 26 lines · Johannes Schindelin
Hi,
Quoted 10 lines · Nguyen Thai Ngoc Duy
Quoted 7 lines · Johannes Schindelin
Quoted 4 lines · Nguyen Thai Ngoc Duy
When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree().
Quoted 5 lines
Quoted 2 lines
I do not understand. core.worktree is either absolute, in which case there is no problem. Or it is relative to where the config lives, no?
The problem is GIT_DIR is not absolute in this case. So cwd must stay where git dir is until it is absolute-ized by setup_work_tree().
I do not see GIT_DIR being set in your test case at all.
I do not see how get_git_work_tree() ro get_relative_cwd() should ever be allowed to chdir().
Quoted 7 lines
_If_ they were (which I strongly doubt), they should chdir() back themselves.
I now wasted easily 30 minutes just trying to make sense of your patch and your response. And I am still puzzled.
Your commit message was of no help.
Alright, let's look at the code.
1. cwd is moved to toplevel working directory by setup_git_directory_gently()
2. setup_git_env() by default will set git_dir variable as ".git" as
part of check_repository_format_gently()
3. now in setup_git_directory() finds out core.worktree, it chdir()
to get relative prefix
4. setup_work_tree() sees that git_dir is not absolute path, it makes
git_dir absolute

Quoted 7 lines

If in step 3, it does not chdir(), step 4 will be right. In this case, step 3 does chdir() and not going back, access to git repository will fail as Geoff Russell discovered.
--
Duy

There seem to be plenty of things happening here which I know nothing about, which
is really why I hit the problem in the first place. I was pretty surprised to find that
my command line switch (--work-tree) was being stored in the config file, which ended up causing the problem. Duy thinks he can fix this, which is fine, but as a matter of principle, I'm not keen on command line switches being magically saved for me when I didn't ask for this to happen. The reason for
using a command line switch is because I want to override default behaviour on this execution, if I wanted to change the default behaviour, then I'd set values in the config file.

Whatever you decide, I reckon its pretty magical to have a software problem, send
an email to a list and get the solution in 45 minutes. I've never got that service from any other piece of software (or hardware)!

Cheers,
Geoff Russell

Johannes Schindelin

2016-06-15 22:45 UTC #8 lore raw

Hi,

Quoted 5 lines · Geoff Russell

I was pretty surprised to find that my command line switch (--work-tree) was being stored in the config file, which ended up causing the problem. Duy thinks he can fix this, which is fine, but as a matter of principle, I'm not keen on command line switches being magically saved for me when I didn't ask for this to happen.

You were initializing a repository. How on earth could you be surprised when _important_ things like work-tree git-dir or if it is bare are stored inside the freshly initialized repository?

Quoted 3 lines

The reason for using a command line switch is because I want to override default behaviour on this execution, if I wanted to change the default behaviour, then I'd set values in the config file.

No, you would not. Because there is no config file yet.

Htrh,
Dscho