threads / patch / 14052

patch, 3 partst3404: slight improvements

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

## tl;dr

21 messages between Jun 20, 2008 and Jun 21, 2008. Diffs are folded; open one to read it.

replies: 20people: 6as markdown or json

Stephan Beyer· Jun 20, 2008, 02:45 UTC · lore

some small changes for rebase-i

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
Stephan Beyer· Jun 20, 2008, 02:45 UTC · re: Stephan Beyer · lore

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(-)
Show changes to t/t3404-rebase-interactive.sh +19 −10
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
Stephan Beyer· Jun 20, 2008, 02:45 UTC · re: Stephan Beyer · lore

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

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(-)
Show changes to git-rebase--interactive.sh +35 −26
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
Stephan Beyer· Jun 20, 2008, 02:45 UTC · re: Stephan Beyer · lore

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

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 git-rebase--interactive.sh |   58 ++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 23 deletions(-)
Show changes to git-rebase--interactive.sh +35 −23
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
Stephan Beyer· Jun 20, 2008, 05:48 UTC · re: Stephan Beyer · lore

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

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
Johannes Schindelin· Jun 20, 2008, 13:15 UTC · re: Stephan Beyer · lore

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

Hi,
I like the patch.  Just a couple comments:
On Fri, 20 Jun 2008, Stephan Beyer wrote:
Show 14 quoted lines
> 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?
Show 20 quoted lines
> +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.

Show 7 quoted lines
>  	''|-h)
>  		usage
>  		;;
> -	*)
> +	--)
> +		shift
> +		test $# -eq 1 -o $# -eq 2 || usage
I consider this a bugfix.  Thanks.
Show 8 quoted lines
> @@ -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

Stephan Beyer· Jun 20, 2008, 18:30 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to t/t3404-rebase-interactive.sh +24 −7
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
Stephan Beyer· Jun 20, 2008, 18:30 UTC · re: Stephan Beyer · lore

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

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(-)
Show changes to git-rebase--interactive.sh +48 −23
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
Brandon Casey· Jun 20, 2008, 18:48 UTC · re: Stephan Beyer · lore

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

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
Stephan Beyer· Jun 20, 2008, 19:00 UTC · re: Brandon Casey · lore

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

Hi,
Show 11 quoted lines
> > @@ -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
しらいしななこ· Jun 20, 2008, 22:18 UTC · re: Stephan Beyer · lore

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

Quoting Stephan Beyer <s-beyer@gmx.net>:
Show 15 quoted lines
> 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/
Stephan Beyer· Jun 21, 2008, 01:46 UTC · re: しらいしななこ · lore

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

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
Junio C Hamano· Jun 21, 2008, 09:46 UTC · re: Stephan Beyer · lore

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

Stephan Beyer <s-beyer@gmx.net> writes:
Show 7 quoted lines
>> > 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.

Stephan Beyer· Jun 21, 2008, 23:55 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to t/t3404-rebase-interactive.sh +34 −9
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
Stephan Beyer· Jun 21, 2008, 23:55 UTC · re: Stephan Beyer · lore

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

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(-)
Show changes to git-rebase--interactive.sh +48 −23
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
Johannes Sixt· Jun 20, 2008, 07:16 UTC · re: Stephan Beyer · lore

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

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 ...
Show 11 quoted lines
>  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
Stephan Beyer· Jun 20, 2008, 08:01 UTC · re: Johannes Sixt · lore

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

Hi,
On Fri, Jun 20, 2008 at 09:16:43AM +0200, Johannes Sixt wrote:
Show 22 quoted lines
> 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
Johannes Sixt· Jun 20, 2008, 08:17 UTC · re: Stephan Beyer · lore

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

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
Johannes Schindelin· Jun 20, 2008, 12:46 UTC · re: Stephan Beyer · lore

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

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

Stephan Beyer· Jun 20, 2008, 18:45 UTC · re: Johannes Schindelin · lore

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

On Fri, Jun 20, 2008 at 01:46:29PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote to Cc git@vger.kernel.org:
Show 9 quoted lines
> 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
Johannes Schindelin· Jun 20, 2008, 13:05 UTC · re: Stephan Beyer · lore

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

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".
Show 12 quoted lines
> 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

← back to recent threads