From: Mark Jamsek <mark@jamsek.com>
Subject: Re: abort commit if merged log messages are left as-is
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Mon, 6 Feb 2023 20:10:20 +1100

On 23-02-06 09:41AM, Omar Polo wrote:
> On 2023/01/28 15:07:55 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > Unless commit -F was used, it should be possible to abort a commit
> > operation by exiting the log message editor without making any
> > modifications to the log message.
> > 
> > This currently does not work as expected in the presence of our new
> > cherrypic/backout log messages, because the prepared_log variable
> > was overloaded for this purpose. Use a different variable, and add
> > merged log messages to the initial log message content buffer.
> > This makes it possible to abort commits as expected.
> > 
> > Erroring out if merged messages are not modified is probably a good
> > thing in any case. I suspect that in virtually all cases the merged
> > log messages should be edited, because the reason for cherrypick or
> > backout should be explained.
> 
> eheh, i agreed with it and was bitten by it a couple of times in the
> past days.  I've often committed something on a wip branch,
> cherrypicked in the main branch, and failed to commit because the
> message didn't change.
> 
> not sure what this tells.  I still think that the default is sane,
> when cherrypicking/backingout from non-local/wip branches the message
> should be expected to change.  I also don't want to go thru the rabbit
> hole of deciding whether a branch should be considered local or wip.
> (and the resulting behaviour could be surprising / annoying.)
> 
> maybe for the commit/cherrypick case we could count the initial
> "comment" line as part of the commit message?  it'd still be possible
> to abort the operation and the message preserved if really desired to.
> just wondering.
> 

Hmm...I somehow missed this one!

I understand both options, and want to preface this with the fact that
I don't feel strongly either way and am perfectly happy with the current
behaviour.

I did consider this while implementing logmsg refs but went with the
original behaviour because, historically, my goto for aborting commits
is to clear the editor and save an empty file (e.g., dG ZZ in vim). This
usually aborts the commit because the commit message can't be empty
(unless explicitly forced, for example, in fossil where you are prompted
to continue with the commit despite an empty message). So this is what
I was doing during testing when I wanted to abort the
cherrypicked/backed-out commit--just save an empty file.

Because of this, combined with the reason op mentioned (i.e., failed
commits when cherrypicking/backing out local commits where you just want
to keep the original message), I'd probably lean toward the original
behaviour with the caveat that we document this with something like
(second sentence added to existing commit doc):

  got commit opens a temporary file in an editor where a log
  message can be written unless the -m option is used or the -F
  and -N options are used together.  The commit can be aborted by
  clearing the temporary file's contents, and saving an empty file.

I really am happy with the new feature irrespective of this aspect.
I just realised I missed this thread and thought I should share my
reasoning behind the original behaviour, which might double as
a solution to the problem op reported.

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68