Re: [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default

[Phillip Wood <phillip.wood@talktalk.net>]

Hi Elijah

On 17/06/18 06:58, Elijah Newren wrote:
> am-based rebases already apply commits with an empty commit message
> without requiring the user to specify an extra flag.  Make merge-based and
> interactive-based rebases behave the same.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt  | 10 ----------
>   git-rebase.sh                 |  2 +-
>   t/t3404-rebase-interactive.sh |  7 ++++---
>   t/t3405-rebase-malformed.sh   | 11 +++--------
>   4 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 7a2ed9efdc..a5608f481f 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -562,16 +562,6 @@ BEHAVIORAL INCONSISTENCIES
>       The --keep-empty option exists for interactive rebases to allow
>       it to keep commits that started empty.
>   
> -  * empty commit messages:
> -
> -    am-based rebase will silently apply commits with empty commit
> -    messages.
> -
> -    merge-based and interactive-based rebases will by default halt
> -    on any such commits.  The --allow-empty-message option exists to
> -    allow interactive-based rebases to apply such commits without
> -    halting.
> -
>     * directory rename detection:
>   
>       merge-based and interactive-based rebases work fine with
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 5f891214fa..bf033da4e5 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -95,7 +95,7 @@ rebase_cousins=
>   preserve_merges=
>   autosquash=
>   keep_empty=
> -allow_empty_message=
> +allow_empty_message=--allow-empty-message

Looking at the option parsing in git-rebase.sh it appears that it does 
not check for --no-allow-empty-message so there's no way to turn off the 
default for those modes that support it. I'm not sure what to think of 
this change, I'm slightly uneasy with changing to default to be 
different from cherry-pick, though one could argue rebasing is a 
different operation and just keeping the existing messages even if they 
are empty is more appropriate and having all the rebase modes do the 
default to the same behaviour is definitely an improvement.

Best Wishes

Phillip

>   signoff=
>   test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
>   case "$(git config --bool commit.gpgsign)" in
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c65826ddac..f84fa63b15 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -553,15 +553,16 @@ test_expect_success '--continue tries to commit, even for "edit"' '
>   '
>   
>   test_expect_success 'aborted --continue does not squash commits after "edit"' '
> +	test_when_finished "git rebase --abort" &&
>   	old=$(git rev-parse HEAD) &&
>   	test_tick &&
>   	set_fake_editor &&
>   	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>   	echo "edited again" > file7 &&
>   	git add file7 &&
> -	test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue &&
> -	test $old = $(git rev-parse HEAD) &&
> -	git rebase --abort
> +	echo all the things >>conflict &&
> +	test_must_fail git rebase --continue &&
> +	test $old = $(git rev-parse HEAD)
>   '
>   
>   test_expect_success 'auto-amend only edited commits after "edit"' '
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index cb7c6de84a..da94dddc86 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -77,19 +77,14 @@ test_expect_success 'rebase commit with diff in message' '
>   '
>   
>   test_expect_success 'rebase -m commit with empty message' '
> -	test_must_fail git rebase -m master empty-message-merge &&
> -	git rebase --abort &&
> -	git rebase -m --allow-empty-message master empty-message-merge
> +	git rebase -m master empty-message-merge
>   '
>   
>   test_expect_success 'rebase -i commit with empty message' '
>   	git checkout diff-in-message &&
>   	set_fake_editor &&
> -	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> -		git rebase -i HEAD^ &&
> -	git rebase --abort &&
> -	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> -		git rebase -i --allow-empty-message HEAD^
> +	env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> +		git rebase -i HEAD^
>   '
>   
>   test_done
>