From: Phillip Wood Date: Wed, 22 Feb 2023 10:16:05 GMT Subject: Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges Message-ID: <36090abb-72a0-f9b7-5a41-ddd8ccde92bd@dunelm.org.uk> In-Reply-To: Hi Alex On 22/02/2023 01:37, Alex Henrie wrote: > On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood wrote: >> >> On 21/02/2023 05:58, Alex Henrie wrote: >>> +test_expect_success 'do not rebase merges unless asked to' ' >>> + git checkout -b rebase-merges-default E && >>> + before="$(git rev-parse --verify HEAD)" && >>> + test_tick && >>> + git rebase --rebase-merges C && >> >> I don't quite follow what this part of the test is for > > The test is modeled after the existing test "do not rebase cousins > unless asked for". First, it verifies that --rebase-merges rebases the > merges, which in this case results in no changes to the branch. Then, > it verifies that `git rebase` without arguments flattens the history. I think "do not rebase cousins unless asked for" is a bit different because it is checking the default for --rebase-merges which seems reasonable. I cannot see the point of checking that --rebase-merges works in this test as we have a whole file of tests that already do that. Best Wishes Phillip >>> +test_expect_success '--no-rebase-merges countermands --rebase-merges' ' >>> + git checkout -b no-rebase-merges E && >>> + git rebase --rebase-merges --no-rebase-merges C && >>> + test_cmp_graph C.. <<-\EOF >>> + * B >>> + * D >>> + o C >>> + EOF >>> +' >> >> This test looks good. I think we could probably have just this second >> test > > I like having a test for the default behavior. Nevertheless, I am > happy to remove that test as requested. Does anyone else have an > opinion? > >> and squash this into the previous patch - improving the >> documentation and test coverage for --no-rebase-merges would make a nice >> self-contained commit. > > Sure, squashing the first two patches is no problem. > > Thanks for the feedback, > > -Alex