revision: avoid writing to const string for parent marks

We take in a "const char *", but may write a NUL into it when parsing
parent marks like "foo^-", since we want to isolate "foo" as a string
for further parsing. This is usually OK, as our "const" strings are
often actually argv strings which are technically writeable, but we'd
segfault with a string literal like:

  handle_revision_arg("HEAD^-", &revs, 0, 0);

Similar to how we handled dotdot in a previous commit, we can avoid this
by making a temporary copy of the left-hand side of the string. The cost
should negligible compared to the rest of the parsing (like actually
parsing commits to create their parent linked-lists).

There is one slightly tricky thing, though. We parse some of the marks
progressively, so that if we see "foo^!" for example, we'll strip that
down to "foo" not just for calling add_parents_only(), but also for the
rest of the function. That makes sense since we eventually want to pass
"foo" to get_oid_with_context(). But it also means that we'll keep
looking for other marks. In particular, "foo^-^!" is valid, though oddly
"foo^!^-" would ignore the "^-". I'm not sure if this is a weird
historical artifact of the implementation, or if there are important
corner cases.

So I've left the behavior unchanged. Each mark we find allocates a
string with the mark stripped, which means we could allocate multiple
times (and carry a free-able pointer for each to the end). But in
practice we won't, because of the three marks, "^@" jumps immediately to
the end without further parsing, and "^-^!" is nonsense that nobody
would pass. So you'd get one allocation in general, and never more than
two.

Another obvious option would be to just copy "arg" up front and be OK
with munging it. But that means we pay the cost even when we find no
marks. We could make a single copy upon finding a mark and then munge,
but that adds extra code to each site (checking whether somebody else
allocated, and if not, adjusting our "mark" pointer to be relative to
the copied string).

I aimed for something that was clear and obvious, if a bit verbose.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/revision.c b/revision.c
index f61262436f5678cdae9afc8ac7b05a97bea7bab1..fda405bf65326a20ed7ed95552d3a0bd13519e2b 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -2147,7 +2147,10 @@ static int handle_dotdot(const char *arg,
 static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
        struct object_context oc = {0};
-       char *mark;
+       const char *mark;
+       char *arg_minus_at = NULL;
+       char *arg_minus_excl = NULL;
+       char *arg_minus_dash = NULL;
        struct object *object;
        struct object_id oid;
        int local_flags;
@@ -2174,18 +2177,17 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 
        mark = strstr(arg, "^@");
        if (mark && !mark[2]) {
-               *mark = 0;
-               if (add_parents_only(revs, arg, flags, 0)) {
+               arg_minus_at = xmemdupz(arg, mark - arg);
+               if (add_parents_only(revs, arg_minus_at, flags, 0)) {
                        ret = 0;
                        goto out;
                }
-               *mark = '^';
        }
        mark = strstr(arg, "^!");
        if (mark && !mark[2]) {
-               *mark = 0;
-               if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))
-                       *mark = '^';
+               arg_minus_excl = xmemdupz(arg, mark - arg);
+               if (add_parents_only(revs, arg_minus_excl, flags ^ (UNINTERESTING | BOTTOM), 0))
+                       arg = arg_minus_excl;
        }
        mark = strstr(arg, "^-");
        if (mark) {
@@ -2199,9 +2201,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
                        }
                }
 
-               *mark = 0;
-               if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
-                       *mark = '^';
+               arg_minus_dash = xmemdupz(arg, mark - arg);
+               if (add_parents_only(revs, arg_minus_dash, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
+                       arg = arg_minus_dash;
        }
 
        local_flags = 0;
@@ -2236,6 +2238,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 
 out:
        object_context_release(&oc);
+       free(arg_minus_at);
+       free(arg_minus_excl);
+       free(arg_minus_dash);
        return ret;
 }