[PATCH 0/4] Re-fix get_pathspec()

[PATCH 0/4] Re-fix get_pathspec()

[Junio C Hamano]

After 1.5.4, we improved get_pathspec() to allow absolute paths, because
people wanted to cut&paste from file managers and such, so that you can
feed absolute paths to "git add" and friends.

However, the paths still must be inside work tree, so there must be a way
to ensure that.  d089eba (setup: sanitize absolute and funny paths in
get_pathspec()) botched the interface, by making the check the
responsibility of the callers, while keeping the interface to the function
intact, which meant that the callers needed to count returned pathspecs
and compared it with the number of paths fed to the function.

This miniseries cleans up the interface by making get_pathspec() to die
again.

Junio C Hamano (4):
  get_pathspec(): die when an out-of-tree path is given
  Revert part of 744dacd (builtin-mv: minimum fix to avoid losing files)
  Revert part of 1abf095 (git-add: adjust to the get_pathspec() changes)
  Revert part of d089eba (setup: sanitize absolute and funny paths in
    get_pathspec())

 builtin-add.c              |   12 ------------
 builtin-ls-files.c         |   11 +----------
 builtin-mv.c               |    6 +-----
 setup.c                    |    2 ++
 t/t3101-ls-tree-dirname.sh |    2 +-
 t/t7010-setup.sh           |    7 ++++---
 6 files changed, 9 insertions(+), 31 deletions(-)

[PATCH 1/4] get_pathspec(): die when an out-of-tree path is given

[Junio C Hamano]

An earlier commit d089ebaa (setup: sanitize absolute and funny paths) made
get_pathspec() aware of absolute paths, but with a botched interface that
forced the callers to count the resulting pathspecs in order to detect
an error of giving a path that is outside the work tree.

This fixes it, by dying inside the function.

We had ls-tree test that relied on a misfeature in the original
implementation of its pathspec handling.  Leading slashes were silently
removed from them.  However we allow giving absolute pathnames (people
want to cut and paste from elsewhere) that are inside work tree these
days, so a pathspec that begin with slash _should_ be treated as a full
path.  The test is adjusted to match the updated rule for get_pathspec().

Earlier I mistook three tests given by Robin that they should succeed, but
these are attempts to add path outside work tree, which should fail
loudly.  These tests also have been fixed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                    |    2 ++
 t/t3101-ls-tree-dirname.sh |    2 +-
 t/t7010-setup.sh           |    7 ++++---
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 89c81e5..c7b1080 100644
--- a/setup.c
+++ b/setup.c
@@ -202,6 +202,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 		const char *p = prefix_path(prefix, prefixlen, *src);
 		if (p)
 			*(dst++) = p;
+		else
+			exit(128); /* error message already given */
 		src++;
 	}
 	*dst = NULL;
diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 39fe267..70f9ce9 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -120,7 +120,7 @@ EOF
 # having 1.txt and path3
 test_expect_success \
     'ls-tree filter odd names' \
-    'git ls-tree $tree 1.txt /1.txt //1.txt path3/1.txt /path3/1.txt //path3//1.txt path3 /path3/ path3// >current &&
+    'git ls-tree $tree 1.txt ./1.txt .//1.txt path3/1.txt path3/./1.txt path3 path3// >current &&
      cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	path3/1.txt
diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index e809e0e..bc8ab6a 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -142,15 +142,16 @@ test_expect_success 'setup deeper work tree' '
 test_expect_success 'add a directory outside the work tree' '(
 	cd tester &&
 	d1="$(cd .. ; pwd)" &&
-	git add "$d1"
+	test_must_fail git add "$d1"
 )'
 
+
 test_expect_success 'add a file outside the work tree, nasty case 1' '(
 	cd tester &&
 	f="$(pwd)x" &&
 	echo "$f" &&
 	touch "$f" &&
-	git add "$f"
+	test_must_fail git add "$f"
 )'
 
 test_expect_success 'add a file outside the work tree, nasty case 2' '(
@@ -158,7 +159,7 @@ test_expect_success 'add a file outside the work tree, nasty case 2' '(
 	f="$(pwd | sed "s/.$//")x" &&
 	echo "$f" &&
 	touch "$f" &&
-	git add "$f"
+	test_must_fail git add "$f"
 )'
 
 test_done
-- 
1.5.4.3.587.g0bdd73

Re: [PATCH 0/4] Re-fix get_pathspec()

[Johannes Schindelin]

Hi,

On Fri, 7 Mar 2008, Junio C Hamano wrote:

> After 1.5.4, we improved get_pathspec() to allow absolute paths, because
> people wanted to cut&paste from file managers and such, so that you can
> feed absolute paths to "git add" and friends.
> 
> However, the paths still must be inside work tree, so there must be a 
> way to ensure that.  d089eba (setup: sanitize absolute and funny paths 
> in get_pathspec()) botched the interface, by making the check the 
> responsibility of the callers, while keeping the interface to the 
> function intact, which meant that the callers needed to count returned 
> pathspecs and compared it with the number of paths fed to the function.
> 
> This miniseries cleans up the interface by making get_pathspec() to die
> again.

We have a few places where you have to pass a "die_on_error" flag.  Why 
not imitate that?

Ciao,
Dscho

Re: [PATCH 0/4] Re-fix get_pathspec()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 7 Mar 2008, Junio C Hamano wrote:
>
>> This miniseries cleans up the interface by making get_pathspec() to die
>> again.
>
> We have a few places where you have to pass a "die_on_error" flag.  Why 
> not imitate that?

Before get_pathspec() was made absolute-path capable, it died on a path
that is outside the work tree, so we know die() there is what the callers
have expected all along.  This miniseries is about fixing the earlier one
that broke that expectation, nothing more.

If enough callers want different behaviours on error, it probably is worth
an API enhancement so that the callers can choose to do their own thing,
but that is a separate topic I haven't seen the need for, and definitely
is not part of this fix.

Re: [PATCH 1/4] get_pathspec(): die when an out-of-tree path is given

[Robin Rosenberg]

My gcc refuses to cooperate, giving me SEGV at line 666 or combine-diff, so 
I'll ask instead of testing.

Do absolute paths to diff still work without exiting?

-- robin

Re: [PATCH 1/4] get_pathspec(): die when an out-of-tree path is given

[Junio C Hamano]

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

> My gcc refuses to cooperate, giving me SEGV at line 666 or combine-diff, so 
> I'll ask instead of testing.
>
> Do absolute paths to diff still work without exiting?

They should, as long as they name inside work tree paths.

As far as I know, the bolted-on "work on files outside" code does not feed
the paths to get_pathspec() (and it shouldn't --- that mode is not a git
operation to begin with, and should work even without having a work tree,
so there is no valid base to make the paths relative to).

But it needs to be checked.

Re: [PATCH 0/4] Re-fix get_pathspec()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> This miniseries cleans up the interface by making get_pathspec() to die
> again.

By the way, this is meant to also fix the breakage of "git clean" run with
a path outside the work tree.  With two patches from Dmitry:

 Make private quote_path() in wt-status.c available as quote_path_relative())
 git-clean: correct printing relative path)

on top of the series, the following tests now pass.

-- >8 --
git-clean: add tests for relative path

This adds tests for recent change by Dmitry to fix the report "git
clean" gives on removed paths, and also makes sure the command detects
paths that is outside working tree.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7300-clean.sh |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 4037142..afccfc9 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -89,6 +89,58 @@ test_expect_success 'git-clean with prefix' '
 	test -f build/lib.so
 
 '
+
+test_expect_success 'git-clean with relative prefix' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	would_clean=$(
+		cd docs &&
+		git clean -n ../src |
+		sed -n -e "s|^Would remove ||p"
+	) &&
+	test "$would_clean" = ../src/part3.c || {
+		echo "OOps <$would_clean>"
+		false
+	}
+'
+
+test_expect_success 'git-clean with absolute path' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	would_clean=$(
+		cd docs &&
+		git clean -n $(pwd)/../src |
+		sed -n -e "s|^Would remove ||p"
+	) &&
+	test "$would_clean" = ../src/part3.c || {
+		echo "OOps <$would_clean>"
+		false
+	}
+'
+
+test_expect_success 'git-clean with out of work tree relative path' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	(
+		cd docs &&
+		test_must_fail git clean -n ../..
+	)
+'
+
+test_expect_success 'git-clean with out of work tree absolute path' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	dd=$(cd .. && pwd) &&
+	(
+		cd docs &&
+		test_must_fail git clean -n $dd
+	)
+'
+
 test_expect_success 'git-clean -d with prefix and path' '
 
 	mkdir -p build docs src/feature &&
-- 
1.5.4.3.587.g0bdd73