Re: Incorrect git-blame result if I use full path to file

Jeff King [off-list ref] writes:

IOW, it's not intended for users to use absolute paths in this way.
However, the results for git-blame are obviously quite confusing. It
might be worth fixing, but I suspect there are many more such traps
waiting in other commands. I wonder if it would make sense to reject
pathspecs starting with '/' entirely, which would at least give us a
saner error message (and I can't think of a time when such a pathspec
would be useful)?

All correct, except...

Even more useful would be to convert
/path/to/repo/file to 'file' internally.

... that might help "cut & paste from file manager" people, and I think
we had comment session for such a patch recently on the list.

Sorry, but I lost track of that the current status of that patch.  Did
it die?

On Sun, Dec 02, 2007 at 06:40:36PM -0800, Junio C Hamano wrote:

quoted

Even more useful would be to convert
/path/to/repo/file to 'file' internally.

... that might help "cut & paste from file manager" people, and I think
we had comment session for such a patch recently on the list.

Sorry, but I lost track of that the current status of that patch.  Did
it die?

I didn't pay attention to it originally, but I assume you mean the
recent patch from Robin Rosenberg (cc'd). Looking it over, I see one
obvious omission: there is no canonicalization of the paths. IOW, I
think it will break in the presence of symlinks (if I specify
/path/to/repo/file, /path/to is a symlink to /other/path, I think the
worktree will end up as /other/path/repo, and fail a string comparison
with /path/to/repo).

-Peff

måndag 03 december 2007 skrev Jeff King:

On Sun, Dec 02, 2007 at 06:40:36PM -0800, Junio C Hamano wrote:

quoted

quoted

Even more useful would be to convert
/path/to/repo/file to 'file' internally.

... that might help "cut & paste from file manager" people, and I think
we had comment session for such a patch recently on the list.

Sorry, but I lost track of that the current status of that patch.  Did
it die?

I didn't pay attention to it originally, but I assume you mean the
recent patch from Robin Rosenberg (cc'd). Looking it over, I see one
obvious omission: there is no canonicalization of the paths. IOW, I
think it will break in the presence of symlinks (if I specify
/path/to/repo/file, /path/to is a symlink to /other/path, I think the
worktree will end up as /other/path/repo, and fail a string comparison
with /path/to/repo).

No it didn't die, it's just not worked on too often. I notes, among, other things
that it's test cases were not correct, besides needing more tests.

Symlinks were not covered.

-- robin

This patch makes it possible to drag files and directories from
a graphical browser and drop them onto a shell and feed them
to common git operations without editing away the path to the
root of the work tree.

Signed-off-by: Robin Rosenberg <redacted>
---

I will not surrender to the fierce competion on this subject. Here is an update
with hopefully correct test cases this time. (Linus. your code did not pass). Like Linus,
this code does not resolve symlinks, but I forgot to state that it is by design. It
solves my problem and happens to solve Anatols problem (actually the same since
passing absolute file names to blame is my most important use case).

-- robin

 builtin-blame.c       |    4 +-
 setup.c               |   53 +++++++++++++++++++++++
 t/t3904-abspatharg.sh |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+), 3 deletions(-)
 create mode 100755 t/t3904-abspatharg.sh

diff --git a/builtin-blame.c b/builtin-blame.c
index c158d31..b905dcf 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c

@@ -1880,9 +1880,7 @@ static unsigned parse_score(const char *arg)
 
 static const char *add_prefix(const char *prefix, const char *path)
 {
-	if (!prefix || !prefix[0])
-		return path;
-	return prefix_path(prefix, strlen(prefix), path);
+	return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
 }
 
 /*

diff --git a/setup.c b/setup.c
index 2c7b5cb..1f0ec79 100644
--- a/setup.c
+++ b/setup.c

@@ -4,9 +4,62 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
+static
+const char *strip_work_tree_path(const char *prefix, int len, const char *path)
+{
+	const char *work_tree = get_git_work_tree();
+	int n = strlen(work_tree);
+
+	if (strncmp(path, work_tree, n))
+		return path;
+
+	if (!prefix && !path[n])
+		return path + n;
+
+	if (!prefix) {
+		if (path[n] == '/')
+			return path + n + 1;
+		else
+			if (path[n])
+				return path;
+			else
+				return path + n;
+	}
+
+	if (prefix && !path[n])
+		return path;
+
+	if (strncmp(path + n + 1, prefix, len - 1)) {
+		fprintf(stderr,"prefix mismatch\n");
+		char *np;
+		int i;
+		int d=0;
+		for (i = 0; i < len; ++i)
+			if (prefix[i] == '/')
+				d++;
+		np = xmalloc(strlen(path + n) + d * 3 + 1);
+		for (i=0; i < d * 3; i += 3)
+			strcpy(np + i, "../");
+		strcpy(np + i, path + n + 1);
+		path = np;
+		return np;
+	}
+
+	if (path[len + n] == '/')
+		return path + len + n + 1;
+	else
+		if (path[len + n])
+			return path;
+		else
+			return path + len + n;
+}
+
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
+	if (is_absolute_path(path))
+		path = strip_work_tree_path(prefix, len, path);
+
 	for (;;) {
 		char c;
 		if (*path != '.')

diff --git a/t/t3904-abspatharg.sh b/t/t3904-abspatharg.sh
new file mode 100755
index 0000000..47f1222
--- /dev/null
+++ b/t/t3904-abspatharg.sh

@@ -0,0 +1,112 @@
+#!/bin/sh
+#
+# Copyright (C) 2007 Robin Rosenberg
+#
+
+test_description='Test absolute filename arguments to various git
+commands.  Absolute arguments pointing to a location within the git
+work tree should behave the same as relative arguments.  '
+
+. ./test-lib.sh
+
+test_expect_success 'add files using absolute path names' '
+	echo a >afile &&
+	echo b >bfile &&
+	git-add afile &&
+	git-add "$(pwd)/bfile" &&
+	test "afile bfile" = "$(echo $(git ls-files))"
+	mkdir x &&
+	(
+		cd x &&
+		echo c >cfile &&
+		echo d >dfile &&
+		git-add cfile &&
+		git-add "$(pwd)"
+	) &&
+	test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
+	git ls-files x >f1 &&
+	git ls-files "$(pwd)/x" >f2 &&
+	diff -u f1 f2
+'
+
+test_expect_success 'commit using absolute path names' '
+	git commit -m "foo" &&
+	echo aa >>bfile &&
+	git commit -m "aa" "$(pwd)/bfile"
+'
+
+test_expect_success 'log using absolute path names' '
+	echo bb >>bfile &&
+	git commit -m "bb" $(pwd)/bfile &&
+
+	git log bfile >f1.txt &&
+	git log "$(pwd)/bfile" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'blame using absolute path names' '
+	git blame bfile >f1.txt &&
+	git blame "$(pwd)/bfile" >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'diff using absolute path names' '
+	git diff HEAD HEAD^ -- "$(pwd)/bfile" >f1.txt &&
+	git diff HEAD HEAD^ -- bfile >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'rm using absolute path names' '
+	git rm "$(pwd)/afile" "$(pwd)/x/cfile" &&
+	test "bfile x/dfile" = "$(echo $(git ls-files))"
+'
+
+test_expect_success 'mv using absolute path names' '
+	git reset --hard &&
+	git mv "$(pwd)/afile" "$(pwd)/dfile" &&
+	test "bfile dfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
+	git mv "$(pwd)/dfile" afile &&
+	test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))"
+'
+
+test_expect_success 'show using absolute path names' '
+	git reset --hard &&
+	git show "$(pwd)/bfile" >f1.txt &&
+	git show bfile >f2.txt &&
+	diff -u f1.txt f2.txt
+'
+
+test_expect_success 'add path in parent directory' '
+	(
+		d1="$(pwd)/x"
+		d2="$(pwd)/x/y"
+		mkdir -p x/y &&
+		echo hello1 >x/fa &&
+		echo hello2 >x/y/fb &&
+		cd x/y &&
+		git add "$d1/fa" "$d2/fb"
+	) &&
+	test "afile bfile x/cfile x/dfile x/fa x/y/fb" = "$(echo $(git ls-files))"
+'
+
+test_expect_success 'add a parent directory' '
+	(
+		d1="$(pwd)/a"
+		d2="$(pwd)/a/b"
+		d3="$(pwd)/a/b/c"
+		mkdir -p a/b/c
+		echo helloa >a/a1 &&
+		echo hellob >a/b/b1 &&
+		echo helloc >a/b/c/c1 &&
+		cd a/b/c &&
+		git add "$d2"
+	) &&
+	test "a/b/b1 a/b/c/c1 afile bfile x/cfile x/dfile x/fa x/y/fb" = "$(echo $(git ls-files))"
+'
+
+test_expect_failure 'add a directory outside the work tree' '
+	d1="(cd .. ; pwd)" &&
+	git add "$d1"
+'
+
+test_done

-- 
1.5.3.5.1.gb2df9

On Mon, Dec 03, 2007 at 09:53:30PM +0100, Robin Rosenberg wrote:

code did not pass). Like Linus, this code does not resolve symlinks,
but I forgot to state that it is by design. It solves my problem and

By design meaning "I didn't feel like implemening it because I do not
personally care" or "I have some reason not to resolve symlinks"?

-Peff

Hi,

On Mon, 3 Dec 2007, Jeff King wrote:

On Mon, Dec 03, 2007 at 09:53:30PM +0100, Robin Rosenberg wrote:

quoted

code did not pass). Like Linus, this code does not resolve symlinks,
but I forgot to state that it is by design. It solves my problem and

By design meaning "I didn't feel like implemening it because I do not
personally care" or "I have some reason not to resolve symlinks"?

IMHO those symlinks would be a nice thing in some corner cases, but 
penalise the common case.  So I tend to believe the latter.  (See also 
Linus' message why he talks about his preference for the die() code path.)

Ciao,
Dscho

tisdag 04 december 2007 skrev Johannes Schindelin:

Hi,

On Mon, 3 Dec 2007, Jeff King wrote:

quoted

On Mon, Dec 03, 2007 at 09:53:30PM +0100, Robin Rosenberg wrote:

quoted

code did not pass). Like Linus, this code does not resolve symlinks,
but I forgot to state that it is by design. It solves my problem and

By design meaning "I didn't feel like implemening it because I do not
personally care" or "I have some reason not to resolve symlinks"?

IMHO those symlinks would be a nice thing in some corner cases, but 
penalise the common case.  So I tend to believe the latter.  (See also 
Linus' message why he talks about his preference for the die() code path.)

Actually the forme.... I don't mind it being fixed if it doesn't cost too much.

-- robin

Hi,

On Tue, 4 Dec 2007, Robin Rosenberg wrote:

tisdag 04 december 2007 skrev Johannes Schindelin:

quoted

On Mon, 3 Dec 2007, Jeff King wrote:

quoted

On Mon, Dec 03, 2007 at 09:53:30PM +0100, Robin Rosenberg wrote:

quoted

code did not pass). Like Linus, this code does not resolve 
symlinks, but I forgot to state that it is by design. It solves my 
problem and

By design meaning "I didn't feel like implemening it because I do 
not personally care" or "I have some reason not to resolve 
symlinks"?

IMHO those symlinks would be a nice thing in some corner cases, but 
penalise the common case.  So I tend to believe the latter.  (See also 
Linus' message why he talks about his preference for the die() code 
path.)

Actually the forme.... I don't mind it being fixed if it doesn't cost 
too much.

I do remember the hassles I went through with get_relative_cwd() until I 
broke down and used chdir() two times (ugly).  So the latter reason is 
good enough that you do not even have to admit to the former reason ;-)

Ciao,
Dscho

On Tue, 4 Dec 2007, Johannes Schindelin wrote:

I do remember the hassles I went through with get_relative_cwd() until I 
broke down and used chdir() two times (ugly).

It really is a pretty heavy and complex operation in UNIX in general (and 
open to various races too), which is why I'd generally suggest avoiding it 
if you at all can.

The sad(?) part is, it's fairly trivial to do inside the Linux kernel (but 
probably not in other operating systems - it's only because of our 
superior dcache that we could do it). So a special system call would be no 
problem at all. But obviously very unportable indeed.

			Linus

On Tue, Dec 04, 2007 at 07:59:43AM -0800, Linus Torvalds wrote:

quoted

I do remember the hassles I went through with get_relative_cwd() until I 
broke down and used chdir() two times (ugly).

It really is a pretty heavy and complex operation in UNIX in general (and 
open to various races too), which is why I'd generally suggest avoiding it 
if you at all can.

It is more expensive, though we will be doing it once per user-supplied
pathspec, so I don't know that it will actually have an impact.

I am concerned that not supporting symlinks will make this feature
unusably annoying for some users. I used to have a home directory that
had a symlink in it, and I frequently ran into these sorts of path
comparison issues ($HOME was /home/peff, so typing ~/repo/file pointed
there, but /home was a symlink to /mnt/data/home, so any routines that
normalize the cwd used /mnt/data/home/repo, and the two never matched
up).

Hrm. Looks like somebody has already helpfully implemented
make_absolute_path, so it would just require calling that on each
argument. Something like this on top of Robin's patch:

diff --git a/setup.c b/setup.c
index 4ee8024..e76c83c 100644
--- a/setup.c
+++ b/setup.c

@@ -58,7 +58,8 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
 	if (is_absolute_path(path))
-		path = strip_work_tree_path(prefix, len, path);
+		path = strip_work_tree_path(prefix, len,
+				xstrdup(make_absolute_path(path)));
 
 	for (;;) {
 		char c;

-Peff

On Tue, 4 Dec 2007, Jeff King wrote:

It is more expensive, though we will be doing it once per user-supplied
pathspec, so I don't know that it will actually have an impact.

Well, I'm more worried about just bugs, actually.

Doing this right is actually rather hard. For example, our current 
"make_absolute_path()" is simply not very good, and it's almost impossible 
to *make* it very good.

Why? It relies on being able to get the current cwd, which isn't always 
even possible on all systems. What about unreadable directories? What 
about just so *deep* directories, that the cwd doesn't fit in the 1kB 
allocated for it? Both do happen (people use executable but non-readable 
directories for security sometimes). I'm also almost certain that you can 
confuse it by renaming directories while that thing is running, etc etc.

IOW, that whole thing is simply a bug waiting to happen. The fact that it 
apparently *always* runs whether needed or not just seems to make it worse 
(ie if we already know our cwd, and the absolute path we have already has 
that as a prefix, just strip it off, don't try to do anything complex, and 
leave the complex and fragile cases for the odd-ball when the simple 
approach doesn't work)

			Linus

On Tue, Dec 04, 2007 at 02:52:15PM -0800, Linus Torvalds wrote:

IOW, that whole thing is simply a bug waiting to happen. The fact that it 
apparently *always* runs whether needed or not just seems to make it worse 
(ie if we already know our cwd, and the absolute path we have already has 
that as a prefix, just strip it off, don't try to do anything complex, and 
leave the complex and fragile cases for the odd-ball when the simple 
approach doesn't work)

Fair enough. Something like this then? It gets called only as a
last-ditch (though I think the 'return path' should simply be a die
-- what is the point of getting a pathspec that isn't in the repo?).

---

diff --git a/setup.c b/setup.c
index 4ee8024..fbb956e 100644
--- a/setup.c
+++ b/setup.c

@@ -5,13 +5,17 @@ static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
 static
-const char *strip_work_tree_path(const char *prefix, int len, const char *path)
+const char *strip_work_tree_path(const char *prefix, int len, const char *path,
+		int canonicalized)
 {
 	const char *work_tree = get_git_work_tree();
 	int n = strlen(work_tree);
 
 	if (strncmp(path, work_tree, n))
-		return path;
+		return canonicalized ?
+			path :
+			strip_work_tree_path(prefix, len,
+					xstrdup(make_absolute_path(path)), 1);
 
 	if (!prefix && !path[n])
 		return path + n;

@@ -58,7 +62,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
 	if (is_absolute_path(path))
-		path = strip_work_tree_path(prefix, len, path);
+		path = strip_work_tree_path(prefix, len, path, 0);
 
 	for (;;) {
 		char c;