Re: 2.19.0 regression: leaving editor with empty commit message doesn't stop rebase [was: Re: [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default]

2018-09-12 Thread Elijah Newren
Hi,

On Tue, Sep 11, 2018 at 7:42 PM SZEDER Gábor  wrote:
> Lately I noticed that occasionally I ended up with an empty commit
> message after an interactive rebase...those empty commit messages are the
> consequence of a regression in v2.19.0, which bisects down to this
> patch.
>
> To reproduce the issue, just start an interactive rebase, choose a
> commit to reword, save, delete all the commit message, save, and BAM!
> there is the commit with the empty message.
>
>
> On Wed, Jun 27, 2018 at 12:23:19AM -0700, Elijah Newren wrote:
> > rebase backends currently behave differently with empty commit messages,
> > largely as a side-effect of the different underlying commands on which
> > they are based.  am-based rebases apply commits with an empty commit
...
> I agree that this is an issue that should be addressed, and also agree
> that it's reasonable to accept an empty commit message, if the
> original commit already had an empty commit message.  (Though perhaps
> not completely silently, but with a warning?  Dunno.)
>
> However, this should only be the case for existing commit messages
> that are taken verbatim, but never in the case when the user is asked
> for a commit message.
...
>   # Please enter the commit message for your changes. Lines starting
>   # with '#' will be ignored, and an empty message aborts the commit.
...
> Let's suppose you start an interactive rebase, choose a commit to
> squash, save the instruction sheet, rebase fires up your editor, and
> then you notice that you mistakenly chose the wrong commit to squash.
> What do you do, how do you abort?

All sound like reasonable arguments to me to differentiate between
commit messages that started empty (which I admit was what I had in
mind) vs. ones where we asked for user input and it came out empty.

Are you cooking up a patch?  I might be able to find a little time to
dig into this tomorrow if no one else is working on it already by
then.  It'll probably conflict a bunch with all the
rewrite-rebase-in-C topics in flight in pu.  :-(


2.19.0 regression: leaving editor with empty commit message doesn't stop rebase [was: Re: [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default]

2018-09-11 Thread SZEDER Gábor


Lately I noticed that occasionally I ended up with an empty commit
message after an interactive rebase.  However, since I didn't do
anything that I haven't already done countless times before, I thought
that one of my local patches touching the area where 'git commit'
calls launch_editor() got messed up.  Unfortunately, it turned out
that my patch is fine, and those empty commit messages are the
consequence of a regression in v2.19.0, which bisects down to this
patch.

To reproduce the issue, just start an interactive rebase, choose a
commit to reword, save, delete all the commit message, save, and BAM!
there is the commit with the empty message.


On Wed, Jun 27, 2018 at 12:23:19AM -0700, Elijah Newren wrote:
> rebase backends currently behave differently with empty commit messages,
> largely as a side-effect of the different underlying commands on which
> they are based.  am-based rebases apply commits with an empty commit
> message without stopping or requiring the user to specify an extra flag.
> (It is interesting to note that am-based rebases are the default rebase
> type, and no one has ever requested a --no-allow-empty-message flag to
> change this behavior.)  merge-based and interactive-based rebases (which
> are ultimately based on git-commit), will currently halt on any such
> commits and require the user to manually specify what to do with the
> commit and continue.
> 
> One possible rationale for the difference in behavior is that the purpose
> of an "am" based rebase is solely to transplant an existing history, while
> an "interactive" rebase is one whose purpose is to polish a series before
> making it publishable.  Thus, stopping and asking for confirmation for a
> possible problem is more appropriate in the latter case.  However, there
> are two problems with this rationale:
> 
>   1) merge-based rebases are also non-interactive and there are multiple
>  types of rebases that use the interactive machinery but are not
>  explicitly interactive (e.g. when either --rebase-merges or
>  --keep-empty are specified without --interactive).  These rebases are
>  also used solely to transplant an existing history, and thus also
>  should default to --allow-empty-message.
> 
>   2) this rationale only says that the user is more accepting of stopping
>  in the case of an explicitly interactive rebase, not that stopping
>  for this particular reason actually makes sense.  Exploring whether
>  it makes sense, requires backing up and analyzing the underlying
>  commands...
> 
> If git-commit did not error out on empty commits by default, accidental
> creation of commits with empty messages would be a very common occurrence
> (this check has caught me many times).  Further, nearly all such empty
> commit messages would be considered an accidental error (as evidenced by a
> huge amount of documentation across version control systems and in various
> blog posts explaining how important commit messages are).  A simple check
> for what would otherwise be a common error thus made a lot of sense, and
> git-commit gained an --allow-empty-message flag for special case
> overrides.  This has made commits with empty messages very rare.
> 
> There are two sources for commits with empty messages for rebase (and
> cherry-pick): (a) commits created in git where the user previously
> specified --allow-empty-message to git-commit, and (b) commits imported
> into git from other version control systems.  In case (a), the user has
> already explicitly specified that there is something special about this
> commit that makes them not want to specify a commit message; forcing them
> to re-specify with every cherry-pick or rebase seems more likely to be
> infuriating than helpful.  In case (b), the commit is highly unlikely to
> have been authored by the person who has imported the history and is doing
> the rebase or cherry-pick, and thus the user is unlikely to be the
> appropriate person to write a commit message for it.  Stopping and
> expecting the user to modify the commit before proceeding thus seems
> counter-productive.

I agree that this is an issue that should be addressed, and also agree
that it's reasonable to accept an empty commit message, if the
original commit already had an empty commit message.  (Though perhaps
not completely silently, but with a warning?  Dunno.)

However, this should only be the case for existing commit messages
that are taken verbatim, but never in the case when the user is asked
for a commit message.

> Further, note that while empty commit messages was a common error case for
> git-commit to deal with, it is a rare case for rebase (or cherry-pick).
> The fact that it is rare raises the question of why it would be worth
> checking and stopping on this particular condition and not others.  For
> example, why doesn't an interactive rebase automatically stop if the
> commit message's first line is 2000 columns long, or is missing a blank
> line after