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

From: Elijah Newren <newren@gmail.com>
Date: Jun 20, 2018, 16:47 UTC
Message-ID: <CABPp-BHrcUHX_zHxpojV5=sxJ1=NoDg9uhxv+NH5BsHsQYavPQ@mail.gmail.com>
In-Reply-To: <e8a6f54e-4361-dc66-c833-6047f8ea2058@talktalk.net>

Hi Phillip,

On Sun, Jun 17, 2018 at 8:30 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 9 quoted lines

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

Here's my extended rationale for why to change the default for rebase -i/-m (and _maybe_ also for cherry-pick):

First, commits with an empty commit message are fairly rare, so it's hard to gather that much data on; we're dealing with out-liers. That also means that folks who have touched the area in the past probably weren't dealing with full information either; they just tweaked what was already there.

It makes sense that git-commit would default to not allowing empty commit messages without a flag. We'd like to warn against that kind of poor practice. However, that implicitly means that commands which build on top of git-commit might copy that default even when it doesn't make sense. So it's not at all clear to me that cherry-pick or rebase -i/-m should have the same default. They do, but that may have been entirely accidental. And when someone comes along and notices the problematic default of stopping with empty commit messages, they assume it was intended and add a flag to change it, as happened for both commands.

On the other hand git-am was designed from the workflow of applying many patches from the beginning, and has a different default. am-based rebases copy that default of silently applying commits with empty commit messages; it may not be clear that rebase should copy the default from am, but it does. Yet, despite the fact that ignoring empty commit messages is the default backend for rebases, and thus is likely used more than the other rebase backends, no one has ever submitted a patch to add a flag to rebase to get the opposite behavior. To me, this argues pretty strongly that not only is ignoring empty messages the right default, that it's a waste of time to implement a flag (e.g. --no-allow-empty-message) providing the opposite behavior for rebases.

It is obvious that one of the two rebase backends have the wrong default, because they should have the same default as each other. I am inclined to believe, based on the above logic, that am-based rebase has the correct default. I also suspect that cherry-pick has the wrong default, though it's not as cut-and-dry since it doesn't have multiple backends with conflicting defaults.

Does that make sense?  Am I overlooking anything important?