From: Elijah Newren Date: Wed, 24 Dec 2025 03:03:07 GMT Subject: Re: [PATCH 0/2] replay: die descriptively when invalid commit-ish Message-ID: In-Reply-To: On Mon, Dec 22, 2025 at 2:04 PM wrote: > > From: Kristoffer Haugsbakk > > You get this error when you for example mistype the argument to > `--onto`: > > fatal: Replaying down to root commit is not supported yet! > > Consider that you might not know yourself that you have mistyped > something; then this looks even more puzzling. You might have given a > range like `main..topic` but the command says that it would need to > replay down to the root commit. > > The only thing that’s happened though is that `NULL` has been > interpreted in the wrong way. > > Let’s instead die immediately when the real error happens, in other > words when we can’t find the commit for the given commit-ish. > > Also add more regression tests. Thanks for working on this. I have nothing to add beyond what others have already commented on the series, except that I can maybe answer one question... > Somewhat unrelated to this change—and caveat not a C programmer—I was > confused by `determine_replay_mode`. Most of the function deals with > three cases: > > if (onto_name) { > ... > } else if (*advance_name) { > ... > } else { > ... > } > > But I don’t get the `else` since you now need to provide either `--onto` > or `--advance`. And both require an argument. So when can the > fallthrough `else` happen? > > I thought that maybe the `else` was old code that predated `--onto` and > `--advance` being mandatory. But that whole method and this check was > added in the same commit, in 22d99f01 (replay: add --advance or > 'cherry-pick' mode, 2023-11-24): > > if (!onto_name && !advance_name) { > error(_("option --onto or --advance is mandatory")); > > But just ignore this section if I’m simply confused. Your assumption was right -- that the final `else` was old code that predated `--onto` and `--advance` being mandatory. git-replay was originally developed in my private branch, but I let other folks know I was experimenting with it. When life pulled me away from git, Christian and Johannes expressed interest in the command, and Christian cleaned up my rough patches, stripping out the clearly half-baked stuff, and also removing some additional things that didn't need to be in the initial version, and sent it upstream. None of us noticed this `else` had turned into dead code in the review. The original had the idea that it would be able to sometimes guess the mode and allow for shorter command lines, but I was still playing around with it and haven't particularly felt the need to push that idea. Even if we eventually decide we want that other handling, removing this `else` clause for now and allowing it to be reintroduced later would probably make for a more coherent patch anyway.