[PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Jay Soffian]

When git am is called w/o arguments, w/o a patch on stdin and the user hits
ctrl-c, it leaves behind a partially populated $dotest directory. After this
commit, it emits usage when called w/o arguments and w/o a patch on stdin.

Noticed by Sverre Rabbelier

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Change from v2: make Junio happy by no longer removing $dotest if git-am is
interupted while mailsplit is running.

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

diff --git a/git-am.sh b/git-am.sh
index b1c05c9..92a64b2 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -254,6 +254,7 @@ else
 		done
 		shift
 	fi
+	test $# = 0 && test -t 0 && usage
 	git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||  {
 		rm -fr "$dotest"
 		exit 1
-- 
1.6.1.224.gb56c

[PATCH v3 2/2] git-am: minor cleanups

[Jay Soffian]

Update usage statement to remove a no-longer supported option, and to hide two
options (one a no-op, one internal) unless --help-all is used.

Use "test -t 0" instead of "tty -s" to detect when stdin is a terminal. (test
-t 0 is used elsewhere in git-am and in other git shell scripts, tty -s is
not, and appears to be deprecated by POSIX)

Use "test ..." instead of "[ ... ]" and "die <msg>" instead of "echo <msg>
>&2; exit 1" to be consistent with rest of script.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Change from v2: mention who deprecated "tty -s" in commit message.

 git-am.sh |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 92a64b2..670fc02 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -8,9 +8,8 @@ OPTIONS_SPEC="\
 git am [options] [<mbox>|<Maildir>...]
 git am [options] (--resolved | --skip | --abort)
 --
-d,dotest=       (removed -- do not use)
 i,interactive   run interactively
-b,binary        (historical option -- no-op)
+b,binary*       (historical option -- no-op)
 3,3way          allow fall back on 3way merging if needed
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
@@ -24,7 +23,7 @@ resolvemsg=     override error message when patch failure occurs
 r,resolved      to be used after a patch failure
 skip            skip the current patch
 abort           restore the original branch and abort the patching operation.
-rebasing        (internal use for git-rebase)"
+rebasing*       (internal use for git-rebase)"
 
 . git-sh-setup
 prefix=$(git rev-parse --show-prefix)
@@ -204,7 +203,7 @@ then
 		# unreliable -- stdin could be /dev/null for example
 		# and the caller did not intend to feed us a patch but
 		# wanted to continue unattended.
-		tty -s
+		test -t 0
 		;;
 	*)
 		false
@@ -281,10 +280,7 @@ fi
 case "$resolved" in
 '')
 	files=$(git diff-index --cached --name-only HEAD --) || exit
-	if [ "$files" ]; then
-	   echo "Dirty index: cannot apply patches (dirty: $files)" >&2
-	   exit 1
-	fi
+	test "$files" && die "Dirty index: cannot apply patches (dirty: $files)"
 esac
 
 if test "$(cat "$dotest/utf8")" = t
-- 
1.6.1.224.gb56c

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Pieter de Bie]

On Jan 28, 2009, at 3:03 PM, Jay Soffian wrote:

> When git am is called w/o arguments, w/o a patch on stdin and the  
> user hits
> ctrl-c, it leaves behind a partially populated $dotest directory.  
> After this
> commit, it emits usage when called w/o arguments and w/o a patch on  
> stdin.

FWIW, I sometimes like to run 'git am', paste in a patch and hit ctrl-d.

I can probably retrain my finger to use 'git am -', but I'm not sure  
if that
works (after this patch)? At least it's not mentioned in the manpage.

- Pieter

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Jay Soffian]

On Wed, Jan 28, 2009 at 11:18 AM, Pieter de Bie <pdebie@ai.rug.nl> wrote:
> FWIW, I sometimes like to run 'git am', paste in a patch and hit ctrl-d.
>
> I can probably retrain my finger to use 'git am -', but I'm not sure if that
> works (after this patch)? At least it's not mentioned in the manpage.

"git am -" doesn't work (before or after this patch), but "cat | git am" will.

j.

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Jan 28, 2009 at 11:18 AM, Pieter de Bie <pdebie@ai.rug.nl> wrote:
>> FWIW, I sometimes like to run 'git am', paste in a patch and hit ctrl-d.
>>
>> I can probably retrain my finger to use 'git am -', but I'm not sure if that
>> works (after this patch)? At least it's not mentioned in the manpage.
>
> "git am -" doesn't work (before or after this patch), but "cat | git am" will.

Another approach we've taken in other places to avoid the "Huh?" that
triggered this thread is to do something like:

        if there is no argument
        then
		if reading from tty
	        then
	        	echo >&2 "Reading from terminal, waiting for input..."
		fi
                process stdin	
	else
        	for arg
                do
                	process $arg
		done
	fi

Unfortunately, this will invalidate your "check -t 0 and error out" patch,
but some people may find it easier to work with and more friendly.

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Jay Soffian]

On Wed, Jan 28, 2009 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Another approach we've taken in other places to avoid the "Huh?" that
> triggered this thread is to do something like:
>
>        if there is no argument
>        then
>                if reading from tty
>                then
>                        echo >&2 "Reading from terminal, waiting for input..."
>                fi
>                process stdin
>        else
>                for arg
>                do
>                        process $arg
>                done
>        fi
>
> Unfortunately, this will invalidate your "check -t 0 and error out" patch,
> but some people may find it easier to work with and more friendly.

Well perhaps we should just deal w/ctrl-c only and ignore the terminal
check altogether.

j.

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Sverre Rabbelier]

On Wed, Jan 28, 2009 at 19:26, Jay Soffian <jaysoffian@gmail.com> wrote:
> Well perhaps we should just deal w/ctrl-c only and ignore the terminal
> check altogether.

That would suit my needs; as long as doing "git am" and then ^C does
not change my worktree and .git directory. Doing a stray "git am
--abort" after aborting does not obliterate my worktree as it does now
(not nice!), even so, it will change where my branch is currently at!

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Jeff King]

On Wed, Jan 28, 2009 at 04:18:30PM +0000, Pieter de Bie wrote:

> FWIW, I sometimes like to run 'git am', paste in a patch and hit ctrl-d.

I have to admit, I usually am opposed to this sort of terminal DWIMmery
for exactly that reason. But I don't personally ever cut and paste into
git-am, so I was trying not to raise a fuss. ;)

-Peff

Re: [PATCH v3 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> When git am is called w/o arguments, w/o a patch on stdin and the user hits
> ctrl-c, it leaves behind a partially populated $dotest directory. After this
> commit, it emits usage when called w/o arguments and w/o a patch on stdin.
>
> Noticed by Sverre Rabbelier
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> Change from v2: make Junio happy by no longer removing $dotest if git-am is
> interupted while mailsplit is running.

Sorry, I think I was misleading.  I did not mean that "Also" change was a
bad thing to do.  It just did not logically belong to this "is it coming
from the tty?" issue.

While you are looking at "am", I have a complaint about a very rough and
dangerous edge around a relatively new "usability" feature.

A scenario goes like this.

 (1) You have a patch, try to apply it to one existing topic branch

	$ git checkout myTopic
	$ git am some-patch

 (2) You see it does not apply, you realize that the patch is more suited
     for another branch.

	$ git checkout anotherTopic
        $ git am

 (3) It still does not apply, you examine the patch, and find flaws in its
     logic and decide not to apply it at all anywhere.

	$ git am --abort

After this, the tip of anotherTopic is updated to where the tip of myTopic
was!

Being able to remove the state directory $GIT_DIR/rebase-apply from the
command line is very useful when you want to get a clean slate to run
another git-am session, and often it is handy to abort the application of
the whole series when you do so (the latter is done by resetting the
original branch back to the original commit).  But being able to switch
branches after starting application of patches from one mailbox is also
useful in practice, so at least the latter should not be done blindly when
the user asks for --abort.  We definitely need some safety here.

Perhaps....

 (1) If --abort is asked for and you are on a branch different from where
     you started your "git am" session from, error out without doing
     anything.

 (2) Add --clean that does what the current --abort does, except for the
     rewinding of the HEAD.

The second one may be useful regardless of the "rough and dangerous edge"
issue above.  The early parts of the series may be useful and you may want
to keep the result from their application, while discarding the remainder
of the series.

Hmm?