Re: [PATCH 2/3] Clean up builtin-update-ref's option parsing
- From
Karl Hasselström <kha@treskal.com>
- Date
- May 25, 2008, 19:32 UTC
- Message-ID
- <20080525193213.GB5674@diana.vm.bytemark.co.uk>
- In-Reply-To
- <7vk5hii5wn.fsf@gitster.siamese.dyndns.org>
On 2008-05-25 11:40:24 -0700, Junio C Hamano wrote:
Show 23 quoted lines
> > 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.Mmmph. The problem here was that I didn't know that argv is NULL-terminated. I just assumed it was bad to access anything beyond the given array length as is usually the case in C. But I really should have taken a hint from the existing code. Sorry for wasting your time with this.
Show 11 quoted lines
> > }
> >
> > + 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?Aaah, I see what you mean. Good point. The empty string should obviously not be allowed as the new ref. In the original code it _is_ allowed as the _old_ value, and interpreted as 0{40}.
I'll update the series. With, among other things, a comment that explains this.
-+-
Many thanks for the review. I'll be back.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle