On 2008-05-25 11:40:24 -0700, Junio C Hamano wrote:
> > 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.
> > }
> >
> > + 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@xxxxxxxxxxx
www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
