Jeff King <peff@xxxxxxxx> writes: > The handle_revision_arg() function has a "dotdot" variable > that it uses to find a ".." or "..." in the argument. If we > don't find one, we look for other marks, like "^!". But we > just keep re-using the "dotdot" variable, which is > confusing. > > Let's introduce a separate "mark" variable that can be used > for these other marks. They still reuse the same variable, > but at least the name is no longer actively misleading. > > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > It may make sense to pull each of these into its own helper. I didn't > really look because they're so small, and because the return semantics > seemed confusing to me. Some of them return, and some of them keep > parsing. Some of them restore the NUL they overwrite, and some do not. > > I didn't dig in to see if there are weird corner cases where they > misbehave. I do not quite know what corner cases you meant, but I agree that many places in the codepath we forget to restore "^" we temporarily overwrite. I suspect that none of them is deliberately leaving "^" unrestored and they are just being careless (or they truly do not care because they assume nobody will look at arg later). And I think not restoring cannot be a correct thing to do. After all of these parsing, add_rev_cmdline() wants to see arg_ intact. But let's keep reading; perhaps they are addressed in a later patch, or they are left as-is, and either is OK.