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>

Jeff King committed Mar 26, 2026 at 15:13 UTC 22b985ef193a18ec0e6602ea1838e3290a351dd6
1 file changed +15 -10
revision.c
+15 -10
@@ -2147,7 +2147,10 @@ static int handle_dotdot(const char *arg,
2147 static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
2148 {
2149 struct object_context oc = {0};
2150 - char *mark;
2150 + const char *mark;
2151 + char *arg_minus_at = NULL;
2152 + char *arg_minus_excl = NULL;
2153 + char *arg_minus_dash = NULL;
2154 struct object *object;
2155 struct object_id oid;
2156 int local_flags;
@@ -2174,18 +2177,17 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
2177
2178 mark = strstr(arg, "^@");
2179 if (mark && !mark[2]) {
2177 - *mark = 0;
2178 - if (add_parents_only(revs, arg, flags, 0)) {
2180 + arg_minus_at = xmemdupz(arg, mark - arg);
2181 + if (add_parents_only(revs, arg_minus_at, flags, 0)) {
2182 ret = 0;
2183 goto out;
2184 }
2182 - *mark = '^';
2185 }
2186 mark = strstr(arg, "^!");
2187 if (mark && !mark[2]) {
2186 - *mark = 0;
2187 - if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))
2188 - *mark = '^';
2188 + arg_minus_excl = xmemdupz(arg, mark - arg);
2189 + if (add_parents_only(revs, arg_minus_excl, flags ^ (UNINTERESTING | BOTTOM), 0))
2190 + arg = arg_minus_excl;
2191 }
2192 mark = strstr(arg, "^-");
2193 if (mark) {
@@ -2199,9 +2201,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
2201 }
2202 }
2203
2202 - *mark = 0;
2203 - if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
2204 - *mark = '^';
2204 + arg_minus_dash = xmemdupz(arg, mark - arg);
2205 + if (add_parents_only(revs, arg_minus_dash, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
2206 + arg = arg_minus_dash;
2207 }
2208
2209 local_flags = 0;
@@ -2236,6 +2238,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
2238
2239 out:
2240 object_context_release(&oc);
2241 + free(arg_minus_at);
2242 + free(arg_minus_excl);
2243 + free(arg_minus_dash);
2244 return ret;
2245 }
2246