some small changes for rebase-i

some small changes for rebase-i

[Stephan Beyer]

Hi,

the following small patchset can be applied to "pu" (i.e. to
js/rebase-i-sequencer, which has been in "next" before it
has been rewound.)

The first adds some tests that seemed useful to me during early
sequencer prototype development,
the second is just one for the look and
the third makes rebase-i use OPTIONS_SPEC.

Regards,
  Stephan

[PATCH 1/3] t3404: slight improvements

[Stephan Beyer]

This patch adds some extra checks into some
test cases and changes "! git ..." into
"test_must_fail git".

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 t/t3404-rebase-interactive.sh |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e6f3fad..daba5fd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -107,7 +107,8 @@ chmod a+x fake-editor.sh
 
 test_expect_success 'no changes are a nop' '
 	git rebase -i F &&
-	test $(git rev-parse I) = $(git rev-parse HEAD)
+	test $(git rev-parse I) = $(git rev-parse HEAD) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2"
 '
 
 test_expect_success 'test the [branch] option' '
@@ -115,7 +116,9 @@ test_expect_success 'test the [branch] option' '
 	git rm file6 &&
 	git commit -m "stop here" &&
 	git rebase -i F branch2 &&
-	test $(git rev-parse I) = $(git rev-parse HEAD)
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
+	test $(git rev-parse I) = $(git rev-parse HEAD) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2"
 '
 
 test_expect_success 'rebase on top of a non-conflicting commit' '
@@ -123,7 +126,9 @@ test_expect_success 'rebase on top of a non-conflicting commit' '
 	git tag original-branch1 &&
 	git rebase -i branch2 &&
 	test file6 = $(git diff --name-only original-branch1) &&
-	test $(git rev-parse I) = $(git rev-parse HEAD~2)
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
+	test $(git rev-parse I) = $(git rev-parse HEAD~2) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1"
 '
 
 test_expect_success 'reflog for the branch shows state before rebase' '
@@ -155,9 +160,12 @@ EOF
 
 test_expect_success 'stop on conflicting pick' '
 	git tag new-branch1 &&
-	! git rebase -i master &&
+	test_must_fail git rebase -i master &&
+	test "$(git rev-parse HEAD~3)" = "$(git rev-parse master)" &&
 	test_cmp expect .git/.dotest-merge/patch &&
 	test_cmp expect2 file1 &&
+	test "$(git-diff --name-status |
+		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
 	test 4 = $(grep -v "^#" < .git/.dotest-merge/done | wc -l) &&
 	test 0 = $(grep -c "^[^#]" < .git/.dotest-merge/git-rebase-todo)
 '
@@ -165,6 +173,7 @@ test_expect_success 'stop on conflicting pick' '
 test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
 	! test -d .git/.dotest-merge
 '
 
@@ -331,7 +340,7 @@ test_expect_success 'interactive -t preserves tags' '
 test_expect_success '--continue tries to commit' '
 	git checkout to-be-rebased &&
 	test_tick &&
-	! git rebase -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue &&
@@ -342,7 +351,7 @@ test_expect_success '--continue tries to commit' '
 test_expect_success 'verbose flag is heeded, even after --continue' '
 	git reset --hard HEAD@{1} &&
 	test_tick &&
-	! git rebase -v -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -v -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	git rebase --continue > output &&
@@ -380,7 +389,7 @@ test_expect_success 'interrupted squash works as expected' '
 	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -398,10 +407,10 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
 	! FAKE_LINES="3 squash 1 2" git rebase -i HEAD~3 &&
 	(echo one; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -449,7 +458,7 @@ test_expect_success 'rebase a commit violating pre-commit' '
 	chmod a+x $PRE_COMMIT &&
 	echo "monde! " >> file1 &&
 	test_tick &&
-	! git commit -m doesnt-verify file1 &&
+	test_must_fail git commit -m doesnt-verify file1 &&
 	git commit -m doesnt-verify --no-verify file1 &&
 	test_tick &&
 	FAKE_LINES=2 git rebase -i HEAD~2
-- 
1.5.5.1.561.gd8556

[PATCH 2/3] rebase-i: slight internal improvements

[Stephan Beyer]

Add commit_message function to get the commit message
from a commit and other slight internal improvements.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 git-rebase--interactive.sh |   61 +++++++++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3f926d8..e8ac2ae 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -25,6 +25,7 @@ SQUASH_MSG="$DOTEST"/message-squash
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
+MARK_PREFIX='refs/rebase-marks'
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
 
@@ -33,7 +34,6 @@ mark the corrected paths with 'git add <paths>', and
 run 'git rebase --continue'"
 export GIT_CHERRY_PICK_HELP
 
-mark_prefix=refs/rebase-marks/
 
 warn () {
 	echo "$*" >&2
@@ -73,13 +73,18 @@ comment_for_reflog () {
 	esac
 }
 
+# Get commit message from commit $1
+commit_message () {
+	git cat-file commit "$1" | sed -e '1,/^$/d'
+}
+
 last_count=
 mark_action_done () {
-	sed -e 1q < "$TODO" >> "$DONE"
-	sed -e 1d < "$TODO" >> "$TODO".new
-	mv -f "$TODO".new "$TODO"
-	count=$(grep -c '^[^#]' < "$DONE")
-	total=$(($count+$(grep -c '^[^#]' < "$TODO")))
+	sed -e 1q "$TODO" >>"$DONE"
+	sed -e 1d "$TODO" >>"$TODO.new"
+	mv -f "$TODO.new" "$TODO"
+	count="$(grep -c '^[^#]' "$DONE")"
+	total="$(expr "$count" + "$(grep -c '^[^#]' "$TODO")")"
 	if test "$last_count" != "$count"
 	then
 		last_count=$count
@@ -88,16 +93,18 @@ mark_action_done () {
 	fi
 }
 
+# Generate message, patch and author script files
 make_patch () {
 	parent_sha1=$(git rev-parse --verify "$1"^) ||
 		die "Cannot get patch for $1^"
 	git diff-tree -p "$parent_sha1".."$1" > "$DOTEST"/patch
 	test -f "$DOTEST"/message ||
-		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
+		commit_message "$1" >"$DOTEST"/message
 	test -f "$DOTEST"/author-script ||
 		get_author_ident_from_commit "$1" > "$DOTEST"/author-script
 }
 
+# Generate a patch and die
 die_with_patch () {
 	make_patch "$1"
 	git rerere
@@ -105,9 +112,9 @@ die_with_patch () {
 }
 
 cleanup_before_quit () {
-	for ref in $(git for-each-ref --format='%(refname)' "${mark_prefix%/}")
+	for ref in $(git for-each-ref --format='%(refname)' "$MARK_PREFIX")
 	do
-		git update-ref -d "$ref" "$ref" || return 1
+		git update-ref -d "$ref" "$ref" || return
 	done
 	rm -rf "$DOTEST"
 }
@@ -118,7 +125,7 @@ die_abort () {
 }
 
 has_action () {
-	grep '^[^#]' "$1" >/dev/null
+	grep -q '^[^#]' "$1"
 }
 
 redo_merge () {
@@ -126,7 +133,7 @@ redo_merge () {
 	shift
 
 	eval "$(get_author_ident_from_commit $rm_sha1)"
-	msg="$(git cat-file commit $rm_sha1 | sed -e '1,/^$/d')"
+	msg="$(commit_message "$rm_sha1")"
 
 	if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
 		GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
@@ -167,37 +174,39 @@ nth_string () {
 }
 
 make_squash_message () {
-	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
-			< "$SQUASH_MSG" | sed -ne '$p')+1))
+	if test -f "$SQUASH_MSG"
+	then
+		count="$(($(sed -n -e 's/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p' \
+			"$SQUASH_MSG" | sed -n -e '$p')+1))"
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
 			/^$/d
-		}' <"$SQUASH_MSG"
+		}' "$SQUASH_MSG"
 	else
 		COUNT=2
 		echo "# This is a combination of two commits."
 		echo "# The first commit's message is:"
 		echo
-		git cat-file commit HEAD | sed -e '1,/^$/d'
+		commit_message HEAD
 	fi
 	echo
 	echo "# This is the $(nth_string $COUNT) commit message:"
 	echo
-	git cat-file commit $1 | sed -e '1,/^$/d'
+	commit_message "$1"
 }
 
 peek_next_command () {
-	sed -n "1s/ .*$//p" < "$TODO"
+	sed -n -e '1s/ .*$//p' "$TODO"
 }
 
+# If $1 is a mark, make a ref from it; otherwise keep it
 mark_to_ref () {
-	if expr "$1" : "^:[0-9][0-9]*$" >/dev/null
-	then
-		echo "$mark_prefix$(printf %d ${1#:})"
-	else
-		echo "$1"
-	fi
+	arg="$1"
+	ref="$(expr "$arg" : '^:0*\([0-9]\+\)$')"
+	test -n "$ref" &&
+		arg="$MARK_PREFIX/$ref"
+	printf '%s\n' "$arg"
+	unset arg ref
 }
 
 do_next () {
@@ -634,7 +643,7 @@ do
 		UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
 		test -z "$ONTO" && ONTO=$UPSTREAM
 
-		if test ! -z "$2"
+		if test -n "$2"
 		then
 			output git show-ref --verify --quiet "refs/heads/$2" ||
 				die "Invalid branchname: $2"
@@ -674,7 +683,7 @@ do
 				create_extended_todo_list
 		else
 			git rev-list --no-merges --reverse --pretty=oneline \
-				 $common_rev_list_opts | sed -n "s/^>/pick /p"
+				 $common_rev_list_opts | sed -n -e 's/^>/pick /p'
 		fi > "$TODO"
 
 		cat >> "$TODO" << EOF
-- 
1.5.5.1.561.gd8556

[PATCH 3/3] Make rebase--interactive use OPTIONS_SPEC

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 git-rebase--interactive.sh |   58 ++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e8ac2ae..aeb9628 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -10,10 +10,25 @@
 # The original idea comes from Eric W. Biederman, in
 # http://article.gmane.org/gmane.comp.version-control.git/22407
 
-USAGE='(--continue | --abort | --skip | [--preserve-merges] [--first-parent]
-	[--preserve-tags] [--verbose] [--onto <branch>] <upstream> [<branch>])'
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC='
+git-rebase -i [options] <upstream> [<branch>]
+git-rebase (--continue | --abort | --skip)
+--
+continue           Continue rebasing process
+abort              Abort rebasing process and restore original branch
+skip               Skip current patch and continue rebasing process
+p,preserve-merges  Try to recreate merges instead of ignoring
+t,preserve-tags    Update tags to the new commit object
+m,merge            Used anyways (no-op)
+i,interactive      Used anyways (no-op)
+onto=              Rebase onto given branch instead of upstream
+v,verbose          Display a diffstat of what changed upstream
+ When preserving merges:
+f,first-parent     Show only commits following the first parent of each commit
+s,strategy=        Use the given merge strategy
+'
 
-OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
@@ -25,6 +40,7 @@ SQUASH_MSG="$DOTEST"/message-squash
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
+ONTO=
 MARK_PREFIX='refs/rebase-marks'
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
@@ -584,7 +600,7 @@ do
 
 		output git reset --hard && do_rest
 		;;
-	-s|--strategy)
+	-s)
 		case "$#,$1" in
 		*,*=*)
 			STRATEGY="-s "$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
@@ -595,32 +611,36 @@ do
 			shift ;;
 		esac
 		;;
-	-m|--merge)
+	-m)
 		# we use merge anyway
 		;;
-	-C*)
-		die "Interactive rebase uses merge, so $1 does not make sense"
-		;;
-	-v|--verbose)
+	-v)
 		VERBOSE=t
 		;;
-	-p|--preserve-merges)
+	-p)
 		PRESERVE_MERGES=t
 		;;
-	-f|--first-parent)
+	-f)
 		FIRST_PARENT=t
 		PRESERVE_MERGES=t
 		;;
-	-t|--preserve-tags)
+	-t)
 		PRESERVE_TAGS=t
 		;;
-	-i|--interactive)
+	-i)
 		# yeah, we know
 		;;
+	--onto)
+		shift
+		ONTO=$(git rev-parse --verify "$1") ||
+			die "Does not point to a valid commit: $1"
+		;;
 	''|-h)
 		usage
 		;;
-	*)
+	--)
+		shift
+		test $# -eq 1 -o $# -eq 2 || usage
 		test -d "$DOTEST" &&
 			die "Interactive rebase already started"
 
@@ -629,15 +649,6 @@ do
 
 		comment_for_reflog start
 
-		ONTO=
-		case "$1" in
-		--onto)
-			ONTO=$(git rev-parse --verify "$2") ||
-				die "Does not point to a valid commit: $2"
-			shift; shift
-			;;
-		esac
-
 		require_clean_work_tree
 
 		UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
@@ -720,6 +731,7 @@ EOF
 			die_abort "Nothing to do"
 
 		output git checkout $ONTO && do_rest
+		exit 0
 		;;
 	esac
 	shift
-- 
1.5.5.1.561.gd8556

Re: [PATCH 3/3] Make rebase--interactive use OPTIONS_SPEC

[Stephan Beyer]

Ouch, here is a little mistake:
> +OPTIONS_SPEC='
> +git-rebase -i [options] <upstream> [<branch>]

Must be:
	OPTIONS_SPEC='git-rebase -i [options] <upstream> [<branch>]
...or the usage string will be empty.

Are there other reasons for a v2?

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 3/3] Make rebase--interactive use OPTIONS_SPEC

[Johannes Schindelin]

Hi,

I like the patch.  Just a couple comments:

On Fri, 20 Jun 2008, Stephan Beyer wrote:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e8ac2ae..aeb9628 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -10,10 +10,25 @@
>  # The original idea comes from Eric W. Biederman, in
>  # http://article.gmane.org/gmane.comp.version-control.git/22407
>  
> -USAGE='(--continue | --abort | --skip | [--preserve-merges] [--first-parent]
> -	[--preserve-tags] [--verbose] [--onto <branch>] <upstream> [<branch>])'
> +OPTIONS_KEEPDASHDASH=
> +OPTIONS_SPEC='
> +git-rebase -i [options] <upstream> [<branch>]
> +git-rebase (--continue | --abort | --skip)

Here, "-i" is not an error, but not required either, so it should be

+git-rebase [-i] (--continue | --abort | --skip)

> +--
> +continue           Continue rebasing process
> +abort              Abort rebasing process and restore original branch
> +skip               Skip current patch and continue rebasing process

I wonder if these options (which really should be subcommands) should not 
go below the options, for chronological reasons: you are likely to need 
the other options before --continue and friends.

> +p,preserve-merges  Try to recreate merges instead of ignoring

s/$/ them/

> +t,preserve-tags    Update tags to the new commit object
> +m,merge            Used anyways (no-op)
> +i,interactive      Used anyways (no-op)

s/anyways/always/

Hmm?

> +onto=              Rebase onto given branch instead of upstream
> +v,verbose          Display a diffstat of what changed upstream
> + When preserving merges:
> +f,first-parent     Show only commits following the first parent of each commit
> +s,strategy=        Use the given merge strategy
> +'
>  
> -OPTIONS_SPEC=
>  . git-sh-setup
>  require_work_tree
>  
> @@ -595,32 +611,36 @@ do
>
> [...]
>
> +	--onto)
> +		shift
> +		ONTO=$(git rev-parse --verify "$1") ||
> +			die "Does not point to a valid commit: $1"
> +		;;

You probably need to introduce checks against "git rebase --continue 
--onto blabla", then.

>  	''|-h)
>  		usage
>  		;;
> -	*)
> +	--)
> +		shift
> +		test $# -eq 1 -o $# -eq 2 || usage

I consider this a bugfix.  Thanks.

> @@ -720,6 +731,7 @@ EOF
>  			die_abort "Nothing to do"
>  
>  		output git checkout $ONTO && do_rest
> +		exit 0
>  		;;
>  	esac
>  	shift

This exit is not really necessary, is it?  Besides, a simple "exit" would 
be more correct: it reuses the exit status of the most recently executed 
command, which is what we want here.

Thanks,
Dscho

[PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[Stephan Beyer]

This patch adds some extra checks (especially branch checks)
test cases and changes "! git ..." into "test_must_fail git".

Also a --onto test case is added.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Dscho,

as you've comman^Wwished ;-)
(except the analism)

Note that I've also added a --onto test.

 t/t3404-rebase-interactive.sh |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e6f3fad..96985ff 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -107,6 +107,7 @@ chmod a+x fake-editor.sh
 
 test_expect_success 'no changes are a nop' '
 	git rebase -i F &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
 	test $(git rev-parse I) = $(git rev-parse HEAD)
 '
 
@@ -115,14 +116,26 @@ test_expect_success 'test the [branch] option' '
 	git rm file6 &&
 	git commit -m "stop here" &&
 	git rebase -i F branch2 &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
 	test $(git rev-parse I) = $(git rev-parse HEAD)
 '
 
+test_expect_success 'test --onto <branch>' '
+	git checkout -b test-onto branch2 &&
+	git rebase -i --onto branch1 F &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
+	test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
+	test $(git rev-parse I) = $(git rev-parse branch2)
+'
+
 test_expect_success 'rebase on top of a non-conflicting commit' '
 	git checkout branch1 &&
 	git tag original-branch1 &&
 	git rebase -i branch2 &&
 	test file6 = $(git diff --name-only original-branch1) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
 	test $(git rev-parse I) = $(git rev-parse HEAD~2)
 '
 
@@ -155,9 +168,12 @@ EOF
 
 test_expect_success 'stop on conflicting pick' '
 	git tag new-branch1 &&
-	! git rebase -i master &&
+	test_must_fail git rebase -i master &&
+	test "$(git rev-parse HEAD~3)" = "$(git rev-parse master)" &&
 	test_cmp expect .git/.dotest-merge/patch &&
 	test_cmp expect2 file1 &&
+	test "$(git-diff --name-status |
+		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
 	test 4 = $(grep -v "^#" < .git/.dotest-merge/done | wc -l) &&
 	test 0 = $(grep -c "^[^#]" < .git/.dotest-merge/git-rebase-todo)
 '
@@ -165,6 +181,7 @@ test_expect_success 'stop on conflicting pick' '
 test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
 	! test -d .git/.dotest-merge
 '
 
@@ -331,7 +348,7 @@ test_expect_success 'interactive -t preserves tags' '
 test_expect_success '--continue tries to commit' '
 	git checkout to-be-rebased &&
 	test_tick &&
-	! git rebase -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue &&
@@ -342,7 +359,7 @@ test_expect_success '--continue tries to commit' '
 test_expect_success 'verbose flag is heeded, even after --continue' '
 	git reset --hard HEAD@{1} &&
 	test_tick &&
-	! git rebase -v -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -v -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	git rebase --continue > output &&
@@ -380,7 +397,7 @@ test_expect_success 'interrupted squash works as expected' '
 	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -398,10 +415,10 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
 	! FAKE_LINES="3 squash 1 2" git rebase -i HEAD~3 &&
 	(echo one; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -449,7 +466,7 @@ test_expect_success 'rebase a commit violating pre-commit' '
 	chmod a+x $PRE_COMMIT &&
 	echo "monde! " >> file1 &&
 	test_tick &&
-	! git commit -m doesnt-verify file1 &&
+	test_must_fail git commit -m doesnt-verify file1 &&
 	git commit -m doesnt-verify --no-verify file1 &&
 	test_tick &&
 	FAKE_LINES=2 git rebase -i HEAD~2
-- 
1.5.6.167.g86f2

[PATCH 2/2] Make rebase--interactive use OPTIONS_SPEC

[Stephan Beyer]

Also add some checks that --continue/--abort/--skip
actions are used without --onto, -p, -t, etc.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

Dscho wrote:
> You probably need to introduce checks against "git rebase --continue
> --onto blabla", then.

Now the is_standalone function does that.

Regards,
  Stephan

PS: I wondered that nobody moaned that the patch is for pu.

 git-rebase--interactive.sh |   71 +++++++++++++++++++++++++++++--------------
 1 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3f926d8..1894c37 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -10,10 +10,27 @@
 # The original idea comes from Eric W. Biederman, in
 # http://article.gmane.org/gmane.comp.version-control.git/22407
 
-USAGE='(--continue | --abort | --skip | [--preserve-merges] [--first-parent]
-	[--preserve-tags] [--verbose] [--onto <branch>] <upstream> [<branch>])'
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git-rebase [-i] [options] [--] <upstream> [<branch>]
+git-rebase [-i] (--continue | --abort | --skip)
+--
+ Available options are:
+p,preserve-merges  Try to recreate merges instead of ignoring them
+t,preserve-tags    Update tags to the new commit object
+m,merge            Always used (no-op)
+i,interactive      Always used (no-op)
+onto=              Rebase onto given branch instead of upstream
+v,verbose          Display a diffstat of what changed upstream
+ When preserving merges:
+f,first-parent     Show only commits following the first parent of each commit
+s,strategy=        Use the given merge strategy
+ Actions:
+continue           Continue rebasing process
+abort              Abort rebasing process and restore original branch
+skip               Skip current patch and continue rebasing process
+"
 
-OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
@@ -25,6 +42,8 @@ SQUASH_MSG="$DOTEST"/message-squash
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
+ONTO=
+MARK_PREFIX='refs/rebase-marks'
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
 
@@ -515,10 +534,19 @@ create_extended_todo_list () {
 	done
 }
 
+is_standalone () {
+	test $# -eq 2 -a "$2" = '--' &&
+	test -z "$ONTO" &&
+	test -z "$PRESERVE_TAGS" &&
+	test -z "$PRESERVE_MERGES" &&
+	test -z "$FIRST_PARENT"
+}
+
 while test $# != 0
 do
 	case "$1" in
 	--continue)
+		is_standalone "$@" || usage
 		comment_for_reflog continue
 
 		test -d "$DOTEST" || die "No interactive rebase running"
@@ -551,6 +579,7 @@ do
 		do_rest
 		;;
 	--abort)
+		is_standalone "$@" || usage
 		comment_for_reflog abort
 
 		git rerere clear
@@ -568,6 +597,7 @@ do
 		exit
 		;;
 	--skip)
+		is_standalone "$@" || usage
 		comment_for_reflog skip
 
 		git rerere clear
@@ -575,7 +605,7 @@ do
 
 		output git reset --hard && do_rest
 		;;
-	-s|--strategy)
+	-s)
 		case "$#,$1" in
 		*,*=*)
 			STRATEGY="-s "$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
@@ -586,32 +616,36 @@ do
 			shift ;;
 		esac
 		;;
-	-m|--merge)
+	-m)
 		# we use merge anyway
 		;;
-	-C*)
-		die "Interactive rebase uses merge, so $1 does not make sense"
-		;;
-	-v|--verbose)
+	-v)
 		VERBOSE=t
 		;;
-	-p|--preserve-merges)
+	-p)
 		PRESERVE_MERGES=t
 		;;
-	-f|--first-parent)
+	-f)
 		FIRST_PARENT=t
 		PRESERVE_MERGES=t
 		;;
-	-t|--preserve-tags)
+	-t)
 		PRESERVE_TAGS=t
 		;;
-	-i|--interactive)
+	-i)
 		# yeah, we know
 		;;
+	--onto)
+		shift
+		ONTO=$(git rev-parse --verify "$1") ||
+			die "Does not point to a valid commit: $1"
+		;;
 	''|-h)
 		usage
 		;;
-	*)
+	--)
+		shift
+		test $# -eq 1 -o $# -eq 2 || usage
 		test -d "$DOTEST" &&
 			die "Interactive rebase already started"
 
@@ -620,15 +654,6 @@ do
 
 		comment_for_reflog start
 
-		ONTO=
-		case "$1" in
-		--onto)
-			ONTO=$(git rev-parse --verify "$2") ||
-				die "Does not point to a valid commit: $2"
-			shift; shift
-			;;
-		esac
-
 		require_clean_work_tree
 
 		UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
-- 
1.5.6.167.g86f2

Re: [PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[Brandon Casey]

Stephan Beyer wrote:

> @@ -380,7 +397,7 @@ test_expect_success 'interrupted squash works as expected' '
>  	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&

These can be converted to use test_must_fail by using a sub-shell
as Junio demonstrated:

	(
		FAKE_LINES="1 squash 3 2" &&
		export FAKE_LINES &&
		test_must_fail git rebase -i HEAD~3
	) &&

-brandon

Re: [PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[Stephan Beyer]

Hi,

> > @@ -380,7 +397,7 @@ test_expect_success 'interrupted squash works as expected' '
> >  	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
> 
> These can be converted to use test_must_fail by using a sub-shell
> as Junio demonstrated:
> 
> 	(
> 		FAKE_LINES="1 squash 3 2" &&
> 		export FAKE_LINES &&
> 		test_must_fail git rebase -i HEAD~3
> 	) &&

Perhaps I'm not consequent, but I thought that it's not worth it ;-)

(There is also another reason: I use a dirty test-lib.sh _hack_ locally,
that allows me to exactly see where a failing test_expect_success patch
fails (that saves some time), but the hack does not work on tests with a
subshell invocation.)

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[しらいしななこ]

Quoting Stephan Beyer <s-beyer@gmx.net>:

> Hi,
>
>> > @@ -380,7 +397,7 @@ test_expect_success 'interrupted squash works as expected' '
>> >  	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
>> 
>> These can be converted to use test_must_fail by using a sub-shell
>> as Junio demonstrated:
>> 
>> 	(
>> 		FAKE_LINES="1 squash 3 2" &&
>> 		export FAKE_LINES &&
>> 		test_must_fail git rebase -i HEAD~3
>> 	) &&
>
> Perhaps I'm not consequent, but I thought that it's not worth it ;-)

Doesn't that logic make the other s/!/test_must_fail/ changes
also not worth it?  What is the reason behind the change?

I think your subject line and the message is worse than your
previous one.  You are saying *HOW* you changed it, without
saying *WHY* nor *WHAT FOR*.

I may have written your log message like this:

	Subject: t3404: tighten git-rebase tests

	In preparation for rewriting git-rebase in C, replace the way a
	failure is currently detected with "! git" to use test_must_fail
	so that we do not confuse a broken rebase that dumps core with a
	correctly failing one.

although I do not know if you are rewriting git-rebase in C
(^_^).  The point I learned from this project is to say why it
is done that way, not how you did it.  The latter can be seen in
the diff.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[Stephan Beyer]

Hi,

> > Perhaps I'm not consequent, but I thought that it's not worth it ;-)
> 
> Doesn't that logic make the other s/!/test_must_fail/ changes
> also not worth it?  What is the reason behind the change?

The s/!/test_must_fail/ is just an "extra" like
 "Hey, you're currently standing, can you bring me some tea?"

In this case:
 "Oh, I'm currently adding some tests, so I can s/!/test_must_fail/"

> I think your subject line and the message is worse than your
> previous one.  You are saying *HOW* you changed it,

Not exactly.
In the previous one I said, what my patch does: improve t3404.
The latter one said it, too, but a little more specific.

> without saying *WHY* nor *WHAT FOR*.

That's right.

The s/!/test_must_fail/ is, as I said, just an "extra".
And one that does no harm at all.

The others are tests that were useful during git sequencer prototype
development, because once a test in the middle of the test suite failed
because the branch was not correctly reset in one of the invocations of
rebase-i in the first tests.

Well, but I wonder if a long explanation is always necessary.
It is on feature patches and bugfix patches.  But here?

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 1/2] t3404: extra checks and s/! git/test_must_fail git/

[Junio C Hamano]

Stephan Beyer <s-beyer@gmx.net> writes:

>> > Perhaps I'm not consequent, but I thought that it's not worth it ;-)
>> 
>> Doesn't that logic make the other s/!/test_must_fail/ changes
>> also not worth it?  What is the reason behind the change?
>
> The s/!/test_must_fail/ is just an "extra" like
>  "Hey, you're currently standing, can you bring me some tea?"

Counting the places that were affected, I would not call which one is primary
change and which one is extra.  The later half of your patch is all about
test_must_fail isn't it?

I am all for making tests more careful, and I think more use of
test_must_fail makes quite a lot of sense.  Please don't do a half-ass job if
you are doing the conversion anyway.

About the commit log message, I tend to agree that your original subject
looked ugly and it would have been nicer to just say "t3404: more strict
tests for git-rebase" or something like that, but probably an empty commit
message body would have been Ok.

[PATCH v3 1/2] t3404: stricter tests for git-rebase--interactive

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

ok, next try.
Because I do not want to do a half-ass job, I changed
the remaining ! git to test_must_fail and changed the commit
message subject and removed the commit message body. ;-)

Again, this only applies cleanly to "pu".

 t/t3404-rebase-interactive.sh |   43 ++++++++++++++++++++++++++++++++--------
 1 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e6f3fad..940eb24 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -107,6 +107,7 @@ chmod a+x fake-editor.sh
 
 test_expect_success 'no changes are a nop' '
 	git rebase -i F &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
 	test $(git rev-parse I) = $(git rev-parse HEAD)
 '
 
@@ -115,14 +116,26 @@ test_expect_success 'test the [branch] option' '
 	git rm file6 &&
 	git commit -m "stop here" &&
 	git rebase -i F branch2 &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
 	test $(git rev-parse I) = $(git rev-parse HEAD)
 '
 
+test_expect_success 'test --onto <branch>' '
+	git checkout -b test-onto branch2 &&
+	git rebase -i --onto branch1 F &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
+	test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
+	test $(git rev-parse I) = $(git rev-parse branch2)
+'
+
 test_expect_success 'rebase on top of a non-conflicting commit' '
 	git checkout branch1 &&
 	git tag original-branch1 &&
 	git rebase -i branch2 &&
 	test file6 = $(git diff --name-only original-branch1) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
+	test $(git rev-parse I) = $(git rev-parse branch2) &&
 	test $(git rev-parse I) = $(git rev-parse HEAD~2)
 '
 
@@ -155,9 +168,12 @@ EOF
 
 test_expect_success 'stop on conflicting pick' '
 	git tag new-branch1 &&
-	! git rebase -i master &&
+	test_must_fail git rebase -i master &&
+	test "$(git rev-parse HEAD~3)" = "$(git rev-parse master)" &&
 	test_cmp expect .git/.dotest-merge/patch &&
 	test_cmp expect2 file1 &&
+	test "$(git-diff --name-status |
+		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
 	test 4 = $(grep -v "^#" < .git/.dotest-merge/done | wc -l) &&
 	test 0 = $(grep -c "^[^#]" < .git/.dotest-merge/git-rebase-todo)
 '
@@ -165,6 +181,7 @@ test_expect_success 'stop on conflicting pick' '
 test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
+	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
 	! test -d .git/.dotest-merge
 '
 
@@ -331,7 +348,7 @@ test_expect_success 'interactive -t preserves tags' '
 test_expect_success '--continue tries to commit' '
 	git checkout to-be-rebased &&
 	test_tick &&
-	! git rebase -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue &&
@@ -342,7 +359,7 @@ test_expect_success '--continue tries to commit' '
 test_expect_success 'verbose flag is heeded, even after --continue' '
 	git reset --hard HEAD@{1} &&
 	test_tick &&
-	! git rebase -v -i --onto new-branch1 HEAD^ &&
+	test_must_fail git rebase -v -i --onto new-branch1 HEAD^ &&
 	echo resolved > file1 &&
 	git add file1 &&
 	git rebase --continue > output &&
@@ -377,10 +394,14 @@ test_expect_success 'interrupted squash works as expected' '
 		git commit -m $n
 	done &&
 	one=$(git rev-parse HEAD~3) &&
-	! FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
+	(
+		FAKE_LINES="1 squash 3 2" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i HEAD~3
+	) &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -395,13 +416,17 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
 		git commit -m $n
 	done &&
 	one=$(git rev-parse HEAD~3) &&
-	! FAKE_LINES="3 squash 1 2" git rebase -i HEAD~3 &&
+	(
+		FAKE_LINES="3 squash 1 2" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i HEAD~3
+	) &&
 	(echo one; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
-	! git rebase --continue &&
+	test_must_fail git rebase --continue &&
 	echo resolved > conflict &&
 	git add conflict &&
 	git rebase --continue &&
@@ -449,7 +474,7 @@ test_expect_success 'rebase a commit violating pre-commit' '
 	chmod a+x $PRE_COMMIT &&
 	echo "monde! " >> file1 &&
 	test_tick &&
-	! git commit -m doesnt-verify file1 &&
+	test_must_fail git commit -m doesnt-verify file1 &&
 	git commit -m doesnt-verify --no-verify file1 &&
 	test_tick &&
 	FAKE_LINES=2 git rebase -i HEAD~2
-- 
1.5.6.310.g344d

[PATCH v3 2/2] Make rebase--interactive use OPTIONS_SPEC

[Stephan Beyer]

Also add some checks that --continue/--abort/--skip
actions are used without --onto, -p, -t, etc.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

I resent this patch, too, because I've changed something:
To make the OPTIONS_SPEC more consistent with other OPTIONS_SPECs,
I lowe-cased the first letters of the help string for each option.

Regards,
  Stephan


 git-rebase--interactive.sh |   71 +++++++++++++++++++++++++++++--------------
 1 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3f926d8..f13768c 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -10,10 +10,27 @@
 # The original idea comes from Eric W. Biederman, in
 # http://article.gmane.org/gmane.comp.version-control.git/22407
 
-USAGE='(--continue | --abort | --skip | [--preserve-merges] [--first-parent]
-	[--preserve-tags] [--verbose] [--onto <branch>] <upstream> [<branch>])'
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git-rebase [-i] [options] [--] <upstream> [<branch>]
+git-rebase [-i] (--continue | --abort | --skip)
+--
+ Available options are
+p,preserve-merges  try to recreate merges instead of ignoring them
+t,preserve-tags    update tags to the new commit object
+m,merge            always used (no-op)
+i,interactive      always used (no-op)
+onto=              rebase onto given branch instead of upstream
+v,verbose          display a diffstat of what changed upstream
+ When preserving merges
+f,first-parent     show only commits following the first parent of each commit
+s,strategy=        use the given merge strategy
+ Actions:
+continue           continue rebasing process
+abort              abort rebasing process and restore original branch
+skip               skip current patch and continue rebasing process
+"
 
-OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
@@ -25,6 +42,8 @@ SQUASH_MSG="$DOTEST"/message-squash
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
+ONTO=
+MARK_PREFIX='refs/rebase-marks'
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
 
@@ -515,10 +534,19 @@ create_extended_todo_list () {
 	done
 }
 
+is_standalone () {
+	test $# -eq 2 -a "$2" = '--' &&
+	test -z "$ONTO" &&
+	test -z "$PRESERVE_TAGS" &&
+	test -z "$PRESERVE_MERGES" &&
+	test -z "$FIRST_PARENT"
+}
+
 while test $# != 0
 do
 	case "$1" in
 	--continue)
+		is_standalone "$@" || usage
 		comment_for_reflog continue
 
 		test -d "$DOTEST" || die "No interactive rebase running"
@@ -551,6 +579,7 @@ do
 		do_rest
 		;;
 	--abort)
+		is_standalone "$@" || usage
 		comment_for_reflog abort
 
 		git rerere clear
@@ -568,6 +597,7 @@ do
 		exit
 		;;
 	--skip)
+		is_standalone "$@" || usage
 		comment_for_reflog skip
 
 		git rerere clear
@@ -575,7 +605,7 @@ do
 
 		output git reset --hard && do_rest
 		;;
-	-s|--strategy)
+	-s)
 		case "$#,$1" in
 		*,*=*)
 			STRATEGY="-s "$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
@@ -586,32 +616,36 @@ do
 			shift ;;
 		esac
 		;;
-	-m|--merge)
+	-m)
 		# we use merge anyway
 		;;
-	-C*)
-		die "Interactive rebase uses merge, so $1 does not make sense"
-		;;
-	-v|--verbose)
+	-v)
 		VERBOSE=t
 		;;
-	-p|--preserve-merges)
+	-p)
 		PRESERVE_MERGES=t
 		;;
-	-f|--first-parent)
+	-f)
 		FIRST_PARENT=t
 		PRESERVE_MERGES=t
 		;;
-	-t|--preserve-tags)
+	-t)
 		PRESERVE_TAGS=t
 		;;
-	-i|--interactive)
+	-i)
 		# yeah, we know
 		;;
+	--onto)
+		shift
+		ONTO=$(git rev-parse --verify "$1") ||
+			die "Does not point to a valid commit: $1"
+		;;
 	''|-h)
 		usage
 		;;
-	*)
+	--)
+		shift
+		test $# -eq 1 -o $# -eq 2 || usage
 		test -d "$DOTEST" &&
 			die "Interactive rebase already started"
 
@@ -620,15 +654,6 @@ do
 
 		comment_for_reflog start
 
-		ONTO=
-		case "$1" in
-		--onto)
-			ONTO=$(git rev-parse --verify "$2") ||
-				die "Does not point to a valid commit: $2"
-			shift; shift
-			;;
-		esac
-
 		require_clean_work_tree
 
 		UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
-- 
1.5.6.310.g344d

Re: [PATCH 2/3] rebase-i: slight internal improvements

[Johannes Sixt]

Stephan Beyer schrieb:
> Add commit_message function to get the commit message
> from a commit and other slight internal improvements.

If by "other slight improvements" you mean ...

>  mark_action_done () {
> -	sed -e 1q < "$TODO" >> "$DONE"
> -	sed -e 1d < "$TODO" >> "$TODO".new
> -	mv -f "$TODO".new "$TODO"
> -	count=$(grep -c '^[^#]' < "$DONE")
> -	total=$(($count+$(grep -c '^[^#]' < "$TODO")))
> +	sed -e 1q "$TODO" >>"$DONE"
> +	sed -e 1d "$TODO" >>"$TODO.new"
> +	mv -f "$TODO.new" "$TODO"
> +	count="$(grep -c '^[^#]' "$DONE")"
> +	total="$(expr "$count" + "$(grep -c '^[^#]' "$TODO")")"

... this ...

>  has_action () {
> -	grep '^[^#]' "$1" >/dev/null
> +	grep -q '^[^#]' "$1"

... and this, etc, then they are not improvements. They make the script
less portable: There are 'grep's that don't have -q, others write the file
name in front of the count, and I _think_ I have encountered 'sed's that
don't take a file name as argument.

This patch is just code churn for which you give no convincing reason in
the commit message why it is good.

-- Hannes

Re: [PATCH 2/3] rebase-i: slight internal improvements

[Stephan Beyer]

Hi,

On Fri, Jun 20, 2008 at 09:16:43AM +0200, Johannes Sixt wrote:
> If by "other slight improvements" you mean ...
> 
> >  mark_action_done () {
> > -	sed -e 1q < "$TODO" >> "$DONE"
> > -	sed -e 1d < "$TODO" >> "$TODO".new
> > -	mv -f "$TODO".new "$TODO"
> > -	count=$(grep -c '^[^#]' < "$DONE")
> > -	total=$(($count+$(grep -c '^[^#]' < "$TODO")))
> > +	sed -e 1q "$TODO" >>"$DONE"
> > +	sed -e 1d "$TODO" >>"$TODO.new"
> > +	mv -f "$TODO.new" "$TODO"
> > +	count="$(grep -c '^[^#]' "$DONE")"
> > +	total="$(expr "$count" + "$(grep -c '^[^#]' "$TODO")")"
> 
> ... this ...
> 
> >  has_action () {
> > -	grep '^[^#]' "$1" >/dev/null
> > +	grep -q '^[^#]' "$1"
> 
> ... and this, etc, then they are not improvements. They make the script
> less portable:

Ok, great to know.

> There are 'grep's that don't have -q,

Well, it's always a little hard to say what is understood by most of the
implementations.  POSIX says, there is grep -q and a few git test
scripts and the Makefile uses it, too.

Ok, so I've looked for a list "the least common denominator of shell
commands you can use to be portable" on the net and found the 
GNU Autoconf documentation:
http://www.gnu.org/software/autoconf/manual/autoconf-2.57/html_node/autoconf_123.html
Looks like *portable* shell programming is no fun :\

> others write the file name in front of the count,

I did not really change anything that's related to the count, btw.


Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 2/3] rebase-i: slight internal improvements

[Johannes Sixt]

Stephan Beyer schrieb:
> Looks like *portable* shell programming is no fun :\

> On Fri, Jun 20, 2008 at 09:16:43AM +0200, Johannes Sixt wrote:
>> others write the file name in front of the count,
> 
> I did not really change anything that's related to the count, btw.

You changed:

-	count=$(grep -c '^[^#]' < "$DONE")
+	count="$(grep -c '^[^#]' "$DONE")"

It turns out that GNU grep and AIX 4.3's grep don't write the file name if
only one name was given. Nevertheless, by passing the data on stdin we are
on the safe side. And, btw, the outer quotes are not needed in this
assignment.

-- Hannes

Re: [PATCH 2/3] rebase-i: slight internal improvements

[Johannes Schindelin]

Hi,

On Fri, 20 Jun 2008, Stephan Beyer wrote:

> Looks like *portable* shell programming is no fun :\

That is right.  That's one of the reasons why I prefer moving scripts to 
builtins: prototyping is good and well, but when you need to put it into 
production, where people have all kinds of weird setups (just think of 
dash in Ubuntu!), it is no fun.

Better to use something portable, such as C.

Which is the whole point of your project, right?  You want to turn the 
real engine into a builtin.

So would you not agree that PATCH 2/3 is rather unnecessary?

Ciao,
Dscho

Re: [PATCH 2/3] rebase-i: slight internal improvements

[Stephan Beyer]

On Fri, Jun 20, 2008 at 01:46:29PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote to Cc git@vger.kernel.org:
> Hi,
> 
> On Fri, 20 Jun 2008, Stephan Beyer wrote:
> 
> > Looks like *portable* shell programming is no fun :\
> 
> That is right.  That's one of the reasons why I prefer moving scripts to 
> builtins: prototyping is good and well, but when you need to put it into 
> production, where people have all kinds of weird setups 

Right.

> (just think of dash in Ubuntu!)

Well, I'm using dash as /bin/sh in Debian.
What's so weird about it?  IIRC it allows POSIX + some Berkeley extensions
and so it is far less weird as the least common demoninator of shell
portability ;-)

Hmm,
For shell portability it'd be cool to have something like a "badsh" (bad
shell) with the whole XCU Utilities as builtins without features and
warnings if features want to be used that are not supported by at least
95% of the systems.  So that scripts could be checked using badsh and
then you know, that it is portable.
I guess nobody ever wrote something like that. ;-)

> Better to use something portable, such as C.

Right.

> So would you not agree that PATCH 2/3 is rather unnecessary?

We wanted to make some upfront patches to am/rebase-i, so that, when the
git-sequencer prototype swoops in, it's easier to see, what is
taken from am and what is taken from rebase-i.
But this seems to be not so easy, so I'm currently thinking that I skip
that and concentrate on the builtin.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 1/3] t3404: slight improvements

[Johannes Schindelin]

Hi,

On Fri, 20 Jun 2008, Stephan Beyer wrote:

> This patch adds some extra checks into some
> test cases and changes "! git ..." into
> "test_must_fail git".

I think your oneline should read "t3404: be more anal".

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e6f3fad..daba5fd 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -107,7 +107,8 @@ chmod a+x fake-editor.sh
>  
>  test_expect_success 'no changes are a nop' '
>  	git rebase -i F &&
> -	test $(git rev-parse I) = $(git rev-parse HEAD)
> +	test $(git rev-parse I) = $(git rev-parse HEAD) &&
> +	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2"
>  '

You could have saved some diff-screen-estate by putting the extra check 
_before_ the original one.

Other than that, I think this patch is fine.

Ciao,
Dscho