git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Make t4201-shortlog.sh test more robust
@ 2017-11-12 15:25 Charles Bailey
  2017-11-12 16:11 ` Jeff King
  0 siblings, 1 reply; 3+ messages in thread
From: Charles Bailey @ 2017-11-12 15:25 UTC (permalink / raw)
  To: git

From: Charles Bailey <cbailey32@bloomberg.net>

The test for '--abbrev' in t4201-shortlog.sh assumes that the commits
generated in the test can always be uniquely abbreviated to 5 hex digits
but this is not always the case. If you were unlucky and happened to run
the test at (say) Thu Jun 22 03:04:49 2017 +0000, you would find that
the first commit generated would collide with a tree object created
later in the same test.

This can be simulated in the version of t4201-shortlog.sh prior to this
commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689
after sourcing test-lib.sh.

Change the test to test --abbrev=35 instead of --abbrev=5 to almost
completely avoid the possibility of a partial collision and add a call
to test_tick in the setup to make the test repeatable.

Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
---
 t/t4201-shortlog.sh | 5 +++--
 t/test-lib.sh       | 7 ++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 9df054b..da10478 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -9,6 +9,7 @@ test_description='git shortlog
 . ./test-lib.sh
 
 test_expect_success 'setup' '
+	test_tick &&
 	echo 1 >a1 &&
 	git add a1 &&
 	tree=$(git write-tree) &&
@@ -59,7 +60,7 @@ fuzz() {
 	file=$1 &&
 	sed "
 			s/$_x40/OBJECT_NAME/g
-			s/$_x05/OBJID/g
+			s/$_x35/OBJID/g
 			s/^ \{6\}[CTa].*/      SUBJECT/g
 			s/^ \{8\}[^ ].*/        CONTINUATION/g
 		" <"$file" >"$file.fuzzy" &&
@@ -81,7 +82,7 @@ test_expect_success 'pretty format' '
 
 test_expect_success '--abbrev' '
 	sed s/SUBJECT/OBJID/ expect.template >expect &&
-	git shortlog --format="%h" --abbrev=5 HEAD >log &&
+	git shortlog --format="%h" --abbrev=35 HEAD >log &&
 	fuzz log >log.predictable &&
 	test_cmp expect log.predictable
 '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9b61f16..116bd6a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -175,9 +175,10 @@ esac
 
 # Convenience
 #
-# A regexp to match 5 and 40 hexdigits
+# A regexp to match 5, 35 and 40 hexdigits
 _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
+_x35="$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
+_x40="$_x35$_x05"
 
 # Zero SHA-1
 _z40=0000000000000000000000000000000000000000
@@ -193,7 +194,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
+export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
 
 # Each test should start with something like this, after copyright notices:
 #
-- 
2.10.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Make t4201-shortlog.sh test more robust
  2017-11-12 15:25 [PATCH] Make t4201-shortlog.sh test more robust Charles Bailey
@ 2017-11-12 16:11 ` Jeff King
  2017-11-13  3:47   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2017-11-12 16:11 UTC (permalink / raw)
  To: Charles Bailey; +Cc: git

On Sun, Nov 12, 2017 at 03:25:23PM +0000, Charles Bailey wrote:

> From: Charles Bailey <cbailey32@bloomberg.net>
> 
> The test for '--abbrev' in t4201-shortlog.sh assumes that the commits
> generated in the test can always be uniquely abbreviated to 5 hex digits
> but this is not always the case. If you were unlucky and happened to run
> the test at (say) Thu Jun 22 03:04:49 2017 +0000, you would find that
> the first commit generated would collide with a tree object created
> later in the same test.
> 
> This can be simulated in the version of t4201-shortlog.sh prior to this
> commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689
> after sourcing test-lib.sh.
> 
> Change the test to test --abbrev=35 instead of --abbrev=5 to almost
> completely avoid the possibility of a partial collision and add a call
> to test_tick in the setup to make the test repeatable.

This generally makes sense to me, but I think there's one thing unsaid
in this last paragraph: why is it OK to switch from 5 to 35?

The answer is that the test is just checking that --abbrev is respected,
and doesn't care whether it's making things larger or smaller. There are
other cases of --abbrev=5 in the test suite which might fall into the
same boat.

You're also really doing two things to fix the problem here, either one
of which would have been sufficient: increasing the abbreviation size
and using test_tick to get a deterministic run.

I'm OK with doing that here, but some of the other --abbrev=5 cases
might already be fine due to using test_tick appropriately.

> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index 9df054b..da10478 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -9,6 +9,7 @@ test_description='git shortlog
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> +	test_tick &&

Charles and I had a discussion off-list whether it's appropriate to
test_tick once, but not for each commit (simply because it's a minor
pain to remember to do so before each commit).

Doing it once is enough to make the test deterministic, and for this
particular case we don't actually care at all whether all of the commits
have the exact same timestamp. So I think it's fine.

One option is to try replacing the ad-hoc commits with test_commit, but
it turns out to be quite ugly in this case, as the tests depend on a lot
of oddly-formatted commit messages.

-Peff

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Make t4201-shortlog.sh test more robust
  2017-11-12 16:11 ` Jeff King
@ 2017-11-13  3:47   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2017-11-13  3:47 UTC (permalink / raw)
  To: Jeff King; +Cc: Charles Bailey, git

Jeff King <peff@peff.net> writes:

> You're also really doing two things to fix the problem here, either one
> of which would have been sufficient: increasing the abbreviation size
> and using test_tick to get a deterministic run.
> ...
> Doing it once is enough to make the test deterministic, and for this
> particular case we don't actually care at all whether all of the commits
> have the exact same timestamp. So I think it's fine.

;-)

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-11-13  3:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-12 15:25 [PATCH] Make t4201-shortlog.sh test more robust Charles Bailey
2017-11-12 16:11 ` Jeff King
2017-11-13  3:47   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git