[PATCH] rebase -x: do not die without -i

[PATCH] rebase -x: do not die without -i

[Stefan Beller]

In the later steps of preparing a patch series I do not want to edit the
patches any more, but just make sure the test suite passes after each
patch. Currently I would run

  EDITOR=true git rebase -i <anchor> -x "make test"

In an ideal world the command would be simpler and just be

  git rebase <anchor> -x "make test"

to run the test for each commit I am about to send out for review.
This patch enables the short line.

While at it, remove the double empty lines in t3404.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Junio,
 
 This is a slightly different approach.
 
 -x doesn't imply -i any more, but just the internal code does that. The user
 will see --exec working just fine without the -i option given.

 Thanks,
 Stefan

 git-rebase.sh                 |  4 +++-
 t/t3404-rebase-interactive.sh | 18 +++++-------------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index cf60c43..232484a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -351,7 +351,9 @@ test $# -gt 2 && usage
 if test -n "$cmd" &&
    test "$interactive_rebase" != explicit
 then
-	die "$(gettext "The --exec option must be used with the --interactive option")"
+	interactive_rebase=implicit
+	GIT_EDITOR=true
+	export GIT_EDITOR
 fi
 
 if test -n "$action"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 544f9ad..2064d88 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -771,7 +771,6 @@ test_expect_success 'rebase-i history with funny messages' '
 	test_cmp expect actual
 '
 
-
 test_expect_success 'prepare for rebase -i --exec' '
 	git checkout master &&
 	git checkout -b execute &&
@@ -780,7 +779,6 @@ test_expect_success 'prepare for rebase -i --exec' '
 	test_commit three_exec main.txt three_exec
 '
 
-
 test_expect_success 'running "git rebase -i --exec git show HEAD"' '
 	set_fake_editor &&
 	git rebase -i --exec "git show HEAD" HEAD~2 >actual &&
@@ -793,7 +791,6 @@ test_expect_success 'running "git rebase -i --exec git show HEAD"' '
 	test_cmp expected actual
 '
 
-
 test_expect_success 'running "git rebase --exec git show HEAD -i"' '
 	git reset --hard execute &&
 	set_fake_editor &&
@@ -807,7 +804,6 @@ test_expect_success 'running "git rebase --exec git show HEAD -i"' '
 	test_cmp expected actual
 '
 
-
 test_expect_success 'running "git rebase -ix git show HEAD"' '
 	git reset --hard execute &&
 	set_fake_editor &&
@@ -835,7 +831,6 @@ test_expect_success 'rebase -ix with several <CMD>' '
 	test_cmp expected actual
 '
 
-
 test_expect_success 'rebase -ix with several instances of --exec' '
 	git reset --hard execute &&
 	set_fake_editor &&
@@ -850,7 +845,6 @@ test_expect_success 'rebase -ix with several instances of --exec' '
 	test_cmp expected actual
 '
 
-
 test_expect_success 'rebase -ix with --autosquash' '
 	git reset --hard execute &&
 	git checkout -b autosquash &&
@@ -876,16 +870,14 @@ test_expect_success 'rebase -ix with --autosquash' '
 	test_cmp expected actual
 '
 
-
-test_expect_success 'rebase --exec without -i shows error message' '
+test_expect_success 'rebase --exec works without -i ' '
 	git reset --hard execute &&
-	set_fake_editor &&
-	test_must_fail git rebase --exec "git show HEAD" HEAD~2 2>actual &&
-	echo "The --exec option must be used with the --interactive option" >expected &&
-	test_i18ncmp expected actual
+	git rebase --exec true HEAD~2 2>actual2 >actual1 &&
+	echo "Successfully rebased and updated refs/heads/autosquash_expected." >expected &&
+	test_i18ncmp expected actual2 &&
+	test_line_count = 2 actual1
 '
 
-
 test_expect_success 'rebase -i --exec without <CMD>' '
 	git reset --hard execute &&
 	set_fake_editor &&
-- 
2.8.0.rc3.1.ge1ac87c.dirty

Re: [PATCH] rebase -x: do not die without -i

[Johannes Sixt]

Am 17.03.2016 um 02:19 schrieb Stefan Beller:
> -test_expect_success 'rebase --exec without -i shows error message' '
> +test_expect_success 'rebase --exec works without -i ' '
>   	git reset --hard execute &&
> -	set_fake_editor &&
> -	test_must_fail git rebase --exec "git show HEAD" HEAD~2 2>actual &&
> -	echo "The --exec option must be used with the --interactive option" >expected &&
> -	test_i18ncmp expected actual
> +	git rebase --exec true HEAD~2 2>actual2 >actual1 &&
> +	echo "Successfully rebased and updated refs/heads/autosquash_expected." >expected &&
> +	test_i18ncmp expected actual2 &&
> +	test_line_count = 2 actual1

We don't have an explicit guideline, but please do not check stderr 
output using test_cmp or test_i18ncmp. The reason is that some shells 
write trace output to stderr when run under 'set -x'. That is, when you 
run this test as

      ./t3404-rebase-interactive.sh -x -v

it will fail because there is now more text in actual2 than expected. We 
have a number of cases like this elsewhere, but we should not stack new 
cases on the pile.

Please use test_i18ngrep:

	test_i18ngrep "Successfully rebased and updated" actual2 &&
	test_line_count = 2 actual1

-- Hannes

Re: [PATCH] rebase -x: do not die without -i

[Johannes Schindelin]

Hi Stefan,

On Wed, 16 Mar 2016, Stefan Beller wrote:

> In the later steps of preparing a patch series I do not want to edit the
> patches any more, but just make sure the test suite passes after each
> patch. Currently I would run
> 
>   EDITOR=true git rebase -i <anchor> -x "make test"
> 
> In an ideal world the command would be simpler and just be
> 
>   git rebase <anchor> -x "make test"
> 
> to run the test for each commit I am about to send out for review.
> This patch enables the short line.

Makes sense.

> While at it, remove the double empty lines in t3404.

Could you split those out? They may interfere with some of my work, and
it's easier to figure things out from a trivial patch than from one
combining unrelated changes.

> -test_expect_success 'rebase --exec without -i shows error message' '
> +test_expect_success 'rebase --exec works without -i ' '
>  	git reset --hard execute &&
> -	set_fake_editor &&
> -	test_must_fail git rebase --exec "git show HEAD" HEAD~2 2>actual &&
> -	echo "The --exec option must be used with the --interactive option" >expected &&
> -	test_i18ncmp expected actual
> +	git rebase --exec true HEAD~2 2>actual2 >actual1 &&

How about '--exec "touch executed"'? You'd want to verify that the command
really was executed...

Ciao,
Dscho

Re: [PATCH] rebase -x: do not die without -i

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In the later steps of preparing a patch series I do not want to edit the
> patches any more, but just make sure the test suite passes after each
> patch. Currently I would run
>
>   EDITOR=true git rebase -i <anchor> -x "make test"

Hmm, I guess that may "work" but it sounds like quite a roundabout
way to "test all commits".  "rebase" is about replaying history to
end up with a set of newly minted commits, and being able to poke at
the state each commit records in the working tree is a side effect.
"rebase -i" may use the same commit object if you didn't actually
make new commit as an optimization, but otherwise, it is like going
through pages of a book, tearing each page to examine it, and
replacing each page with a photocopy of it before going to examine
the next page.  Which makes me feel somewhat dirty X-<.

In other words, that looks like a workaround for not having

    $ git for-each-rev -x "$command" old..new

where you can write "sh -c 'git checkout $1 && make test' -" as
your $command.

Re: [PATCH] rebase -x: do not die without -i

[Johannes Schindelin]

Hi Junio,

On Thu, 17 Mar 2016, Junio C Hamano wrote:

> I guess that may "work" but it sounds like quite a roundabout
> way to "test all commits".  "rebase" is about replaying history to
> end up with a set of newly minted commits, and being able to poke at
> the state each commit records in the working tree is a side effect.

I think there is a misunderstanding here. Stefan does not only want to
test all of the commits in the patch series. Stefan wants to make sure
that all patches in the series result in revisions that pass the test
suite.

In other words, it is not only about testing, it is also about fixing when
things break.

And that is very, very much the purpose of the interactive rebase.

>     $ git for-each-rev -x "$command" old..new
> 
> where you can write "sh -c 'git checkout $1 && make test' -" as
> your $command.

You meant

	git rev-list old...new |
	while read rev
	do
		$command || break
	done

?

Again, this might *test* the revisions. But it does nothing to help fixing
any patch in the series.

Ciao,
Dscho

Re: [PATCH] rebase -x: do not die without -i

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> And that is very, very much the purpose of the interactive rebase.
>
>>     $ git for-each-rev -x "$command" old..new
>> 
>> where you can write "sh -c 'git checkout $1 && make test' -" as
>> your $command.
>
> You meant
>
> 	git rev-list old...new |
> 	while read rev
> 	do
> 		$command || break
> 	done
>
> ?

Yeah, if I actually felt the lack of "for-each-ref -x" a problem (I
don't), that is what I would have used instead (but with just two
dots ;-).

But you are correct to point out that I didn't consider that
"... and I want to fix right there if it breaks" is a part of the
use case, mainly because the log message said this:

    In the later steps of preparing a patch series I do not want to
    edit the patches any more, but just make sure the test suite
    passes after each patch.

and partly I lack imagination.

If you throw in that extra "I want to fix right there if it breaks"
requirement, it makes perfect sense to make use of the sequencing
machinery we implement for "rebase -i", as the while loop above does
not give you the same sequencing without extra work.

So perhaps the idea of the patch is good with a better explanation.