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

From: Phillip Wood <phillip.wood123@gmail.com>
To: brianmlyles@gmail.com, git@vger.kernel.org
Cc: me@ttaylorr.com, newren@gmail.com
Subject: Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
Date: Tue, 23 Jan 2024 14:23:52 +0000	[thread overview]
Message-ID: <7bf5036b-9f55-4451-a13c-8a2c815dfbb7@gmail.com> (raw)
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>

Hi Brian

Let me start by saying that overall I'm impressed with the quality of 
this submission. I've left quite a few comments but for a first patch 
series it is very good.

On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> 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`.

I think it might be more persuasive to start the commit message by 
explaining what user visible change you're trying to make and why rather 
than concentrating on the implementation details.

> 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.

Do you have a practical example of where you want to keep the commits 
that become empty but not the ones that start empty? I agree there is a 
distinction but I think the common case is that the user wants to keep 
both types of empty commit or none. I'm not against giving the user the 
option to keep one or the other if it is useful but I'm wary of changing 
the default.

> 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.

rebase always sets "opts->allow_empty = 1" in 
builtin/rebase.c:get_replay_opts() and if the user passes 
--no-keep-empty drops commits that start empty from the list of commits 
to be picked. This is slightly confusing but is more efficient as we 
don't do waste time trying to pick a commit we're going to drop. Can we 
do something similar for "git cherry-pick"? When cherry-picking a 
sequence of commits I think it should just work because the code is 
shared with rebase, for a single commit we'd need to add a test to see 
if it is empty in single_pick() before calling pick_commits().

> 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

I'm not sure that is a good idea as it is hiding an error that we didn't 
hit before because we returned early.

> 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.

Thank you for being clear about the change in behavior, as I said above 
I'm wary of changing the default unless there is a compelling reason but 
I'm happy to support

     git cherry-pick --keep-redundant-commits --no-allow-empty

if it is needed.

> 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.

As others have mentioned I think it would be useful to have a 
cover-letter where we can discuss the aim of the patch series 
independently of the individual patches.

> 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.

I think that is a good aim

> 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.

As I said above I think it would be worth looking at what "git rebase" 
does to see if we can do the same thing for "git cherry-pick".

 > [...]> +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

When using test_must_fail it is a good idea to check the error message 
to make sure that it's failing for the reason we expect (see patch 4).

Best Wishes

Phillip