On Wed, May 24, 2017 at 11:45:39AM +0900, Junio C Hamano wrote:
> > 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.
I don't really know what corner cases I meant, either. :) I just saw
that the code looked funny, but nobody noticed for the common cases, so
I presumed any misbehavior would be from uncommon ones.
As far as "maybe not restoring is intentional", I wondered if there are
cases where we might allow multiple marks. E.g., if we wanted to allow
"foo^@^!", then we might need to progressively pull items off the end,
shortening the string. But I don't think that can be correct:
- these marks generally don't make sense to combine in the first place
- even if we allowed combinations, since we make only a single pass
through the function, we'd require the combinations to come in a
particular order
- even if it were intentional to do so, we'd still be adding weird
stuff to add_rev_cmdline(), as you noted
But I had some other questions, too, about what's supposed to be in the
"name" field of "pending" in some of these cases. For instance, try:
git tag foo 6a0bc7cf0efbefa5a949d958947e68e29534f04d
git log --oneline --source foo^-
All of the commits are marked as coming from "foo". That's because of
two things:
1. When we parse "^-", we turn the first character to NUL. So our call
to add_parents_only() sees just "foo", with no tail.
2. We never restore the "^". So later when we add "foo" itself, the
arg name is still "foo".
So arguably that's correct (these all came from "foo", which is a
resolvable name, and I think that's what the name field of
add_pending_object is going for). But that means add_rev_cmdline() sees
the same munged string, which is probably wrong. We can't possibly get
that case right with munging since the add_rev_cmdline() and
add_pending_object() calls come in pairs. We'd have to actually copy the
pending name into a separate string instead.
So like I said, I was sufficiently confused about what was supposed to
happen that I didn't try fixing it.
-Peff