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
