[PATCH] allow git-am to run in a subdirectory

[PATCH] allow git-am to run in a subdirectory

[Jeff King]

We just move to the top of the tree and proceed. This
shouldn't break any existing callers, since the behavior was
previously disallowed.

Signed-off-by: Jeff King <peff@peff.net>
---
I can't imagine anyone would be confused by this behavior. The only
other behavior that would make sense is perhaps trying to apply from the
given prefix rather than moving to the toplevel, but that just seems
crazy and useless to me.

It will fail to find your mbox file if you specified a relative path
and it had to change directories. I don't know if that case is worth
handling.

My use case, btw, is that I have an "apply this patch" macro in my MUA.
For some messages, it's easy to guess "cd /my/git/repo && git am". But
for others, the repo is not obvious, so I apply in the current
directory. If I am at the root of the repo, it works fine. If I'm not,
then it fails annoyingly.

 git-am.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 2ecebc4..a2c6fea 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -2,6 +2,7 @@
 #
 # Copyright (c) 2005, 2006 Junio C Hamano
 
+SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git-am [options] <mbox>|<Maildir>...
@@ -25,6 +26,7 @@ skip            skip the current patch"
 . git-sh-setup
 set_reflog_action am
 require_work_tree
+cd_to_toplevel
 
 git var GIT_COMMITTER_IDENT >/dev/null || exit
 
-- 
1.5.4.3.422.g55194

Re: [PATCH] allow git-am to run in a subdirectory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I can't imagine anyone would be confused by this behavior.

As long as you do not lose sight of the mailbox parameter by chdir'ing
around, I am Ok with the patch.

... and after I started writing that, I find...

> ...
> It will fail to find your mbox file if you specified a relative path
> and it had to change directories. I don't know if that case is worth
> handling.

Hmmm ;-).

Re: [PATCH] allow git-am to run in a subdirectory

[Jeff King]

On Fri, Feb 29, 2008 at 11:38:55PM -0800, Junio C Hamano wrote:

> As long as you do not lose sight of the mailbox parameter by chdir'ing
> around, I am Ok with the patch.
> 
> ... and after I started writing that, I find...

Ugh. Below is a patch that saves the original pwd and prefixes it for
relative paths. However:

  - it probably doesn't correctly determine absolute versus relative
    paths on Windows. I don't think we have a solution for fixing this
    within shell scripts.

  - it will eat newlines in parameters names (actually, turning them
    into spaces)

The problem is that I need to turn the original "$@" into a new "$@"
that is correctly prefixed, which requires proper quoting. Please, spend
some of your shell guru points to show me how to do this correctly and
portably.

We could wait on doing the 'cd_to_toplevel' until after the mailsplit,
but then your .dotest will end up in a prefixed directory (and of course
we have a similar munging problem when we point .dotest to the right
spot).

Maybe I should just wait for a git-am rewrite in C. ;)

---
diff --git a/git-am.sh b/git-am.sh
index 2ecebc4..a5e0c43 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -2,6 +2,7 @@
 #
 # Copyright (c) 2005, 2006 Junio C Hamano
 
+SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git-am [options] <mbox>|<Maildir>...
@@ -25,6 +26,13 @@ skip            skip the current patch"
 . git-sh-setup
 set_reflog_action am
 require_work_tree
+orig_pwd=$(pwd)
+cd_to_toplevel
+if test "$(pwd)" = "$orig_pwd"; then
+	orig_pwd=
+else
+	orig_pwd="$orig_pwd/"
+fi
 
 git var GIT_COMMITTER_IDENT >/dev/null || exit
 
@@ -121,6 +129,21 @@ reread_subject () {
 	git stripspace <"$1" | sed -e 1q
 }
 
+shellquote() {
+	printf \'
+	printf "$1" | sed "s/'/\\'/g"
+	echo \'
+}
+
+handle_file_args() {
+	for i in "$@"; do
+		case "$i" in
+		/*) shellquote "$i";;
+		 *) shellquote "$orig_pwd$i";;
+		esac
+	done
+}
+
 prec=4
 dotest=.dotest sign= utf8=t keep= skip= interactive= resolved= binary=
 resolvemsg= resume=
@@ -148,7 +171,7 @@ do
 	--skip)
 		skip=t ;;
 	-d|--dotest)
-		shift; dotest=$1;;
+		shift; eval dotest=`handle_file_args "$1"` ;;
 	--resolvemsg)
 		shift; resolvemsg=$1 ;;
 	--whitespace)
@@ -163,6 +186,8 @@ do
 	shift
 done
 
+eval set -- `handle_file_args "$@"`
+
 # If the dotest directory exists, but we have finished applying all the
 # patches in them, clear it out.
 if test -d "$dotest" &&

Re: [PATCH] allow git-am to run in a subdirectory

[Junio C Hamano]

Sorry for making you do an extra work.  I hoped ;-) was explicit enough
but apparently it was not...

Re: [PATCH] allow git-am to run in a subdirectory

[Jeff King]

On Sat, Mar 01, 2008 at 12:15:39AM -0800, Junio C Hamano wrote:

> Sorry for making you do an extra work.  I hoped ;-) was explicit enough
> but apparently it was not...

Heh. I was a little uncomfortable submitting a patch that didn't totally
work anyway. Of course, I'm not sure the result is that much better, and
it's a lot more complex.

-Peff

Re: [PATCH] allow git-am to run in a subdirectory

[Jay Soffian]

On Sat, Mar 1, 2008 at 3:12 AM, Jeff King <peff@peff.net> wrote:
>  The problem is that I need to turn the original "$@" into a new "$@"
>  that is correctly prefixed, which requires proper quoting.

Perhaps rev-parse could be taught to optionally convert relative
paths to absolute?

j.

Re* [PATCH] allow git-am to run in a subdirectory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The problem is that I need to turn the original "$@" into a new "$@"
> that is correctly prefixed, which requires proper quoting.

Perhaps like this?

---
 git-am.sh            |   19 +++++++++++++
 t/t4150-am-subdir.sh |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index a2c6fea..de34636 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -24,6 +24,7 @@ r,resolved      to be used after a patch failure
 skip            skip the current patch"
 
 . git-sh-setup
+prefix=$(git rev-parse --show-prefix)
 set_reflog_action am
 require_work_tree
 cd_to_toplevel
@@ -206,6 +207,24 @@ else
 	# Start afresh.
 	mkdir -p "$dotest" || exit
 
+	if test -n "$prefix" && test $# != 0
+	then
+		first=t
+		for arg
+		do
+			test -n "$first" && {
+				set x
+				first=
+			}
+			case "$arg" in
+			/*)
+				set "$@" "$arg" ;;
+			*)
+				set "$@" "$prefix$arg" ;;
+			esac
+		done
+		shift
+	fi
 	git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||  {
 		rm -fr "$dotest"
 		exit 1
diff --git a/t/t4150-am-subdir.sh b/t/t4150-am-subdir.sh
new file mode 100755
index 0000000..929d2cb
--- /dev/null
+++ b/t/t4150-am-subdir.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='git am running from a subdirectory'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo hello >world &&
+	git add world &&
+	test_tick &&
+	git commit -m initial &&
+	git tag initial &&
+	echo goodbye >world &&
+	git add world &&
+	test_tick &&
+	git commit -m second &&
+	git format-patch --stdout HEAD^ >patchfile &&
+	: >expect
+'
+
+test_expect_success 'am regularly from stdin' '
+	git checkout initial &&
+	git am <patchfile &&
+	git diff master >actual &&
+	diff -u expect actual
+'
+
+test_expect_success 'am regularly from file' '
+	git checkout initial &&
+	git am patchfile &&
+	git diff master >actual &&
+	diff -u expect actual
+'
+
+test_expect_success 'am regularly from stdin in subdirectory' '
+	rm -fr subdir &&
+	git checkout initial &&
+	(
+		mkdir -p subdir &&
+		cd subdir &&
+		git am <../patchfile
+	) &&
+	git diff master>actual &&
+	diff -u expect actual
+'
+
+test_expect_success 'am regularly from file in subdirectory' '
+	rm -fr subdir &&
+	git checkout initial &&
+	(
+		mkdir -p subdir &&
+		cd subdir &&
+		git am ../patchfile
+	) &&
+	git diff master >actual &&
+	diff -u expect actual
+'
+
+test_expect_success 'am regularly from file in subdirectory with full path' '
+	rm -fr subdir &&
+	git checkout initial &&
+	P=$(pwd) &&
+	(
+		mkdir -p subdir &&
+		cd subdir &&
+		git am "$P/patchfile"
+	) &&
+	git diff master >actual &&
+	diff -u expect actual
+'
+
+test_done

Re: Re* [PATCH] allow git-am to run in a subdirectory

[Jeff King]

On Sun, Mar 02, 2008 at 10:46:31PM -0800, Junio C Hamano wrote:

> > The problem is that I need to turn the original "$@" into a new "$@"
> > that is correctly prefixed, which requires proper quoting.
> 
> Perhaps like this?

Yes. For some reason I didn't think of doing 'set "$@" "$new"' to
rebuild the list one by one.

> +	if test -n "$prefix" && test $# != 0
> +	then
> +		first=t
> +		for arg
> +		do
> +			test -n "$first" && {
> +				set x
> +				first=
> +			}
> +			case "$arg" in
> +			/*)
> +				set "$@" "$arg" ;;
> +			*)
> +				set "$@" "$prefix$arg" ;;
> +			esac
> +		done
> +		shift
> +	fi

This handles the mailbox parameters. The only other file parameter is
$dotest. If I do "cd t && git am --dotest=.foo", then should it be
"t/.foo" or ".foo"?

-Peff

Re: Re* [PATCH] allow git-am to run in a subdirectory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The only other file parameter is
> $dotest. If I do "cd t && git am --dotest=.foo", then should it be
> "t/.foo" or ".foo"?

That brings up an interesting point.

Have people actually used --dotest=<foo> ever?

As far as I know, I do not think it was useful in the real world.  Too
many things assumed that the spool directory for "am" is .dotest.

And I am very much tempted to say we should remove that option.

Re: Re* [PATCH] allow git-am to run in a subdirectory

[Johannes Schindelin]

Hi,

On Sun, 2 Mar 2008, Junio C Hamano wrote:

> Have people actually used --dotest=<foo> ever?
> 
> As far as I know, I do not think it was useful in the real world.  Too 
> many things assumed that the spool directory for "am" is .dotest.
> 
> And I am very much tempted to say we should remove that option.

Concur.  While at it, we could move both .dotest/ and .git/.dotest-merge/
to .git/dotest/, no?

I know, I was opposed to this change, but let's just see how many heads 
pop up all of a sudden on the mailing list, okay?

Ciao,
Dscho

Re: Re* [PATCH] allow git-am to run in a subdirectory

[Junio C Hamano]

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

> Concur.  While at it, we could move both .dotest/ and .git/.dotest-merge/
> to .git/dotest/, no?

We could, yes, while at it, no.