Hi Brian
Here are some code comments now I've realized why we need to change it
On 19/01/2024 05:59, brianmlyles@xxxxxxxxx wrote:
From: Brian Lyles <brianmlyles@xxxxxxxxx>
> Documentation/git-cherry-pick.txt | 10 +++++++---
> builtin/revert.c | 4 ----
> sequencer.c | 18 ++++++++++--------
> t/t3505-cherry-pick-empty.sh | 5 +++++
> 4 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt
b/Documentation/git-cherry-pick.txt
> index fdcad3d200..806295a730 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 e6f9a1ad26..b2cfde7a87 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -136,10 +136,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;
I'm wary of this, especially after Juino's comments in
https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/
The documentation changes look if we do decide to change the default.
> 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 d584cac8ed..582bde8d46 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
> *
> * (4) we allow both.
> */
The comment above should be updated if we change the behavior of this
function.
> - if (!opts->allow_empty)
> - return 0; /* let "git commit" barf as necessary */
> -
Here we stop returning early if allow_empty is not set - this allows us
to apply allow_empty only to commits that start empty below.
> index_unchanged = is_index_unchanged(r);
> - if (index_unchanged < 0)
> + if (index_unchanged < 0) {
> + if (!opts->allow_empty)
> + return 0;
> return index_unchanged;
> + }
I don't think we want to hide the error here or below from
originally_empty()
> if (!index_unchanged)
> return 0; /* we do not have to say --allow-empty */
>
> - if (opts->keep_redundant_commits)
> - return 1;
> -
We move this check so that we do not unconditionally keep commits that
are initially empty when opts->keep_redundant_commits is set.
> originally_empty = is_original_commit_empty(commit);
> - if (originally_empty < 0)
> + if (originally_empty < 0) {
> + if (!opts->allow_empty)
> + return 0;
> return originally_empty;
> + }
> if (originally_empty)
> + return opts->allow_empty;
Now opts->allow_empty only applies to commits that were originally empty
> + else if (opts->keep_redundant_commits)
> return 1;
This ensures we keep commits that become empty when
opts->redundant_commits is set.
The changes to allow_empty() all looks good apart from hiding errors
from index_unchanged() and originally_empty()
Best Wishes
Phillip
