| Message ID | 20240210074859.552497-7-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: > As noted in the git-cherry-pick(1) docs, `--keep-redundant-commits` > implies `--allow-empty`, despite the two having distinct, > non-overlapping meanings: > > - `allow_empty` refers specifically to commits which start empty, as > indicated by the documentation for `--allow-empty` within > git-cherry-pick(1): > > "Note also, that use of this option only 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 are > dropped. To force the inclusion of those commits use > --keep-redundant-commits." > > - `keep_redundant_commits` refers specifically to commits that do not > start empty, but become empty due to the content already existing in > the target history. This is indicated by the documentation for > `--keep-redundant-commits` within git-cherry-pick(1): > > "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. Implies --allow-empty." > > This implication of `--allow-empty` therefore seems incorrect: One > should be able to keep a commit that becomes empty without also being > forced to pick commits that start as empty. However, the following > series of commands results in both the commit that became empty and the > commit that started empty being picked, despite only > `--keep-redundant-commits` being specified: > > git init > echo "a" >test > git add test > git commit -m "Initial commit" > echo "b" >test > git commit -am "a -> b" > git commit --allow-empty -m "empty" > git cherry-pick --keep-redundant-commits HEAD^ HEAD > > The same cherry-pick with `--allow-empty` would fail on the redundant > commit, and with neither option would fail on the empty commit. > > Do not imply `--allow-empty` when using `--keep-redundant-commits` with > git-cherry-pick(1). > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > > This is the second half of the first commit[1] in v1, which has now been > split up. > > This commit proposes a breaking change, albeit one that seems correct > and relatively minor to me. If this change is deemed too controversial, > I am prepared to drop it from the series. See Junio's[2] and > Phillip's[3] comments on v1 for additional context. 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. Best Wishes Phillip > [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/ > [2]: https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/ > [3]: https://lore.kernel.org/git/8ff4650c-f84f-41bd-a46c-3b845ff29b70@gmail.com/ > > Documentation/git-cherry-pick.txt | 10 +++++++--- > builtin/revert.c | 4 ---- > t/t3505-cherry-pick-empty.sh | 6 ++++++ > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index fdcad3d200..c88bb88822 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -131,8 +131,8 @@ effect to your index in a row. > even without this option. Note also, that use of this option only > 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 are dropped. To force the inclusion of those commits > - use `--keep-redundant-commits`. > + previous commit will cause the cherry-pick to fail. To force the > + inclusion of those commits, use `--keep-redundant-commits`. > > --allow-empty-message:: > By default, cherry-picking a commit with an empty message will fail. > @@ -144,7 +144,11 @@ effect to your index in a row. > 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. Implies `--allow-empty`. > + 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`. > > --strategy=<strategy>:: > Use the given merge strategy. Should only be used once. > diff --git a/builtin/revert.c b/builtin/revert.c > index 89821bab95..d83977e36e 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -134,10 +134,6 @@ 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; > > - /* implies allow_empty */ > - if (opts->keep_redundant_commits) > - opts->allow_empty = 1; > - > if (cleanup_arg) { > opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); > opts->explicit_cleanup = 1; > diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh > index eba3c38d5a..2709cfc677 100755 > --- a/t/t3505-cherry-pick-empty.sh > +++ b/t/t3505-cherry-pick-empty.sh > @@ -59,6 +59,12 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' ' > test_must_fail git cherry-pick empty-change-branch > ' > > +test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' ' > + git checkout main && > + test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch 2>output && > + test_grep "The previous cherry-pick is now empty" output > +' > + > test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' ' > git checkout main && > git cherry-pick --allow-empty empty-change-branch
Phillip Wood <phillip.wood123@gmail.com> writes: > ... 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 share that feeling exactly. There are good reasons to keep a commit that starts as empty (as much as creating an empty commit in the first place), so if anything, a more common workflow element would be to drop the ones that have become unnecessary (e.g. because the upstream already added a change that is equivalent to what is being picked) while keeping the ones that are empty from the start (e.g. in some workflows an empty commit can be used as a container of metainfo---you can imagine that in an N-commit chain leading to the tip of a topic branch forked from the master branch, the topmost commit is an empty one with the cover letter being prepared, so that the resulting topic branch can be either (1) made into a patch series with an advanced version of "git format-patch" that knows how to use such an empty top commit in the cover letter message, or (2) merged to the mainline via a pull request, with an advanced version of "git merge" that notices the empty commit at the tip, and makes a merge with the commit topic~1 while using the empty top commit to write the message for the merge commit. 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". Thanks.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index fdcad3d200..c88bb88822 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -131,8 +131,8 @@ effect to your index in a row. even without this option. Note also, that use of this option only 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 are dropped. To force the inclusion of those commits - use `--keep-redundant-commits`. + previous commit will cause the cherry-pick to fail. To force the + inclusion of those commits, use `--keep-redundant-commits`. --allow-empty-message:: By default, cherry-picking a commit with an empty message will fail. @@ -144,7 +144,11 @@ effect to your index in a row. 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. Implies `--allow-empty`. + 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`. --strategy=<strategy>:: Use the given merge strategy. Should only be used once. diff --git a/builtin/revert.c b/builtin/revert.c index 89821bab95..d83977e36e 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -134,10 +134,6 @@ 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; - /* implies allow_empty */ - if (opts->keep_redundant_commits) - opts->allow_empty = 1; - if (cleanup_arg) { opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); opts->explicit_cleanup = 1; diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh index eba3c38d5a..2709cfc677 100755 --- a/t/t3505-cherry-pick-empty.sh +++ b/t/t3505-cherry-pick-empty.sh @@ -59,6 +59,12 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' ' test_must_fail git cherry-pick empty-change-branch ' +test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' ' + git checkout main && + test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch 2>output && + test_grep "The previous cherry-pick is now empty" output +' + test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' ' git checkout main && git cherry-pick --allow-empty empty-change-branch
As noted in the git-cherry-pick(1) docs, `--keep-redundant-commits` implies `--allow-empty`, despite the two having distinct, non-overlapping meanings: - `allow_empty` refers specifically to commits which start empty, as indicated by the documentation for `--allow-empty` within git-cherry-pick(1): "Note also, that use of this option only 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 are dropped. To force the inclusion of those commits use --keep-redundant-commits." - `keep_redundant_commits` refers specifically to commits that do not start empty, but become empty due to the content already existing in the target history. This is indicated by the documentation for `--keep-redundant-commits` within git-cherry-pick(1): "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. Implies --allow-empty." This implication of `--allow-empty` therefore seems incorrect: One should be able to keep a commit that becomes empty without also being forced to pick commits that start as empty. However, the following series of commands results in both the commit that became empty and the commit that started empty being picked, despite only `--keep-redundant-commits` being specified: git init echo "a" >test git add test git commit -m "Initial commit" echo "b" >test git commit -am "a -> b" git commit --allow-empty -m "empty" git cherry-pick --keep-redundant-commits HEAD^ HEAD The same cherry-pick with `--allow-empty` would fail on the redundant commit, and with neither option would fail on the empty commit. Do not imply `--allow-empty` when using `--keep-redundant-commits` with git-cherry-pick(1). Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- This is the second half of the first commit[1] in v1, which has now been split up. This commit proposes a breaking change, albeit one that seems correct and relatively minor to me. If this change is deemed too controversial, I am prepared to drop it from the series. See Junio's[2] and Phillip's[3] comments on v1 for additional context. [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/ [2]: https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/ [3]: https://lore.kernel.org/git/8ff4650c-f84f-41bd-a46c-3b845ff29b70@gmail.com/ Documentation/git-cherry-pick.txt | 10 +++++++--- builtin/revert.c | 4 ---- t/t3505-cherry-pick-empty.sh | 6 ++++++ 3 files changed, 13 insertions(+), 7 deletions(-)