Hi Brian
On 26/02/2024 03:32, Brian Lyles wrote:
Hi Phillip and Junio
On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@xxxxxxxxx> wrote:
On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
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.
I think that's fair. I am okay dropping this potentially disruptive
change.
It sounds like you are on board with `--empty=keep` not having the same
implication?
Yes indeed
That said...
On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
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".
Junio -- can you clarify this part?
So I do not see much reason to allow the decoupling, even with the new
"empty=keep" thing that does not imply "allow-empty"
I'm not 100% sure if you are saying that you want `--empty=keep` to
*also* imply `--allow-empty`, or that you simply want
`--keep-redundant-commits` to continue implying `--allow-empty`
*despite* the new `--empty=keep` no implying the same.
FWIW I read it as the latter, but I can't claim to know what was in
Junio's mind when he wrote it.
On the one hand, I agree with Phillip's sentiment of "if we were
starting from scratch there would be no reason to tie --apply-empty and
--keep-redundant-commits together" (though your points perhaps provide
such a reason). On the other, if both `--keep-redundant-commits` and
`--empty=keep` behave the same way, it makes sense to soft-deprecate
`--keep-redundant-commits` as I have currently done later in this
series. If they do not behave the same way, that deprecation makes less
sense and we have two very similar (but not quite identical) options.
Just to make sure we're on the same page, the options I see are:
- (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply
`--allow-empty`, `--keep-redundant-commits` is soft-deprecated
- (B): Both `--keep-redundant-commits` and `--empty=keep` imply
`--allow-empty`, `--keep-redundant-commits` is soft-deprecated
- (C): Both `--keep-redundant-commits` and `--empty=keep` imply
`--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated
as it is more descriptive as noted by Junio here[1]
- (D): `--keep-redundant-commits` continues to imply `--allow-empty` but
`--empty=keep` does not. `--keep-redundant-commits` is *not*
soft-deprecated as it behaves differently.
(A) represents this v2 of the patch.
I'm coming around to (B) based on Junio's workflow concerns, but to be
honest I am fine with any of these options. Junio, I *think* you're
saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay
with (D) based on your response -- how do you feel about (B)?
Yes, I'd prefer (D) as I think it gets confusing if some values of
--empty imply --allow-empty and others don't. I could live with (B) though.
Best Wishes
Phillip
