# [PATCH 0/2] rebase -i --root cleanups 22 messages from 2009-01-25 to 2009-02-02. Participants: Johannes Schindelin, Thomas Rast, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/17361 ## Johannes Schindelin, 2009-01-25 23:31 Subject: [PATCH 0/2] rebase -i --root cleanups Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901260029480.14855%40racer ``` 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(-) ``` ## Johannes Schindelin, 2009-01-25 23:31 Subject: [PATCH 1/2] rebase -i --root: simplify code Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901260031460.14855%40racer In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2009-01-25 23:32 Subject: [PATCH 2/2] rebase -i --root: fix check for number of arguments Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901260032000.14855%40racer In-Reply-To: ``` Signed-off-by: Johannes Schindelin --- 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 ``` ## Thomas Rast, 2009-01-25 23:49 Subject: Re: [PATCH 1/2] rebase -i --root: simplify code Message-ID: <200901260049.25563.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901260049.25563.trast%40student.ethz.ch In-Reply-To: ``` 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 ``` ## Thomas Rast, 2009-01-25 23:53 Subject: Re: [PATCH 1/2] rebase -i --root: simplify code Message-ID: <200901260053.06315.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901260053.06315.trast%40student.ethz.ch In-Reply-To: <200901260049.25563.trast@student.ethz.ch> ``` 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 ``` ## Thomas Rast, 2009-01-26 00:07 Subject: Re: [PATCH 2/2] rebase -i --root: fix check for number of arguments Message-ID: <200901260108.07402.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901260108.07402.trast%40student.ethz.ch In-Reply-To: ``` Johannes Schindelin wrote: > > Signed-off-by: Johannes Schindelin > --- > 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 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 ``` ## Johannes Schindelin, 2009-01-26 00:44 Subject: Re: [PATCH 1/2] rebase -i --root: simplify code Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901260143540.14855%40racer In-Reply-To: <200901260049.25563.trast@student.ethz.ch> ``` 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 ``` ## Johannes Schindelin, 2009-01-26 00:49 Subject: Re: [PATCH 2/2] rebase -i --root: fix check for number of arguments Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901260148290.14855%40racer In-Reply-To: <200901260108.07402.trast@student.ethz.ch> ``` Hi, On Mon, 26 Jan 2009, Thomas Rast wrote: > Johannes Schindelin wrote: > > > > Signed-off-by: Johannes Schindelin > > --- > > 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 > > 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 ``` ## Junio C Hamano, 2009-01-26 05:54 Subject: Re: [PATCH 1/2] rebase -i --root: simplify code Message-ID: <7vtz7ma9z1.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtz7ma9z1.fsf%40gitster.siamese.dyndns.org In-Reply-To: <200901260053.06315.trast@student.ethz.ch> ``` Thomas Rast 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 ``` ## Thomas Rast, 2009-01-26 09:05 Subject: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <1232960722-17480-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1232960722-17480-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <7vtz7ma9z1.fsf@gitster.siamese.dyndns.org> ``` From: Junio C Hamano 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 Signed-off-by: Thomas Rast --- 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 < 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 ``` ## Junio C Hamano, 2009-01-26 20:47 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <7viqo14wwx.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7viqo14wwx.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1232960722-17480-1-git-send-email-trast@student.ethz.ch> ``` Thomas Rast writes: > From: Junio C Hamano > > 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 > Signed-off-by: Thomas Rast > --- > > 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. ``` ## Junio C Hamano, 2009-01-26 21:05 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <7veiyp4w2m.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7veiyp4w2m.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1232960722-17480-1-git-send-email-trast@student.ethz.ch> ``` Thomas Rast 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? ``` ## Jeff King, 2009-01-26 21:09 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <20090126210942.GH27604@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090126210942.GH27604%40coredump.intra.peff.net In-Reply-To: <7veiyp4w2m.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Jeff King, 2009-01-26 21:12 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <20090126211212.GB28354@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090126211212.GB28354%40coredump.intra.peff.net In-Reply-To: <20090126210942.GH27604@coredump.intra.peff.net> ``` 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 ``` ## Thomas Rast, 2009-01-26 21:28 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <200901262228.13104.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901262228.13104.trast%40student.ethz.ch In-Reply-To: <7veiyp4w2m.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Thomas Rast 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 ``` ## Junio C Hamano, 2009-01-26 21:49 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <7vzlhd3fh7.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzlhd3fh7.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7veiyp4w2m.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > Thomas Rast 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 ``` ## Junio C Hamano, 2009-01-27 00:29 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <7v7i4h382q.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7i4h382q.fsf%40gitster.siamese.dyndns.org In-Reply-To: <200901262228.13104.trast@student.ethz.ch> ``` Thomas Rast 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. ``` ## Thomas Rast, 2009-01-30 22:43 Subject: Re: [PATCH] rebase -i: correctly remember --root flag across --continue Message-ID: <200901302343.39921.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901302343.39921.trast%40student.ethz.ch In-Reply-To: <7vzlhd3fh7.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Thomas Rast, 2009-01-30 22:47 Subject: [PATCH 1/2] t3412: clean up GIT_EDITOR usage Message-ID: <1233355621-4783-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233355621-4783-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <200901302343.39921.trast@student.ethz.ch> ``` 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 --- 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 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 ' ' 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 ' ' 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 ``` ## Thomas Rast, 2009-01-30 22:47 Subject: [PATCH 2/2] t3412: use log|name-rev instead of log --graph Message-ID: <1233355621-4783-2-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1233355621-4783-2-git-send-email-trast%40student.ethz.ch In-Reply-To: <1233355621-4783-1-git-send-email-trast@student.ethz.ch> ``` 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 --- 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 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 ``` ## Johannes Schindelin, 2009-02-01 22:24 Subject: Re: [PATCH 1/2] t3412: clean up GIT_EDITOR usage Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902012322490.3586%40pacific.mpi-cbg.de In-Reply-To: <1233355621-4783-1-git-send-email-trast@student.ethz.ch> ``` 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 > --- > 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 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 ``` ## Thomas Rast, 2009-02-02 08:39 Subject: Re: [PATCH 1/2] t3412: clean up GIT_EDITOR usage Message-ID: <200902020939.24131.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902020939.24131.trast%40student.ethz.ch In-Reply-To: ``` 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 ```