{"thread":{"id":"60764","subject":"[PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options","startedAt":"2024-01-19T06:13:28Z","lastAt":"2024-03-27T16:37:33Z","messageCount":118,"participants":["brianmlyles@gmail.com","Kristoffer Haugsbakk","Brian Lyles","Junio C Hamano","Phillip Wood","Elijah Newren","Jean-Noël AVILA","phillip.wood123@gmail.com","Dirk Gouders"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"487042","messageId":"20240119060721.3734775-2-brianmlyles@gmail.com","threadId":"60764","inReplyTo":null,"subject":"[PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options","fromName":"","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-01-19T05:59:17Z","receivedAt":"2024-01-19T06:13:28Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"From: Brian Lyles \n\nPreviously, a consumer of the sequencer that wishes to take advantage of\neither the `keep_redundant_commits` or `drop_redundant_commits` feature\nmust also specify `allow_empty`.\n\nThe only consumer of `drop_redundant_commits` is `git-rebase`, which\nalready allows empty commits by default and simply always enables\n`allow_empty`. `keep_redundant_commits` was also consumed by\n`git-cherry-pick`, which had to specify `allow-empty` when\n`keep_redundant_commits` was specified in order for the sequencer's\n`allow_empty()` to actually respect `keep_redundant_commits`.\n\nThe latter is an interesting case: As noted in the docs, this means that\n`--keep-redundant-commits` implies `--allow-empty`, despite the two\nhaving distinct, non-overlapping meanings:\n\n- `allow_empty` refers specifically to commits which start empty, as\n indicated by the documentation for `--allow-empty` within\n `git-cherry-pick`:\n\n \"Note also, that use of this option only keeps commits that were\n initially empty (i.e. the commit recorded the same tree as its\n parent). Commits which are made empty due to a previous commit are\n dropped. To force the inclusion of those commits use\n --keep-redundant-commits.\"\n\n- `keep_redundant_commits` refers specifically to commits that do not\n start empty, but become empty due to the content already existing in\n the target history. This is indicated by the documentation for\n `--keep-redundant-commits` within `git-cherry-pick`:\n\n \"If a commit being cherry picked duplicates a commit already in the\n current history, it will become empty. By default these redundant\n commits cause cherry-pick to stop so the user can examine the commit.\n This option overrides that behavior and creates an empty commit\n object. Implies --allow-empty.\"\n\nThis implication of `--allow-empty` therefore seems incorrect: One\nshould be able to keep a commit that becomes empty without also being\nforced to pick commits that start as empty. However, today, the\nfollowing series of commands would result in both the commit that became\nempty and the commit that started empty being picked despite only\n`--keep-redundant-commits` being specified:\n\n git init\n echo \"a\" >test\n git add test\n git commit -m \"Initial commit\"\n echo \"b\" >test\n git commit -am \"a -> b\"\n git commit --allow-empty -m \"empty\"\n git cherry-pick --keep-redundant-commits HEAD^ HEAD\n\nThe same cherry-pick with `--allow-empty` would fail on the redundant\ncommit, and with neither option would fail on the empty commit.\n\nIn a future commit, an `--empty` option will be added to\n`git-cherry-pick`, meaning that `drop_redundant_commits` will be\navailable in that command. For that to be possible with the current\nimplementation of the sequencer's `allow_empty()`, `git-cherry-pick`\nwould need to specify `allow_empty` with `drop_redundant_commits` as\nwell, which is an even less intuitive implication of `--allow-empty`: in\norder to prevent redundant commits automatically, initially-empty\ncommits would need to be kept automatically.\n\nInstead, this commit rewrites the `allow_empty()` logic to remove the\nover-arching requirement that `allow_empty` be specified in order to\nreach any of the keep/drop behaviors. Only if the commit was originally\nempty will `allow_empty` have an effect.\n\nFor some amount of backwards compatibility with the existing code and\ntests, I have opted to preserve the behavior of returning 0 when:\n\n- `allow_empty` is specified, and\n- either `is_index_unchanged` or `is_original_commit_empty` indicates an\n error\n\nThis is primarily out of caution -- I am not positive what downstream\nimpacts this might have.\n\nNote that this commit is a breaking change: `--keep-redundant-commits`\nwill no longer imply `--allow-empty`. It would be possible to maintain\nthe current behavior of `--keep-redundant-commits` implying\n`--allow-empty` if it were needed to avoid a breaking change, but I\nbelieve that decoupling them entirely is the correct behavior.\n\nSigned-off-by: Brian Lyles \n---\n\nDisclaimer: This is my first contribution to the git project, and thus\nmy first attempt at submitting a patch via `git-send-email`. It is also\nthe first time I've touched worked in C in over a decade, and I really\ndidn't work with it much before that either. I welcome any and all\nfeedback on what I may have gotten wrong regarding the patch submission\nprocess, the code changes, or my commit messages.\n\nThis is the first in a series of commits that aims to introduce an\n`--empty` option to `git-cherry-pick` that provides the same flexibility\nas the `--empty` options for `git-rebase` and `git-am`, as well as\nimprove the consistency in the values and documentation for this option\nacross the three commands.\n\nThe main thing that may be controversial with this particular commit is\nthat I am proposing a breaking change. As described in the above\nmessage, I do not think that it makes sense to tie `--allow-empty` and\n`--keep-redundant-commits` together since they appear to be intended to\nwork with different types of empty commits. That being said, if it is\ndeemed unacceptable to make this breaking change, we can consider an\nalternative approach where we maintain the behavior of\n`--keep-redundant-commits` implying `--allow-empty`, while preventing\nthe need for the future `--empty=drop` to have that same implication.\n\n Documentation/git-cherry-pick.txt | 10 +++++++---\n builtin/revert.c | 4 ----\n sequencer.c | 18 ++++++++++--------\n t/t3505-cherry-pick-empty.sh | 5 +++++\n 4 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex fdcad3d200..806295a730 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -131,8 +131,8 @@ effect to your index in a row.\n \teven without this option. Note also, that use of this option only\n \tkeeps commits that were initially empty (i.e. the commit recorded the\n \tsame tree as its parent). Commits which are made empty due to a\n-\tprevious commit are dropped. To force the inclusion of those commits\n-\tuse `--keep-redundant-commits`.\n+\tprevious commit will cause the cherry-pick to fail. To force the\n+\tinclusion of those commits use `--keep-redundant-commits`.\n \n --allow-empty-message::\n \tBy default, cherry-picking a commit with an empty message will fail.\n@@ -144,7 +144,11 @@ effect to your index in a row.\n \tcurrent history, it will become empty. By default these\n \tredundant commits cause `cherry-pick` to stop so the user can\n \texamine the commit. This option overrides that behavior and\n-\tcreates an empty commit object. Implies `--allow-empty`.\n+\tcreates an empty commit object. Note that use of this option only\n+\tresults in an empty commit when the commit was not initially empty,\n+\tbut rather became empty due to a previous commit. Commits that were\n+\tinitially empty will cause the cherry-pick to fail. To force the\n+\tinclusion of those commits use `--allow-empty`.\n \n --strategy=::\n \tUse the given merge strategy. Should only be used once.\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e6f9a1ad26..b2cfde7a87 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -136,10 +136,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n \n-\t/* implies allow_empty */\n-\tif (opts->keep_redundant_commits)\n-\t\topts->allow_empty = 1;\n-\n \tif (cleanup_arg) {\n \t\topts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);\n \t\topts->explicit_cleanup = 1;\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..582bde8d46 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,\n \t *\n \t * (4) we allow both.\n \t */\n-\tif (!opts->allow_empty)\n-\t\treturn 0; /* let \"git commit\" barf as necessary */\n-\n \tindex_unchanged = is_index_unchanged(r);\n-\tif (index_unchanged < 0)\n+\tif (index_unchanged < 0) {\n+\t\tif (!opts->allow_empty)\n+\t\t\treturn 0;\n \t\treturn index_unchanged;\n+\t}\n \tif (!index_unchanged)\n \t\treturn 0; /* we do not have to say --allow-empty */\n \n-\tif (opts->keep_redundant_commits)\n-\t\treturn 1;\n-\n \toriginally_empty = is_original_commit_empty(commit);\n-\tif (originally_empty < 0)\n+\tif (originally_empty < 0) {\n+\t\tif (!opts->allow_empty)\n+\t\t\treturn 0;\n \t\treturn originally_empty;\n+\t}\n \tif (originally_empty)\n+\t\treturn opts->allow_empty;\n+\telse if (opts->keep_redundant_commits)\n \t\treturn 1;\n \telse if (opts->drop_redundant_commits)\n \t\treturn 2;\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex eba3c38d5a..6adfd25351 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -59,6 +59,11 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '\n \ttest_must_fail git cherry-pick empty-change-branch\n '\n \n+test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' '\n+\tgit checkout main &&\n+\ttest_must_fail git cherry-pick --keep-redundant-commits empty-change-branch\n+'\n+\n test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '\n \tgit checkout main &&\n \tgit cherry-pick --allow-empty empty-change-branch\n-- \n2.41.0\n\n"},{"id":"487043","messageId":"20240119060721.3734775-3-brianmlyles@gmail.com","threadId":"60764","inReplyTo":"20240119060721.3734775-2-brianmlyles@gmail.com","subject":"[PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`","fromName":"","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-01-19T05:59:18Z","receivedAt":"2024-01-19T06:13:30Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"From: Brian Lyles \n\nBoth of these pages document very similar `--empty` options, but with\ndifferent styles. This commit aims to make them more consistent.\n\nIn a future commit, we'll be documenting a new `--empty` option for\n`git-cherry-pick`, making the consistency even more relevant.\n\nSigned-off-by: Brian Lyles \n---\n Documentation/git-am.txt | 18 ++++++++++++------\n Documentation/git-rebase.txt | 17 ++++++++++++-----\n 2 files changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex e080458d6c..77df5e606a 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -67,12 +67,18 @@ OPTIONS\n \tThis flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).\n \n --empty=(stop|drop|keep)::\n-\tBy default, or when the option is set to 'stop', the command\n-\terrors out on an input e-mail message lacking a patch\n-\tand stops in the middle of the current am session. When this\n-\toption is set to 'drop', skip such an e-mail message instead.\n-\tWhen this option is set to 'keep', create an empty commit,\n-\trecording the contents of the e-mail message as its log.\n+\tHow to handle an e-mail message lacking a patch:\n++\n+--\n+`stop`;;\n+\tThe command will fail, stopping in the middle of the current `am`\n+\tsession. This is the default behavior.\n+`drop`;;\n+\tThe e-mail message will be skipped.\n+`keep`;;\n+\tAn empty commit will be created, with the contents of the e-mail\n+\tmessage as its log.\n+--\n \n -m::\n --message-id::\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex b4526ca246..3ee85f6d86 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -293,13 +293,20 @@ See also INCOMPATIBLE OPTIONS below.\n \tHow to handle commits that are not empty to start and are not\n \tclean cherry-picks of any upstream commit, but which become\n \tempty after rebasing (because they contain a subset of already\n-\tupstream changes). With drop (the default), commits that\n-\tbecome empty are dropped. With keep, such commits are kept.\n-\tWith ask (implied by `--interactive`), the rebase will halt when\n-\tan empty commit is applied allowing you to choose whether to\n-\tdrop it, edit files more, or just commit the empty changes.\n+\tupstream changes):\n++\n+--\n+`drop`;;\n+\tThe empty commit will be dropped. This is the default behavior.\n+`keep`;;\n+\tThe empty commit will be kept.\n+`ask`;;\n+\tThe rebase will halt when the empty commit is applied, allowing you to\n+\tchoose whether to drop it, edit files more, or just commit the empty\n+\tchanges. This option is implied when `--interactive` is specified.\n \tOther options, like `--exec`, will use the default of drop unless\n \t`-i`/`--interactive` is explicitly specified.\n+--\n +\n Note that commits which start empty are kept (unless `--no-keep-empty`\n is specified), and commits which are clean cherry-picks (as determined\n-- \n2.41.0\n\n"},{"id":"487044","messageId":"20240119060721.3734775-4-brianmlyles@gmail.com","threadId":"60764","inReplyTo":"20240119060721.3734775-2-brianmlyles@gmail.com","subject":"[PATCH 3/4] rebase: Update `--empty=ask` to `--empty=drop`","fromName":"","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-01-19T05:59:19Z","receivedAt":"2024-01-19T06:13:31Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"From: Brian Lyles \n\nWhen `git-am` got its own `--empty` option in 7c096b8d61 (am: support\n--empty=