From: phillip.wood123@gmail.com Date: Tue, 27 Feb 2024 10:39:33 GMT Subject: Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling Message-ID: <1004c565-ee6c-4aa4-8226-47d0ef7c8631@gmail.com> In-Reply-To: <17b74c2ffa1884ed.70b1dd9aae081c6e.203dcd72f6563036@zivdesk> 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 wrote: >> >> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood 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 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