Hi Brian
On 27/01/2024 23:30, Brian Lyles wrote:
On Tue, Jan 23, 2024 at 8:23 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 19/01/2024 05:59, brianmlyles@xxxxxxxxx wrote:
From: Brian Lyles <brianmlyles@xxxxxxxxx>
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.
I struggled a bit with this initially because the motivation behind the
change in this particular commit was driven by a technical issue in my
mind. The side-effect with git-cherry-pick(s) `--allow-empty` and
`--keep-redundant-commits` was mildly problematic, but less concerning
that the future problem that we'd have once git-cherry-pick(1) got the
more robust `--empty` option in a later commit in this series.
I think my problem came down to this commit trying to solve two problems
at once -- the underlying technical concern _and_ the git-cherry-pick(1)
behavior.
In v2, I intend to break this commit into two:
- Update `allow_empty()` to not require `allow_empty`, but without
actually changing any consumers (and thus without making any
functional change)
- Update git-cherry-pick(1) such that `--keep-redundant-commits` no
longer implies `--allow-empty`.
This allows me to better justify the technical change technically and
the functional change functionally, while also making it easier to drop
the functional change if we decide that a breaking change is not
warranted to address this.
That sounds like a good strategy. I'm wondering if we should not change
the behavior of `--keep-redundant-commits` to avoid breaking existing
users but have `--empty=keep|drop` not imply `--allow-empty`. What ever
we do we'll annoy someone. It is confusing to have subtly different
behaviors for `--keep-redundant-commits` and `--empty=keep` but it
avoids breaking existing users. If we change `--keep-redundant-commits`
we potentially upset existing users but we don't confuse others with the
subtle difference between the two.
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.
That practical example is documented in the initial discussion[1], which
I should have ought to have linked in a cover letter for this series
(and will do so in v2). I'll avoid copying the details here, but we'd
very much like to be able to programmatically drop the commits that
become empty when doing the automated cherry-pick described there.
[1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@xxxxxxxxxxxxxx/
Maybe I've missed something but that seems to be an argument for
implementing `--empty=drop` which is completely reasonable but doesn't
explain why someone using `--keep-redundant-commits` would want to keep
the commits that become empty while dropping the commits that start empty.
[...]
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.
I totally understand being wary here.
I've certainly convinced myself that having the future `--empty=drop`
behavior introduced later in this patch should not imply
`--allow-empty`.
I agree with that
I also _think_ that the existing behavior of `--keep-redundant-commits`
is probably technically not ideal or correct, but could be convinced
that changing it now is not worthwhile. I will defer to group consensus
here.
There is definitely an argument that the existing behavior conflates the
two flavors of empty commit. I think one can also argue that the
conflation is beneficial as most of the time users don't care about the
distinction when using `--keep-redundant-commits` and so it is not worth
changing the behavior of the existing option.
I sounds like you've got a good plan for v2, I look forward to reading it.
Best Wishes
Phillip
