On Tue, Jan 26, 2010 at 11:58:00AM -0800, Junio C Hamano wrote:
> > The first one is that @{usptream} silently becomes @{0}. I think
> > we need to double-check whether approxidate found absolutely nothing,
> > and complain if that is the case.
>
> This is not a new problem introduced by Dscho's @{u} series; it was there
> even before 861f00e (fix reflog approxidate parsing bug, 2008-04-30).
True, though now that there is something besides an approxidate to
misspell, it is slightly worse. :)
> Introduce approxidate_careful() that takes an optional pointer to an
> integer, that gets assigned 1 when the input does not make sense as a
> timestamp.
A minor nit, but wouldn't:
int approxidate_careful(const char *str, unsigned long *out);
returning an error code be the more usual pattern for a function with
error-plus-output (your approxidate wrapper would have to be a function then,
not a macro)?
> As I am too lazy to fix all the callers that use approxidate(), most of
> the callers do not take advantage of the error checking, but convert the
> code to parse reflog to use it as a demonstration.
I think that is fine, and was the approach I was also going to take.
Approxidate was meant to be "try to make sense of anything". It is
mainly a problem here because we are combining it with other input, and
it is hard to tell a misspelling of that other input from a nonsensical
approxidate. Now that the _careful infrastructure is there, we can fix
other callsites if people care.
> @@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
> } else if (0 <= nth)
> at_time = 0;
> else {
> + int errors = 0;
> char *tmp = xstrndup(str + at + 2, reflog_len);
> - at_time = approxidate(tmp);
> + at_time = approxidate_careful(tmp, &errors);
> + if (errors)
> + die("Bogus timestamp '%s'", tmp);
> free(tmp);
I was just going to "return -1" here, which yields:
$ git show @{bogosity}
fatal: ambiguous argument '@{bogosity}': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
instead of
$ git show @{bogosity}
fatal: Bogus timestamp 'bogosity'
I can't imagine anybody being upset that they had a path '@{usptream}'
in their repository which was prematurely interpreted as a bogus ref
(especially since it is _always_ a ref in the current behavior), but it
just seemed more consistent with the rest of get_sha1_basic, which
generally does not die.
On the other hand, I think the error message the user sees in your case
is probably more helpful.
> +test_expect_success '@{30.years.ago} shows old' '
> + check_at @{30.years.ago} one
Side note: I chose this because we needed to go back from the current
time beyond where test_tick would place the commit. Which means this
test has a 2035 bug. :)
I had originally had 100.years.ago, but we seem to have some data-type
issues with our date handling. We represent seconds-since-epoch as
unsigned long, which means:
- even though approxidate handles it internally, we can't represent
dates earlier than the epoch. If you simply store approxidate's
output as a signed time_t, as test-date does, it does work back to
1901. But the reflog code treats it as unsigned, so 100.years.ago,
though representable, is considered to be far in the future.
- on many platforms ulong is 32 bits, meaning we have 2038 problems
(though because it's unsigned, I am not sure if we actually have
2106 problems; I suspect it may be a mix because of the way
different parts of the system use time_t versus ulong). Presumably
time_t will move to 64 bits in the next decade or two, and hopefully
ulong along with it.
Now obviously neither is a pressing issue. Probably nobody is importing
any pre-epoch commits, and we have a few decades to worry about future
issues. But I did legitimately see the bug when trying to guess a "long
time ago" value. It might be nice to use a signed time_t, and to deal
with the overflow (even if it is just to barf and say "bad time" instead
of silently producing wrong, future results).
-Peff
--
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