On 4/18/2023 1:54 AM, Elijah Newren wrote: > On Mon, Apr 17, 2023 at 7:05 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: >> >> On 4/15/2023 3:07 PM, Elijah Newren wrote: >>> In short, I don't want another limited rebase; I want a more generic tool. >> >> The primary value of git-replay (to me) is that we can do "simple" rebases >> without a worktree or user interaction. To me, simple rebases mean apply a >> linear sequence of commits from a single branch onto another branch (and >> specifying an "old base" is simple enough to include here). It also means >> that we abort completely if there is a conflict (and handle conflict >> resolution in later changes). >> >> The sooner we can deliver that, the better we can deliver on its potential >> (and expand its functionality to be more generic). > > Limiting the initial scope of this tool to a smaller set of features > is okay (and, in fact, Christian has already done that), but we really > need to avoid painting ourselves into a corner if we change the design > as part of that limiting. As I stated previously[1], I'm worried this > is happening. > > [1] https://lore.kernel.org/git/CABPp-BH7ZPBtV5Hu_-_yVdqVmiydW7_s_LYAtwbPuYSbRQiuQw@xxxxxxxxxxxxxx/, > under "from my view" > >> And this is generally my preference, but we shouldn't get functionality >> "by accident" > > All of the specific flags and cases you brought up so far were ones I > already carefully considered over a year ago, so the "by accident" > comment seems a little unfair. > >> but instead do it with full intention > > The intention is listed in the subject of the commit message of this > patch. I've also explicitly stated my desire on this list to make a > tool which replays based on a general range expression multiple > times[2][3][4][5]. And there are tests for general range expressions > in this series being reviewed. I don't understand why you might think > I didn't intend to use general range expressions. It's one thing to describe commit ranges, and another to include every possible rev-list option. > If you want to move git-replay away from setup_revisions(), at a > minimum I think we need a proposal that can be extended to the cases I > highlighted previously: > * allow specifying several negative revisions (or maybe even zero of them) > * allow specifying several positive revisions > * allow standard basic range syntax, i.e. A..B I think supporting these descriptive ranges is nice, but doesn't _need_ to be in v1 of the tool. If we need to bake them into the CLI from v1 in order to ensure compatibility, then I understand that. > * allow --first-parent > * allow --ancestry-path[=<commit>] > I think it should also be able to eventually support > * --branches[=<pattern>] > * --not > * --tags[=<pattern>] > * --remotes[=<pattern>] > * --glob=<pattern> > * --exclude=<glob-pattern> > * --all However, I think very few of these should be generally supported, and if there are reasons to include some then they should be motivated by a specific use case and tested directly. As it is, these tags do something in this v1, but we can't really be sure that they work because we're not testing them. That is my primary complaint. And testing each individually isn't enough, because they can combine in all sorts of ways. > That's a long list of things to parse, which setup_revisions() happens > to handle nicely. You are right that setup_revisions() also has lots > of options for generating non-contiguous history that may make little > or no sense for replay (and makes no sense for fast-export either). > So, I am willing to consider other solutions if anyone has one, but > only alternative solutions which can eventually handle the above > requirements. In particular, "three commits as positional arguments" > (with one implicitly being considered a negative ref) conflicts with > the first three bullet points above. The only way I can see using setup_revisions() safely is if you make sure to reject any arguments that start with "--" (or perhaps "-" because some of those options may have single-character versions). This could be extended to an allowlist if we find a motivation to include other options (I see "--not" as being a likely candidate) in patches that test that interaction. Or, could we extract the portion of setup_revisions() that parses just the revision ranges in to a new setup_revision_ranges() method? It could reject any options that are not directly about revision ranges. This option makes less sense if we are going the allowlist approach. Thanks, -Stolee
