| Message ID | 20240210074859.552497-9-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] sequencer: Do not require `allow_empty` for redundant commit options | expand |
Hello, There's a typo. On Saturday, 10 February 2024 08:43:56 CET Brian Lyles wrote: > +-- > ++ > +Note that this option species how to handle a commit that was not initially this option *specifies* > +empty, but rather became empty due to a previous commit. Commits that were > +initially empty will cause the cherry-pick to fail. To force the inclusion of > +those commits, use `--allow-empty`. > ++ > + > --keep-redundant-commits:: > - If a commit being cherry picked duplicates a commit already in the > - current history, it will become empty. By default these > - redundant commits cause `cherry-pick` to stop so the user can > - examine the commit. This option overrides that behavior and > - creates an empty commit object. Note that use of this option only > - results in an empty commit when the commit was not initially empty, > - but rather became empty due to a previous commit. Commits that were > - initially empty will cause the cherry-pick to fail. To force the > - inclusion of those commits use `--allow-empty`. > + Deprecated synonym for `--empty=keep`. > > --strategy=<strategy>:: > Use the given merge strategy. Should only be used once. Otherwise, documentation looks good to me. JN
On Sun, Feb 11, 2024 at 2:50 PM Jean-Noël AVILA <jn.avila@free.fr> wrote: > Hello, > > There's a typo. > > On Saturday, 10 February 2024 08:43:56 CET Brian Lyles wrote: >> +-- >> ++ >> +Note that this option species how to handle a commit that was not initially > > this option *specifies* Thank you for catching this, I'll correct it in v3.
Hi Brian On 10/02/2024 07:43, Brian Lyles wrote: > As with git-rebase(1) and git-am(1), git-cherry-pick(1) can result in a > commit being made redundant if the content from the picked commit is > already present in the target history. However, git-cherry-pick(1) does > not have the same options available that git-rebase(1) and git-am(1) have. > > There are three things that can be done with these redundant commits: > drop them, keep them, or have the cherry-pick stop and wait for the user > to take an action. git-rebase(1) has the `--empty` option added in commit > e98c4269c8 (rebase (interactive-backend): fix handling of commits that > become empty, 2020-02-15), which handles all three of these scenarios. > Similarly, git-am(1) got its own `--empty` in 7c096b8d61 (am: support > --empty=<option> to handle empty patches, 2021-12-09). > > git-cherry-pick(1), on the other hand, only supports two of the three > possiblities: Keep the redundant commits via `--keep-redundant-commits`, > or have the cherry-pick fail by not specifying that option. There is no > way to automatically drop redundant commits. > > In order to bring git-cherry-pick(1) more in-line with git-rebase(1) and > git-am(1), this commit adds an `--empty` option to git-cherry-pick(1). It > has the same three options (keep, drop, and stop), and largely behaves > the same. The notable difference is that for git-cherry-pick(1), the > default will be `stop`, which maintains the current behavior when the > option is not specified. > > The `--keep-redundant-commits` option will be documented as a deprecated > synonym of `--empty=keep`, and will be supported for backwards > compatibility for the time being. I'm leaning towards leaving `--keep-redundant-commits` alone. That introduces an inconsistency between `--keep-redundant-commits` and `--empty=keep` as the latter does not imply `--allow-empty` but it does avoid breaking existing users. We could document `--keep-redundant-commits` as predating `--empty` and behaving like `--empty=keep --allow-empty`. The documentation and implementation of the new option look good modulo the typo that has already been pointed out and a couple of small comments below. > +enum empty_action { > + EMPTY_COMMIT_UNSPECIFIED = 0, We tend to use -1 for unspecified options > + STOP_ON_EMPTY_COMMIT, /* output errors and stop in the middle of a cherry-pick */ > + DROP_EMPTY_COMMIT, /* skip with a notice message */ > + KEEP_EMPTY_COMMIT, /* keep recording as empty commits */ > +}; > +test_expect_success 'cherry-pick persists --empty=stop correctly' ' > + pristine_detach initial && > + # to make sure that the session to cherry-pick a sequence > + # gets interrupted, use a high-enough number that is larger > + # than the number of parents of any commit we have created > + mainline=4 && > + test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick && > + test_path_is_file .git/sequencer/opts && > + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits && > + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits > +' Thanks for adding these tests to check that the --empty option persists. Usually for tests like this we prefer to check the user visible behavior rather than the implementation details (I suspect we have some older tests that do the latter). To check the behavior we usually arrange for a merge conflict but using -m is a creative alternative, then we'd run "git cherry-pick --continue" and check that the commits that become empty have been preserved or dropped or that the cherry-pick stops. Best Wishes Phillip
On Thu, Feb 22, 2024 at 10:36 AM <phillip.wood123@gmail.com> wrote: > I'm leaning towards leaving `--keep-redundant-commits` alone. That > introduces an inconsistency between `--keep-redundant-commits` and > `--empty=keep` as the latter does not imply `--allow-empty` but it does > avoid breaking existing users. We could document > `--keep-redundant-commits` as predating `--empty` and behaving like > `--empty=keep --allow-empty`. The documentation and implementation of > the new option look good modulo the typo that has already been pointed > out and a couple of small comments below. I think I'm on board with leaving `--keep-redundant-commits` alone. I'm on the fence about having `--empty=keep` imply `--allow-empty` after seeing Junio's concerns. I laid out the options that I see in a reply to patch 6/8[1] and would appreciate input there. I'll adjust the details of this commit in v3 based on what we decide there. [1]: https://lore.kernel.org/git/17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk/ > >> +enum empty_action { >> + EMPTY_COMMIT_UNSPECIFIED = 0, > > We tend to use -1 for unspecified options Thanks, I'll update this in v3. >> +test_expect_success 'cherry-pick persists --empty=stop correctly' ' >> + pristine_detach initial && >> + # to make sure that the session to cherry-pick a sequence >> + # gets interrupted, use a high-enough number that is larger >> + # than the number of parents of any commit we have created >> + mainline=4 && >> + test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick && >> + test_path_is_file .git/sequencer/opts && >> + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits && >> + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits >> +' > > Thanks for adding these tests to check that the --empty option persists. > Usually for tests like this we prefer to check the user visible behavior > rather than the implementation details (I suspect we have some older > tests that do the latter). To check the behavior we usually arrange for > a merge conflict but using -m is a creative alternative, then we'd run > "git cherry-pick --continue" and check that the commits that become > empty have been preserved or dropped or that the cherry-pick stops. Indeed, I was modelling these new tests after other existing tests in this file. While I agree with you in theory, I am hesitant to make these new tests drastically different from the existing tests that are testing the same mechanisms (and appear to be very intentionally testing that the options are persisted in that config file). I'm also hesitant to update the existing tests as part of this series (primarily due to a lack of familiarity, and partially to avoid scope creep of the series). How concerned are you about the current implementation? Does it make sense to you to defer this suggestion to a future series that cleans up the tests to do more user-oriented checks?
Hi Brian On 23/02/2024 06:58, Brian Lyles wrote: > > On Thu, Feb 22, 2024 at 10:36 AM <phillip.wood123@gmail.com> wrote: > >> I'm leaning towards leaving `--keep-redundant-commits` alone. That >> introduces an inconsistency between `--keep-redundant-commits` and >> `--empty=keep` as the latter does not imply `--allow-empty` but it does >> avoid breaking existing users. We could document >> `--keep-redundant-commits` as predating `--empty` and behaving like >> `--empty=keep --allow-empty`. The documentation and implementation of >> the new option look good modulo the typo that has already been pointed >> out and a couple of small comments below. > > I think I'm on board with leaving `--keep-redundant-commits` alone. I'm > on the fence about having `--empty=keep` imply `--allow-empty` after > seeing Junio's concerns. I laid out the options that I see in a reply to > patch 6/8[1] and would appreciate input there. I'll adjust the details > of this commit in v3 based on what we decide there. I'll take a look at that in the next couple of days > [1]: https://lore.kernel.org/git/17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk/ >> >>> +enum empty_action { >>> + EMPTY_COMMIT_UNSPECIFIED = 0, >> >> We tend to use -1 for unspecified options > > Thanks, I'll update this in v3. > >>> +test_expect_success 'cherry-pick persists --empty=stop correctly' ' >>> + pristine_detach initial && >>> + # to make sure that the session to cherry-pick a sequence >>> + # gets interrupted, use a high-enough number that is larger >>> + # than the number of parents of any commit we have created >>> + mainline=4 && >>> + test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick && >>> + test_path_is_file .git/sequencer/opts && >>> + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits && >>> + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits >>> +' >> >> Thanks for adding these tests to check that the --empty option persists. >> Usually for tests like this we prefer to check the user visible behavior >> rather than the implementation details (I suspect we have some older >> tests that do the latter). To check the behavior we usually arrange for >> a merge conflict but using -m is a creative alternative, then we'd run >> "git cherry-pick --continue" and check that the commits that become >> empty have been preserved or dropped or that the cherry-pick stops. > > Indeed, I was modelling these new tests after other existing tests in > this file. While I agree with you in theory, I am hesitant to make these > new tests drastically different from the existing tests that are testing > the same mechanisms (and appear to be very intentionally testing that > the options are persisted in that config file). I'm also hesitant to > update the existing tests as part of this series (primarily due to a > lack of familiarity, and partially to avoid scope creep of the series). I certainly don't think it should be up to you to update the existing tests. I'm not sure adding more tests in the same pattern is a good idea though. Apart from the fact that they are testing an implementation detail rather than the user facing behavior they don't actually check that the option is respected by "git cherry-pick --continue", only that we save it when stopping for a conflict resolution. > How concerned are you about the current implementation? Does it make > sense to you to defer this suggestion to a future series that cleans up > the tests to do more user-oriented checks? I think adding tests that follow a pattern we want to change is just storing up work for the future and makes it less likely we'll improve things because it will be more work to do so. Best Wishes Phillip
Hi Phillip On Sun, Feb 25, 2024 at 10:57 AM <phillip.wood123@gmail.com> wrote: >> How concerned are you about the current implementation? Does it make >> sense to you to defer this suggestion to a future series that cleans up >> the tests to do more user-oriented checks? > > I think adding tests that follow a pattern we want to change is just > storing up work for the future and makes it less likely we'll improve > things because it will be more work to do so. That's fair. I'll rework these in v3. Thanks!
Hi Phillip and Junio On Sun, Feb 25, 2024 at 10:57 AM <phillip.wood123@gmail.com> wrote: > On 23/02/2024 06:58, Brian Lyles wrote: >> I think I'm on board with leaving `--keep-redundant-commits` alone. I'm >> on the fence about having `--empty=keep` imply `--allow-empty` after >> seeing Junio's concerns. I laid out the options that I see in a reply to >> patch 6/8[1] and would appreciate input there. I'll adjust the details >> of this commit in v3 based on what we decide there. > > I'll take a look at that in the next couple of days > >> [1]: https://lore.kernel.org/git/17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk/ I'm not quite sure what happened here, but it seems that: - The above link is to the wrong email, and - The email I meant to link to isn't showing up in the archive for some reason, despite clearly showing as sent in my mailbox Apologies for the confusion -- I'm not sure what happened. In an attempt to keep this conversation on track, I've copied my original attempted reply to Phillip's[2] and Junio's[3] replies below. [2]: https://lore.kernel.org/git/3f276e10-7b03-4480-a157-47a7648e7f19@gmail.com/ [3]: https://lore.kernel.org/git/xmqqwmqwcpf7.fsf@gitster.g/ On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@gmail.com> wrote: > > On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > > I agree that if we were starting from scratch there would be no reason > > to tie --apply-empty and --keep-redundant-commits together but I'm not > > sure it is worth the disruption of changing it now. We're about to add > > empty=keep which won't imply --allow-empty for anyone who wants that > > behavior and I still tend to think the practical effect of implying > > --allow-empty with --keep-redundant-commits is largely beneficial as I'm > > skeptical that users want to keep commits that become empty but not the > > ones that started empty. > > I think that's fair. I am okay dropping this potentially disruptive > change. > > It sounds like you are on board with `--empty=keep` not having the same > implication? > > That said... > > On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote: > > > I do not quite see a good reason to do the opposite, dropping > > commits that started out as empty but keeping the ones that have > > become empty. Such a behaviour has additional downside that after > > such a cherry-pick, when you cherry-pick the resulting history onto > > yet another base, your precious "were not empty but have become so > > during the initial cherry-pick" commits will appear as commits that > > were empty from the start. So I do not see much reason to allow the > > decoupling, even with the new "empty=keep" thing that does not imply > > "allow-empty". > > Junio -- can you clarify this part? > > > So I do not see much reason to allow the decoupling, even with the new > > "empty=keep" thing that does not imply "allow-empty" > > I'm not 100% sure if you are saying that you want `--empty=keep` to > *also* imply `--allow-empty`, or that you simply want > `--keep-redundant-commits` to continue implying `--allow-empty` > *despite* the new `--empty=keep` no implying the same. > > On the one hand, I agree with Phillip's sentiment of "if we were > starting from scratch there would be no reason to tie --apply-empty and > --keep-redundant-commits together" (though your points perhaps provide > such a reason). On the other, if both `--keep-redundant-commits` and > `--empty=keep` behave the same way, it makes sense to soft-deprecate > `--keep-redundant-commits` as I have currently done later in this > series. If they do not behave the same way, that deprecation makes less > sense and we have two very similar (but not quite identical) options. > > Just to make sure we're on the same page, the options I see are: > > - (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply > `--allow-empty`, `--keep-redundant-commits` is soft-deprecated > - (B): Both `--keep-redundant-commits` and `--empty=keep` imply > `--allow-empty`, `--keep-redundant-commits` is soft-deprecated > - (C): Both `--keep-redundant-commits` and `--empty=keep` imply > `--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated > as it is more descriptive as noted by Junio here[1] > - (D): `--keep-redundant-commits` continues to imply `--allow-empty` but > `--empty=keep` does not. `--keep-redundant-commits` is *not* > soft-deprecated as it behaves differently. > > (A) represents this v2 of the patch. > > I'm coming around to (B) based on Junio's workflow concerns, but to be > honest I am fine with any of these options. Junio, I *think* you're > saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay > with (D) based on your response -- how do you feel about (B)? > > [1]: https://lore.kernel.org/git/xmqq8r4gnd3c.fsf@gitster.g/ Thank you both for your review and insight!
Hi Brian On 26/02/2024 03:32, Brian Lyles wrote: > Hi Phillip and Junio > On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@gmail.com> wrote: >> >> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >>> I agree that if we were starting from scratch there would be no reason >>> to tie --apply-empty and --keep-redundant-commits together but I'm not >>> sure it is worth the disruption of changing it now. We're about to add >>> empty=keep which won't imply --allow-empty for anyone who wants that >>> behavior and I still tend to think the practical effect of implying >>> --allow-empty with --keep-redundant-commits is largely beneficial as I'm >>> skeptical that users want to keep commits that become empty but not the >>> ones that started empty. >> >> I think that's fair. I am okay dropping this potentially disruptive >> change. >> >> It sounds like you are on board with `--empty=keep` not having the same >> implication? Yes indeed >> That said... >> >> On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote: >> >>> I do not quite see a good reason to do the opposite, dropping >>> commits that started out as empty but keeping the ones that have >>> become empty. Such a behaviour has additional downside that after >>> such a cherry-pick, when you cherry-pick the resulting history onto >>> yet another base, your precious "were not empty but have become so >>> during the initial cherry-pick" commits will appear as commits that >>> were empty from the start. So I do not see much reason to allow the >>> decoupling, even with the new "empty=keep" thing that does not imply >>> "allow-empty". >> >> Junio -- can you clarify this part? >> >>> So I do not see much reason to allow the decoupling, even with the new >>> "empty=keep" thing that does not imply "allow-empty" >> >> I'm not 100% sure if you are saying that you want `--empty=keep` to >> *also* imply `--allow-empty`, or that you simply want >> `--keep-redundant-commits` to continue implying `--allow-empty` >> *despite* the new `--empty=keep` no implying the same. FWIW I read it as the latter, but I can't claim to know what was in Junio's mind when he wrote it. >> On the one hand, I agree with Phillip's sentiment of "if we were >> starting from scratch there would be no reason to tie --apply-empty and >> --keep-redundant-commits together" (though your points perhaps provide >> such a reason). On the other, if both `--keep-redundant-commits` and >> `--empty=keep` behave the same way, it makes sense to soft-deprecate >> `--keep-redundant-commits` as I have currently done later in this >> series. If they do not behave the same way, that deprecation makes less >> sense and we have two very similar (but not quite identical) options. >> >> Just to make sure we're on the same page, the options I see are: >> >> - (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply >> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated >> - (B): Both `--keep-redundant-commits` and `--empty=keep` imply >> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated >> - (C): Both `--keep-redundant-commits` and `--empty=keep` imply >> `--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated >> as it is more descriptive as noted by Junio here[1] >> - (D): `--keep-redundant-commits` continues to imply `--allow-empty` but >> `--empty=keep` does not. `--keep-redundant-commits` is *not* >> soft-deprecated as it behaves differently. >> >> (A) represents this v2 of the patch. >> >> I'm coming around to (B) based on Junio's workflow concerns, but to be >> honest I am fine with any of these options. Junio, I *think* you're >> saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay >> with (D) based on your response -- how do you feel about (B)? Yes, I'd prefer (D) as I think it gets confusing if some values of --empty imply --allow-empty and others don't. I could live with (B) though. Best Wishes Phillip
phillip.wood123@gmail.com writes: >>>> I do not quite see a good reason to do the opposite, dropping >>>> commits that started out as empty but keeping the ones that have >>>> become empty. Such a behaviour has additional downside that after >>>> such a cherry-pick, when you cherry-pick the resulting history onto >>>> yet another base, your precious "were not empty but have become so >>>> during the initial cherry-pick" commits will appear as commits that >>>> were empty from the start. So I do not see much reason to allow the >>>> decoupling, even with the new "empty=keep" thing that does not imply >>>> "allow-empty". >>> >>> Junio -- can you clarify this part? >>> >>>> So I do not see much reason to allow the decoupling, even with the new >>>> "empty=keep" thing that does not imply "allow-empty" >>> >>> I'm not 100% sure if you are saying that you want `--empty=keep` to >>> *also* imply `--allow-empty`, or that you simply want >>> `--keep-redundant-commits` to continue implying `--allow-empty` >>> *despite* the new `--empty=keep` no implying the same. > > FWIW I read it as the latter, but I can't claim to know what was in > Junio's mind when he wrote it. Given that "drop what was empty originally while keeping what became empty" would "lose" what it wanted to keep (i.e. the one that has become empty") when used to cherry-pick the result of doing such a cherry-pick, I do not think allowing such combination makes as much sense as the opposite "keep what was empty originally while dropping what became empty", which does not have such a property. And it does not matter if that is expressed via the combination of existing --allow-empty and --keep-redundant-commits options, or the newly proposed --empty=keep option. If we start allowing the "drop what was originally empty and keep what has become empty" combination if we make empty=keep not to imply --allow-empty, I do not think it is a good idea. That was what was on my mind when I wrote it. It may be that I was not following the discussion correctly, and making "empty=keep" not to imply "allow-empty" does *not* allow a request to "drop what was originally empty, keep what has become empty". If that is the case, then I have no objection to making "empty=keep" not to imply "allow-empty". Thanks.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index c88bb88822..a444d960b1 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -132,23 +132,35 @@ effect to your index in a row. keeps commits that were initially empty (i.e. the commit recorded the same tree as its parent). Commits which are made empty due to a previous commit will cause the cherry-pick to fail. To force the - inclusion of those commits, use `--keep-redundant-commits`. + inclusion of those commits, use `--empty=keep`. --allow-empty-message:: By default, cherry-picking a commit with an empty message will fail. This option overrides that behavior, allowing commits with empty messages to be cherry picked. +--empty=(drop|keep|stop):: + How to handle commits being cherry-picked that are redundant with + changes already in the current history. ++ +-- +`drop`;; + The commit will be dropped. +`keep`;; + The commit will be kept. +`stop`;; + The cherry-pick will stop when the commit is applied, allowing + you to examine the commit. This is the default behavior. +-- ++ +Note that this option species how to handle a commit that was not initially +empty, but rather became empty due to a previous commit. Commits that were +initially empty will cause the cherry-pick to fail. To force the inclusion of +those commits, use `--allow-empty`. ++ + --keep-redundant-commits:: - If a commit being cherry picked duplicates a commit already in the - current history, it will become empty. By default these - redundant commits cause `cherry-pick` to stop so the user can - examine the commit. This option overrides that behavior and - creates an empty commit object. Note that use of this option only - results in an empty commit when the commit was not initially empty, - but rather became empty due to a previous commit. Commits that were - initially empty will cause the cherry-pick to fail. To force the - inclusion of those commits use `--allow-empty`. + Deprecated synonym for `--empty=keep`. --strategy=<strategy>:: Use the given merge strategy. Should only be used once. diff --git a/builtin/revert.c b/builtin/revert.c index 48c426f277..27efb6284b 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -43,6 +43,31 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; } +enum empty_action { + EMPTY_COMMIT_UNSPECIFIED = 0, + STOP_ON_EMPTY_COMMIT, /* output errors and stop in the middle of a cherry-pick */ + DROP_EMPTY_COMMIT, /* skip with a notice message */ + KEEP_EMPTY_COMMIT, /* keep recording as empty commits */ +}; + +static int parse_opt_empty(const struct option *opt, const char *arg, int unset) +{ + int *opt_value = opt->value; + + BUG_ON_OPT_NEG(unset); + + if (!strcmp(arg, "stop")) + *opt_value = STOP_ON_EMPTY_COMMIT; + else if (!strcmp(arg, "drop")) + *opt_value = DROP_EMPTY_COMMIT; + else if (!strcmp(arg, "keep")) + *opt_value = KEEP_EMPTY_COMMIT; + else + return error(_("invalid value for '%s': '%s'"), "--empty", arg); + + return 0; +} + static int option_parse_m(const struct option *opt, const char *arg, int unset) { @@ -85,6 +110,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); const char *cleanup_arg = NULL; + enum empty_action empty_opt = EMPTY_COMMIT_UNSPECIFIED; int cmd = 0; struct option base_options[] = { OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), @@ -114,7 +140,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")), OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")), OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")), - OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")), + OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("deprecated: use --empty=keep instead")), + OPT_CALLBACK_F(0, "empty", &empty_opt, "(stop|drop|keep)", + N_("how to handle commits that become empty"), + PARSE_OPT_NONEG, parse_opt_empty), OPT_END(), }; options = parse_options_concat(options, cp_extra); @@ -134,6 +163,11 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, prepare_repo_settings(the_repository); the_repository->settings.command_requires_full_index = 0; + if (opts->action == REPLAY_PICK) { + opts->drop_redundant_commits = (empty_opt == DROP_EMPTY_COMMIT); + opts->keep_redundant_commits = opts->keep_redundant_commits || (empty_opt == KEEP_EMPTY_COMMIT); + } + if (cleanup_arg) { opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); opts->explicit_cleanup = 1; @@ -164,6 +198,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, "--keep-redundant-commits", opts->keep_redundant_commits, + "--empty", empty_opt != EMPTY_COMMIT_UNSPECIFIED, NULL); } diff --git a/sequencer.c b/sequencer.c index 3f41863dae..509a5244d2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2921,6 +2921,9 @@ static int populate_opts_cb(const char *key, const char *value, else if (!strcmp(key, "options.allow-empty-message")) opts->allow_empty_message = git_config_bool_or_int(key, value, ctx->kvi, &error_flag); + else if (!strcmp(key, "options.drop-redundant-commits")) + opts->drop_redundant_commits = + git_config_bool_or_int(key, value, ctx->kvi, &error_flag); else if (!strcmp(key, "options.keep-redundant-commits")) opts->keep_redundant_commits = git_config_bool_or_int(key, value, ctx->kvi, &error_flag); @@ -3465,6 +3468,9 @@ static int save_opts(struct replay_opts *opts) if (opts->allow_empty_message) res |= git_config_set_in_file_gently(opts_file, "options.allow-empty-message", "true"); + if (opts->drop_redundant_commits) + res |= git_config_set_in_file_gently(opts_file, + "options.drop-redundant-commits", "true"); if (opts->keep_redundant_commits) res |= git_config_set_in_file_gently(opts_file, "options.keep-redundant-commits", "true"); diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh index 2709cfc677..669416c158 100755 --- a/t/t3505-cherry-pick-empty.sh +++ b/t/t3505-cherry-pick-empty.sh @@ -90,7 +90,7 @@ test_expect_success 'cherry-pick a commit that becomes no-op (prep)' ' git commit -m "add file2 on the side" ' -test_expect_success 'cherry-pick a no-op without --keep-redundant' ' +test_expect_success 'cherry-pick a no-op with neither --keep-redundant nor --empty' ' git reset --hard && git checkout fork^0 && test_must_fail git cherry-pick main @@ -105,4 +105,25 @@ test_expect_success 'cherry-pick a no-op with --keep-redundant' ' test_cmp expect actual ' +test_expect_success 'cherry-pick a no-op with --empty=stop' ' + git reset --hard && + git checkout fork^0 && + test_must_fail git cherry-pick --empty=stop main 2>output && + test_grep "The previous cherry-pick is now empty" output +' + +test_expect_success 'cherry-pick a no-op with --empty=drop' ' + git reset --hard && + git checkout fork^0 && + git cherry-pick --empty=drop main && + test_commit_message HEAD -m "add file2 on the side" +' + +test_expect_success 'cherry-pick a no-op with --empty=keep' ' + git reset --hard && + git checkout fork^0 && + git cherry-pick --empty=keep main && + test_commit_message HEAD -m "add file2 on main" +' + test_done diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 72020a51c4..5f6c45dfe3 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -90,6 +90,46 @@ test_expect_success 'cherry-pick persists opts correctly' ' test_cmp expect actual ' +test_expect_success 'cherry-pick persists --empty=stop correctly' ' + pristine_detach initial && + # to make sure that the session to cherry-pick a sequence + # gets interrupted, use a high-enough number that is larger + # than the number of parents of any commit we have created + mainline=4 && + test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick && + test_path_is_file .git/sequencer/opts && + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits && + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits +' + +test_expect_success 'cherry-pick persists --empty=drop correctly' ' + pristine_detach initial && + # to make sure that the session to cherry-pick a sequence + # gets interrupted, use a high-enough number that is larger + # than the number of parents of any commit we have created + mainline=4 && + test_expect_code 128 git cherry-pick -s -m $mainline --empty=drop initial..anotherpick && + test_path_is_file .git/sequencer/opts && + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits && + echo "true" >expect && + git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits >actual && + test_cmp expect actual +' + +test_expect_success 'cherry-pick persists --empty=keep correctly' ' + pristine_detach initial && + # to make sure that the session to cherry-pick a sequence + # gets interrupted, use a high-enough number that is larger + # than the number of parents of any commit we have created + mainline=4 && + test_expect_code 128 git cherry-pick -s -m $mainline --empty=keep initial..anotherpick && + test_path_is_file .git/sequencer/opts && + echo "true" >expect && + git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits >actual && + test_cmp expect actual && + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits +' + test_expect_success 'revert persists opts correctly' ' pristine_detach initial && # to make sure that the session to revert a sequence diff --git a/t/t3515-cherry-pick-incompatible-options.sh b/t/t3515-cherry-pick-incompatible-options.sh index 6100ab64fd..b2780fdbf3 100755 --- a/t/t3515-cherry-pick-incompatible-options.sh +++ b/t/t3515-cherry-pick-incompatible-options.sh @@ -31,4 +31,18 @@ test_expect_success '--keep-redundant-commits is incompatible with operations' ' git cherry-pick --abort ' +test_expect_success '--empty 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 --empty=stop --continue 2>output && + test_grep "fatal: cherry-pick: --empty cannot be used with --continue" output && + test_must_fail git cherry-pick --empty=stop --skip 2>output && + test_grep "fatal: cherry-pick: --empty cannot be used with --skip" output && + test_must_fail git cherry-pick --empty=stop --abort 2>output && + test_grep "fatal: cherry-pick: --empty cannot be used with --abort" output && + test_must_fail git cherry-pick --empty=stop --quit 2>output && + test_grep "fatal: cherry-pick: --empty cannot be used with --quit" output && + git cherry-pick --abort +' + test_done
As with git-rebase(1) and git-am(1), git-cherry-pick(1) can result in a commit being made redundant if the content from the picked commit is already present in the target history. However, git-cherry-pick(1) does not have the same options available that git-rebase(1) and git-am(1) have. There are three things that can be done with these redundant commits: drop them, keep them, or have the cherry-pick stop and wait for the user to take an action. git-rebase(1) has the `--empty` option added in commit e98c4269c8 (rebase (interactive-backend): fix handling of commits that become empty, 2020-02-15), which handles all three of these scenarios. Similarly, git-am(1) got its own `--empty` in 7c096b8d61 (am: support --empty=<option> to handle empty patches, 2021-12-09). git-cherry-pick(1), on the other hand, only supports two of the three possiblities: Keep the redundant commits via `--keep-redundant-commits`, or have the cherry-pick fail by not specifying that option. There is no way to automatically drop redundant commits. In order to bring git-cherry-pick(1) more in-line with git-rebase(1) and git-am(1), this commit adds an `--empty` option to git-cherry-pick(1). It has the same three options (keep, drop, and stop), and largely behaves the same. The notable difference is that for git-cherry-pick(1), the default will be `stop`, which maintains the current behavior when the option is not specified. The `--keep-redundant-commits` option will be documented as a deprecated synonym of `--empty=keep`, and will be supported for backwards compatibility for the time being. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- Documentation/git-cherry-pick.txt | 32 +++++++++++------ builtin/revert.c | 37 ++++++++++++++++++- sequencer.c | 6 ++++ t/t3505-cherry-pick-empty.sh | 23 +++++++++++- t/t3510-cherry-pick-sequence.sh | 40 +++++++++++++++++++++ t/t3515-cherry-pick-incompatible-options.sh | 14 ++++++++ 6 files changed, 140 insertions(+), 12 deletions(-)