--no-edit not respected after conflict

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

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

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

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

>
> Elijah Newren <newren@gmail.com> writes:
>
> > === 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.
>
> I read the intention behind existing "edit if isatty" as "this is an
> operation the human reader deserves a chance to explain what was
> done and why by default".  For example, I read the first entry in
> your table as: Even if there is no conflict, there should be a
> convincing explanation when you revert.  On the other hand, if you
> are cherry-picking without any conflict, the intention should be
> clear enough in the original commit log message, which ought to be
> written why applying that change is a good idea, so it would make
> sense not to invoke editor in that case.
>
> If an operation deserves a chance to be explained even in a cleanly
> auto resolved case, it does deserve the chance even more if hand
> resolution was required---in addition to the original "what and
> why", the resolution of the conflict is an additional reason why the
> human should be given a chance to explain.
>
> But if it is an automated process, there is no reason to fail the
> operation merely because the process is run unattended.  So my
> recommendation for "regardless of isatty" part is "do not force
> editing".  The same is true for a human user who declines the chance
> to explain him/herself with an explicit "--no-edit".

> On Tue, Mar 23, 2021 at 6:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> === 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.
>>
>> I read the intention behind existing "edit if isatty" as "this is an
>> operation the human reader deserves a chance to explain what was
>> done and why by default".  For example, I read the first entry in
>> your table as: Even if there is no conflict, there should be a
>> convincing explanation when you revert.  On the other hand, if you
>> are cherry-picking without any conflict, the intention should be
>> clear enough in the original commit log message, which ought to be
>> written why applying that change is a good idea, so it would make
>> sense not to invoke editor in that case.
>>
>> If an operation deserves a chance to be explained even in a cleanly
>> auto resolved case, it does deserve the chance even more if hand
>> resolution was required---in addition to the original "what and
>> why", the resolution of the conflict is an additional reason why the
>> human should be given a chance to explain.
>>
>> But if it is an automated process, there is no reason to fail the
>> operation merely because the process is run unattended.  So my
>> recommendation for "regardless of isatty" part is "do not force
>> editing".  The same is true for a human user who declines the chance
>> to explain him/herself with an explicit "--no-edit".
> 
> Thanks.
> 
> Renato: potential fix over here:
> https://lore.kernel.org/git/pull.988.git.git.1616742969145.gitgitgadget@gmail.com/T/#u.
> Could you give it a try?