| Message ID | 20240210074859.552497-8-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] sequencer: Do not require `allow_empty` for redundant commit options | expand |
Hi Brian On 10/02/2024 07:43, Brian Lyles wrote: > When `--keep-redundant-commits` was added in b27cfb0d8d > (git-cherry-pick: Add keep-redundant-commits option, 2012-04-20), it was > not marked as incompatible with the various operations needed to > continue or exit a cherry-pick (`--continue`, `--skip`, `--abort`, and > `--quit`). > > Enforce this incompatibility via `verify_opt_compatible` like we do for > the other various options. > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > > This commit was not present in v1 either. It addresses an existing issue > that I noticed after Phillip pointed out the same deficiency for my new > `--empty` option introduced in the ultimate commit in this series. Well spotted, do we really need a new test file just for this though? I wonder if the new test would be better off living in t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh Best Wishes Phillip > [1]: https://lore.kernel.org/git/CAHPHrSf+joHe6ikErHLgWrk-_qjSROS-dXCHagxWGDAF=2deDg@mail.gmail.com/ > > builtin/revert.c | 1 + > t/t3515-cherry-pick-incompatible-options.sh | 34 +++++++++++++++++++++ > 2 files changed, 35 insertions(+) > create mode 100755 t/t3515-cherry-pick-incompatible-options.sh > > diff --git a/builtin/revert.c b/builtin/revert.c > index d83977e36e..48c426f277 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -163,6 +163,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, > "--ff", opts->allow_ff, > "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, > "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, > + "--keep-redundant-commits", opts->keep_redundant_commits, > NULL); > } > > diff --git a/t/t3515-cherry-pick-incompatible-options.sh b/t/t3515-cherry-pick-incompatible-options.sh > new file mode 100755 > index 0000000000..6100ab64fd > --- /dev/null > +++ b/t/t3515-cherry-pick-incompatible-options.sh > @@ -0,0 +1,34 @@ > +#!/bin/sh > + > +test_description='test if cherry-pick detects and aborts on incompatible options' > + > +. ./test-lib.sh > + > +test_expect_success setup ' > + > + echo first > file1 && > + git add file1 && > + test_tick && > + git commit -m "first" && > + > + echo second > file1 && > + git add file1 && > + test_tick && > + git commit -m "second" > +' > + > +test_expect_success '--keep-redundant-commits is incompatible with operations' ' > + test_must_fail git cherry-pick HEAD 2>output && > + test_grep "The previous cherry-pick is now empty" output && > + test_must_fail git cherry-pick --keep-redundant-commits --continue 2>output && > + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --continue" output && > + test_must_fail git cherry-pick --keep-redundant-commits --skip 2>output && > + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --skip" output && > + test_must_fail git cherry-pick --keep-redundant-commits --abort 2>output && > + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --abort" output && > + test_must_fail git cherry-pick --keep-redundant-commits --quit 2>output && > + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --quit" output && > + git cherry-pick --abort > +' > + > +test_done
On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > Well spotted, do we really need a new test file just for this though? I > wonder if the new test would be better off living in > t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh I was modelling this off of 't3422-rebase-incompatible-options.sh'. Additionally, I do feel like these tests are only tangentially related to the tests that actually exercise the features themselves. Notably, the setup requirements are drastically different (simpler) since the test should fail long before any setup actually matters. For that reason, I think a separate file where other future tests for incompatible options can also live does make sense. Is there any particular downside to the new file that I am unaware of?
"Brian Lyles" <brianmlyles@gmail.com> writes: >> Well spotted, do we really need a new test file just for this though? I >> wonder if the new test would be better off living in >> t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh > > I was modelling this off of 't3422-rebase-incompatible-options.sh'. > Additionally, I do feel like these tests are only tangentially related > to the tests that actually exercise the features themselves. Notably, > the setup requirements are drastically different (simpler) since the > test should fail long before any setup actually matters. It is exactly the reason why we do not want to waste a scarce resource, the test file number, when we do not have to. Sanity checking the command line options and failing when they do not make sense is a small part of what a command needs to do; you are exactly right to say "they do not really exercise the main feature". As these "we expect this to fail" tests do not depend on and do not have to modify the state of the test repository, we do not even have to do any extra set-up over what the existing test script already establishes, no? No additional set-up is even better than a simpler but non-zero set-up we need to newly do, right?
Hi Brian On 23/02/2024 06:23, Brian Lyles wrote: > On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> Well spotted, do we really need a new test file just for this though? I >> wonder if the new test would be better off living in >> t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh > > I was modelling this off of 't3422-rebase-incompatible-options.sh'. The rebase case is more complicated due to different options being supported by the two different backends. Thankfully here we only have to worry about options that are incompatible with "--continue/--abort" and so adding "--continue rejects --foo" into the file that tests option "--foo" keeps everything together. > Additionally, I do feel like these tests are only tangentially related > to the tests that actually exercise the features themselves. Notably, > the setup requirements are drastically different (simpler) since the > test should fail long before any setup actually matters. For that > reason, I think a separate file where other future tests for > incompatible options can also live does make sense. > > Is there any particular downside to the new file that I am unaware of? The main downside is that it spreads the tests for a particular option over several test files. There is also an overhead in setting up the repository at the start of each test file. Best Wishes Phillip
Hi Phillip On Sun, Feb 25, 2024 at 10:58 AM <phillip.wood123@gmail.com> wrote: > Hi Brian > > On 23/02/2024 06:23, Brian Lyles wrote: >> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >>> Well spotted, do we really need a new test file just for this though? I >>> wonder if the new test would be better off living in >>> t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh >> >> I was modelling this off of 't3422-rebase-incompatible-options.sh'. > > The rebase case is more complicated due to different options being > supported by the two different backends. Thankfully here we only have to > worry about options that are incompatible with "--continue/--abort" and > so adding "--continue rejects --foo" into the file that tests option > "--foo" keeps everything together. > >> Additionally, I do feel like these tests are only tangentially related >> to the tests that actually exercise the features themselves. Notably, >> the setup requirements are drastically different (simpler) since the >> test should fail long before any setup actually matters. For that >> reason, I think a separate file where other future tests for >> incompatible options can also live does make sense. >> >> Is there any particular downside to the new file that I am unaware of? > > The main downside is that it spreads the tests for a particular option > over several test files. There is also an overhead in setting up the > repository at the start of each test file. That makes sense. I'll move these tests into `t3505-cherry-pick-empty.sh` for v3, along with the corresponding incompatibility tests for `--empty` introduced in the ultimate commit for the series.
diff --git a/builtin/revert.c b/builtin/revert.c index d83977e36e..48c426f277 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -163,6 +163,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, "--ff", opts->allow_ff, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, + "--keep-redundant-commits", opts->keep_redundant_commits, NULL); } diff --git a/t/t3515-cherry-pick-incompatible-options.sh b/t/t3515-cherry-pick-incompatible-options.sh new file mode 100755 index 0000000000..6100ab64fd --- /dev/null +++ b/t/t3515-cherry-pick-incompatible-options.sh @@ -0,0 +1,34 @@ +#!/bin/sh + +test_description='test if cherry-pick detects and aborts on incompatible options' + +. ./test-lib.sh + +test_expect_success setup ' + + echo first > file1 && + git add file1 && + test_tick && + git commit -m "first" && + + echo second > file1 && + git add file1 && + test_tick && + git commit -m "second" +' + +test_expect_success '--keep-redundant-commits is incompatible with operations' ' + test_must_fail git cherry-pick HEAD 2>output && + test_grep "The previous cherry-pick is now empty" output && + test_must_fail git cherry-pick --keep-redundant-commits --continue 2>output && + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --continue" output && + test_must_fail git cherry-pick --keep-redundant-commits --skip 2>output && + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --skip" output && + test_must_fail git cherry-pick --keep-redundant-commits --abort 2>output && + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --abort" output && + test_must_fail git cherry-pick --keep-redundant-commits --quit 2>output && + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --quit" output && + git cherry-pick --abort +' + +test_done
When `--keep-redundant-commits` was added in b27cfb0d8d (git-cherry-pick: Add keep-redundant-commits option, 2012-04-20), it was not marked as incompatible with the various operations needed to continue or exit a cherry-pick (`--continue`, `--skip`, `--abort`, and `--quit`). Enforce this incompatibility via `verify_opt_compatible` like we do for the other various options. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- This commit was not present in v1 either. It addresses an existing issue that I noticed after Phillip pointed out the same deficiency for my new `--empty` option introduced in the ultimate commit in this series. [1]: https://lore.kernel.org/git/CAHPHrSf+joHe6ikErHLgWrk-_qjSROS-dXCHagxWGDAF=2deDg@mail.gmail.com/ builtin/revert.c | 1 + t/t3515-cherry-pick-incompatible-options.sh | 34 +++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100755 t/t3515-cherry-pick-incompatible-options.sh