Re: [PATCH v3] sequencer: fix edit handling for cherry-pick and revert messages

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Wed, 31 Mar 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> save_opts() should save any non-default values.  It was intended to do
> this, but since most options in struct replay_opts default to 0, it only
> saved non-zero values.  Unfortunately, this does not always work for
> options.edit.  Roughly speaking, options.edit had a default value of 0
> for cherry-pick but a default value of 1 for revert.  Make save_opts()
> record a value whenever it differs from the default.
>
> options.edit was also overly simplistic; we had more than two cases.
> The behavior that previously existed was as follows:
>
>                        Non-conflict commits    Right after Conflict
>     revert             Edit iff isatty(0)      Edit (ignore isatty(0))
>     cherry-pick        No edit                 See above
>     Specify --edit     Edit (ignore isatty(0)) See above
>     Specify --no-edit  (*)                     See above
>
>     (*) Before stopping for conflicts, No edit is the behavior.  After
>         stopping for conflicts, the --no-edit flag is not saved so see
>         the first two rows.
>
> However, the expected behavior is:
>
>                        Non-conflict commits    Right after Conflict
>     revert             Edit iff isatty(0)      Edit iff isatty(0)
>     cherry-pick        No edit                 Edit iff isatty(0)
>     Specify --edit     Edit (ignore isatty(0)) Edit (ignore isatty(0))
>     Specify --no-edit  No edit                 No edit
>
> In order to get the expected behavior, we need to change options.edit
> to a tri-state: unspecified, false, or true.  When specified, we follow
> what it says.  When unspecified, we need to check whether the current
> commit being created is resolving a conflict as well as consulting
> options.action and isatty(0).  While at it, add a should_edit() utility
> function that compresses options.edit down to a boolean based on the
> additional information for the non-conflict case.
>
> continue_single_pick() is the function responsible for resuming after
> conflict cases, regardless of whether there is one commit being picked
> or many.  Make this function stop assuming edit behavior in all cases,
> so that it can correctly handle !isatty(0) and specific requests to not
> edit the commit message.
>
> Reported-by: Renato Botelho <garga@freebsd.org>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     sequencer: fix edit handling for cherry-pick and revert messages
>
>     Changes since v2:
>
>      * Changed to use <0 for unspecified (instead of == -1), >0 for true
>        (instead of == 1).
>      * Removed assert() statement.
>      * Removed unnecessary "want_edit" local variable
>
>     Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah
>     Newren newren@gmail.com

Looks good, thank you!

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks,
Dscho