--no-edit not respected after conflict

--no-edit not respected after conflict

[Renato Botelho]

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

Re: --no-edit not respected after conflict

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

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?
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: --no-edit not respected after conflict

[Renato Botelho]

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.

-- 
Renato Botelho

Re: --no-edit not respected after conflict

[Elijah Newren]

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.

Re: --no-edit not respected after conflict

[Elijah Newren]

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?

Re: --no-edit not respected after conflict

[Junio C Hamano]

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

Re: --no-edit not respected after conflict

[Elijah Newren]

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?

Re: --no-edit not respected after conflict

[Renato Botelho]

On 26/03/21 04:19, Elijah Newren wrote:
> 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?

It worked! Thanks!

-- 
Renato Botelho