Message ID | 20230602102533.876905-1-christian.couder@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Introduce new `git replay` command | expand |
Christian Couder <christian.couder@gmail.com> writes: > # Changes between v2 and v3 > > Thanks to Elijah and Junio for their suggestions on the previous > version! The very few and minor changes compared to v2 are: > > * The patch series was rebased onto master at v2.41.0. It is good to say this than not to say it, but without "in order to ..." it does not help very much. I was hoping "by rebasing, we can avoid unnecessary conflicts with what happened in the upstream in the meantime, namely, modify-remove conflict of X is now gone" or something like that.
On Sat, Jun 3, 2023 at 3:42 AM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > # Changes between v2 and v3 > > > > Thanks to Elijah and Junio for their suggestions on the previous > > version! The very few and minor changes compared to v2 are: > > > > * The patch series was rebased onto master at v2.41.0. > > It is good to say this than not to say it, but without "in order to > ..." it does not help very much. I was hoping "by rebasing, we can > avoid unnecessary conflicts with what happened in the upstream in > the meantime, namely, modify-remove conflict of X is now gone" or > something like that. The reason was because v2 had conflicts with a patch series from Elijah that changed "#include ...". You asked me to squash a small patch that fixed it, and I did squash it into v3, but I thought it was safer to also rebase.
Hi Christian & Elijah, On Thu, 7 Sep 2023, Christian Couder wrote: > A previous commit changed `git replay` to make it accept standard > revision ranges using the setup_revisions() function. While this is a > good thing to make this command more standard and more flexible, it has > the downside of enabling many revision related options accepted and eaten > by setup_revisions(). > > Some of these options might make sense, but others, like those > generating non-contiguous history, might not. Anyway those we might want > to allow should probably be tested and perhaps documented a bit, which > could be done in future work. > > For now it is just simpler and safer to just disallow all of them, so > let's do that. > > Other commands, like `git fast-export`, currently allow all these > revision specific options even though some of them might not make sense, > as these commands also use setup_revisions() but do not check the > options that might be passed to this function. > > So a way to fix those commands as well as git replay could be to improve > or refactor the setup_revisions() mechanism to let callers allow and > disallow options in a relevant way for them. Such improvements are > outside the scope of this work though. > > Pathspecs, which are also accepted and eaten by setup_revisions(), are > likely to result in disconnected history. That could perhaps be useful, > but that would need tests and documentation, which can be added in > future work. So, while at it, let's disallow them too. As pointed out elsewhere in this mail thread, I consider this patch to do more harm than good. After switching the command to plumbingmanipulators it should be possible to just forego all command-line validation and leave that job to the caller. Therefore I would love to see this patch dropped. Ciao, Johannes
Hi Dscho, On Thu, Sep 7, 2023 at 1:03 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > As pointed out elsewhere in this mail thread, I consider this patch to do > more harm than good. After switching the command to plumbingmanipulators > it should be possible to just forego all command-line validation and leave > that job to the caller. > > Therefore I would love to see this patch dropped. Ok, I have dropped this patch in version 5.