Re: [PATCH 6/6] do not discard const: the ugly truth
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 26, 2026, 17:07 UTC
- Message-ID
- <xmqqfr5mr028.fsf@gitster.g>
- In-Reply-To
- <fe9c86af4825a81b2618ae8ffc8be12300058af2.1774537954.git.git@grubix.eu>
Michael J Gruber <git@grubix.eu> writes:
Show 6 quoted lines
> ISOC23 reveals that we mutate argv strings in place. Confess to this > with explicit casts. > --- > builtin/rev-parse.c | 8 ++++---- > revision.c | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-)
Forgot to sign-off?
Show 16 quoted lines
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 01a62800e8..f429793b6f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -265,7 +265,7 @@ static int show_file(const char *arg, int output_prefix)
> return 0;
> }
>
> -static int try_difference(const char *arg)
> +static int try_difference(char *arg)
> {
> char *dotdot;
> struct object_id start_oid;
> @@ -325,7 +325,7 @@ static int try_difference(const char *arg)
> return 0;
> }This one is unfortunate in that in the end the incoming arg is temporarily truncated by substituting the first "." in the ".." found in the string with "\0", and then restored to the original value before returning to the caller, so unless the caller is handing a piece of memory in a read-only segment, nobody should hurt or even notice.
Show 17 quoted lines
> -static int try_parent_shorthands(const char *arg)
> +static int try_parent_shorthands(char *arg)
> {
> char *dotdot;
> struct object_id oid;
> @@ -1145,9 +1145,9 @@ int cmd_rev_parse(int argc,
> }
>
> /* Not a flag argument */
> - if (try_difference(arg))
> + if (try_difference((char *) arg))
> continue;
> - if (try_parent_shorthands(arg))
> + if (try_parent_shorthands((char *) arg))
> continue;
> name = arg;
> type = NORMAL;The same, with "^" in magic sequences "^!", "^@", and "^-".
Show 13 quoted lines
> diff --git a/revision.c b/revision.c
> index 31808e3df0..a28b14a2ea 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2132,7 +2132,7 @@ static int handle_dotdot(const char *arg,
> int cant_be_filename)
> {
> struct object_context a_oc = {0}, b_oc = {0};
> - char *dotdot = strstr(arg, "..");
> + char *dotdot = (char *) strstr(arg, "..");
> int ret;
>
> if (!dotdot)The patch takes a different strategy to deal with this one, even though the pattern should be exactly the same as try_difference() we saw earlier. Shouldn't we take the same "internally we know we muck with the string temporarily, but externally we pretend that we take a const pointer because we revert our temporary modification" approach in builtin/rev-parse.c too?
One thing that _could_ break if we did so is when the callers do pass a string in read-only segment to these functions, trusting the function signature that takes a const pointer promises them that it is safe. And to prepare for it, the approach you took in builtin/rev-parse.c to be honest about it to the callers is safer.
So in that sense, perhaps this function should be updated to take a non-const pointer to arg instead of sprinkling casts in the body?
Show 24 quoted lines
> @@ -2176,7 +2176,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
> goto out;
> }
>
> - mark = strstr(arg, "^@");
> + mark = (char *) strstr(arg, "^@");
> if (mark && !mark[2]) {
> *mark = 0;
> if (add_parents_only(revs, arg, flags, 0)) {
> @@ -2185,13 +2185,13 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
> }
> *mark = '^';
> }
> - mark = strstr(arg, "^!");
> + mark = (char *) strstr(arg, "^!");
> if (mark && !mark[2]) {
> *mark = 0;
> if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))
> *mark = '^';
> }
> - mark = strstr(arg, "^-");
> + mark = (char *) strstr(arg, "^-");
> if (mark) {
> int exclude_parent = 1;Ditto.