Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities
On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee <derrickstolee@github.com> wrote:
Show 16 quoted lines
>
> On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:
> > We had a report about --update-refs being ignored when --whitespace=fix was
> > passed, confusing an end user. These were already marked as incompatible in
> > the manual, but the code didn't check for the incompatibility and provide an
> > error to the user.
> >
> > Folks brought up other flags tangentially when reviewing an earlier round of
> > this series, and I found we had more that were missing checks, and that we
> > were missing some testcases, and that the documentation was wrong on some of
> > the relevant options. So, I ended up getting lots of little fixes to
> > straighten these all out.
>
> Wow, this really expanded since I last looked at it. Thanks for taking on all
> of that extra work! (That was not my intention when recommending that you take
> over the fix.)
Yeah, I know you were willing to let some of the work wait for some
future series, and I intended to take that route. But...
* both you and Phillip brought up --autosquash, and it turns out we
didn't check it
* the above made me check the whole list of incompatibilities and
discover other missing checks
* adding tests made me discover that the documented
incompatibilities had a few issues
* you mentioned that an explicit --apply wasn't checked either, and
since I was already diving in I figured I might as well handle that
too
* even though you mentioned the config fix wasn't needed, both Junio
and Phillip brought it up as well,
and based on the various feedback gotten so far, I started think
that just addressing it might require
less work than going through more back and forth on review of the
feature without that implementation.Show 14 quoted lines
> > Changes since v3:
> >
> > * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
> > the testcases (Thanks to Phillip for pointing out my error)
> > * I went ahead and implemented the better error message when the merge
> > backend is triggered solely via config options.
>
> I really appreciate this extra attention to detail. I'm also really happy with
> how you implemented it, using different variables to signal how the option was
> specified (and using -1 for "unset" in both cases).
>
> While I had not been following version 2 or 3, I read this version in its
> entirety and everything looked good to me. These improvements to our docs,
> tests, and implementation will be felt by users.
Elijah Newren <newren@gmail.com>