Re: [PATCH 6/6] do not discard const: the ugly truth

[Junio C Hamano <gitster@pobox.com>]

Michael J Gruber <git@grubix.eu> writes:

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

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

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

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

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