Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing
Karl Hasselström <kha@treskal.com> writes:
Show 6 quoted lines
> 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.
Show 31 quoted lines
> 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?
Show 12 quoted lines
> 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);
> }
Karl Hasselström
