Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options

> From: Brian Lyles <brianmlyles@gmail.com>
> 
> Previously, a consumer of the sequencer that wishes to take advantage of
> either the `keep_redundant_commits` or `drop_redundant_commits` feature
> must also specify `allow_empty`.
> 
> The only consumer of `drop_redundant_commits` is `git-rebase`, which
> already allows empty commits by default and simply always enables
> `allow_empty`. `keep_redundant_commits` was also consumed by
> `git-cherry-pick`, which had to specify `allow-empty` when
> `keep_redundant_commits` was specified in order for the sequencer's
> `allow_empty()` to actually respect `keep_redundant_commits`.

> The latter is an interesting case: As noted in the docs, this means that
> `--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`:
> 
>    "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`:
> 
>    "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, today, the
> following series of commands would result 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.
> 
> In a future commit, an `--empty` option will be added to
> `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> available in that command. For that to be possible with the current
> implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> would need to specify `allow_empty` with `drop_redundant_commits` as
> well, which is an even less intuitive implication of `--allow-empty`: in
> order to prevent redundant commits automatically, initially-empty
> commits would need to be kept automatically.
>
> Instead, this commit rewrites the `allow_empty()` logic to remove the
> over-arching requirement that `allow_empty` be specified in order to
> reach any of the keep/drop behaviors. Only if the commit was originally
> empty will `allow_empty` have an effect.

> For some amount of backwards compatibility with the existing code and
> tests, I have opted to preserve the behavior of returning 0 when:
> 
> - `allow_empty` is specified, and
> - either `is_index_unchanged` or `is_original_commit_empty` indicates an
>    error

> This is primarily out of caution -- I am not positive what downstream
> impacts this might have.
> 
> Note that this commit is a breaking change: `--keep-redundant-commits`
> will no longer imply `--allow-empty`. It would be possible to maintain
> the current behavior of `--keep-redundant-commits` implying
> `--allow-empty` if it were needed to avoid a breaking change, but I
> believe that decoupling them entirely is the correct behavior.

> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> 
> Disclaimer: This is my first contribution to the git project, and thus
> my first attempt at submitting a patch via `git-send-email`. It is also
> the first time I've touched worked in C in over a decade, and I really
> didn't work with it much before that either. I welcome any and all
> feedback on what I may have gotten wrong regarding the patch submission
> process, the code changes, or my commit messages.

> This is the first in a series of commits that aims to introduce an
> `--empty` option to `git-cherry-pick` that provides the same flexibility
> as the `--empty` options for `git-rebase` and `git-am`, as well as
> improve the consistency in the values and documentation for this option
> across the three commands.

> The main thing that may be controversial with this particular commit is
> that I am proposing a breaking change. As described in the above
> message, I do not think that it makes sense to tie `--allow-empty` and
> `--keep-redundant-commits` together since they appear to be intended to
> work with different types of empty commits. That being said, if it is
> deemed unacceptable to make this breaking change, we can consider an
> alternative approach where we maintain the behavior of
> `--keep-redundant-commits` implying `--allow-empty`, while preventing
> the need for the future `--empty=drop` to have that same implication.