[PATCHV2 0/2] Decouple rebase --exec from --interactive

[PATCHV2 0/2] Decouple rebase --exec from --interactive

[Stefan Beller]

Johannes Sixt:
 * Using test_i18ngrep instead of test_i18ncmp
 * test_line_count on the lines of the execution output instead
   of rebase stdout

Johannes Schindelin:
 * It's a patch series now (whitespace fixes are in patch 2)
 * you proposed '--exec "touch executed", I'd go with
   --exec "echo foo >>file" so I can count it was executed twice.

Junio:
 * added words to the commit message. :)
   (Essentially quoting Johannes that I am not just testing but also
   stopping for failure at the right point.)

Matthieu: (who preferred responding "private reply, top posting and probably HTML, sent from a phone" ;)
> Your patch makes the user feel like he is using non-interactive
> rebase while we internally go through the interactive code path.
> Did you make sure that the difference between interactive and non
> interactive do not show up by surprise? For example, iirc the interactive
> rebase does not accept --preserve-merges. What happens if the user asks
> --exec --preserve-merges?
>
> Also, does the doc need update? 

I (as a user) did not know there is a non-interactive mode for rebase,
assuming -i would show the editor and that would have been the only
difference.

I think it won't show up as a surprise (in v1) as the changed code is testing for
being non explicit via:

  if test -n "$cmd" &&
     test "$interactive_rebase" != explicit
  then
  -	die(...) 
  +	interactive_rebase=implied
  fi

and that is the last occurrence where interactive_rebase is touched for writing.

Quoting from the man page for `--preserve-merges`:
    This uses the --interactive machinery internally, but combining it with
    the --interactive option explicitly is generally not a good idea unless
    you know what you are doing (see BUGS below).

The docs have a similar explanation just the other way round [--exec is supposed
to be used with --interactive, but I avoided using the word "suppose" as that
sounds like preaching to me (Thou' shall not use --exec without --interactive);
I did not copy over the "if you know what you're doing" as it doesn't help the
user here. How are they supposed to know if they are proficient enough for using
it without -i? And we don't have a BUGS section to point at.]

> What happens if the user asks --exec --preserve-merges?

We're fine. When parsing `--preserve-merges`:
	--preserve-merges)
		preserve_merges=t
		test -z "$interactive_rebase" && interactive_rebase=implied

Essentially we did the same with --exec in v1 just after parsing all options,
now I realize we can do it the same way instead of afterwards, just tossing
in:

  test -z "$interactive_rebase" && interactive_rebase=implied


Stefan Beller (2):
  rebase -x: do not die without -i
  t3404: cleanup double empty lines between tests

 Documentation/git-rebase.txt  |  6 +++---
 git-rebase.sh                 |  7 +------
 t/t3404-rebase-interactive.sh | 18 +++++-------------
 3 files changed, 9 insertions(+), 22 deletions(-)

-- 
2.8.0.rc3.2.ga804a9e

[PATCHV2 1/2] 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. As things can still break, I'd want
to be able to fix those failures directly at the commit, so it is not
sufficient to just use:

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

as it only tests and does not stop at a breakage to fix it up.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-rebase.txt  |  6 +++---
 git-rebase.sh                 |  7 +------
 t/t3404-rebase-interactive.sh | 12 +++++-------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6ed610a..0387b40 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -391,9 +391,6 @@ idea unless you know what you are doing (see BUGS below).
 	final history. <cmd> will be interpreted as one or more shell
 	commands.
 +
-This option can only be used with the `--interactive` option
-(see INTERACTIVE MODE below).
-+
 You may execute several commands by either using one instance of `--exec`
 with several commands:
 +
@@ -406,6 +403,9 @@ or by giving more than one `--exec`:
 If `--autosquash` is used, "exec" lines will not be appended for
 the intermediate commits, and will only appear at the end of each
 squash/fixup series.
++
+This uses the `--interactive` machinery internally, but it can be run
+without an explicit `--interactive`.
 
 --root::
 	Rebase all commits reachable from <branch>, instead of
diff --git a/git-rebase.sh b/git-rebase.sh
index cf60c43..0bf41ee 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -248,6 +248,7 @@ do
 		;;
 	--exec=*)
 		cmd="${cmd}exec ${1#--exec=}${LF}"
+		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
 	--interactive)
 		interactive_rebase=explicit
@@ -348,12 +349,6 @@ do
 done
 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")"
-fi
-
 if test -n "$action"
 then
 	test -z "$in_progress" && die "$(gettext "No rebase in progress?")"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 544f9ad..c8cc03d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -876,16 +876,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
+	rm -rf exec_output &&
+	git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
+	test_i18ngrep  "Successfully rebased and updated" actual &&
+	test_line_count = 2 exec_output
 '
 
-
 test_expect_success 'rebase -i --exec without <CMD>' '
 	git reset --hard execute &&
 	set_fake_editor &&
-- 
2.8.0.rc3.2.ga804a9e

[PATCHV2 2/2] t3404: cleanup double empty lines between tests

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t3404-rebase-interactive.sh | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c8cc03d..f932639 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 &&
-- 
2.8.0.rc3.2.ga804a9e

Re: [PATCHV2 1/2] 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"
>
> 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. As things can still break, I'd want
> to be able to fix those failures directly at the commit, so it is not
> sufficient to just use:
>
>         git rev-list old...new |
>         while read rev
>         do
>                 $command || break
>         done
>
> as it only tests and does not stop at a breakage to fix it up.

That is overly verbose.

    In the later steps of preparing a patch series I do not want to
    edit or reorder the patches any more, but just make sure the
    test suite passes after each patch and also to fix breakage
    right there if some of the steps fail.  I could run

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

    but it would be simpler if it can be spelled like so:

      git rebase <anchor> -x "make test"

says the same thing.

If you said "I want to fix breakage right there as well" upfront,
like in the above shortened example, nobody would imagine that
"rev-list | while .. do .. done" could be a viable substitute.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index cf60c43..0bf41ee 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -248,6 +248,7 @@ do
>  		;;
>  	--exec=*)
>  		cmd="${cmd}exec ${1#--exec=}${LF}"
> +		test -z "$interactive_rebase" && interactive_rebase=implied
>  		;;
>  	--interactive)
>  		interactive_rebase=explicit
> @@ -348,12 +349,6 @@ do
>  done
>  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")"
> -fi
> -

This makes the code structure for $cmd more in line with how the
"--preserve-merges" option is handled, which feels just right ;-)

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 544f9ad..c8cc03d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -876,16 +876,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
> +	rm -rf exec_output &&
> +	git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
> +	test_i18ngrep  "Successfully rebased and updated" actual &&
> +	test_line_count = 2 exec_output
>  '

Shouldn't this test make sure that

	GIT_EDITOR=something git rebase --exec "..." $range

does not invoke 'something', as the point of the new feature is that
you will not have to see the editor to reorder the commits?

> -
>  test_expect_success 'rebase -i --exec without <CMD>' '
>  	git reset --hard execute &&
>  	set_fake_editor &&

Re: [PATCHV2 1/2] rebase -x: do not die without -i

[Johannes Schindelin]

Hi Stefan,

On Thu, 17 Mar 2016, Stefan Beller wrote:

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

As Junio pointed out, there should be only two, not three dots.

Ciao,
Dscho

[PATCHv3 1/2] rebase: decouple --exec from --interactive

[Stefan Beller]

In the later steps of preparing a patch series I do not want to
edit or reorder the patches any more, but just make sure the
test suite passes after each patch and also to fix breakage
right there if some of the steps fail.  I could run

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

but it would be simpler if it can be spelled like so:

    git rebase <anchor> -x "make test"

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

  Thanks Junio, Johannes for review!
 
 * Reworded the commit message (took your suggestion)
 
 * Diff to v2 in t3404:
        test_expect_success 'rebase --exec works without -i ' '
                git reset --hard execute &&
                rm -rf exec_output &&
        -	git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
        +	EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
                test_i18ngrep  "Successfully rebased and updated" actual &&
        -	test_line_count = 2 exec_output
        +	test_line_count = 2 exec_output &&
        +	test_path_is_missing invoked_editor
        '
  * I just resend this patch instead of the whole series, so do not expect a
    [PATCHv3 2/2] nor cover letter 0/2
        

 Documentation/git-rebase.txt  |  6 +++---
 git-rebase.sh                 |  7 +------
 t/t3404-rebase-interactive.sh | 13 ++++++-------
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6ed610a..0387b40 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -391,9 +391,6 @@ idea unless you know what you are doing (see BUGS below).
 	final history. <cmd> will be interpreted as one or more shell
 	commands.
 +
-This option can only be used with the `--interactive` option
-(see INTERACTIVE MODE below).
-+
 You may execute several commands by either using one instance of `--exec`
 with several commands:
 +
@@ -406,6 +403,9 @@ or by giving more than one `--exec`:
 If `--autosquash` is used, "exec" lines will not be appended for
 the intermediate commits, and will only appear at the end of each
 squash/fixup series.
++
+This uses the `--interactive` machinery internally, but it can be run
+without an explicit `--interactive`.
 
 --root::
 	Rebase all commits reachable from <branch>, instead of
diff --git a/git-rebase.sh b/git-rebase.sh
index cf60c43..0bf41ee 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -248,6 +248,7 @@ do
 		;;
 	--exec=*)
 		cmd="${cmd}exec ${1#--exec=}${LF}"
+		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
 	--interactive)
 		interactive_rebase=explicit
@@ -348,12 +349,6 @@ do
 done
 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")"
-fi
-
 if test -n "$action"
 then
 	test -z "$in_progress" && die "$(gettext "No rebase in progress?")"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 544f9ad..21b1f95 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -876,16 +876,15 @@ 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
+	rm -rf exec_output &&
+	EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
+	test_i18ngrep  "Successfully rebased and updated" actual &&
+	test_line_count = 2 exec_output &&
+	test_path_is_missing invoked_editor
 '
 
-
 test_expect_success 'rebase -i --exec without <CMD>' '
 	git reset --hard execute &&
 	set_fake_editor &&
-- 
2.8.0.rc3.10.gafa8710.dirty

Re: [PATCHv3 1/2] rebase: decouple --exec from --interactive

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In the later steps of preparing a patch series I do not want to
> edit or reorder the patches any more, but just make sure the
> test suite passes after each patch and also to fix breakage
> right there if some of the steps fail.  I could run
>
>     EDITOR=true git rebase -i <anchor> -x "make test"
>
> but it would be simpler if it can be spelled like so:
>
>     git rebase <anchor> -x "make test"
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>   Thanks Junio, Johannes for review!
>  
>  * Reworded the commit message (took your suggestion)
>  
>  * Diff to v2 in t3404:
>         test_expect_success 'rebase --exec works without -i ' '
>                 git reset --hard execute &&
>                 rm -rf exec_output &&
>         -	git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
>         +	EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&

Hmph.  If you add "-i" to the command line, do you see the
'invoked_editor' file created?

I ask this because I thought we override GIT_EDITOR, which has
higher precedence than EDITOR, in the test-lib framework.

>                 test_i18ngrep  "Successfully rebased and updated" actual &&
>         -	test_line_count = 2 exec_output
>         +	test_line_count = 2 exec_output &&
>         +	test_path_is_missing invoked_editor
>         '
>   * I just resend this patch instead of the whole series, so do not expect a
>     [PATCHv3 2/2] nor cover letter 0/2
>         
>
>  Documentation/git-rebase.txt  |  6 +++---
>  git-rebase.sh                 |  7 +------
>  t/t3404-rebase-interactive.sh | 13 ++++++-------
>  3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 6ed610a..0387b40 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -391,9 +391,6 @@ idea unless you know what you are doing (see BUGS below).
>  	final history. <cmd> will be interpreted as one or more shell
>  	commands.
>  +
> -This option can only be used with the `--interactive` option
> -(see INTERACTIVE MODE below).
> -+
>  You may execute several commands by either using one instance of `--exec`
>  with several commands:
>  +
> @@ -406,6 +403,9 @@ or by giving more than one `--exec`:
>  If `--autosquash` is used, "exec" lines will not be appended for
>  the intermediate commits, and will only appear at the end of each
>  squash/fixup series.
> ++
> +This uses the `--interactive` machinery internally, but it can be run
> +without an explicit `--interactive`.
>  
>  --root::
>  	Rebase all commits reachable from <branch>, instead of
> diff --git a/git-rebase.sh b/git-rebase.sh
> index cf60c43..0bf41ee 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -248,6 +248,7 @@ do
>  		;;
>  	--exec=*)
>  		cmd="${cmd}exec ${1#--exec=}${LF}"
> +		test -z "$interactive_rebase" && interactive_rebase=implied
>  		;;
>  	--interactive)
>  		interactive_rebase=explicit
> @@ -348,12 +349,6 @@ do
>  done
>  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")"
> -fi
> -
>  if test -n "$action"
>  then
>  	test -z "$in_progress" && die "$(gettext "No rebase in progress?")"
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 544f9ad..21b1f95 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -876,16 +876,15 @@ 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
> +	rm -rf exec_output &&
> +	EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
> +	test_i18ngrep  "Successfully rebased and updated" actual &&
> +	test_line_count = 2 exec_output &&
> +	test_path_is_missing invoked_editor
>  '
>  
> -
>  test_expect_success 'rebase -i --exec without <CMD>' '
>  	git reset --hard execute &&
>  	set_fake_editor &&

Re: [PATCHv3 1/2] rebase: decouple --exec from --interactive

[Junio C Hamano]

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

>>         +	EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
>
> Hmph.  If you add "-i" to the command line, do you see the
> 'invoked_editor' file created?
>
> I ask this because I thought we override GIT_EDITOR, which has
> higher precedence than EDITOR, in the test-lib framework.

False alarm.  We do use EDITOR=: and arrange GIT_EDITOR from the
outside world to be ignored by unsetting in test-lib.sh, so
exporting EDITOR here should be safe and OK.

Will queue once I manage to locate 2/2 ;-).

Re: [PATCHv3 1/2] rebase: decouple --exec from --interactive

[Stefan Beller]

On Fri, Mar 18, 2016 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>  * Diff to v2 in t3404:
>>         test_expect_success 'rebase --exec works without -i ' '
>>                 git reset --hard execute &&
>>                 rm -rf exec_output &&
>>         -     git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
>>         +     EDITOR="echo >invoked_editor" git rebase --exec "echo a line >>exec_output"  HEAD~2 2>actual &&
>
> Hmph.  If you add "-i" to the command line, do you see the
> 'invoked_editor' file created?

just tested that by adding

    EDITOR="echo >invoked_editor" git rebase -i ...
    test_path_is_file invoked_editor

and it works. I have a patch here with s/EDITOR/EDITOR/,
which I can send out...... and your follow up email came in,
so I stop bothering about this patch until further reviews, ok?

Re: [PATCHV2 0/2] Decouple rebase --exec from --interactive

[Matthieu Moy]

Stefan Beller <sbeller@google.com> writes:

>> What happens if the user asks --exec --preserve-merges?
>
> We're fine. When parsing `--preserve-merges`:
> 	--preserve-merges)
> 		preserve_merges=t
> 		test -z "$interactive_rebase" && interactive_rebase=implied

Perfect, thanks for checking (which I couldn't do myself from a phone).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/