| Message ID | 20230221055805.210951-2-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] rebase: document the --no-rebase-merges option | expand |
Hi Alex Thanks for adding some tests, On 21/02/2023 05:58, Alex Henrie wrote: > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index fa2a06c19f..e0d910c229 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -250,6 +250,31 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' > EOF > ' > > +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 > + test_cmp_rev HEAD $before && > + test_tick && > + git rebase C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > +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 and squash this into the previous patch - improving the documentation and test coverage for --no-rebase-merges would make a nice self-contained commit. Best Wishes Phillip > test_expect_success 'do not rebase cousins unless asked for' ' > git checkout -b cousins main && > before="$(git rev-parse --verify HEAD)" &&
On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> 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. > > +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
Hi Alex On 22/02/2023 01:37, Alex Henrie wrote: > On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> 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
On Wed, Feb 22, 2023 at 3:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 22/02/2023 01:37, Alex Henrie wrote: > > On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> 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. I've removed the test in v4. Thanks again for the feedback. -Alex
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index fa2a06c19f..e0d910c229 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -250,6 +250,31 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' EOF ' +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 && + test_cmp_rev HEAD $before && + test_tick && + git rebase C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +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 +' + test_expect_success 'do not rebase cousins unless asked for' ' git checkout -b cousins main && before="$(git rev-parse --verify HEAD)" &&
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)