| Message ID | 20200728233446.3066485-19-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SHA-256, part 3/3 | expand |
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.
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.
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' '
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(-)