Re: [PATCH 03/15] handle_revision_arg: stop using "dotdot" as a generic pointer

[Jeff King]

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

Re: [PATCH 03/15] handle_revision_arg: stop using "dotdot" as a generic pointer

[Junio C Hamano]

Jeff King  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 
> ---
> 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.

[PATCH 03/15] handle_revision_arg: stop using "dotdot" as a generic pointer

[Jeff King]

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 
---
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.

 revision.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/revision.c b/revision.c
index b6031fb35..9c683874d 100644
--- a/revision.c
+++ b/revision.c
@@ -1433,6 +1433,7 @@ int handle_revision_arg(const char *arg_, struct rev_info 
*revs, int flags, unsi
 {
struct object_context oc;
char *dotdot;
+   char *mark;
struct object *object;
unsigned char sha1[20];
int local_flags;
@@ -1529,33 +1530,33 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi
*dotdot = '.';
}
 
-   dotdot = strstr(arg, "^@");
-   if (dotdot && !dotdot[2]) {
-   *dotdot = 0;
+   mark = strstr(arg, "^@");
+   if (mark && !mark[2]) {
+   *mark = 0;
if (add_parents_only(revs, arg, flags, 0))
return 0;
-   *dotdot = '^';
+   *mark = '^';
}
-   dotdot = strstr(arg, "^!");
-   if (dotdot && !dotdot[2]) {
-   *dotdot = 0;
+   mark = strstr(arg, "^!");
+   if (mark && !mark[2]) {
+   *mark = 0;
if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | 
BOTTOM), 0))
-   *dotdot = '^';
+   *mark = '^';
}
-   dotdot = strstr(arg, "^-");
-   if (dotdot) {
+   mark = strstr(arg, "^-");
+   if (mark) {
int exclude_parent = 1;
 
-   if (dotdot[2]) {
+   if (mark[2]) {
char *end;
-   exclude_parent = strtoul(dotdot + 2, , 10);
+   exclude_parent = strtoul(mark + 2, , 10);
if (*end != '\0' || !exclude_parent)
return -1;
}
 
-   *dotdot = 0;
+   *mark = 0;
if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | 
BOTTOM), exclude_parent))
-   *dotdot = '^';
+   *mark = '^';
}
 
local_flags = 0;
-- 
2.13.0.219.g63f6bc368