Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing

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

Karl Hasselström <kha@treskal.com> writes:

> builtin-update-ref's option parsing was somewhat tricky to follow,
> especially if the -d option was given. This patch cleans it upp a bit
> (including fixing an out-of-bounds argv access), at the expense of
> making it a bit longer.
>
> Signed-off-by: Karl Hasselström <kha@treskal.com>

Longer is fine but I am afraid that this patch is not much easier to
follow than the original, does not fix anything, and introduces a new bug.

> diff --git a/builtin-update-ref.c b/builtin-update-ref.c
> index e90737c..0d3eb8e 100644
> --- a/builtin-update-ref.c
> +++ b/builtin-update-ref.c
> @@ -27,25 +27,29 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  	if (msg && !*msg)
>  		die("Refusing to perform update with empty message.");
>  
> -	if (argc < 2 || argc > 3)
> -		usage_with_options(git_update_ref_usage, options);
> -	refname = argv[0];
> -	value   = argv[1];
> -	oldval  = argv[2];
> -
> -	if (get_sha1(value, sha1))
> -		die("%s: not a valid SHA1", value);
> -
>  	if (delete) {
> -		if (oldval)
> +		if (argc != 2)
>  			usage_with_options(git_update_ref_usage, options);
> -		return delete_ref(refname, sha1);
> +		refname = argv[0];
> +		value = NULL;
> +		oldval = argv[1];
> +	} else {
> +		if (argc < 2 || argc > 3)
> +			usage_with_options(git_update_ref_usage, options);
> +		refname = argv[0];
> +		value = argv[1];
> +		oldval = argc > 2 ? argv[2] : NULL;

When (argc == 3), argv[2] has the old value string given on the command
line.  When (argc == 2), argv[2] has the terminating NULL pointer.  So
either case you can safely use argv[2].  You do not allow other cases
upfront, so I do not understand why you need this conditional expression?

IOW, I do not see "an out-of-bounds argv access" in the original, and you
are making this assignment harder to follow.

>  	}
>  
> +	if (value && *value && get_sha1(value, sha1))
> +		die("%s: not a valid SHA1", value);

Dropping *value in the sequence may fix it but I think this is wrong.

We used to barf if you said "git update-ref refs/heads/master '' master"
because it would be nonsense to give an empty string as the new value of
the ref, didn't we?  Doesn't this change break it?  Does your set of
additional tests in [1/3] catch it?

>  	hashclr(oldsha1);
>  	if (oldval && *oldval && get_sha1(oldval, oldsha1))
>  		die("%s: not a valid old SHA1", oldval);
>  
> -	return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
> -			  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
> +	if (delete)
> +		return delete_ref(refname, oldsha1);
> +	else
> +		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
> +				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
>  }