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

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

[David Symonds]

Based on gitte's idea.

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

diff --git a/git-checkout.sh b/git-checkout.sh
index 8993920..b2c50aa 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -134,9 +134,10 @@ 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 "$(git-rev-parse --show-cdup)" &&
+		 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.5.1529.g69a1-dirty

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

[Junio C Hamano]

David Symonds <dsymonds@gmail.com> writes:

> diff --git a/git-checkout.sh b/git-checkout.sh
> index 8993920..b2c50aa 100755
> --- a/git-checkout.sh
> +++ b/git-checkout.sh
> @@ -134,9 +134,10 @@ 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 "$(git-rev-parse --show-cdup)" &&
> +		 git checkout-index -f -u --stdin)

Have you tested this patch from the toplevel of any tree, where
"git-rev-parse --show-cdup" would yield an empty string?

I also wonder how this patch (with an obvious fix to address the
above point) would interact with GIT_DIR and/or GIT_WORK_TREE in
the environment.

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

[David Symonds]

On Nov 8, 2007 7:30 PM, Junio C Hamano <junio@pobox.com> wrote:
>
> Have you tested this patch from the toplevel of any tree, where
> "git-rev-parse --show-cdup" would yield an empty string?

No, I didn't. Arguably, "git-rev-parse --show-cdup" should always
return a path to the top-level, which would make this kind of
construction much simpler. Is there anything that relies upon it
returning the empty string when you're in the top-level directory?

> I also wonder how this patch (with an obvious fix to address the
> above point) would interact with GIT_DIR and/or GIT_WORK_TREE in
> the environment.

No idea. I'm still learning my way around the git codebase, so I was
hoping for some review and feedback from more experienced Gits.


Dave.

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

[Johannes Schindelin]

Hi,

On Fri, 9 Nov 2007, David Symonds wrote:

> On Nov 8, 2007 7:30 PM, Junio C Hamano <junio@pobox.com> wrote:
> >
> > Have you tested this patch from the toplevel of any tree, where
> > "git-rev-parse --show-cdup" would yield an empty string?
> 
> No, I didn't. Arguably, "git-rev-parse --show-cdup" should always return 
> a path to the top-level, which would make this kind of construction much 
> simpler.

As it is, we have a convenience function for this, to make it much 
simpler: cd_to_toplevel in git-sh-setup.

> > I also wonder how this patch (with an obvious fix to address the above 
> > point) would interact with GIT_DIR and/or GIT_WORK_TREE in the 
> > environment.
> 
> No idea. I'm still learning my way around the git codebase, so I was
> hoping for some review and feedback from more experienced Gits.

It _should_ work with GIT_DIR/GIT_WORK_TREE, as the full name is relative 
to the project root, and that's exactly where cd_to_toplevel is jumping 
to.  And the first call to ls-files should make certain that the paths are 
correct, so there should be no confusion either.

But yes, I have been burnt by that work tree stuff too many times, so I'd 
appreciate tests for that (both positive and negative ones).

Ciao,
Dscho