[PATCH] Make Git accept absolute path names for files within the work tree

[PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---
 setup.c               |   16 ++++++++++++++
 t/t3904-abspatharg.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 0 deletions(-)
 create mode 100755 t/t3904-abspatharg.sh

Was it this simple?

diff --git a/setup.c b/setup.c
index 43cd3f9..9b3a9ff 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,22 @@ static int inside_work_tree = -1;
 
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
+	if (is_absolute_path(path)) {
+		const char *work_tree = get_git_work_tree();
+		int n = strlen(work_tree);
+		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
+			if (path[n])
+				path += 1;
+			path += n;
+			if (prefix && !strncmp(path, prefix, len - 1)) {
+			    if (path[len - 1] == '/')
+				    path += len;
+			    else
+				    if (!path[len - 1])
+					    path += len - 1;
+			}
+		}
+	}
 	const char *orig = path;
 	for (;;) {
 		char c;
diff --git a/t/t3904-abspatharg.sh b/t/t3904-abspatharg.sh
new file mode 100755
index 0000000..aa47602
--- /dev/null
+++ b/t/t3904-abspatharg.sh
@@ -0,0 +1,53 @@
+#!/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) &&
+cd .. &&
+test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
+test "$(echo $(git ls-files x))" = "$(echo $(git ls-files $(pwd)/x))"
+'
+
+test_expect_success 'commit using absolute path names' '
+git commit -m "foo" &&
+echo aa >>bfile &&
+git commit -m "bb" $(pwd)/bfile
+'
+
+test_expect_success 'log using absolute path names' '
+git log afile >f1.txt &&
+git log $(pwd)/afile >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_expect_success 'blame using absolute path names' '
+git blame afile >f1.txt &&
+git blame $(pwd)/afile >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_expect_success 'diff using absolute path names' '
+git diff HEAD^ -- $(pwd)/afile >f1.txt &&
+git diff HEAD^ -- afile >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_done
-- 
1.5.3.5.1.gb2df9

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Was it this simple?
>
> diff --git a/setup.c b/setup.c
> index 43cd3f9..9b3a9ff 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -6,6 +6,22 @@ static int inside_work_tree = -1;
>  
>  const char *prefix_path(const char *prefix, int len, const char *path)
>  {
> +	if (is_absolute_path(path)) {
> +		const char *work_tree = get_git_work_tree();
> +		int n = strlen(work_tree);
> +		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
> +			if (path[n])
> +				path += 1;
> +			path += n;
> +			if (prefix && !strncmp(path, prefix, len - 1)) {
> +			    if (path[len - 1] == '/')
> +				    path += len;
> +			    else
> +				    if (!path[len - 1])
> +					    path += len - 1;
> +			}
> +		}
> +	}
>  	const char *orig = path;

Decl after statement.

I do not think there is fundamental reason to object to this change, as
long as the prefixing is done to the path that is trying to name a path
in the working tree.

Also some codepath that does not require any work tree may want to call
prefix_path().  I do not know what would happen in such a case.

Although I didn't look at all the callers, I think the caller from
config.c is not talking about a path in the work tree, and not all users
of config.c need to have work-tree.

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Johannes Sixt]

Robin Rosenberg schrieb:
> +test_expect_success 'add files using absolute path names' '
> +echo a >afile &&
> +echo b >bfile &&
> +git-add afile &&
> +git-add $(pwd)/bfile &&

Please use "$(pwd)/bfile" (d-quotes) throughout. Otherwise the test would 
fail if conducted in a directory with spaces.

-- Hannes

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

tisdag 27 november 2007 skrev Johannes Sixt:
> Robin Rosenberg schrieb:
> > +test_expect_success 'add files using absolute path names' '
> > +echo a >afile &&
> > +echo b >bfile &&
> > +git-add afile &&
> > +git-add $(pwd)/bfile &&
> 
> Please use "$(pwd)/bfile" (d-quotes) throughout. Otherwise the test would 
> fail if conducted in a directory with spaces.

AFAIK, None of the tests works in directories with spaces. But I'll fix it, just in case, so 
noone can blame /me/ for the tests breaking in /my home/. I cannot verify it because
the testing infrastructure breaks before the actual tests start executing.

-- robin

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

tisdag 27 november 2007 skrev Junio C Hamano:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
[...]
> >  	const char *orig = path;
> 
> Decl after statement.
Stupid C..
> 
> I do not think there is fundamental reason to object to this change, as
> long as the prefixing is done to the path that is trying to name a path
> in the working tree.
> 
> Also some codepath that does not require any work tree may want to call
> prefix_path().  I do not know what would happen in such a case.
ok, I'll add a test for whether we have a work tree.

> Although I didn't look at all the callers, I think the caller from
> config.c is not talking about a path in the work tree, and not all users
> of config.c need to have work-tree.

Oh dear. That wasn't on master when I looked. That one seems to be the only "other"
use and it is wrapped inside an if (!absolute_path()) tests so my magic does not conflict
there.

-- robin

[PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---
 setup.c               |   19 +++++++++++++++++
 t/t3904-abspatharg.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100755 t/t3904-abspatharg.sh

diff --git a/setup.c b/setup.c
index f512ea0..ffc30bf 100644
--- a/setup.c
+++ b/setup.c
@@ -7,6 +7,25 @@ static int inside_work_tree = -1;
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
+	const char *work_tree = get_git_work_tree();
+	if (is_absolute_path(path) && work_tree) {
+		int n = strlen(work_tree);
+		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
+			if (path[n])
+				path += n + 1;
+			else
+				path += n;
+
+			if (prefix && !strncmp(path, prefix, len - 1)) {
+				if (path[len - 1] == '/')
+					path += len;
+				else
+					if (!path[len - 1])
+						path += len - 1;
+			}
+		}
+	}
+
 	for (;;) {
 		char c;
 		if (*path != '.')
diff --git a/t/t3904-abspatharg.sh b/t/t3904-abspatharg.sh
new file mode 100755
index 0000000..cd4a52e
--- /dev/null
+++ b/t/t3904-abspatharg.sh
@@ -0,0 +1,55 @@
+#!/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)" &&
+cd .. &&
+test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
+git ls-files x >f1 &&
+git ls-files "$(pwd)/x" >f2 &&
+diff f1 f2
+'
+
+test_expect_success 'commit using absolute path names' '
+git commit -m "foo" &&
+echo aa >>bfile &&
+git commit -m "bb" "$(pwd)/bfile"
+'
+
+test_expect_success 'log using absolute path names' '
+git log afile >f1.txt &&
+git log "$(pwd)/afile" >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_expect_success 'blame using absolute path names' '
+git blame afile >f1.txt &&
+git blame "$(pwd)/afile" >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_expect_success 'diff using absolute path names' '
+git diff HEAD^ -- "$(pwd)/afile" >f1.txt &&
+git diff HEAD^ -- afile >f2.txt &&
+diff f1.txt f2.txt
+'
+
+test_done
-- 
1.5.3.5.1.gb2df9

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> 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 <robin.rosenberg@dewire.com>
> ---
>  setup.c               |   19 +++++++++++++++++
>  t/t3904-abspatharg.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 0 deletions(-)
>  create mode 100755 t/t3904-abspatharg.sh
>
> diff --git a/setup.c b/setup.c
> index f512ea0..ffc30bf 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -7,6 +7,25 @@ static int inside_work_tree = -1;
>  const char *prefix_path(const char *prefix, int len, const char *path)
>  {
>  	const char *orig = path;
> +	const char *work_tree = get_git_work_tree();
> +	if (is_absolute_path(path) && work_tree) {
> +		int n = strlen(work_tree);
> +		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
> +			if (path[n])
> +				path += n + 1;
> +			else
> +				path += n;
> +
> +			if (prefix && !strncmp(path, prefix, len - 1)) {
> +				if (path[len - 1] == '/')
> +					path += len;
> +				else
> +					if (!path[len - 1])
> +						path += len - 1;
> +			}
> +		}
> +	}
> +

This looks somewhat tighter than the previous one, but still made me
worried if the caller of prefix_path() has run the setup sequence enough
so that calling get_git_work_tree() is safe, so I ended up auditing the
callpath.  At least, I do not want to see that unconditional call to
get_git_work_tree() when we do not need to do this "ah prefix got an
unusual absolute path" stuff.

 * builtin-init-db.c uses prefix_path() to find where the template is
   (this is mingw fallout change); in general, I do not think we would
   want to trigger repository nor worktree discovery inside init-db,
   although I suspect this particular callpath could be made Ok (because
   it is taken only when template_dir is not absolute) if you do not
   unconditionally call get_git_work_tree() in prefix_path().

 * config.c uses prefix_path() to find the ETC_GITCONFIG that is not
   absolute (again, mingw fallout).  When git_config() is called, we
   already should have discovered repository but worktree may not have
   been found yet (config.worktree can be used to specify where it is,
   so you have a chicken and egg problem).  Again, this particular
   callpath happens to be Ok because this is used only for non-absolute
   path, but that is a bit subtle.

 * get_pathspec() uses prefix_path() for obvious reasons, and the prefix
   it gets must have been discovered by finding out where the worktree
   is, so by definition that one is safe.

Everybody else you would get from "git grep prefix_path" are after the
proper setup, so they should all be safe. 


> diff --git a/t/t3904-abspatharg.sh b/t/t3904-abspatharg.sh
> new file mode 100755
> index 0000000..cd4a52e
> --- /dev/null
> +++ b/t/t3904-abspatharg.sh
> @@ -0,0 +1,55 @@
> +#!/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" &&

This looks quite dense.  With indent like other existing tests this
would become a bit easier to read.

> +test "afile bfile" = "$(echo $(git ls-files))"

Hmmm.  Looks a bit ugly but Ok.

> +mkdir x &&
> +cd x &&
> +echo c >cfile &&
> +echo d >dfile &&
> +git-add cfile &&
> +git-add "$(pwd)" &&
> +cd .. &&

If this sequence fails in the middle, the next test will execute in a
wrong directory.  Instead of "cd there && ... && cd .. &&", it is safer
to do "( cd there && ... ) &&".

> +test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
> +git ls-files x >f1 &&
> +git ls-files "$(pwd)/x" >f2 &&
> +diff f1 f2

"diff -u" is much easier for people who ends up reading the output when
something goes wrong.

Cases that are still not covered:

	try to add a file in the parent directory from a subdirectory,
	using absolute path;

	try to add a file in a directory that is too high (i.e. outside
	the work tree), using absolute path;

The latter I think should fail.  People tend to forget writing negative
tests when they are too excited with their shiny new features.

Perhaps adding something like:

	try to add a sub-subdirectory using an absolute path, and make
	sure paths in subdirectory are not added;

would also be prudent to catch future possible breakages (off by one
cutting the common directory prefix one level too deep or something),
but that is probably just me who tends to be paranoid.

Exactly the same comments apply to tests for other commands.  Also I
think mv, rm, reset and checkout take pathnames.

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> diff --git a/setup.c b/setup.c
> index f512ea0..ffc30bf 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -7,6 +7,25 @@ static int inside_work_tree = -1;
>  const char *prefix_path(const char *prefix, int len, const char *path)
>  {
>  	const char *orig = path;
> +	const char *work_tree = get_git_work_tree();
> +	if (is_absolute_path(path) && work_tree) {

Could you structure this part to read like this, into a separate
function:

static const char *strip_work_tree_path(const char *prefix, int len, const char *path)
{
	...
}

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);
	...


About the part that would be moved out of line with such a
restructuring,

> +		int n = strlen(work_tree);
> +		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
> +			if (path[n])
> +				path += n + 1;
> +			else
> +				path += n;
> +
> +			if (prefix && !strncmp(path, prefix, len - 1)) {
> +				if (path[len - 1] == '/')
> +					path += len;
> +				else
> +					if (!path[len - 1])
> +						path += len - 1;
> +			}

This makes me wonder what happens if after stripping the worktree path
path does not match the prefix.

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

onsdag 28 november 2007 skrev Junio C Hamano:
> This looks somewhat tighter than the previous one, but still made me
> worried if the caller of prefix_path() has run the setup sequence enough
> so that calling get_git_work_tree() is safe, so I ended up auditing the
> callpath.  At least, I do not want to see that unconditional call to
> get_git_work_tree() when we do not need to do this "ah prefix got an
> unusual absolute path" stuff.
> 
>  * builtin-init-db.c uses prefix_path() to find where the template is
>    (this is mingw fallout change); in general, I do not think we would
>    want to trigger repository nor worktree discovery inside init-db,
>    although I suspect this particular callpath could be made Ok (because
>    it is taken only when template_dir is not absolute) if you do not
>    unconditionally call get_git_work_tree() in prefix_path().
> 
>  * config.c uses prefix_path() to find the ETC_GITCONFIG that is not
>    absolute (again, mingw fallout).  When git_config() is called, we
>    already should have discovered repository but worktree may not have
>    been found yet (config.worktree can be used to specify where it is,
>    so you have a chicken and egg problem).  Again, this particular
>    callpath happens to be Ok because this is used only for non-absolute
>    path, but that is a bit subtle.

I wonder if this usage in config and initdb is what prefix_path() was intended for.  The
interface is declared in cache.h and there are error conditions like '%s is outside repository'.

Maybe we should have a boolean indicating whether the arguments refer to filespecs
or not to make this clear or rewite the mingw fallouts in some other way.

>  * get_pathspec() uses prefix_path() for obvious reasons, and the prefix
>    it gets must have been discovered by finding out where the worktree
>    is, so by definition that one is safe.
> 
> Everybody else you would get from "git grep prefix_path" are after the
> proper setup, so they should all be safe. 

Thanks for looking at the usages. I'll come up with some more tests too, though writing
negative tests sometimes is a challenge. Tests all to easily fail for the wrong reason which
is bad when we expect them to fail for the right reason.

-- robin

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Maybe we should have a boolean indicating whether the arguments refer
> to filespecs or not to make this clear or rewite the mingw fallouts in
> some other way.

I wondered the same, but my vote would be to make them separate
functions.  You would need a function to abstract out pasting of paths
to ease MinGW people even without this "ah absolute -- let's make it
relative to work tree first" hack, anyway.

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Jeff King]

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;

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Linus Torvalds]

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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Jeff King]

On Tue, Dec 04, 2007 at 07:59:43AM -0800, Linus Torvalds 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.

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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Linus Torvalds]

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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Johannes Schindelin]

Hi,

On Tue, 4 Dec 2007, Robin Rosenberg wrote:

> tisdag 04 december 2007 skrev Johannes Schindelin:
> 
> > On Mon, 3 Dec 2007, Jeff King wrote:
> > 
> > > 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"?
> > 
> > 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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

tisdag 04 december 2007 skrev Johannes Schindelin:
> Hi,
> 
> On Mon, 3 Dec 2007, Jeff King wrote:
> 
> > 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"?
> 
> 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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Johannes Schindelin]

Hi,

On Mon, 3 Dec 2007, Jeff King wrote:

> 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"?

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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Jeff King]

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

Re: [PATCH] Make Git accept absolute path names for files within the work tree

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> I will not surrender to the fierce competion on this subject. Here is
> an update with hopefully correct test cases this time.

Yay, that's the spirit!

> ... Like Linus, this code does not resolve symlinks,
> but I forgot to state that it is by design.

Perhaps state it in the commit log message?

>  static const char *add_prefix(const char *prefix, const char *path)
>  {
> +	return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
>  }

Ok; prefix_path can get NULL prefix (not complaining; just a reminder in
the following discussion).

> 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)

Style.  "static" not on its own line.

> +{
> +	const char *work_tree = get_git_work_tree();
> +	int n = strlen(work_tree);

Preconditions.

 * prefix could be NULL or path to the subdirectory the user's
   non-absolute path should be relative to, expressed as a relative path
   to the top of the work tree, including a trailing slash.  len is the
   length of the prefix string.

 * path was determined by the caller to be absolute.

 * It is assumed that get_git_work_tree() always gives absolute path,
   and without trailing slash.

 * Does prefix always NULL if we are at the top, and never "", I wonder.
   But lets assume that, too.

> +	if (strncmp(path, work_tree, n))
> +		return path;

If the given path is outside the work tree, return absolute as-is.
After this point we know path matches the work tree

> +	if (!prefix && !path[n])
> +		return path + n;

If we are at the top of the work tree and path names the top of the work
tree, then we return "".

> +	if (!prefix) {
> +		if (path[n] == '/')
> +			return path + n + 1;

If we are at the top of the work tree and the path names the top of the
work tree followed by a slash and then something, that is a path inside
the work tree.  Return relative to the top of the work tree.

> +		else
> +			if (path[n])
> +				return path;
> +			else
> +				return path + n;
> +	}

Style.  "else if" would give you shallower indentation.  We know path[n]
was not slash, and if it is not NUL then path is not inside the work
tree but is a neighbour (e.g. worktree is /a/b and path is /a/bc).
Return absolute.  Otherwise the path names the top of the work tree
itself so we return "".

Now at this point, we know we are in a subdirectory, because the above
if (!prefix) part always return.  So the test for prefix here is
unnecessary.

> +	if (prefix && !path[n])
> +		return path;

If we are in a subdirectory, and path names the top of the work tree, we
return it as-is (i.e. absolute).  This feels a bit inconsistent with the
part that follows, which tries to make things relative by using "../",
doesn't it?

> +	if (strncmp(path + n + 1, prefix, len - 1)) {

For !prefix case we have determined path is not merely a neighbour, but
we haven't checked that in this codepath.  If the parameters were like
this:

	path      = /axbc/e
        work_tree = /a
	n         = 2
        prefix    =    bc/
	len       = 3

this check says "fine, path is under prefix and we won't add ../
uplevels".  You need to have

	if (path[n] != '/')
        	return path;

before this strncmp() for it to work, don't you?

In addition, by comparing (len - 1) excluding the trailing slash of
prefix, I think you would let

	path      = /a/bcye

slip through as well.  That is inside the work_tree but outside of your
prefix.

> +		fprintf(stderr,"prefix mismatch\n");

Stray debugging fprintf.

> +		char *np;
> +		int i;
> +		int d=0;

Style "d = 0" (and "decl after statement").

> +		for (i = 0; i < len; ++i)

Style.  Distracts the reader by forcing him to wonder needlessly if
there is a particular reason for pre-increment of i instead of the usual
post-increment.

> +			if (prefix[i] == '/')
> +				d++;
> +		np = xmalloc(strlen(path + n) + d * 3 + 1);

At this point (assuming that the above if (strncmp()) rejected the path
outside the prefix correctly), we know that we would need to go d levels
up to reach the top of the work tree.

> +		for (i=0; i < d * 3; i += 3)

Style. "i = 0".

> +			strcpy(np + i, "../");
> +		strcpy(np + i, path + n + 1);

As path+n+1 is relative to the work tree, this will make it relative,
which is good.

> +		path = np;
> +		return np;
> +	}

Assuming the if (strncmp()) above correctly handled the path outside
prefix, we are dealing with the path that is inside prefix at this
point.  (len+n) is the length of the prefix directory expressed as an
absolute path.

> +	if (path[len + n] == '/')
> +		return path + len + n + 1;

So strip the absolute prefix would make the result relative to the
prefix directory.  Nice.

> +	else
> +		if (path[len + n])
> +			return path;

The same comment on "else if" applies.  path[len+n] was not slash so
path was not inside the prefix after all.  Oops?  The "if outside
prefix we uplevel with ../" logic above should have handled this case
and we should not be here.

> +		else
> +			return path + len + n;
> +}

path[len+n] was NUL, which means taht the user named the prefix
directory, and we return "".

Isn't this _overly_ complicated?  I think what this function wants to do
is:

 * See if path is outside the work tree, and return absolute if so.

 * Come up with the absolute path for the prefix (if NULL then that is
   the same as work tree) directory, without a trailing slash, and call
   it X.

 * Is path the same as the X?  If so, "" is what you want.

 * Is path a prefix of the "X/"?  If so strip "X/" and return.

 * Find the longuest common leading directory of path and "X/" and call
   it "C/".  Note that this is guaranteed to be inside work tree because
   we rejected paths outside work tree upfront.

 * Count slashes between "C/" and "X/" and come up with necessary
   uplevel "../".  Strip "C/" from path and prepend the uplevel.

[PATCH] Make Git accept absolute path names for files within the work tree

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---

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