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

Jeff King [off-list ref] writes:

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)?

I don't have strong preference either way; the one in the patch was
modelled after setup_git_directory_gently(&nongit_ok), and slightly easier
to work with for existing callers that don't care enough.

quoted

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

Good point.  Let's change it to silently return -1 and let the caller take
care of it.  Perhaps there are some callers that say "does this name an
object?  If not, let's try pathname".

quoted

+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. :)

Can't we use an absolute date, given that test_tick gives fixed timestamp
sequence to pretend as if we were still in 2005 when we are running these
tests?

 sha1_name.c          |    4 ++--
 t/t0101-at-syntax.sh |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index f4a74fe..04fb3b8 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -398,9 +398,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 			int errors = 0;
 			char *tmp = xstrndup(str + at + 2, reflog_len);
 			at_time = approxidate_careful(tmp, &errors);
-			if (errors)
-				die("Bogus timestamp '%s'", tmp);
 			free(tmp);
+			if (errors)
+				return -1;
 		}
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {

diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
index ccabc37..5e298c5 100755
--- a/t/t0101-at-syntax.sh
+++ b/t/t0101-at-syntax.sh

@@ -26,8 +26,8 @@ test_expect_success '@{now} shows current' '
 	check_at @{now} two
 '
 
-test_expect_success '@{30.years.ago} shows old' '
-	check_at @{30.years.ago} one
+test_expect_success '@{2001-09-17} (before the first commit) shows old' '
+	check_at @{2001-09-17} one
 '
 
 test_expect_success 'silly approxidates work' '

On Wed, Jan 27, 2010 at 10:50:07AM -0800, Junio C Hamano wrote:

quoted

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)?

I don't have strong preference either way; the one in the patch was
modelled after setup_git_directory_gently(&nongit_ok), and slightly easier
to work with for existing callers that don't care enough.

Looks like you have already pushed out the original patch, so let's not
worry about it.

Yes, I don't know why I was so concerned with using a relative
approxidate when an absolute one would suffice. However, we should make
a matching change in the silly approxidate entry, too. Like this:

-- >8 --
Subject: [PATCH] t0101: use absolute date

The original version used relative approxidates, which don't
reproduce as reliably as absolute ones. Commit 6c647a fixed
this for one case, but missed the "silly" case.

Signed-off-by: Jeff King <redacted>
---
 t/t0101-at-syntax.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh
index 5e298c5..a1998b5 100755
--- a/t/t0101-at-syntax.sh
+++ b/t/t0101-at-syntax.sh

@@ -31,7 +31,7 @@ test_expect_success '@{2001-09-17} (before the first commit) shows old' '
 '
 
 test_expect_success 'silly approxidates work' '
-	check_at @{3.hot.dogs.and.30.years.ago} one
+	check_at @{3.hot.dogs.on.2001-09-17} one
 '
 
 test_expect_success 'notice misspelled upstream' '