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

From: Elijah Newren <newren@gmail.com>
Date: Mar 29, 2021, 21:25 UTC
Message-ID: <CABPp-BF5fWanFcgOc1Dq0uT8cYCFjiaSO6i4PF9a4Xpg3vEf7Q@mail.gmail.com>
In-Reply-To: <440980f5-81c9-4201-61a4-29acb0346c65@gmail.com>

On Mon, Mar 29, 2021 at 2:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 9 quoted lines

>
> On 26/03/2021 07:16, 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.  Unfortunatley,
>
> s/Unfortunatley/Unfortunately/

Thanks, will fix.

> also s/iff/if/ in a few places below.

I want behavior for both isatty(0) and !isatty(0) to be specified by the table, so iff is correct; see https://www.lexico.com/definition/iff

[...]

Show 7 quoted lines

> > Make continue_single_pick() (which is the function responsible for
> > resuming after conflict cases)
>
> It might be worth emphasizing that despite its name
> continue_single_pick() is used to commit conflict resolutions regardless
> of the number of picks - I had to check the code to see what it was
> doing in the multi-pick case.

I'll edit the description to make this more clear.

[...]

Show 10 quoted lines

> These tests check that the options are saved but do not check what we do
> with them. It would be good to have a test that checked we actually open
> the editor when we should to test the new code in
> continue_single_pick(), however as that code calls isatty() that may be
> tricky
>
>
> I was surprised how big a change was required to the existing code but
> it seems this is surprising tricky to get right - I cannot think of any
> simplifications.

Thanks for taking a look.  :-)