Re: --no-edit not respected after conflict

[Elijah Newren <newren@gmail.com>]

On Mon, Mar 22, 2021 at 10:14 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Mar 22, 2021 at 6:09 AM Renato Botelho <garga@freebsd.org> wrote:
> >
> > On 19/03/21 18:30, brian m. carlson wrote:
> > > On 2021-03-19 at 14:44:30, Renato Botelho wrote:
> > >> I was reverting multiple commits using --no-edit parameter and after one of
> > >> those commits conflicted and I resolved using mergetool, no-edit option was
> > >> not respected anymore and next commits opened editor for me to review commit
> > >> message.
> > >
> > > I'm not sure I understand what you're seeing here, and I think maybe if
> > > I knew that I could provide more useful information.  Could you maybe
> > > provide the set of commands that you're running up to and when you see
> > > this problem, or even better, a reproduction testcase?
> > >
> >
> > I ran `git revert --no-edit commit1 commit2 ... commitN` and one of
> > those reverts had a conflict and the process stopped waiting for a
> > resolution.
> >
> > I ran `git mergetool` and resolved the conflict, then ran `git revert
> > --continue` and then it ignored --no-edit parameter for all other
> > commits and opened $EDITOR for me to edit commit message.
> >
> > I managed to reproduce it on a testing repository doing following steps:
> >
> > % echo a > file
> > % git init
> > % git add file
> > % git commit -m a
> > % echo b > file; git commit -a -m b
> > % echo c > file; git commit -a -m c
> > % echo d > file; git commit -a -m d
> > % echo e > file; git commit -a -m e
> > % git log --oneline
> >
> > d3ec7fc e
> > 23ad2b7 d
> > 2265c82 c
> > 5e0c98a b
> > b34f81a a
> >
> > % git revert --no-edit d3ec7fc 2265c82 5e0c98a
> >
> > It will revert d3ec7fc without any interaction, as expected, then will
> > stop the process on 2265c82 due to conflict and after resolve conflict
> > when I do:
> >
> > % git revert --continue
> >
> > --no-edit parameter will be ignored when reverting 5e0c98a.
>
> Thanks for the testcase.  I can reproduce.
>
> sequencer.c:save_opts() will only save non-zero values (and since
> options.edit defaults to 1, it'll only save the default value).
>
> sequencer.c:continue_single_pick() was written assuming struct
> replay_opts was not necessary, so even if opts->edit is 0, it just
> runs a plain "git commit" anyway.  It should include --no-edit
> --cleanup=strip.
>
> I've got a patch that fixes both issues, but need to make a proper
> testcase and whatnot.  Maybe I'll have time to do that tonight.

This turns out to be messier than I expected, and I still don't know
what correct behavior is for two related cases.  As best I've figured,
current behavior and expected behavior is as follows (note the
question marks for two entries in the expected behavior table):

=== Current behavior ===
                   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.

=== Expected behavior ===

                   Non-conflict commits    Right after Conflict
revert             Edit iff isatty(0)      Edit (regardless of isatty(0)?)
cherry-pick        No edit                 Edit (regardless of isatty(0)?)
Specify --edit     Edit (ignore isatty(0)) Edit (ignore isatty(0))
Specify --no-edit  No edit                 No edit


The thing I'm unsure on is the !isatty(0) handling for revert &
cherry-pick right after a conflict when neither --edit nor --no-edit
are specified.  Should that attempt to launch an editor, which is
likely to fail?  It does currently, but given that it ignored
--no-edit previously it may have just been an oversight rather than
intentional behavior.

Thoughts?