threads / patch / 41722

patchrebase -x: do not die without -i

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

## tl;dr

6 messages between Mar 17, 2016 and Mar 17, 2016. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Stefan Beller· Mar 17, 2016, 01:19 UTC · lore

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(-)
Show changes to 2 files +8 −14

git-rebase.sh, t/t3404-rebase-interactive.sh

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
Johannes Sixt· Mar 17, 2016, 06:20 UTC · re: Stefan Beller · lore

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

Am 17.03.2016 um 02:19 schrieb Stefan Beller:
Show 11 quoted lines
> -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
Johannes Schindelin· Mar 17, 2016, 06:48 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Wed, 16 Mar 2016, Stefan Beller wrote:
Show 12 quoted lines
> 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.

Show 8 quoted lines
> -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

Junio C Hamano· Mar 17, 2016, 07:50 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> 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.

Johannes Schindelin· Mar 17, 2016, 13:11 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Mar 17, 2016, 18:10 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 16 quoted lines
> 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.

← back to recent threads