Re: [PATCH 1/2] t1506: more test for @{upstream} syntax

From: Jeff King <peff@peff.net>
Date: Jan 27, 2010, 10:24 UTC
Message-ID: <20100127102417.GD6262@coredump.intra.peff.net>
In-Reply-To: <7vockgzliv.fsf@alter.siamese.dyndns.org>

On Tue, Jan 26, 2010 at 11:58:00AM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> > 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.

Show 11 quoted lines

> @@ -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