On Mon, Apr 17, 2023 at 7:05 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > On 4/15/2023 3:07 PM, Elijah Newren wrote: > > On Fri, Apr 14, 2023 at 7:23 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > >> Sending arbitrary command-line arguments to setup_revisions() > >> creates an opportunity for behavior you are not expecting. > > >> [unstated: and what about other similar options?] > > > > I'd really rather not have an allowlist of which revision options > > should be allowed for use by git-replay. A denylist might be okay, if > > also implemented for fast-export, but I'm more of the opinion that we > > just document that both commands only make sense with "contiguous" > > history. > > > > Most importantly, though, at a minimum I do absolutely want to allow > > several negative revisions to be specified, several positive revisions > > to be specified, and special flags like --ancestry-path or > > --first-parent to be specified. There may well be additional flags, > > both current and future, that should be allowed too. > > > > 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. [2] https://lore.kernel.org/git/CABPp-BEAqP7maTVw82Qr8mn-sxPzXmHnE_mTKf2pg6hVYAJSUw@xxxxxxxxxxxxxx/ [3] https://lore.kernel.org/git/CABPp-BHmj+QCBFDrH77iNfEU41V=UDu7nhBYkAbCsbXhshJzzw@xxxxxxxxxxxxxx/ [4] https://lore.kernel.org/git/CABPp-BHARfYcsEM7Daeb7+vYxeB9Awo8=qbrOMXG6BQ0gX1RiA@xxxxxxxxxxxxxx/ [5] https://lore.kernel.org/git/CABPp-BEOV53oBoBp4YjiRfksZMmAADanZUUemhxwn7Wor=m-nA@xxxxxxxxxxxxxx/ > and strong testing. The series as it stands already has some good testing for multiple divergent branches, which already takes it beyond the simplistic cases you were focusing on. Yes, there should obviously be tests for even more cases, and yes I did leave this series in a work-in-progress state nearly a year ago. My Git time has been limited, and so I have tended to only work on things that needed short bursts of attention, like responding to the list, or the cache.h refactoring... But I'm kind of at a loss trying to understand this paragraph. Am I misinterpreting what you are saying? > I will always think that certain shortcuts should not be used. This > includes passing arguments directly to setup_revisions() and using > pathspec parsing when a path string will work just fine. > > If we have a need for a feature, then we should add it explicitly and > carefully. The surface area of setup_revisions() is far too large to have > confidence that we are not exposing bugs for users to trip on later. My original plan was to just extend rebase rather than create a new command. While it'd be an extraordinary amount of work to change rebase to work without touching the working tree or index, it didn't look completely insurmountable. The first thing I found that did look completely insurmountable was extending it to handle rebasing several divergent branches possibly sharing some common history. Since handling possibly-divergent branches was one of my primary motivating goals, I'm really rather unwilling to accept the suggestion that we should strip the tool and copy rebase's broken design, since that would prevent the tool from ever being extended to do what I want and what I *already* implemented. There may be some kind of middle ground we can find, but the suggestion elsewhere in this thread (to make git-replay take three commits as positional arguments, with one an assumed negative ref) is copying that exact inflexible design flaw. 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 * 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 There may well be others I missed in glancing over the list. 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.
