Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities
Date: Mon, 23 Jan 2023 18:05:45 -0800	[thread overview]
Message-ID: <CABPp-BG4mUv0W1Duo3Jm6iOALY4Z2sT3o3nqDhz2sboe3AH1aQ@mail.gmail.com> (raw)
In-Reply-To: <1d9748df-4f54-908d-75cf-49ff1d154fcb@github.com>

On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> 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.

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

Thanks for reading!