[PATCH] git-checkout: Support relative paths containing "..".

[PATCH] git-checkout: Support relative paths containing "..".

[David Symonds]

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
 git-checkout.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-checkout.sh b/git-checkout.sh
index c00cedd..aa724ac 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -133,9 +133,9 @@ Did you intend to checkout '$@' which can not be resolved as commit?"
 	fi
 
 	# Make sure the request is about existing paths.
-	git ls-files --error-unmatch -- "$@" >/dev/null || exit
-	git ls-files -- "$@" |
-	git checkout-index -f -u --stdin
+	git ls-files --full-name --error-unmatch -- "$@" >/dev/null || exit
+	git ls-files --full-name -- "$@" |
+		(cd_to_toplevel && git checkout-index -f -u --stdin)
 
 	# Run a post-checkout hook -- the HEAD does not change so the
 	# current HEAD is passed in for both args
-- 
1.5.3.1

[PATCH] git-checkout: Test for relative path use.

[David Symonds]

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
	Test 5 in this series fails because of a bug in git-ls-files, where
		git-ls-files t/../
	(with or without --full-name) returns no files.

 t/t2008-checkout-subdir.sh |   79 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 79 insertions(+), 0 deletions(-)
 create mode 100755 t/t2008-checkout-subdir.sh

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
new file mode 100755
index 0000000..f226511
--- /dev/null
+++ b/t/t2008-checkout-subdir.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 David Symonds
+
+test_description='git checkout from subdirectories'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	echo "base" > file0 &&
+	git add file0 &&
+	mkdir dir1 &&
+	echo "hello" > dir1/file1 &&
+	git add dir1/file1 &&
+	mkdir dir2 &&
+	echo "bonjour" > dir2/file2 &&
+	git add dir2/file2 &&
+	test_tick &&
+	git commit -m "populate tree"
+
+'
+
+test_expect_success 'remove and restore with relative path' '
+
+	cd dir1 &&
+	rm ../file0 &&
+	git checkout HEAD -- ../file0 &&
+	test "base" = "$(cat ../file0)" &&
+	rm ../dir2/file2 &&
+	git checkout HEAD -- ../dir2/file2 &&
+	test "bonjour" = "$(cat ../dir2/file2)" &&
+	rm ../file0 ./file1 &&
+	git checkout HEAD -- .. &&
+	test "base" = "$(cat ../file0)" &&
+	test "hello" = "$(cat file1)" &&
+	cd -
+
+'
+
+test_expect_success 'checkout with empty prefix' '
+
+	rm file0 &&
+	git checkout HEAD -- file0 &&
+	test "base" = "$(cat file0)"
+
+'
+
+test_expect_success 'checkout with simple prefix' '
+
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1 &&
+	test "hello" = "$(cat dir1/file1)" &&
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1/file1 &&
+	test "hello" = "$(cat dir1/file1)"
+
+'
+
+test_expect_success 'checkout with complex relative path' '
+
+	rm file1 &&
+	git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
+
+'
+
+test_expect_failure 'relative path outside tree should fail' \
+	'git checkout HEAD -- ../../Makefile'
+
+test_expect_failure 'incorrect relative path to file should fail (1)' \
+	'git checkout HEAD -- ../file0'
+
+test_expect_failure 'incorrect relative path should fail (2)' \
+	'cd dir1 && git checkout HEAD -- ./file0'
+
+test_expect_failure 'incorrect relative path should fail (3)' \
+	'cd dir1 && git checkout HEAD -- ../../file0'
+
+test_done
-- 
1.5.3.1

Re: [PATCH] git-checkout: Test for relative path use.

[Junio C Hamano]

David Symonds <dsymonds@gmail.com> writes:

> Signed-off-by: David Symonds <dsymonds@gmail.com>
> ---
> 	Test 5 in this series fails because of a bug in git-ls-files, where
> 		git-ls-files t/../
> 	(with or without --full-name) returns no files.

Heh, you shouldn't do that ;-)

Seriously, that's a long standing limitation in the code, not to
deal with arbitrary combination of ups and downs, but I do not
think there is any fundamental reason to disallow something
like:

	cd Documentation && git ls-files --full-name ../t

Patches welcome.

Re: [PATCH] git-checkout: Test for relative path use.

[David Symonds]

On Nov 9, 2007 12:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Symonds <dsymonds@gmail.com> writes:
>
> > Signed-off-by: David Symonds <dsymonds@gmail.com>
> > ---
> >       Test 5 in this series fails because of a bug in git-ls-files, where
> >               git-ls-files t/../
> >       (with or without --full-name) returns no files.
>
> Heh, you shouldn't do that ;-)
>
> Seriously, that's a long standing limitation in the code, not to
> deal with arbitrary combination of ups and downs, but I do not
> think there is any fundamental reason to disallow something
> like:
>
>         cd Documentation && git ls-files --full-name ../t
>
> Patches welcome.

So you're otherwise happy with my tests, despite one of them
triggering an (unrelated to git-checkout) bug? Or would you prefer I
remove that particular failure from the tests and resend?


Dave.

Re: [PATCH] git-checkout: Test for relative path use.

[Junio C Hamano]

"David Symonds" <dsymonds@gmail.com> writes:

> On Nov 9, 2007 12:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Seriously, that's a long standing limitation in the code, not to
>> deal with arbitrary combination of ups and downs, but I do not
>> think there is any fundamental reason to disallow something
>> like:
>>
>>         cd Documentation && git ls-files --full-name ../t
>>
>> Patches welcome.
>
> So you're otherwise happy with my tests, despite one of them
> triggering an (unrelated to git-checkout) bug? Or would you prefer I
> remove that particular failure from the tests and resend?

Are you really asking my preference?  A patch to ls-files to
make the test pass is my preference, of course ;-).

Haven't read your tests, though, but I see capable people
already commented on the initial round so I do not expect it to
be problematic.

Re: [PATCH] git-checkout: Test for relative path use.

[David Symonds]

On Nov 9, 2007 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Are you really asking my preference?  A patch to ls-files to
> make the test pass is my preference, of course ;-).
>
> Haven't read your tests, though, but I see capable people
> already commented on the initial round so I do not expect it to
> be problematic.

I'm going after low-hanging fruit whilst I get familiar with Git's
internal structure. I can try to tackle this ls-files problem as a
separate thing.


Dave.

Re: [PATCH] git-checkout: Test for relative path use.

[Johannes Sixt]

David Symonds schrieb:
> +test_expect_success 'remove and restore with relative path' '
> +
> +	cd dir1 &&
> +	rm ../file0 &&
> +	git checkout HEAD -- ../file0 &&
> +	test "base" = "$(cat ../file0)" &&
> +	rm ../dir2/file2 &&
> +	git checkout HEAD -- ../dir2/file2 &&
> +	test "bonjour" = "$(cat ../dir2/file2)" &&
> +	rm ../file0 ./file1 &&
> +	git checkout HEAD -- .. &&
> +	test "base" = "$(cat ../file0)" &&
> +	test "hello" = "$(cat file1)" &&
> +	cd -

What if this test fails? Then the rest of the tests run from the wrong 
directory. You should put the test in parenthesis (and drop the cd -).

-- Hannes

Re: [PATCH] git-checkout: Test for relative path use.

[David Symonds]

On Nov 9, 2007 6:13 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> David Symonds schrieb:
> > +test_expect_success 'remove and restore with relative path' '
> > +
> > +     cd dir1 &&
> > +     rm ../file0 &&
> > +     git checkout HEAD -- ../file0 &&
> > +     test "base" = "$(cat ../file0)" &&
> > +     rm ../dir2/file2 &&
> > +     git checkout HEAD -- ../dir2/file2 &&
> > +     test "bonjour" = "$(cat ../dir2/file2)" &&
> > +     rm ../file0 ./file1 &&
> > +     git checkout HEAD -- .. &&
> > +     test "base" = "$(cat ../file0)" &&
> > +     test "hello" = "$(cat file1)" &&
> > +     cd -
>
> What if this test fails? Then the rest of the tests run from the wrong
> directory. You should put the test in parenthesis (and drop the cd -).

Looking at the existing tests which, when they change directories,
don't cd back to where they were; they "cd .." at the start of the
next test. I'll add a "cd .." to the relevant bits of my tests.


Dave.

[PATCH] git-checkout: Test for relative path use.

[David Symonds]

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
	Tests that change directories now change back at the start of the
	next test. I don't know what to do about that last test, though.

 t/t2008-checkout-subdir.sh |   81 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100755 t/t2008-checkout-subdir.sh

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
new file mode 100755
index 0000000..41e76c9
--- /dev/null
+++ b/t/t2008-checkout-subdir.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 David Symonds
+
+test_description='git checkout from subdirectories'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	echo "base" > file0 &&
+	git add file0 &&
+	mkdir dir1 &&
+	echo "hello" > dir1/file1 &&
+	git add dir1/file1 &&
+	mkdir dir2 &&
+	echo "bonjour" > dir2/file2 &&
+	git add dir2/file2 &&
+	test_tick &&
+	git commit -m "populate tree"
+
+'
+
+test_expect_success 'remove and restore with relative path' '
+
+	cd dir1 &&
+	rm ../file0 &&
+	git checkout HEAD -- ../file0 &&
+	test "base" = "$(cat ../file0)" &&
+	rm ../dir2/file2 &&
+	git checkout HEAD -- ../dir2/file2 &&
+	test "bonjour" = "$(cat ../dir2/file2)" &&
+	rm ../file0 ./file1 &&
+	git checkout HEAD -- .. &&
+	test "base" = "$(cat ../file0)" &&
+	test "hello" = "$(cat file1)"
+
+'
+
+# currently in dir1/
+test_expect_success 'checkout with empty prefix' '
+
+	cd .. &&
+	rm file0 &&
+	git checkout HEAD -- file0 &&
+	test "base" = "$(cat file0)"
+
+'
+
+test_expect_success 'checkout with simple prefix' '
+
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1 &&
+	test "hello" = "$(cat dir1/file1)" &&
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1/file1 &&
+	test "hello" = "$(cat dir1/file1)"
+
+'
+
+test_expect_success 'checkout with complex relative path' '
+
+	rm file1 &&
+	git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
+
+'
+
+test_expect_failure 'relative path outside tree should fail' \
+	'git checkout HEAD -- ../../Makefile'
+
+test_expect_failure 'incorrect relative path to file should fail (1)' \
+	'git checkout HEAD -- ../file0'
+
+test_expect_failure 'incorrect relative path should fail (2)' \
+	'cd dir1 && git checkout HEAD -- ./file0'
+
+# currently in dir1/
+test_expect_failure 'incorrect relative path should fail (3)' \
+	'git checkout HEAD -- ../../file0'
+
+test_done
-- 
1.5.3.1

Re: [PATCH] git-checkout: Test for relative path use.

[Junio C Hamano]

"David Symonds" <dsymonds@gmail.com> writes:

> Looking at the existing tests which, when they change directories,
> don't cd back to where they were; they "cd .." at the start of the
> next test. I'll add a "cd .." to the relevant bits of my tests.

Do not follow the bad examples, please.

Re: [PATCH] git-checkout: Test for relative path use.

[David Symonds]

On Nov 9, 2007 7:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > Looking at the existing tests which, when they change directories,
> > don't cd back to where they were; they "cd .." at the start of the
> > next test. I'll add a "cd .." to the relevant bits of my tests.
>
> Do not follow the bad examples, please.

So what would you prefer? Bracketing the whole test in parentheses
looks ugly, but I can do that if that's the only option. If I look at
t5510-fetch.sh (one of yours, Junio), there is no directory
restoration in the case of test failure, as in my original patch.

Perhaps test_ok_ and test_failure_ in test-lib.sh should restore the directory?


Dave.

Re: [PATCH] git-checkout: Test for relative path use.

[Junio C Hamano]

"David Symonds" <dsymonds@gmail.com> writes:

> So what would you prefer? Bracketing the whole test in parentheses
> looks ugly, but I can do that if that's the only option. If I look at
> t5510-fetch.sh (one of yours, Junio), there is no directory
> restoration in the case of test failure, as in my original patch.

Yes, that is what I was referring to as "bad examples".  The way
t4116 goes down to different directory do not look ugly to me.

Re: [PATCH] git-checkout: Test for relative path use.

[David Symonds]

On Nov 9, 2007 8:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > So what would you prefer? Bracketing the whole test in parentheses
> > looks ugly, but I can do that if that's the only option. If I look at
> > t5510-fetch.sh (one of yours, Junio), there is no directory
> > restoration in the case of test failure, as in my original patch.
>
> Yes, that is what I was referring to as "bad examples".  The way
> t4116 goes down to different directory do not look ugly to me.

Okay, thanks -- that's a useful example. I'll resend the patch shortly.


Dave.

[PATCH] git-checkout: Test for relative path use.

[David Symonds]

Signed-off-by: David Symonds <dsymonds@gmail.com>
---
 t/t2008-checkout-subdir.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100755 t/t2008-checkout-subdir.sh

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
new file mode 100755
index 0000000..98d8eb3
--- /dev/null
+++ b/t/t2008-checkout-subdir.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 David Symonds
+
+test_description='git checkout from subdirectories'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	echo "base" > file0 &&
+	git add file0 &&
+	mkdir dir1 &&
+	echo "hello" > dir1/file1 &&
+	git add dir1/file1 &&
+	mkdir dir2 &&
+	echo "bonjour" > dir2/file2 &&
+	git add dir2/file2 &&
+	test_tick &&
+	git commit -m "populate tree"
+
+'
+
+test_expect_success 'remove and restore with relative path' '
+
+	(
+		cd dir1 &&
+		rm ../file0 &&
+		git checkout HEAD -- ../file0 &&
+		test "base" = "$(cat ../file0)" &&
+		rm ../dir2/file2 &&
+		git checkout HEAD -- ../dir2/file2 &&
+		test "bonjour" = "$(cat ../dir2/file2)" &&
+		rm ../file0 ./file1 &&
+		git checkout HEAD -- .. &&
+		test "base" = "$(cat ../file0)" &&
+		test "hello" = "$(cat file1)"
+	)
+
+'
+
+test_expect_success 'checkout with empty prefix' '
+
+	rm file0 &&
+	git checkout HEAD -- file0 &&
+	test "base" = "$(cat file0)"
+
+'
+
+test_expect_success 'checkout with simple prefix' '
+
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1 &&
+	test "hello" = "$(cat dir1/file1)" &&
+	rm dir1/file1 &&
+	git checkout HEAD -- dir1/file1 &&
+	test "hello" = "$(cat dir1/file1)"
+
+'
+
+test_expect_success 'checkout with complex relative path' '
+
+	rm file1 &&
+	git checkout HEAD -- ../dir1/../dir1/file1 && test -f ./file1
+
+'
+
+test_expect_failure 'relative path outside tree should fail' \
+	'git checkout HEAD -- ../../Makefile'
+
+test_expect_failure 'incorrect relative path to file should fail (1)' \
+	'git checkout HEAD -- ../file0'
+
+test_expect_failure 'incorrect relative path should fail (2)' \
+	'( cd dir1 && git checkout HEAD -- ./file0 )'
+
+test_expect_failure 'incorrect relative path should fail (3)' \
+	'( cd dir1 && git checkout HEAD -- ../../file0 )'
+
+test_done
-- 
1.5.3.1

Re: [PATCH] git-checkout: Test for relative path use.

[Robin Rosenberg]

fredag 09 november 2007 skrev Junio C Hamano:
> David Symonds <dsymonds@gmail.com> writes:
> 
> > Signed-off-by: David Symonds <dsymonds@gmail.com>
> > ---
> > 	Test 5 in this series fails because of a bug in git-ls-files, where
> > 		git-ls-files t/../
> > 	(with or without --full-name) returns no files.
> 
> Heh, you shouldn't do that ;-)
> 
> Seriously, that's a long standing limitation in the code, not to
> deal with arbitrary combination of ups and downs, but I do not
> think there is any fundamental reason to disallow something
> like:
> 
> 	cd Documentation && git ls-files --full-name ../t
> 
> Patches welcome.

I'm for allowing it, but then it should really be all over, not just some arbitrary
command. Everywhere or not at all.

-- robin