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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2016, 21:58 UTC
Message-ID: <xmqqd1qszs03.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458251060-311-2-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 22 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. 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.

Show 22 quoted lines

> 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 ;-)

Show 21 quoted lines

> 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 &&