Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)

From: Rubén Justo <rjusto@gmail.com>
Date: Feb 8, 2023, 00:43 UTC
Message-ID: <19e3e0d9-406e-ac2f-c43a-b4e994035529@gmail.com>
In-Reply-To: <f4be3c97-eb15-790e-9746-96d0c9bbc5a7@dunelm.org.uk>

On 07-feb-2023 10:52:39, Phillip Wood wrote:

Show 21 quoted lines

> Hi Rubén
> 
> On 06/02/2023 23:16, Rubén Justo wrote:
> > On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > > Let's add a test to notice if this changes in the future.
> > > 
> > > I for one would find this series much easier to follow if you started
> > > with this test, possibly with a test_expect_failure, and as we fix the
> > > relevant code flip them (both this and the subsequent one) to run
> > > successfully, and include them as part of the commit that fixes the
> > > bug).
> > > 
> > > Maybe there's reasons for why that's tricky to do in this case, so
> > > please ignore this if so.
> > 
> > I'll give it try, I like the idea.  Thanks.
> 
> Squashing the last three commits together so that the tests are introduced
> in the same commit as the fix as Junio suggested in his comments on the
> previous round would be very welcome.

Yes, I considered that, but I think that keeping the tests in their own commit is reasonable. The tests protect against a malfunction that we did not notice when the implementation was done. The commits reference (and use similar subjects) to the original commit where we should have introduced the tests. If the tests fail, I think it will be easier and less confusing to reach the original commit where the implementation was done if we keep them separated, rather than combining all three commits.

I'm going to reorder the commits and change to use test_expect_failure(). This way the commit with the fix will also be linked.