Re: [PATCH v2] rebase: add --allow-empty-message option

[Junio C Hamano]

Johannes Schindelin  writes:

> Very nice. I looked over the patch (sorry, I have too little time to test
> this thoroughly, but then, it is the custom on this here mailing list to
> just review the patch as per the mail) and it looks very good to me.
>
> Junio, if you like, please add a "Reviewed-by:" line for me.

Will do.  You do not have to apologize for not "testing" it; nobody
expects _you_ to test every change you come across on the list, and
other people (including the CI) are testing without advertising ;-).

Re: [PATCH v2] rebase: add --allow-empty-message option

[Johannes Schindelin]

Hi Genki,

On Sun, 4 Feb 2018, Genki Sky wrote:

> This option allows commits with empty commit messages to be rebased,
> matching the same option in git-commit and git-cherry-pick. While empty
> log messages are frowned upon, sometimes one finds them in older
> repositories (e.g. translated from another VCS [0]), or have other
> reasons for desiring them. The option is available in git-commit and
> git-cherry-pick, so it is natural to make other git tools play nicely
> with them. Adding this as an option allows the default to be "give the
> user a chance to fix", while not interrupting the user's workflow
> otherwise [1].
> 
>   [0]: https://stackoverflow.com/q/8542304
>   [1]: https://public-inbox.org/git/7vd33afqjh@alter.siamese.dyndns.org/
> 
> To implement this, add a new --allow-empty-message flag. Then propagate
> it to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'
> within the rebase scripts.
> 
> Signed-off-by: Genki Sky 
> ---
> 
> Notes:
> 
>   Thanks for the initial feedback, here are the changes from [v1]:
>   - Made my commit message include the main motivations inline.
>   - Moved new tests to t/t3405-rebase-malformed.sh, which has the
> relevant test description: "rebase should handle arbitrary git
> message".
>   - Accordingly re-used existing test setup.
>   - Minimized tests to just one for git-rebase--interactive.sh and one
> for git-rebase--merge.sh. First, one test per file keeps things
> focused while getting most benefit (other code within same file is
> likely to be noticed by modifiers). And, while git-rebase--am.sh
> does have one cherry-pick, it is only for a special case with
> --keep-empty. So git-rebase--am.sh otherwise doesn't have this
> empty-message issue.
> 
>   In general, there was a concern of over-testing, and over-checking
>   implementation details. So, this time, I erred on the conservative
>   side.
> 
>   [v1]: 
> https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git@genki.is/t/

Very nice. I looked over the patch (sorry, I have too little time to test
this thoroughly, but then, it is the custom on this here mailing list to
just review the patch as per the mail) and it looks very good to me.

Junio, if you like, please add a "Reviewed-by:" line for me.

Thanks!
Johannes

[PATCH v2] rebase: add --allow-empty-message option

[Genki Sky]

This option allows commits with empty commit messages to be rebased,
matching the same option in git-commit and git-cherry-pick. While empty
log messages are frowned upon, sometimes one finds them in older
repositories (e.g. translated from another VCS [0]), or have other
reasons for desiring them. The option is available in git-commit and
git-cherry-pick, so it is natural to make other git tools play nicely
with them. Adding this as an option allows the default to be "give the
user a chance to fix", while not interrupting the user's workflow
otherwise [1].

  [0]: https://stackoverflow.com/q/8542304
  [1]: https://public-inbox.org/git/7vd33afqjh@alter.siamese.dyndns.org/

To implement this, add a new --allow-empty-message flag. Then propagate
it to all calls of 'git commit', 'git cherry-pick', and 'git rebase--helper'
within the rebase scripts.

Signed-off-by: Genki Sky 
---

Notes:

  Thanks for the initial feedback, here are the changes from [v1]:
  - Made my commit message include the main motivations inline.
  - Moved new tests to t/t3405-rebase-malformed.sh, which has the
relevant test description: "rebase should handle arbitrary git
message".
  - Accordingly re-used existing test setup.
  - Minimized tests to just one for git-rebase--interactive.sh and one
for git-rebase--merge.sh. First, one test per file keeps things
focused while getting most benefit (other code within same file is
likely to be noticed by modifiers). And, while git-rebase--am.sh
does have one cherry-pick, it is only for a special case with
--keep-empty. So git-rebase--am.sh otherwise doesn't have this
empty-message issue.

  In general, there was a concern of over-testing, and over-checking
  implementation details. So, this time, I erred on the conservative
  side.

  [v1]: 
https://public-inbox.org/git/9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git@genki.is/t/

 Documentation/git-rebase.txt |  5 +
 builtin/rebase--helper.c |  2 ++
 git-rebase--am.sh|  1 +
 git-rebase--interactive.sh   | 25 +++--
 git-rebase--merge.sh |  3 ++-
 git-rebase.sh|  5 +
 t/t3405-rebase-malformed.sh  | 23 +++
 7 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8a861c1e0..d713951b8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it 
defaults to HEAD.
Keep the commits that do not change anything from its
parents in the result.

+--allow-empty-message::
+   By default, rebasing commits with an empty message will fail.
+   This option overrides that behavior, allowing commits with empty
+   messages to be rebased.
+
 --skip::
Restart the rebasing process by skipping the current patch.

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7daee544b..2090114e9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
+   OPT_BOOL(0, "allow-empty-message", _empty_message,
+   N_("allow commits with empty messages")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 14c50782e..0f7805179 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -46,6 +46,7 @@ then
# makes this easy
git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
$allow_rerere_autoupdate --right-only "$revisions" \
+   $allow_empty_message \
${restrict_revision+^$restrict_revision}
ret=$?
 else
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d47bd2959..81c5b4287 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -281,7 +281,7 @@ pick_one () {

test -d "$rewritten" &&
pick_one_preserving_merges "$@" && return
-   output eval git cherry-pick $allow_rerere_autoupdate \
+   output eval git cherry-pick $allow_rerere_autoupdate 
$allow_empty_message \
${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")} \
"$strategy_args" $empty_args $ff "$@"

@@ -406,6 +406,7 @@ pick_one_preserving_merges () {
;;
*)
output eval git cherry-pick $allow_rerere_autoupdate \
+   $allow_empty_message \