git-add says 'pathspec did not match any files' for git repository in /

Hey folks,

I've been using git for keeping config files using a repository in /.
In 1.7.4.1, this seems to be broken again. Committing, diffing and
status work this time, but git add gives a pathspec did not match any
files error when adding a file while the current working directory is
not /.

This is best illustrated by an example:

root@ldap:/# git init
Initialized empty Git repository in /.git/
root@ldap:/# cd etc/
root@ldap:/etc# gt add fstab 
bash: gt: command not found
root@ldap:/etc# git add fstab
fatal: pathspec 'fstab' did not match any files
root@ldap:/etc# ls -l fstab 
-rw-r--r-- 1 root root 37 Mar 24 16:58 fstab
root@ldap:/etc# cd /
root@ldap:/# git add /etc/fstab
root@ldap:/etc# git --version 
git version 1.7.4.1


All of this worked properly in 1.7.1. There was a different problem in
1.7.2, but that was fixed then (and with that fix, the above problem
didn't occur). I haven't tested 1.7.3.

I don't have time to fix this right now, so I'll be sticking to 1.7.1
for now. Perhaps someone else want to look into this.

Gr.

Matthijs

On Fri, Mar 25, 2011 at 11:02:54AM +0100, Matthijs Kooijman wrote:

Hey folks,

I've been using git for keeping config files using a repository in /.
In 1.7.4.1, this seems to be broken again. Committing, diffing and
status work this time, but git add gives a pathspec did not match any
files error when adding a file while the current working directory is
not /.

We have t1509 to guard these cases (and it does indeed show
breakages). The problem is that the test requires chroot and cannot be
run automatically. I need to think of writing better tests.

The following makes t1509 pass again for me. You may want to try and
see if it fixes it for you.

diff --git a/setup.c b/setup.c
index 03cd84f..c18ea9c 100644
--- a/setup.c
+++ b/setup.c

@@ -390,15 +390,25 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		return NULL;
 	}
 
-	if (!prefixcmp(cwd, worktree) &&
-	    cwd[strlen(worktree)] == '/') { /* cwd inside worktree */
-		set_git_dir(real_path(gitdirenv));
-		if (chdir(worktree))
-			die_errno("Could not chdir to '%s'", worktree);
-		cwd[len++] = '/';
-		cwd[len] = '\0';
-		free(gitfile);
-		return cwd + strlen(worktree) + 1;
+	if (!prefixcmp(cwd, worktree)) {
+		if (strlen(worktree) == offset_1st_component(worktree)) {
+			set_git_dir(real_path(gitdirenv));
+			if (chdir(worktree))
+				die_errno("Could not chdir to '%s'", worktree);
+			cwd[len++] = '/';
+			cwd[len] = '\0';
+			free(gitfile);
+			return cwd + offset_1st_component(worktree);
+		}
+		if (cwd[strlen(worktree)] == '/') { /* cwd inside worktree */
+			set_git_dir(real_path(gitdirenv));
+			if (chdir(worktree))
+				die_errno("Could not chdir to '%s'", worktree);
+			cwd[len++] = '/';
+			cwd[len] = '\0';
+			free(gitfile);
+			return cwd + strlen(worktree) + 1;
+		}
 	}
 
 	/* cwd outside worktree */

-- 
Duy

The same old problem reappears after setup code is reworked. We tend
to assume there is at least one path component in a path and forget
that path can be simply '/'.

Reported-by: Matthijs Kooijman <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 cache.h |    2 ++
 path.c  |   35 +++++++++++++++++++++++++++++++++++
 setup.c |    5 ++---
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index f765cf5..f579807 100644
--- a/cache.h
+++ b/cache.h

@@ -741,6 +741,8 @@ int longest_ancestor_length(const char *path, const char *prefix_list);
 char *strip_path_suffix(const char *path, const char *suffix);
 int daemon_avoid_alias(const char *path);
 int offset_1st_component(const char *path);
+int is_subdir_or_same(const char *subdir, const char *dir);
+const char *skip_dir(const char *dir, int skip);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);

diff --git a/path.c b/path.c
index 4d73cc9..9f77aea 100644
--- a/path.c
+++ b/path.c

@@ -662,3 +662,38 @@ int offset_1st_component(const char *path)
 		return 2 + is_dir_sep(path[2]);
 	return is_dir_sep(path[0]);
 }
+
+/* return 1 if subdir is either dir or inside dir */
+int is_subdir_or_same(const char *subdir, const char *dir)
+{
+	if (!*subdir || !*dir)
+		die("BUG: how can I compare a dir to empty?");
+
+	while (*dir && *subdir && *dir == *subdir) {
+		dir++;
+		subdir++;
+	}
+
+	/* help/me vs hell/yeah */
+	if (*dir && *subdir)
+		return 0;
+
+	if (!*subdir)
+		return !*dir;	/* same dir */
+
+	/* foo/bar vs foo/ */
+	if (is_dir_sep(dir[-1]))
+		return is_dir_sep(subdir[-1]);
+
+	/* foo/bar vs foo */
+	return is_dir_sep(*subdir);
+}
+
+/* skip 'skip' chars and the trailing separator if any */
+const char *skip_dir(const char *dir, int skip)
+{
+	dir += skip;
+	if (is_dir_sep(*dir))
+		dir++;
+	return dir;
+}

diff --git a/setup.c b/setup.c
index 03cd84f..79f8ea7 100644
--- a/setup.c
+++ b/setup.c

@@ -390,15 +390,14 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		return NULL;
 	}
 
-	if (!prefixcmp(cwd, worktree) &&
-	    cwd[strlen(worktree)] == '/') { /* cwd inside worktree */
+	if (is_subdir_or_same(cwd, worktree)) {	/* cwd inside worktree? */
 		set_git_dir(real_path(gitdirenv));
 		if (chdir(worktree))
 			die_errno("Could not chdir to '%s'", worktree);
 		cwd[len++] = '/';
 		cwd[len] = '\0';
 		free(gitfile);
-		return cwd + strlen(worktree) + 1;
+		return skip_dir(cwd, strlen(worktree));
 	}
 
 	/* cwd outside worktree */

-- 
1.7.4.74.g639db

Nguyễn Thái Ngọc Duy venit, vidit, dixit 25.03.2011 14:49:

The same old problem reappears after setup code is reworked. We tend
to assume there is at least one path component in a path and forget
that path can be simply '/'.

Reported-by: Matthijs Kooijman <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 cache.h |    2 ++
 path.c  |   35 +++++++++++++++++++++++++++++++++++
 setup.c |    5 ++---
 3 files changed, 39 insertions(+), 3 deletions(-)

Wait wait, the bug bisects to

490544b (get_cwd_relative(): do not misinterpret suffix as subdirectory,
2010-05-22)

for me, using

git bisect run sh -c "make || exit 125; cd /etc; GIT_DIR=/tmp/a/.git
~/src/git/git-add fstab || exit 1"

and a repo in /tmp/a/.git with core.worktree set to "/".

The issue is that in get_relative_cwd() of dir.c, "dir" (and maybe
("cwd") may or may not end in "/", so even with dir="/etc/" and
cwd="/etc" we would not recognize we are within the repo. Patch for that
is coming.

Note that by doing something like the above, we can test / without being
root as long as we have files there which we can rely on being readable,
or can rely on /tmp being there.

Michael

2011/3/25 Michael J Gruber [off-list ref]:

Wait wait, the bug bisects to

490544b (get_cwd_relative(): do not misinterpret suffix as subdirectory,
2010-05-22)

Well, we've had a few bugs in this area lately :) That one was fixed
by fbbb4e1 (get_cwd_relative(): do not misinterpret root path -
2010-11-20), which was also noticed by Mathijs.

for me, using

git bisect run sh -c "make || exit 125; cd /etc; GIT_DIR=/tmp/a/.git
~/src/git/git-add fstab || exit 1"

and a repo in /tmp/a/.git with core.worktree set to "/".

The issue is that in get_relative_cwd() of dir.c, "dir" (and maybe
("cwd") may or may not end in "/", so even with dir="/etc/" and
cwd="/etc" we would not recognize we are within the repo. Patch for that
is coming.

Note that by doing something like the above, we can test / without being
root as long as we have files there which we can rely on being readable,
or can rely on /tmp being there.

Sounds good, as long as we stick to standard paths, like /etc/fstab.
But that won't work on Windows. I don't even know if any file is
always at a known location in C:\.
-- 
Duy

get_relative_cwd() tries to determine a common prefix for dir and cwd.
The fix in
490544b (get_cwd_relative(): do not misinterpret suffix as subdirectory, 2010-05-22)
made the logic less naive (so that foo-bar is not misdetected as being
within foo) but broke some other cases, in particular foo not being
detected as being within foo/ any more.

Fix it by taking into a account that a directory name may or may not end
in /.

Document with a test.

Reported-by: Matthijs Kooijman <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
So I think this would be a proper fix.

I tried to be nice and base it on the commit which I bisected the problem to. I
saw much too late that their were later (failed) attempts at fixing this and a
major rewrite of the test, which is quite a nuisance. Anyway, this problem needs a fix,
and by taking get_cwd_relative() from this patch on top of next the problem is fixed.
I have not looked into resolving the merge conflict in the test.

 dir.c               |   23 +++++++++++++++--------
 t/t1501-worktree.sh |   13 +++++++++++++
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/dir.c b/dir.c
index 5615f33..b090cd0 100644
--- a/dir.c
+++ b/dir.c

@@ -956,16 +956,23 @@ char *get_relative_cwd(char *buffer, int size, const char *dir)
 		dir++;
 		cwd++;
 	}
-	if (*dir)
+
+	/* dir has more than just '/' left */
+	if (*dir && ((*dir != '/') || *(dir+1)))
 		return NULL;
-	switch (*cwd) {
-	case '\0':
+	/* *dir is now '\0' or '/' followed by '\0' */
+	if (cwd == buffer) /* we did not move */
+		return (*cwd) ? NULL : cwd;
+	/* we have moved at least by 1 */
+	if (*dir) /* == '/' */
+		return (*cwd) ? NULL : cwd;
+	if (!(*cwd)) /* both ended */
 		return cwd;
-	case '/':
-		return cwd + 1;
-	default:
-		return NULL;
-	}
+	if (*(dir-1) == '/') /* must have been common */
+		return cwd;
+	if (*cwd == '/')
+		return cwd+1;
+	return NULL;
 }
 
 int is_inside_dir(const char *dir)

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index bd8b607..f0dbdd8 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh

@@ -63,6 +63,19 @@ cd sub/dir || exit 1
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+say "core.worktree = absolute path/"
+GIT_DIR=$(pwd)/repo.git
+GIT_CONFIG=$GIT_DIR/config
+git config core.worktree "$(pwd)/work/"
+test_rev_parse 'outside'      false false false
+cd work2
+test_rev_parse 'outside2'     false false false
+cd ../work || exit 1
+test_rev_parse 'inside'       false false true ''
+cd sub/dir || exit 1
+test_rev_parse 'subdirectory' false false true sub/dir/
+cd ../../.. || exit 1
+
 say "GIT_WORK_TREE=relative path (override core.worktree)"
 GIT_DIR=$(pwd)/repo.git
 GIT_CONFIG=$GIT_DIR/config

-- 
1.7.4.1.607.g888da

2011/3/25 Michael J Gruber [off-list ref]:

get_relative_cwd() tries to determine a common prefix for dir and cwd.
The fix in
490544b (get_cwd_relative(): do not misinterpret suffix as subdirectory, 2010-05-22)
made the logic less naive (so that foo-bar is not misdetected as being
within foo) but broke some other cases, in particular foo not being
detected as being within foo/ any more.

I'd rather kill this function off. It's only used in is_inside_dir(),
we can be replaced with is_subdir_or_same() in my previous patch (more
or less the same function with get_relative_cwd, but less cryptic).

I tried something similar (basically core.worktree = $(pwd)/work/) but
could not reproduce. Note that worktree will be normalized by
real_path() (or get_absolute_path() earlier) and the trailing '/' may
have been removed.
-- 
Duy