Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options
On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 24/01/2023 15:41, Elijah Newren wrote:
Show 22 quoted lines
> >> - /*
> >> - * --keep-base implements --reapply-cherry-picks by altering
> >> upstream so
> >> - * it works with both backends.
> >> - */
> >> - if (options.reapply_cherry_picks && !keep_base)
> >> + if (options.reapply_cherry_picks < 0)
> >> + /*
> >> + * --keep-base defaults to --reapply-cherry-picks to
> >> + * avoid losing commits when using this option.
> >> + */
> >
> > I know you were copying the previous comment, but this comment is
> > really confusing to me. Shouldn't it read "--reapply-cherry-picks
> > defaults to --keep-base" instead of vice-versa?
>
> Clearly this comment has not been as helpful as I indented it to be. I
> think maybe we should spell out the defaults with and without
> --keep-base. Perhaps something like
>
> default to --no-reapply-cherry-picks unless --keep-base is given in
> which case default to --reapply-cherry-picks
I like that; sounds good to me.
Show 21 quoted lines
> >> + options.reapply_cherry_picks = keep_base;
> >> + else if (!keep_base)
> >> + /*
> >> + * --keep-base implements --reapply-cherry-picks by
> >
> > Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> > both cases? Especially given how many times I missed it?
>
> This has obviously proved to be confusing. The aim was to explain that
> in order to work with the apply backend "[--reapply-cherry-picks]
> --keep-base" was doing something unusual with `upstream` to reapply
> cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
> anything unusual with `upstream`. I don't think changing it to
> --[no-]reapply-cherry-picks quite captures that. I came up with
>
> To support --reapply-cherry-picks (which is not supported by the apply
> backend) --keep-base alters upstream to prevent cherry picked commits
> from being dropped.
>
> but it really needs to mention that --keep-base also supports
> --no-reapply-cherry-picks in the usual way
Somewhat wordy, but perhaps:
/*
* The apply backend always searches for and drops cherry
* picks. This is often not wanted with --keep-base, so
* --keep-base allows --reapply-cherry-picks to be
* simulated by altering the upstream such that
* cherry-picks cannot be detected and thus all commits are
* reapplied. Thus, --[no-]reapply-cherry-picks is
* supported when --keep-base is specified, but not when
* --keep-base is left out.
*/?
Elijah Newren <newren@gmail.com>