git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Han-Wen Nienhuys <hanwen@google.com>
Subject: [PATCH 3/4] t5539: make timestamp requirements more explicit
Date: Thu, 9 Jul 2020 16:39:09 -0400	[thread overview]
Message-ID: <20200709203909.GC661064@coredump.intra.peff.net> (raw)
In-Reply-To: <20200709203336.GA2748777@coredump.intra.peff.net>

The test for "no shallow lines after receiving ACK ready" is very
sensitive to the timestamps of the commits we create. It's looking for
the fetch negotiation to send a "ready", which in turn depends on the
order in which we traverse commits during the negotiation.

It works reliably now because the base commit "7" is created without
test_commit, and thus gets a commit time matching the current system
clock. Whereas the new commits created in this test do use test_commit,
and get the usual test_tick time from 2005. So the fetch into the
"clone" repository results in a commit graph like this (I omitted some
of the "unrelated" commits for clarity; they're all just a sequence of
test_ticks):

  $ git log --graph --format='%ct %s %d'
  * 1112912953 new  (origin/master, origin/HEAD)
  * 1594322236 7  (grafted, master)
  * 1112912893 unrelated15  (origin/unrelated15, unrelated15)
  [...]
  * 1112912053 unrelated1  (origin/unrelated1, unrelated1)
  * 1112911993 new-too  (HEAD -> newnew, tag: new-too)

The important things to see are:

  - "7" is way in the future compared to the other commits

  - "new-too" in the fetching repo is older than "new" (and its
    "unrelated" ancestors) in the shallow repo

If we change our "setup shallow clone" step to use test_tick, too (and
get rid of the dependency on the system clock), then the test will fail.
The resulting graph looks like this:

  $ git log --graph --format='%ct %s %d'
  * 1112913373 new  (origin/master, origin/HEAD)
  * 1112912353 7  (grafted, master)
  * 1112913313 unrelated15  (origin/unrelated15, unrelated15)
  [...]
  * 1112912473 unrelated1  (origin/unrelated1, unrelated1)
  * 1112912413 new-too  (HEAD -> newnew, tag: new-too)

Our "new-too" is still older than "new" and "unrelated", but now "7" is
older than all of them (because it advanced test_tick, which the other
tests built on top of). In the original, we advertised "7" as the first
"have" before anything else, but now "new-too" is more recent. You'd see
the same thing in the unlikely event that the system clock was set
before our test_tick default in 2005.

Let's make the timing requirements more explicit. The important thing is
that the client advertise all of its shared commits first, before
presenting its unique "new-too" commit. We can do that and get rid of
the system clock dependency at the same time by creating all of the
shared commits around time X (using test_tick), and then creating
"new-too" with some time long before X. The resulting graph looks like
this:

  $ git log --graph --format='%ct %s %d'
  * 1500001380 new  (origin/master, origin/HEAD)
  * 1500000420 7  (grafted, master)
  * 1500001320 unrelated15  (origin/unrelated15, unrelated15)
  [...]
  * 1500000480 unrelated1  (origin/unrelated1, unrelated1)
  * 1400000060 new-too  (HEAD -> newnew, tag: new-too)

That also lets us get rid of the hacky test_tick added by f0e802ca20
(t5539: update a flaky test, 2014-07-14). That was clearly dancing
around the same problem, but only addressed the relationship between
commits created in the two subshells (which did use test_tick, but
overlapped because increments of test_tick in subshells are lost). Now
that we're using consistent and well-placed times for both lines of
history, we don't have to care about a one-tick difference between the
two sides.

Signed-off-by: Jeff King <peff@peff.net>
---
Curiously, the test still passes if "7" has the same timestamp as
"new-too"! That's why I didn't notice the failure in the original
thread, where I looked at setting the deterministic default time to the
default test_tick time. I didn't dig in, but I believe it only worked
because we currently use insertion order to break timestamp ties. And
since "master" sorts before "newnew", we'd queue "7" before "new-too".

 t/t5539-fetch-http-shallow.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index c0d02dee89..82aa99ae87 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -9,10 +9,12 @@ start_httpd
 commit() {
 	echo "$1" >tracked &&
 	git add tracked &&
+	test_tick &&
 	git commit -m "$1"
 }
 
 test_expect_success 'setup shallow clone' '
+	test_tick=1500000000 &&
 	commit 1 &&
 	commit 2 &&
 	commit 3 &&
@@ -48,7 +50,6 @@ EOF
 test_expect_success 'no shallow lines after receiving ACK ready' '
 	(
 		cd shallow &&
-		test_tick &&
 		for i in $(test_seq 15)
 		do
 			git checkout --orphan unrelated$i &&
@@ -66,6 +67,7 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
 	(
 		cd clone &&
 		git checkout --orphan newnew &&
+		test_tick=1400000000 &&
 		test_commit new-too &&
 		# NEEDSWORK: If the overspecification of the expected result is reduced, we
 		# might be able to run this test in all protocol versions.
-- 
2.27.0.730.gcc195a083d


  parent reply	other threads:[~2020-07-09 20:39 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-09 20:33 [PATCH 0/4] deterministic commit timestamps in tests Jeff King
2020-07-09 20:34 ` [PATCH 1/4] t6000: use test_tick consistently Jeff King
2020-07-09 20:35 ` [PATCH 2/4] t9700: loosen ident timezone regex Jeff King
2020-07-09 20:39 ` Jeff King [this message]
2020-07-09 20:42 ` [PATCH 4/4] test-lib: set deterministic default author/committer date Jeff King
2020-07-10 22:02 ` [PATCH 0/4] deterministic commit timestamps in tests Junio C Hamano
2020-07-14 12:31   ` Jeff King
2020-07-14 12:33     ` Jeff King
2020-07-14 21:47       ` Eric Wong
2020-07-15  7:42         ` Jeff King
2020-07-15 14:50           ` Junio C Hamano
2020-07-15 15:04             ` Junio C Hamano
2020-07-16 10:43               ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200709203909.GC661064@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=hanwen@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).