Hi Elijah, On Wed, 31 Mar 2021, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@xxxxxxxxx> > > 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@xxxxxxxxxxx> > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> > --- > 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@xxxxxxxxxxx Signed-off-by: Elijah > Newren newren@xxxxxxxxx Looks good, thank you! Reviewed-by: Johannes Schindelin <johannes.schindelin@xxxxxx> Thanks, Dscho
