| Message ID | 20240119060721.3734775-5-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 On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote: > From: Brian Lyles <brianmlyles@gmail.com> > ---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 > +--empty=(stop|drop|keep):: > + How to handle commits being cherry-picked that are redundant with > + changes already in the current history. > ++ > +-- Trailing whitespace on this line.
On Sat, Jan 20, 2024 at 2:24 PM Kristoffer Haugsbakk <code@khaugsbakk.name> wrote: > On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote: > > From: Brian Lyles <brianmlyles@gmail.com> > > ---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 > > +--empty=(stop|drop|keep):: > > + How to handle commits being cherry-picked that are redundant with > > + changes already in the current history. > > ++ > > +-- > > Trailing whitespace on this line. Thank you -- This will be corrected with v2. Is the sample pre-commit hook the ideal way to prevent this in the future? Or is there some config I could set globally to enforce this across repositories? I was having a little trouble finding a good way to accomplish this globally. Thanks, Brian Lyles
On Sun, Jan 21, 2024, at 19:28, Brian Lyles wrote: > Is the sample pre-commit hook the ideal way to prevent this in the > future? Or is there some config I could set globally to enforce this > across repositories? I was having a little trouble finding a good way to > accomplish this globally. I don’t know of any global config. So a pre-commit hook is probably the safest bet. Personally I set all my editors to remove trailing space and they very seldom mess it up. :)
Brian Lyles <brianmlyles@gmail.com> writes: >> Trailing whitespace on this line. > > Thank you -- This will be corrected with v2. > > Is the sample pre-commit hook the ideal way to prevent this in the > future? The example we ship in templates/hooks--pre-commit.sample in our source has such a check at the end of it.
Hi Brian On 21/01/2024 18:28, Brian Lyles wrote: > Is the sample pre-commit hook the ideal way to prevent this in the > future? Or is there some config I could set globally to enforce this > across repositories? I was having a little trouble finding a good way to > accomplish this globally. If you want to run the same hooks in all your repositories then you can run 'git config --global core.hooksPath <my-hooks-path>' and git will look for hooks in 'my-hooks-path' instead of '.git/hooks'. It makes it tricky to run different linters in different projects though. I'll try and take a proper look at these patches in the next couple of days. Best Wishes Phillip
On Sun, Jan 21, 2024, at 19:28, Brian Lyles wrote: > Thank you -- This will be corrected with v2. > > Is the sample pre-commit hook the ideal way to prevent this in the > future? Or is there some config I could set globally to enforce this > across repositories? I was having a little trouble finding a good way to > accomplish this globally. > > Thanks, > Brian Lyles Oh, and this thread reminded me https://lore.kernel.org/git/xmqqle8hrtcs.fsf@gitster.g/T/#t that editorconfig[1] has this option: ``` trim_trailing_whitespace = true ``` So I guess that should be enough for all editors that respect this config (although I haven’t tested it).
On Sun, Jan 21, 2024 at 4:05 PM Kristoffer Haugsbakk <code@khaugsbakk.name> wrote: > I don’t know of any global config. So a pre-commit hook is probably the > safest bet. Personally I set all my editors to remove trailing space and > they very seldom mess it up. :) Fair point -- Apparently this was a gap in my nvim config. Easily added. On Mon, Jan 22, 2024 at 2:55 PM Kristoffer Haugsbakk <code@khaugsbakk.name> wrote: > Oh, and this thread reminded me https://lore.kernel.org/git/xmqqle8hrtcs.fsf@gitster.g/T/#t > > that editorconfig[1] has this option: > > ``` > trim_trailing_whitespace = true > ``` > > So I guess that should be enough for all editors that respect this > config (although I haven’t tested it). > >
On Tue, Jan 23, 2024, at 06:23, Brian Lyles wrote: >> Oh, and this thread reminded me https://lore.kernel.org/git/xmqqle8hrtcs.fsf@gitster.g/T/#t >> >> that editorconfig[1] has this option: >> >> ``` >> trim_trailing_whitespace = true >> ``` >> […] > > Is there a good reason that this should not just be added to the > `.editorconfig` in this repository? Would a patch for this be welcome? I was thinking the same thing when I saw that other thread. It doesn’t hurt to try. I was also curious about some subtleties like: does it dictate enforcing this for all the lines of a touched files or only the modified ones? Because the latter is much more useful.
Hi Brian On 19/01/2024 05:59, brianmlyles@gmail.com wrote: > From: Brian Lyles <brianmlyles@gmail.com> > > As with `git-rebase` and `git-am`, `git-cherry-pick` 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` does > not have the same options available that `git-rebase` and `git-am` 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` 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` got its own `--empty` in 7c096b8d61 (am: support > --empty=<option> to handle empty patches, 2021-12-09). > > `git-cherry-pick`, 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` more in-line with `git-rebase` and > `git-am`, this commit adds an `--empty` option to `git-cherry-pick`. It > has the same three options (keep, drop, and stop), and largely behaves > the same. The notable difference is that for `git-cherry-pick`, the > default will be `stop`, which maintains the current behavior when the > option is not specified. Thanks for the well explained commit message > 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 not sure if we need to deprecate it as in "it will be removed in the future" or just reduce it prominence in favor of --empty > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > Documentation/git-cherry-pick.txt | 28 ++++++++++++++++++------- > builtin/revert.c | 35 ++++++++++++++++++++++++++++++- > sequencer.c | 6 ++++++ > t/t3505-cherry-pick-empty.sh | 26 ++++++++++++++++++++++- > 4 files changed, 86 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index 806295a730..8c20a10d4b 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -132,23 +132,37 @@ 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. > > ---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 > +--empty=(stop|drop|keep):: > + How to handle commits being cherry-picked that are redundant with > + changes already in the current history. > ++ > +-- > +`stop`;; I'm still on the fence about "stop" vs "ask". I see in your tests you've accidentally used "ask" which makes me wonder if that is the more familiar term for users who probably use "git rebase" more often than "git am". > + The cherry-pick will stop when the empty commit is applied, allowing > + you to examine the commit. This is the default behavior. > +`drop`;; > + The empty commit will be dropped. > +`keep`;; > + The empty commit will be kept. 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`. > +-- > ++ > +Note that commits which start empty will cause the cherry-pick to fail (unless > +`--allow-empty` is specified). > ++ > + > +--keep-redundant-commits:: > + 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 b2cfde7a87..1491c45e26 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -45,6 +45,30 @@ 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 { > + STOP_ON_EMPTY_COMMIT = 0, /* 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) > { > @@ -87,6 +111,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; > int cmd = 0; > struct option base_options[] = { > OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), > @@ -116,7 +141,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); > @@ -136,6 +164,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); > + } The code changes look good but I think we want to update verify_opt_compatible() to check for "--empty" being combined with "--continue" etc. as well. > if (cleanup_arg) { > opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); > opts->explicit_cleanup = 1; > diff --git a/sequencer.c b/sequencer.c > index 582bde8d46..c49c27c795 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2934,6 +2934,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); > @@ -3478,6 +3481,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"); It is good that we're saving the option - it would be good to add a test to check that we remember --empty after stopping for a conflict resolution. > 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 6adfd25351..ae0cf7886a 100755 > --- a/t/t3505-cherry-pick-empty.sh > +++ b/t/t3505-cherry-pick-empty.sh > @@ -89,7 +89,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 > @@ -104,4 +104,28 @@ 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=ask' ' > + git reset --hard && > + git checkout fork^0 && > + test_must_fail git cherry-pick --empty=ask main This is an example of why it is a good idea to check the error message when using "test_must_fail" as here the test will fail due to a bad value passed to "--empty" rather than for the reason we want the test to check. It would be good to add a separate test to check that we reject invalid "--empty" values. > +' > + > +test_expect_success 'cherry-pick a no-op with --empty=drop' ' > + git reset --hard && > + git checkout fork^0 && > + git cherry-pick --empty=drop main && > + git show -s --format=%s >actual && > + echo "add file2 on the side" >expect && > + test_cmp expect actual I think you could simplify this by using test_commit_message Best Wishes Phillip
Brian Lyles <brianmlyles@gmail.com> writes: > I do see that there are ~130 files with trailing whitespace in maint > today, though I suspect that most of those are not intentional. I got curious and took a look at files that has hits with "lines that end with SP or HT": $ git grep -l -e '[ ]$' There are some that can be cleaned up, but many of them are intentional. For example, CODE_OF_CONDUCT.md has these two (shown with $$$) I think can be removed without breaking markdown: Community Impact Guidelines were inspired by $$$ [Mozilla's code of conduct enforcement ladder][Mozilla CoC]. For answers to common questions about this code of conduct, see the FAQ at [https://www.contributor-covenant.org/faq][FAQ]. Translations are available $$$ at [https://www.contributor-covenant.org/translations][translations]. The one in Documentation/user-manual.txt is on borderline. They appear in a sample output from a command the user is typing (again, $$$ shows where the SP at the end of line is): diff --git a/init-db.c b/init-db.c index 65898fa..b002dc6 100644 --- a/init-db.c +++ b/init-db.c @@ -7,7 +7,7 @@ $$$ int main(int argc, char **argv) ... As its purpose is to show human readers what they see in their terminal should _look_ like, we _could_ do without the single space on these otherwise empty lines, which denotes an empty line that hasn't changed in the diff output. But it would no longer match byte-for-byte with what we are trying to illustrate. There are many hits from the above grep in t/t[0-9][0-9][0-9][0-9]/ directories; these are canonical/expected output used in tests and the spaces at the end of these lines are expected.
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for the well explained commit message ;-) >> 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 not sure if we need to deprecate it as in "it will be removed in > the future" or just reduce it prominence in favor of --empty True, especially since --empty=keep is much less descriptive and the part after "note that ..." below will take a long time before sticking in readers' brain. >> +--empty=(stop|drop|keep):: >> + How to handle commits being cherry-picked that are redundant with >> + changes already in the current history. It might make it easier to understand if we moved the description in 'keep' that says something about --allow-empty here, as it should apply to other two choices if I understand correctly. Let me try: This option specifies how a commit that is not originally empty but becomes a no-op when cherry-picked due to earlier changes already applied or already in the current history. Regardless of this this option, `cherry-pick` will fail on a commit that is empty in the original history---see `--allow-empty` to pass them intact. or something. Then the description of `keep` can become just as short as other two, e.g. a single sentence "The commit that becomes empty will be kept". >> ... >> + The cherry-pick will stop when the empty commit is applied, allowing >> + you to examine the commit. This is the default behavior. >> +`drop`;; >> + The empty commit will be dropped. >> +`keep`;; >> + The empty commit will be kept. 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`. >> +-- >> ++ >> +Note that commits which start empty will cause the cherry-pick to fail (unless >> +`--allow-empty` is specified).
Junio C Hamano <gitster@pobox.com> writes: > I got curious and took a look at files that has hits with "lines > that end with SP or HT": > > $ git grep -l -e '[ ]$' Another more interest way to check is to do this: $ git diff --check $(git hash-object --stdin -t tree </dev/null) which will not just catch trailing whitespaces but other whitespace errors. Good thing is that the projects can even customize the rules used for various paths using the attributes mechanism. Here is a sample patch based on what the above command line found. ------ >8 ----------- >8 ----------- >8 ----------- >8 ------ Subject: [PATCH] docs: a few whitespace fixes Some documentation files have 8-space indented lines where other indented lines in the same list use a single HT to indent. As Documentation/.gitattributes says *.txt files use the default whitespace rules, let's fix some of them as a practice. Note that git-add documentation has other instances of space indented lines, but they are samples of manually aligned output of the "git add -i" command and it would be better to keep them as-is. Which in turn may mean we may want to loosen the whitespace rules for some parts of the documentation files, but we currently do not have such a feature. The attribute based whitespace rules apply to the whole file. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/diff-format.txt | 8 ++++---- Documentation/git-add.txt | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git c/Documentation/diff-format.txt w/Documentation/diff-format.txt index a3ae8747a2..bcaf9ca608 100644 --- c/Documentation/diff-format.txt +++ w/Documentation/diff-format.txt @@ -8,16 +8,16 @@ These commands all compare two sets of things; what is compared differs: git-diff-index <tree-ish>:: - compares the <tree-ish> and the files on the filesystem. + compares the <tree-ish> and the files on the filesystem. git-diff-index --cached <tree-ish>:: - compares the <tree-ish> and the index. + compares the <tree-ish> and the index. git-diff-tree [-r] <tree-ish-1> <tree-ish-2> [<pattern>...]:: - compares the trees named by the two arguments. + compares the trees named by the two arguments. git-diff-files [<pattern>...]:: - compares the index and the files on the filesystem. + compares the index and the files on the filesystem. The "git-diff-tree" command begins its output by printing the hash of what is being compared. After that, all the commands print one output diff --git c/Documentation/git-add.txt w/Documentation/git-add.txt index ed44c1cb31..e1a5c27acd 100644 --- c/Documentation/git-add.txt +++ w/Documentation/git-add.txt @@ -73,7 +73,7 @@ in linkgit:gitglossary[7]. -v:: --verbose:: - Be verbose. + Be verbose. -f:: --force::
[+cc Junio] On Tue, Jan 23, 2024 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Brian > > > On 19/01/2024 05:59, brianmlyles@gmail.com wrote: > > > 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 not sure if we need to deprecate it as in "it will be removed in the > future" or just reduce it prominence in favor of --empty This is also related to Junio's comment: > On Tue, Jan 23, 2024 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote: > > True, especially since --empty=keep is much less descriptive and the > part after "note that ..." below will take a long time before > sticking in readers' brain. My primary motivation here was simply for consistency with `--empty` for both git-rebase(1) and git-am(1). In theory, I am not opposed to updating this patch to instead simply add a `--drop-redundant-commits` option if we feel that provides better usability. However, I think that the consistency would be better. I will happily defer to the group consensus here, with the options I see being: 1. No deprecation: just make `--keep-redundant-commits` a synonym of `--empty=keep` 2. Soft deprecation: Give a warning when `--keep-redundant-commits` is used 3. Add `--drop-redundant-commits` instead of `--empty` My preference would be 2, followed by 1 and then 3. > I'm still on the fence about "stop" vs "ask". I see in your tests you've > accidentally used "ask" which makes me wonder if that is the more > familiar term for users who probably use "git rebase" more often than > "git am". Oh, thank you for catching that. The cause here was actually because I had initially written these tests prior to adding the "ask -> stop" change rather than familiarity. I simply failed to update the tests after moving things around. > The code changes look good but I think we want to update > verify_opt_compatible() to check for "--empty" being combined with > "--continue" etc. as well. It looks like `--keep-redundant-commits` was not being included in these checks previously. I suspect that to be an oversight though. I can add this for v2. > > > if (cleanup_arg) { > > opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); > > opts->explicit_cleanup = 1; > > diff --git a/sequencer.c b/sequencer.c > > index 582bde8d46..c49c27c795 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -2934,6 +2934,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); > > @@ -3478,6 +3481,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"); > > It is good that we're saving the option - it would be good to add a test > to check that we remember --empty after stopping for a conflict resolution. I can add a test for this in v2 > > 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 6adfd25351..ae0cf7886a 100755 > > --- a/t/t3505-cherry-pick-empty.sh > > +++ b/t/t3505-cherry-pick-empty.sh > > @@ -89,7 +89,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 > > @@ -104,4 +104,28 @@ 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=ask' ' > > + git reset --hard && > > + git checkout fork^0 && > > + test_must_fail git cherry-pick --empty=ask main > > This is an example of why it is a good idea to check the error message > when using "test_must_fail" as here the test will fail due to a bad > value passed to "--empty" rather than for the reason we want the test to > check. It would be good to add a separate test to check that we reject > invalid "--empty" values. An excellent catch, thank you. Will be addressed in v2 > > +' > > + > > +test_expect_success 'cherry-pick a no-op with --empty=drop' ' > > + git reset --hard && > > + git checkout fork^0 && > > + git cherry-pick --empty=drop main && > > + git show -s --format=%s >actual && > > + echo "add file2 on the side" >expect && > > + test_cmp expect actual > > I think you could simplify this by using test_commit_message Thanks for pointing that function out -- you're absolutely right. Will be addressed in v2. Thanks for the review, Brian Lyles
Hi Junio On Tue, Jan 23, 2024 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Thanks for the well explained commit message > > ;-) > > >> 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 not sure if we need to deprecate it as in "it will be removed in > > the future" or just reduce it prominence in favor of --empty > > True, especially since --empty=keep is much less descriptive and the > part after "note that ..." below will take a long time before > sticking in readers' brain. I responded to this in my reply to Phillip, and CC'd you there. > >> +--empty=(stop|drop|keep):: > >> + How to handle commits being cherry-picked that are redundant with > >> + changes already in the current history. > > It might make it easier to understand if we moved the description in > 'keep' that says something about --allow-empty here, as it should > apply to other two choices if I understand correctly. Let me try: > > This option specifies how a commit that is not originally empty > but becomes a no-op when cherry-picked due to earlier changes > already applied or already in the current history. Regardless > of this this option, `cherry-pick` will fail on a commit that is > empty in the original history---see `--allow-empty` to pass them > intact. > > or something. Then the description of `keep` can become just as > short as other two, e.g. a single sentence "The commit that becomes > empty will be kept". Thank you for this suggestion. You are correct that the difference between `--empty` and `allow-empty` is relevant regardless of the option selected by the user. In fact, this entire tidbit is somewhat duplicative with the note I already added after the options: > Note that commits which start empty will cause the cherry-pick to fail > (unless `--allow-empty` is specified). I'll clean this up in v2. Here's what I am thinking currently: > --empty=(stop|drop|keep):: > How to handle commits being cherry-picked that are redundant with > changes already in the current history. > + > -- > `stop`;; > The cherry-pick will stop when the commit is applied, allowing > you to examine the commit. This is the default behavior. > `drop`;; > The commit will be dropped. > `keep`;; > The commit will be kept. > -- > + > 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`. > + Thank you, Brian Lyles
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 806295a730..8c20a10d4b 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -132,23 +132,37 @@ 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. ---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 +--empty=(stop|drop|keep):: + How to handle commits being cherry-picked that are redundant with + changes already in the current history. ++ +-- +`stop`;; + The cherry-pick will stop when the empty commit is applied, allowing + you to examine the commit. This is the default behavior. +`drop`;; + The empty commit will be dropped. +`keep`;; + The empty commit will be kept. 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`. +-- ++ +Note that commits which start empty will cause the cherry-pick to fail (unless +`--allow-empty` is specified). ++ + +--keep-redundant-commits:: + 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 b2cfde7a87..1491c45e26 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -45,6 +45,30 @@ 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 { + STOP_ON_EMPTY_COMMIT = 0, /* 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) { @@ -87,6 +111,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; int cmd = 0; struct option base_options[] = { OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), @@ -116,7 +141,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); @@ -136,6 +164,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; diff --git a/sequencer.c b/sequencer.c index 582bde8d46..c49c27c795 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2934,6 +2934,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); @@ -3478,6 +3481,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 6adfd25351..ae0cf7886a 100755 --- a/t/t3505-cherry-pick-empty.sh +++ b/t/t3505-cherry-pick-empty.sh @@ -89,7 +89,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 @@ -104,4 +104,28 @@ 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=ask' ' + git reset --hard && + git checkout fork^0 && + test_must_fail git cherry-pick --empty=ask main +' + +test_expect_success 'cherry-pick a no-op with --empty=drop' ' + git reset --hard && + git checkout fork^0 && + git cherry-pick --empty=drop main && + git show -s --format=%s >actual && + echo "add file2 on the side" >expect && + test_cmp expect actual +' + +test_expect_success 'cherry-pick a no-op with --empty=keep' ' + git reset --hard && + git checkout fork^0 && + git cherry-pick --empty=keep main && + git show -s --format=%s >actual && + echo "add file2 on main" >expect && + test_cmp expect actual +' + test_done