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

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

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Nguyen Thai Ngoc Duy]

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

[PATCH] setup: return correct prefix if worktree is '/'

[Nguyễn Thái Ngọc 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 <matthijs@stdin.nl>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH] setup: return correct prefix if worktree is '/'

[Michael J Gruber]

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 <matthijs@stdin.nl>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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

Re: [PATCH] setup: return correct prefix if worktree is '/'

[Nguyen Thai Ngoc Duy]

2011/3/25 Michael J Gruber <git@drmicha.warpmail.net>:
> 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

[PATCH] dir.c: do not trip over difference in "/"

[Michael J Gruber]

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 <matthijs@stdin.nl>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
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

Re: [PATCH] dir.c: do not trip over difference in "/"

[Nguyen Thai Ngoc Duy]

2011/3/25 Michael J Gruber <git@drmicha.warpmail.net>:
> 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).

> 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

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