[v5,18/39] t8002: make hash size independent
diff mbox series

Message ID 20200728233446.3066485-19-sandals@crustytoothpaste.net
State New
Headers show
Series
  • SHA-256, part 3/3
Related show

Commit Message

brian m. carlson July 28, 2020, 11:34 p.m. UTC
Compute the length of an object ID instead of hard-coding 40-based
values.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t8002-blame.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Eric Sunshine July 29, 2020, 2:24 a.m. UTC | #1
On Tue, Jul 28, 2020 at 7:35 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Compute the length of an object ID instead of hard-coding 40-based
> values.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> @@ -6,6 +6,10 @@ test_description='git blame'
> +test_expect_success 'setup' '
> +       hexsz=$(test_oid hexsz)
> +'

In the previous version of this series, the "setup" test  invoked
test_oid_init() as its very first step. This version doesn't. As a
reviewer, I was caught off-guard by this unexpected and unexplained
difference between versions. The script works fine without
test_oid_init() anyhow since test-lib.sh invokes test_oid_init(), so
the test_oid_init() call introduced here by the previous version was
redundant.

Some of the patches in this series add test_oid_init() calls to their
"setup" tests, while others don't, which makes for a somewhat
confusing impression as one reads the series. In general, it would be
nice for the patches to paint a consistent picture (i.e either
uniformly employ test_oid_init() or don't), however, I would not want
to see a re-roll just for that. Also, since the final patch of the
series ends up removing all those test_oid_init() calls anyhow, it's
all straightened out in the end.
brian m. carlson July 29, 2020, 10:31 p.m. UTC | #2
On 2020-07-29 at 02:24:42, Eric Sunshine wrote:
> In the previous version of this series, the "setup" test  invoked
> test_oid_init() as its very first step. This version doesn't. As a
> reviewer, I was caught off-guard by this unexpected and unexplained
> difference between versions. The script works fine without
> test_oid_init() anyhow since test-lib.sh invokes test_oid_init(), so
> the test_oid_init() call introduced here by the previous version was
> redundant.
> 
> Some of the patches in this series add test_oid_init() calls to their
> "setup" tests, while others don't, which makes for a somewhat
> confusing impression as one reads the series. In general, it would be
> nice for the patches to paint a consistent picture (i.e either
> uniformly employ test_oid_init() or don't), however, I would not want
> to see a re-roll just for that. Also, since the final patch of the
> series ends up removing all those test_oid_init() calls anyhow, it's
> all straightened out in the end.

Good point.  I'll try to remove them from the existing tests which add
them in the rest of the series.

Patch
diff mbox series

diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index eea048e52c..c3b70b025e 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -6,6 +6,10 @@  test_description='git blame'
 PROG='git blame -c'
 . "$TEST_DIRECTORY"/annotate-tests.sh
 
+test_expect_success 'setup' '
+	hexsz=$(test_oid hexsz)
+'
+
 test_expect_success 'blame untracked file in empty repo' '
 	>untracked &&
 	test_must_fail git blame untracked
@@ -105,17 +109,17 @@  test_expect_success 'blame --abbrev=<n> works' '
 '
 
 test_expect_success 'blame -l aligns regular and boundary commits' '
-	check_abbrev 40 -l HEAD &&
-	check_abbrev 39 -l ^HEAD
+	check_abbrev $hexsz         -l HEAD &&
+	check_abbrev $((hexsz - 1)) -l ^HEAD
 '
 
-test_expect_success 'blame --abbrev=40 behaves like -l' '
-	check_abbrev 40 --abbrev=40 HEAD &&
-	check_abbrev 39 --abbrev=40 ^HEAD
+test_expect_success 'blame --abbrev with full length behaves like -l' '
+	check_abbrev $hexsz         --abbrev=$hexsz HEAD &&
+	check_abbrev $((hexsz - 1)) --abbrev=$hexsz ^HEAD
 '
 
-test_expect_success '--no-abbrev works like --abbrev=40' '
-	check_abbrev 40 --no-abbrev
+test_expect_success '--no-abbrev works like --abbrev with full length' '
+	check_abbrev $hexsz --no-abbrev
 '
 
 test_expect_success '--exclude-promisor-objects does not BUG-crash' '