[PATCH 0/2] rebase -i --root cleanups

[PATCH 0/2] rebase -i --root cleanups

[Johannes Schindelin]

This is just the first part of my rebase revamp; it would be good to
get the trivial stuff out of the way early, so that I do not have to
send a large patch series nobody wants to read because of its size.

Thomas, these two patches happen to touch your --root realm, maybe you 
have comments.

Johannes Schindelin (2):
  rebase -i --root: simplify code
  rebase -i --root: fix check for number of arguments

 git-rebase--interactive.sh |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

[PATCH 1/2] rebase -i --root: simplify code

[Johannes Schindelin]

When we rebase with --root, what we are actually picking are the commits
in $ONTO..$HEAD.  So $ONTO is really our $UPSTREAM.  Spell it out.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 21ac20c..6e2bf25 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -581,13 +581,15 @@ first and then run 'git rebase --continue' again."
 		if test -z "$REBASE_ROOT"
 		then
 			UPSTREAM_ARG="$1"
-			UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
+			UPSTREAM=$(git rev-parse --verify "$1") ||
+				die "Invalid base"
 			test -z "$ONTO" && ONTO=$UPSTREAM
 			shift
 		else
 			UPSTREAM_ARG=--root
 			test -z "$ONTO" &&
 				die "You must specify --onto when using --root"
+			UPSTREAM=$ONTO
 		fi
 		run_pre_rebase_hook "$UPSTREAM_ARG" "$@"
 
@@ -648,16 +650,9 @@ first and then run 'git rebase --continue' again."
 
 		SHORTHEAD=$(git rev-parse --short $HEAD)
 		SHORTONTO=$(git rev-parse --short $ONTO)
-		if test -z "$REBASE_ROOT"
-			# this is now equivalent to ! -z "$UPSTREAM"
-		then
-			SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
-			REVISIONS=$UPSTREAM...$HEAD
-			SHORTREVISIONS=$SHORTUPSTREAM..$SHORTHEAD
-		else
-			REVISIONS=$ONTO...$HEAD
-			SHORTREVISIONS=$SHORTHEAD
-		fi
+		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
+		REVISIONS=$UPSTREAM...$HEAD
+		SHORTREVISIONS=$SHORTUPSTREAM..$SHORTHEAD
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
 			--abbrev=7 --reverse --left-right --topo-order \
 			$REVISIONS | \
-- 
1.6.1.482.g7d54be

[PATCH 2/2] rebase -i --root: fix check for number of arguments

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6e2bf25..5df35b2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -571,7 +571,8 @@ first and then run 'git rebase --continue' again."
 		;;
 	--)
 		shift
-		test ! -z "$REBASE_ROOT" -o $# -eq 1 -o $# -eq 2 || usage
+		test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 ||
+		test ! -z "$REBASE_ROOT" -a $# -le 1 || usage
 		test -d "$DOTEST" &&
 			die "Interactive rebase already started"
 
-- 
1.6.1.482.g7d54be

Re: [PATCH 1/2] rebase -i --root: simplify code

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

Johannes Schindelin wrote:
> 
> When we rebase with --root, what we are actually picking are the commits
> in $ONTO..$HEAD.  So $ONTO is really our $UPSTREAM.  Spell it out.
[...]
> +			UPSTREAM=$ONTO

While I think the simplification is reasonable, it breaks this check:

  get_saved_options () {
  # ...
          test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
  }

So you'll have to change that too.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] rebase -i --root: simplify code

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

Thomas Rast wrote:
>           test ! -s "$DOTEST"/upstream && REBASE_ROOT=t

Actually, I think that test never worked (and it's clearly my fault).

The corresponding 'echo $UPSTREAM > "$DOTEST"/upstream' just expanded
to 'echo > ...', resulting in a file containing a single newline, but
never a zero-length file.  Duh.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] rebase -i --root: fix check for number of arguments

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

Johannes Schindelin wrote:
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 6e2bf25..5df35b2 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -571,7 +571,8 @@ first and then run 'git rebase --continue' again."
>  		;;
>  	--)
>  		shift
> -		test ! -z "$REBASE_ROOT" -o $# -eq 1 -o $# -eq 2 || usage
> +		test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 ||
> +		test ! -z "$REBASE_ROOT" -a $# -le 1 || usage
>  		test -d "$DOTEST" &&
>  			die "Interactive rebase already started"

Acked-by: Thomas Rast <trast@student.ethz.ch>

I'll postpone 1/2 till I've had enough sleep to check whether
--continue ever needed to know about --root, and either remove or fix
the remembering.  (Sorry for the noise.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] rebase -i --root: simplify code

[Johannes Schindelin]

Hi,

On Mon, 26 Jan 2009, Thomas Rast wrote:

> Johannes Schindelin wrote:
> > 
> > When we rebase with --root, what we are actually picking are the commits
> > in $ONTO..$HEAD.  So $ONTO is really our $UPSTREAM.  Spell it out.
> [...]
> > +			UPSTREAM=$ONTO
> 
> While I think the simplification is reasonable, it breaks this check:
> 
>   get_saved_options () {
>   # ...
>           test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
>   }
> 
> So you'll have to change that too.

Will fix tomorrow.

Thanks,
Dscho

Re: [PATCH 2/2] rebase -i --root: fix check for number of arguments

[Johannes Schindelin]

Hi,

On Mon, 26 Jan 2009, Thomas Rast wrote:

> Johannes Schindelin wrote:
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  git-rebase--interactive.sh |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 6e2bf25..5df35b2 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -571,7 +571,8 @@ first and then run 'git rebase --continue' again."
> >  		;;
> >  	--)
> >  		shift
> > -		test ! -z "$REBASE_ROOT" -o $# -eq 1 -o $# -eq 2 || usage
> > +		test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 ||
> > +		test ! -z "$REBASE_ROOT" -a $# -le 1 || usage
> >  		test -d "$DOTEST" &&
> >  			die "Interactive rebase already started"
> 
> Acked-by: Thomas Rast <trast@student.ethz.ch>
> 
> I'll postpone 1/2 till I've had enough sleep to check whether
> --continue ever needed to know about --root, and either remove or fix
> the remembering.  (Sorry for the noise.)

It does need to remember, as pick_one wants to look at the parent to be 
able to fast-forward.

But the sane fix is to add a file "$DOTEST"/root in case we're running 
under --root.

Ciao,
Dscho

Re: [PATCH 1/2] rebase -i --root: simplify code

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Thomas Rast wrote:
>>           test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
>
> Actually, I think that test never worked (and it's clearly my fault).
>
> The corresponding 'echo $UPSTREAM > "$DOTEST"/upstream' just expanded
> to 'echo > ...', resulting in a file containing a single newline, but
> never a zero-length file.  Duh.

Since you never use the value stored in "$DOTEST/upstream" for anything
else anyway, how about doing something like this instead?  It would make
the meaning of the file used as a state variable much clearer.

It may break hooks and outside scripts that look at $DOTEST/upstream.  I
didn't check.

The hunk in the middle is to protect you against an environment variable
UPSTREAM the user may have before starting "rebase -i".  There could be
other state variables you added in recent commit d911d14 (rebase -i: learn
to rebase root commit, 2009-01-02) that needs similar protection.  Please
check.

 git-rebase--interactive.sh |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git c/git-rebase--interactive.sh w/git-rebase--interactive.sh
index 21ac20c..17cf0e5 100755
--- c/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -456,7 +456,7 @@ get_saved_options () {
 	test -d "$REWRITTEN" && PRESERVE_MERGES=t
 	test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 	test -f "$DOTEST"/verbose && VERBOSE=t
-	test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
+	test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
 }
 
 while test $# != 0
@@ -585,6 +585,7 @@ first and then run 'git rebase --continue' again."
 			test -z "$ONTO" && ONTO=$UPSTREAM
 			shift
 		else
+			UPSTREAM=
 			UPSTREAM_ARG=--root
 			test -z "$ONTO" &&
 				die "You must specify --onto when using --root"
@@ -611,7 +612,12 @@ first and then run 'git rebase --continue' again."
 			echo "detached HEAD" > "$DOTEST"/head-name
 
 		echo $HEAD > "$DOTEST"/head
-		echo $UPSTREAM > "$DOTEST"/upstream
+		case "$REBASE_ROOT" in
+		'')
+			rm -f "$DOTEST"/rebase-root ;;
+		*)
+			: >"$DOTEST"/rebase-root ;;
+		esac
 		echo $ONTO > "$DOTEST"/onto
 		test -z "$STRATEGY" || echo "$STRATEGY" > "$DOTEST"/strategy
 		test t = "$VERBOSE" && : > "$DOTEST"/verbose

[PATCH] rebase -i: correctly remember --root flag across --continue

[Thomas Rast]

From: Junio C Hamano <gitster@pobox.com>

d911d14 (rebase -i: learn to rebase root commit, 2009-01-02) tried to
remember the --root flag across a merge conflict in a broken way.
Introduce a flag file $DOTEST/rebase-root to fix and clarify.

While at it, also make sure $UPSTREAM is always initialized to guard
against existing values in the environment.

[tr: added tests]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Junio C Hamano wrote:
> Since you never use the value stored in "$DOTEST/upstream" for anything
> else anyway, how about doing something like this instead?  It would make
> the meaning of the file used as a state variable much clearer.

Yes, thanks, a patch precisely "like this" is in fact the right fix.

I came up with some tests that try a conflicted --root rebase of each
flavour, to guard against the problem in the future.  I wasn't
entirely sure how to shape this into a patch, but here's a version
that forges patch message and sign-off in your name.

Dscho, with that confusion cleared, you can add my Ack to your 1/2
(unchanged, though I'm afraid you'll get a textual conflict).


 git-rebase--interactive.sh |   10 ++++-
 t/t3412-rebase-root.sh     |   88 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 21ac20c..17cf0e5 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -456,7 +456,7 @@ get_saved_options () {
 	test -d "$REWRITTEN" && PRESERVE_MERGES=t
 	test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 	test -f "$DOTEST"/verbose && VERBOSE=t
-	test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
+	test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
 }
 
 while test $# != 0
@@ -585,6 +585,7 @@ first and then run 'git rebase --continue' again."
 			test -z "$ONTO" && ONTO=$UPSTREAM
 			shift
 		else
+			UPSTREAM=
 			UPSTREAM_ARG=--root
 			test -z "$ONTO" &&
 				die "You must specify --onto when using --root"
@@ -611,7 +612,12 @@ first and then run 'git rebase --continue' again."
 			echo "detached HEAD" > "$DOTEST"/head-name
 
 		echo $HEAD > "$DOTEST"/head
-		echo $UPSTREAM > "$DOTEST"/upstream
+		case "$REBASE_ROOT" in
+		'')
+			rm -f "$DOTEST"/rebase-root ;;
+		*)
+			: >"$DOTEST"/rebase-root ;;
+		esac
 		echo $ONTO > "$DOTEST"/onto
 		test -z "$STRATEGY" || echo "$STRATEGY" > "$DOTEST"/strategy
 		test t = "$VERBOSE" && : > "$DOTEST"/verbose
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 6359580..29bb6d0 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -184,4 +184,92 @@ test_expect_success 'pre-rebase hook stops rebase -i' '
 	test 0 = $(git rev-list other...stops2 | wc -l)
 '
 
+test_expect_success 'remove pre-rebase hook' '
+	rm -f .git/hooks/pre-rebase
+'
+
+test_expect_success 'set up a conflict' '
+	git checkout master &&
+	echo conflict > B &&
+	git add B &&
+	git commit -m conflict
+'
+
+test_expect_success 'rebase --root with conflict (first part)' '
+	git checkout -b conflict1 other &&
+	test_must_fail git rebase --root --onto master &&
+	git ls-files -u | grep "B$"
+'
+
+test_expect_success 'fix the conflict' '
+	echo 3 > B &&
+	git add B
+'
+
+cat > expect-conflict <<EOF
+6
+5
+4
+3
+conflict
+2
+1
+EOF
+
+test_expect_success 'rebase --root with conflict (second part)' '
+	git rebase --continue &&
+	git log --pretty=tformat:"%s" > conflict1 &&
+	test_cmp expect-conflict conflict1
+'
+
+test_expect_success 'rebase -i --root with conflict (first part)' '
+	git checkout -b conflict2 other &&
+	GIT_EDITOR=: test_must_fail git rebase -i --root --onto master &&
+	git ls-files -u | grep "B$"
+'
+
+test_expect_success 'fix the conflict' '
+	echo 3 > B &&
+	git add B
+'
+
+test_expect_success 'rebase -i --root with conflict (second part)' '
+	git rebase --continue &&
+	git log --pretty=tformat:"%s" > conflict2 &&
+	test_cmp expect-conflict conflict2
+'
+
+sed 's/#/ /g' > expect-conflict-p <<'EOF'
+*   Merge branch 'third' into other
+|\##
+| * 6
+* |   Merge branch 'side' into other
+|\ \##
+| * | 5
+* | | 4
+|/ /##
+* | 3
+|/##
+* conflict
+* 2
+* 1
+EOF
+
+test_expect_success 'rebase -i -p --root with conflict (first part)' '
+	git checkout -b conflict3 other &&
+	GIT_EDITOR=: test_must_fail git rebase -i -p --root --onto master &&
+	git ls-files -u | grep "B$"
+'
+
+test_expect_success 'fix the conflict' '
+	echo 3 > B &&
+	git add B
+'
+
+test_expect_success 'rebase -i -p --root with conflict (second part)' '
+	git rebase --continue &&
+	git log --graph --topo-order --pretty=tformat:"%s" > conflict3 &&
+	test_cmp expect-conflict-p conflict3
+'
+
 test_done
-- 
1.6.1.469.g6f3d5

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> d911d14 (rebase -i: learn to rebase root commit, 2009-01-02) tried to
> remember the --root flag across a merge conflict in a broken way.
> Introduce a flag file $DOTEST/rebase-root to fix and clarify.
>
> While at it, also make sure $UPSTREAM is always initialized to guard
> against existing values in the environment.
>
> [tr: added tests]
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> Junio C Hamano wrote:
>> Since you never use the value stored in "$DOTEST/upstream" for anything
>> else anyway, how about doing something like this instead?  It would make
>> the meaning of the file used as a state variable much clearer.
>
> Yes, thanks, a patch precisely "like this" is in fact the right fix.
>
> I came up with some tests that try a conflicted --root rebase of each
> flavour, to guard against the problem in the future.  I wasn't
> entirely sure how to shape this into a patch, but here's a version
> that forges patch message and sign-off in your name.
>
> Dscho, with that confusion cleared, you can add my Ack to your 1/2
> (unchanged, though I'm afraid you'll get a textual conflict).

Ok, so I'll queue this to 'master' as a bugfix.

Thanks.

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> +test_expect_success 'rebase -i --root with conflict (first part)' '
> +	git checkout -b conflict2 other &&
> +	GIT_EDITOR=: test_must_fail git rebase -i --root --onto master &&
> +	git ls-files -u | grep "B$"
> +'

Maybe I am misrecalling things but didn't we have reports from people on
some platforms that single-shot exporting of the environment like this one
does not work for them?

> +test_expect_success 'fix the conflict' '
> +	echo 3 > B &&
> +	git add B
> +'
> +
> +test_expect_success 'rebase -i --root with conflict (second part)' '
> +	git rebase --continue &&
> +	git log --pretty=tformat:"%s" > conflict2 &&
> +	test_cmp expect-conflict conflict2
> +'
> +
> +sed 's/#/ /g' > expect-conflict-p <<'EOF'
> +*   Merge branch 'third' into other
> +|\##
> +| * 6
> +* |   Merge branch 'side' into other
> +|\ \##
> +| * | 5
> +* | | 4
> +|/ /##
> +* | 3
> +|/##
> +* conflict
> +* 2
> +* 1
> +EOF

I do not like this very much.  Future improvements of the graph drawing
algorithm (one obvious "flaw" you are exposing by the above is that it has
trailing whitespaces that can be trimmed, and somebody else may be
inclined to fix) would break the expectation this test vector has.

Do you have to compare the topology this way, or are there other more
reliable ways?

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Jeff King]

On Mon, Jan 26, 2009 at 01:05:37PM -0800, Junio C Hamano wrote:

> > +test_expect_success 'rebase -i --root with conflict (first part)' '
> > +	git checkout -b conflict2 other &&
> > +	GIT_EDITOR=: test_must_fail git rebase -i --root --onto master &&
> > +	git ls-files -u | grep "B$"
> > +'
> 
> Maybe I am misrecalling things but didn't we have reports from people on
> some platforms that single-shot exporting of the environment like this one
> does not work for them?

I don't think you are misrecalling. The problem is with one-shot
variables and functional calls. See 09b78bc1.

-Peff

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Jeff King]

On Mon, Jan 26, 2009 at 04:09:42PM -0500, Jeff King wrote:

> > Maybe I am misrecalling things but didn't we have reports from people on
> > some platforms that single-shot exporting of the environment like this one
> > does not work for them?
> 
> I don't think you are misrecalling. The problem is with one-shot
> variables and functional calls. See 09b78bc1.

Err, _function_ calls. Need sleep badly.

-Peff

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > +	GIT_EDITOR=: test_must_fail git rebase -i --root --onto master &&
> 
> Maybe I am misrecalling things but didn't we have reports from people on
> some platforms that single-shot exporting of the environment like this one
> does not work for them?

Then that deserves a fix, though I should point out that the original
patches (now on master) have the same code in them.

[So many quirks and so little time!]

> > +sed 's/#/ /g' > expect-conflict-p <<'EOF'
> > +*   Merge branch 'third' into other
> > +|\##
> > +| * 6
> > +* |   Merge branch 'side' into other
> > +|\ \##
> > +| * | 5
> > +* | | 4
> > +|/ /##
> > +* | 3
> > +|/##
> > +* conflict
> > +* 2
> > +* 1
> > +EOF
> 
> I do not like this very much.  Future improvements of the graph drawing
> algorithm (one obvious "flaw" you are exposing by the above is that it has
> trailing whitespaces that can be trimmed, and somebody else may be
> inclined to fix) would break the expectation this test vector has.
> 
> Do you have to compare the topology this way, or are there other more
> reliable ways?

It would certainly be possible to test the SHA1 of the resulting
branch tip, but t/README says I shouldn't.  Or we could spell it out
as a series of 'parent of X is Y' and 'message of Y is foo' tests,
which seems rather messy.

The above approach at least has the advantage that a test failure due
to format change can be diagnosed very quickly just from the diff that
is shown with -v.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
> ...
>> +sed 's/#/ /g' > expect-conflict-p <<'EOF'
>> +*   Merge branch 'third' into other
>> +|\##
>> +| * 6
>> +* |   Merge branch 'side' into other
>> +|\ \##
>> +| * | 5
>> +* | | 4
>> +|/ /##
>> +* | 3
>> +|/##
>> +* conflict
>> +* 2
>> +* 1
>> +EOF
>
> I do not like this very much.  Future improvements of the graph drawing
> algorithm (one obvious "flaw" you are exposing by the above is that it has
> trailing whitespaces that can be trimmed, and somebody else may be
> inclined to fix) would break the expectation this test vector has.
>
> Do you have to compare the topology this way, or are there other more
> reliable ways?

Perhaps something like this.

 t/t3412-rebase-root.sh |   36 +++++++++++++++++++++---------------
 1 files changed, 21 insertions(+), 15 deletions(-)

diff --git i/t/t3412-rebase-root.sh w/t/t3412-rebase-root.sh
index 29bb6d0..2408cf8 100755
--- i/t/t3412-rebase-root.sh
+++ w/t/t3412-rebase-root.sh
@@ -240,19 +240,24 @@ test_expect_success 'rebase -i --root with conflict (second part)' '
 '
 
 sed 's/#/ /g' > expect-conflict-p <<'EOF'
-*   Merge branch 'third' into other
-|\##
-| * 6
-* |   Merge branch 'side' into other
-|\ \##
-| * | 5
-* | | 4
-|/ /##
-* | 3
-|/##
-* conflict
-* 2
-* 1
+commit conflict3 conflict3~1 conflict3^2
+Merge branch 'third' into other
+commit conflict3^2 conflict3~4
+6
+commit conflict3~1 conflict3~2 conflict3~1^2
+Merge branch 'side' into other
+commit conflict3~1^2 conflict3~3
+5
+commit conflict3~2 conflict3~3
+4
+commit conflict3~3 conflict3~4
+3
+commit conflict3~4 conflict3~5
+conflict
+commit conflict3~5 conflict3~6
+2
+commit conflict3~6
+1
 EOF
 
 test_expect_success 'rebase -i -p --root with conflict (first part)' '
@@ -268,8 +273,9 @@ test_expect_success 'fix the conflict' '
 
 test_expect_success 'rebase -i -p --root with conflict (second part)' '
 	git rebase --continue &&
-	git log --graph --topo-order --pretty=tformat:"%s" > conflict3 &&
-	test_cmp expect-conflict-p conflict3
+	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
+	git name-rev --stdin --name-only --refs=refs/heads/conflict3 >out &&
+	test_cmp expect-conflict-p out
 '
 
 test_done

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> It would certainly be possible to test the SHA1 of the resulting
> branch tip, but t/README says I shouldn't.

Correct.  The test should convert the SHA-1 back to some symbolic form
that is stable for comparison.

Re: [PATCH] rebase -i: correctly remember --root flag across --continue

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

Junio C Hamano wrote:
> Perhaps something like this.
> +	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
> +	git name-rev --stdin --name-only --refs=refs/heads/conflict3 >out &&

Clever.  I'll follow up with cleanup patches for the rest of the
test.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 1/2] t3412: clean up GIT_EDITOR usage

[Thomas Rast]

a6c7a27 (rebase -i: correctly remember --root flag across --continue,
2009-01-26) introduced a more portable GIT_EDITOR usage, but left the
old tests unchanged.

Since we never use the editor (all tests run the rebase script as
proposed by rebase -i), just disable it outright, which simplifies the
tests.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t3412-rebase-root.sh |   38 +++++++++++++-------------------------
 1 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 9fc528f..8a9154a 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -6,6 +6,10 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 '
 . ./test-lib.sh
 
+# we always run the interactive rebases unchanged, so just disable the editor
+GIT_EDITOR=:
+export GIT_EDITOR
+
 test_expect_success 'prepare repository' '
 	test_commit 1 A &&
 	test_commit 2 A &&
@@ -59,7 +63,7 @@ test_expect_success 'pre-rebase got correct input (2)' '
 
 test_expect_success 'rebase -i --root --onto <newbase>' '
 	git checkout -b work3 other &&
-	GIT_EDITOR=: git rebase -i --root --onto master &&
+	git rebase -i --root --onto master &&
 	git log --pretty=tformat:"%s" > rebased3 &&
 	test_cmp expect rebased3
 '
@@ -70,7 +74,7 @@ test_expect_success 'pre-rebase got correct input (3)' '
 
 test_expect_success 'rebase -i --root --onto <newbase> <branch>' '
 	git branch work4 other &&
-	GIT_EDITOR=: git rebase -i --root --onto master work4 &&
+	git rebase -i --root --onto master work4 &&
 	git log --pretty=tformat:"%s" > rebased4 &&
 	test_cmp expect rebased4
 '
@@ -81,7 +85,7 @@ test_expect_success 'pre-rebase got correct input (4)' '
 
 test_expect_success 'rebase -i -p with linear history' '
 	git checkout -b work5 other &&
-	GIT_EDITOR=: git rebase -i -p --root --onto master &&
+	git rebase -i -p --root --onto master &&
 	git log --pretty=tformat:"%s" > rebased5 &&
 	test_cmp expect rebased5
 '
@@ -111,7 +115,7 @@ EOF
 
 test_expect_success 'rebase -i -p with merge' '
 	git checkout -b work6 other &&
-	GIT_EDITOR=: git rebase -i -p --root --onto master &&
+	git rebase -i -p --root --onto master &&
 	git log --graph --topo-order --pretty=tformat:"%s" > rebased6 &&
 	test_cmp expect-side rebased6
 '
@@ -142,7 +146,7 @@ EOF
 
 test_expect_success 'rebase -i -p with two roots' '
 	git checkout -b work7 other &&
-	GIT_EDITOR=: git rebase -i -p --root --onto master &&
+	git rebase -i -p --root --onto master &&
 	git log --graph --topo-order --pretty=tformat:"%s" > rebased7 &&
 	test_cmp expect-third rebased7
 '
@@ -158,22 +162,14 @@ EOF
 
 test_expect_success 'pre-rebase hook stops rebase' '
 	git checkout -b stops1 other &&
-	(
-		GIT_EDITOR=:
-		export GIT_EDITOR
-		test_must_fail git rebase --root --onto master
-	) &&
+	test_must_fail git rebase --root --onto master &&
 	test "z$(git symbolic-ref HEAD)" = zrefs/heads/stops1
 	test 0 = $(git rev-list other...stops1 | wc -l)
 '
 
 test_expect_success 'pre-rebase hook stops rebase -i' '
 	git checkout -b stops2 other &&
-	(
-		GIT_EDITOR=:
-		export GIT_EDITOR
-		test_must_fail git rebase --root --onto master
-	) &&
+	test_must_fail git rebase --root --onto master &&
 	test "z$(git symbolic-ref HEAD)" = zrefs/heads/stops2
 	test 0 = $(git rev-list other...stops2 | wc -l)
 '
@@ -218,11 +214,7 @@ test_expect_success 'rebase --root with conflict (second part)' '
 
 test_expect_success 'rebase -i --root with conflict (first part)' '
 	git checkout -b conflict2 other &&
-	(
-		GIT_EDITOR=:
-		export GIT_EDITOR
-		test_must_fail git rebase -i --root --onto master
-	) &&
+	test_must_fail git rebase -i --root --onto master &&
 	git ls-files -u | grep "B$"
 '
 
@@ -260,11 +252,7 @@ EOF
 
 test_expect_success 'rebase -i -p --root with conflict (first part)' '
 	git checkout -b conflict3 other &&
-	(
-		GIT_EDITOR=:
-		export GIT_EDITOR
-		test_must_fail git rebase -i -p --root --onto master
-	) &&
+	test_must_fail git rebase -i -p --root --onto master &&
 	git ls-files -u | grep "B$"
 '
 
-- 
1.6.1.2.464.g6066

[PATCH 2/2] t3412: use log|name-rev instead of log --graph

[Thomas Rast]

Replace all 'git log --graph' calls for history verification with the
combination of 'git log ...| git name-rev' first introduced by a6c7a27
(rebase -i: correctly remember --root flag across --continue,
2009-01-26).  This should be less susceptible to format changes than
the --graph code.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t3412-rebase-root.sh |   65 ++++++++++++++++++++++++++++-------------------
 1 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 8a9154a..57a3cad 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -10,6 +10,12 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 GIT_EDITOR=:
 export GIT_EDITOR
 
+log_with_names () {
+	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
+	git name-rev --stdin --name-only --refs=refs/heads/$1
+}
+
+
 test_expect_success 'prepare repository' '
 	test_commit 1 A &&
 	test_commit 2 A &&
@@ -102,21 +108,25 @@ test_expect_success 'set up merge history' '
 	git merge side
 '
 
-sed 's/#/ /g' > expect-side <<'EOF'
-*   Merge branch 'side' into other
-|\##
-| * 5
-* | 4
-|/##
-* 3
-* 2
-* 1
+cat > expect-side <<'EOF'
+commit work6 work6~1 work6^2
+Merge branch 'side' into other
+commit work6^2 work6~2
+5
+commit work6~1 work6~2
+4
+commit work6~2 work6~3
+3
+commit work6~3 work6~4
+2
+commit work6~4
+1
 EOF
 
 test_expect_success 'rebase -i -p with merge' '
 	git checkout -b work6 other &&
 	git rebase -i -p --root --onto master &&
-	git log --graph --topo-order --pretty=tformat:"%s" > rebased6 &&
+	log_with_names work6 > rebased6 &&
 	test_cmp expect-side rebased6
 '
 
@@ -129,25 +139,29 @@ test_expect_success 'set up second root and merge' '
 	git merge third
 '
 
-sed 's/#/ /g' > expect-third <<'EOF'
-*   Merge branch 'third' into other
-|\##
-| * 6
-* |   Merge branch 'side' into other
-|\ \##
-| * | 5
-* | | 4
-|/ /##
-* | 3
-|/##
-* 2
-* 1
+cat > expect-third <<'EOF'
+commit work7 work7~1 work7^2
+Merge branch 'third' into other
+commit work7^2 work7~4
+6
+commit work7~1 work7~2 work7~1^2
+Merge branch 'side' into other
+commit work7~1^2 work7~3
+5
+commit work7~2 work7~3
+4
+commit work7~3 work7~4
+3
+commit work7~4 work7~5
+2
+commit work7~5
+1
 EOF
 
 test_expect_success 'rebase -i -p with two roots' '
 	git checkout -b work7 other &&
 	git rebase -i -p --root --onto master &&
-	git log --graph --topo-order --pretty=tformat:"%s" > rebased7 &&
+	log_with_names work7 > rebased7 &&
 	test_cmp expect-third rebased7
 '
 
@@ -263,8 +277,7 @@ test_expect_success 'fix the conflict' '
 
 test_expect_success 'rebase -i -p --root with conflict (second part)' '
 	git rebase --continue &&
-	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
-	git name-rev --stdin --name-only --refs=refs/heads/conflict3 >out &&
+	log_with_names conflict3 >out &&
 	test_cmp expect-conflict-p out
 '
 
-- 
1.6.1.2.464.g6066

Re: [PATCH 1/2] t3412: clean up GIT_EDITOR usage

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Thomas Rast wrote:

> a6c7a27 (rebase -i: correctly remember --root flag across --continue,
> 2009-01-26) introduced a more portable GIT_EDITOR usage, but left the
> old tests unchanged.
> 
> Since we never use the editor (all tests run the rebase script as
> proposed by rebase -i), just disable it outright, which simplifies the
> tests.
> 
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  t/t3412-rebase-root.sh |   38 +++++++++++++-------------------------
>  1 files changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 9fc528f..8a9154a 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -6,6 +6,10 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
>  '
>  . ./test-lib.sh
>  
> +# we always run the interactive rebases unchanged, so just disable the editor
> +GIT_EDITOR=:
> +export GIT_EDITOR
> +

According to my analysis, this is unneeded.  Just leave GIT_EDITOR alone 
in the whole test.

Ciao,
Dscho

Re: [PATCH 1/2] t3412: clean up GIT_EDITOR usage

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

Johannes Schindelin wrote:
> On Fri, 30 Jan 2009, Thomas Rast wrote:
> > +# we always run the interactive rebases unchanged, so just disable the editor
> > +GIT_EDITOR=:
> > +export GIT_EDITOR
> > +
> 
> According to my analysis, this is unneeded.  Just leave GIT_EDITOR alone 
> in the whole test.

You're right, test-lib.sh sets GIT_EDITOR= and EDITOR=:, so the above
is unneeded (but harmless).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]