Hi Dscho, On Thu, Sep 7, 2023 at 1:02 PM Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > Hi Christian & Elijah, > > On Thu, 7 Sep 2023, Christian Couder wrote: > > > From: Elijah Newren <newren@xxxxxxxxx> > > > > Let's set the rev walking options we need after calling > > setup_revisions() instead of before. This makes it clearer which options > > we need. > > In light of the currently open issue about command-line validation, this > change does more than this paragraph lets on: It hardcodes certain > settings, overriding (silently) any rev-list options the user might have > passed. > > Is there any chance that we can avoid this change? In the version 5 I just sent, I have changed the commit message and the code. The commit messages says: " replay: change rev walking options Let's set the rev walking options we need after calling setup_revisions() instead of before. This enforces options we always want. We want the command to work from older commits to newer ones by default, but we are Ok with letting users reverse that, using --reverse, if that's what they really want. Also we don't want history simplification, as we want to deal with all the commits in the affected range. " So about "revs.reverse" the code is now: revs.reverse = !revs.reverse; which seems to be what you suggested elsewhere. Apart from that I think it's fair to enforce some values for a few options. This way we can make the command work the way we want by default, get consistent behavior and avoid users shooting themselves in the foot for now. If more flexibility is needed and useful in the future, then we might allow it in future patches with proper justification, tests and docs. There is still a lot of flexibility left especially as the patch that disallowed some rev walking options and pathspecs has been removed as you suggested. > > Also we don't want history simplification, as we want to deal with all > > the commits in the affected range. > > This, however, is a good change. It deserves to live in its own commit, > with its own commit message, in particular because it is not obvious from > the attribute names which ones we're talking about (I guess it's `limited` > and `simplify_history`, not just the latter. We only enforce "revs.sort_order", "revs.topo_order" and yeah "revs.simplify_history". Also I don't think it makes much sense to separate these changes in different commits/patches. I am Ok to improve the commit message if you think it's worth it, but the patch series is designed to make it easy to review the changes from the "fast-rebase" test-tool to a good "replay" plumbing command, and I don't think separating those related changes would improve things much.
