[1/2] t1450: robustify `remove_object()`

Commit Message

Johannes Schindelin Feb. 10, 2021, 6:01 p.m. UTC

From: Johannes Schindelin <[e-mail]>

This function can be simplified by using the `test_oid_to_path()`
helper, which incidentally also makes it more robust by not relying on
the exact file system layout of the loose object files.

While at it, do not define those functions in a test case, it buys us
nothing.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/t1450-fsck.sh | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Feb. 10, 2021, 8:36 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> -test_expect_success 'setup: helpers for corruption tests' '
> -	sha1_file() {
> -		remainder=${1#??} &&
> -		firsttwo=${1%$remainder} &&
> -		echo ".git/objects/$firsttwo/$remainder"
> -	} &&
> +sha1_file () {
> +	git rev-parse --git-path objects/$(test_oid_to_path "$1")
> +}

Yeah, back when 90cf590f (fsck: optionally show more helpful info
for broken links, 2016-07-17) originally introduced this pattern,
we didn't have nicely abstracted helper, but now we do, and there
is no reason not to use it.  Nice.

> -	remove_object() {
> -		rm "$(sha1_file "$1")"
> -	}
> -'
> +remove_object() {

Just like you did for the other one, let's insert SP before () for
consistency here.

> +	rm "$(sha1_file "$1")"
> +}
>  
>  test_expect_success 'object with bad sha1' '
>  	sha=$(echo blob | git hash-object -w --stdin) &&

Nicely done.  

Reviewed-by: Junio C Hamano <[e-mail]>

Taylor Blau Feb. 10, 2021, 11:20 p.m. UTC | #2

On Wed, Feb 10, 2021 at 12:36:19PM -0800, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > -test_expect_success 'setup: helpers for corruption tests' '
> > -	sha1_file() {
> > -		remainder=${1#??} &&
> > -		firsttwo=${1%$remainder} &&
> > -		echo ".git/objects/$firsttwo/$remainder"
> > -	} &&
> > +sha1_file () {
> > +	git rev-parse --git-path objects/$(test_oid_to_path "$1")
> > +}
>
> Yeah, back when 90cf590f (fsck: optionally show more helpful info
> for broken links, 2016-07-17) originally introduced this pattern,
> we didn't have nicely abstracted helper, but now we do, and there
> is no reason not to use it.  Nice.

This has nothing to do with this series, but I do notice a number of
other uses of test_oid_to_path that are doing this exact thing. In fact,
many of them don't use "git rev-parse --git-path", which would be
better.

I wonder if it's worth a clean-up on top to consolidate all of those
"combine the loose object path of the object with xyz OID and the
$GIT_DIR/objects directory".

In either case -- and I think I'm pretty clearly being pedantic at this
point -- do you suppose it's worthwhile to rename sha1_file to something
that doesn't have sha1 in it?

Thanks,
Taylor

Junio C Hamano Feb. 11, 2021, 12:10 a.m. UTC | #3

Taylor Blau <[e-mail]> writes:

> On Wed, Feb 10, 2021 at 12:36:19PM -0800, Junio C Hamano wrote:
>> "Johannes Schindelin via GitGitGadget" <[e-mail]>
>> writes:
>>
>> > -test_expect_success 'setup: helpers for corruption tests' '
>> > -	sha1_file() {
>> > -		remainder=${1#??} &&
>> > -		firsttwo=${1%$remainder} &&
>> > -		echo ".git/objects/$firsttwo/$remainder"
>> > -	} &&
>> > +sha1_file () {
>> > +	git rev-parse --git-path objects/$(test_oid_to_path "$1")
>> > +}
>>
>> Yeah, back when 90cf590f (fsck: optionally show more helpful info
>> for broken links, 2016-07-17) originally introduced this pattern,
>> we didn't have nicely abstracted helper, but now we do, and there
>> is no reason not to use it.  Nice.
>
> This has nothing to do with this series, but I do notice a number of
> other uses of test_oid_to_path that are doing this exact thing. In fact,
> many of them don't use "git rev-parse --git-path", which would be
> better.
>
> I wonder if it's worth a clean-up on top to consolidate all of those
> "combine the loose object path of the object with xyz OID and the
> $GIT_DIR/objects directory".
>
> In either case -- and I think I'm pretty clearly being pedantic at this
> point -- do you suppose it's worthwhile to rename sha1_file to something
> that doesn't have sha1 in it?

Possibly.  That is probably outside the scope of this topic, but we
see such SHA -> HASH clean-up patches in different places, and this
certainly is a fair game for such a clean-up, I would think.

Thanks.

Patch

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 02478bc4ece2..779f700ac4a0 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -41,17 +41,13 @@  test_expect_success 'HEAD is part of refs, valid objects appear valid' '
 # specific corruption you test afterwards, lest a later test trip over
 # it.
 
-test_expect_success 'setup: helpers for corruption tests' '
-	sha1_file() {
-		remainder=${1#??} &&
-		firsttwo=${1%$remainder} &&
-		echo ".git/objects/$firsttwo/$remainder"
-	} &&
+sha1_file () {
+	git rev-parse --git-path objects/$(test_oid_to_path "$1")
+}
 
-	remove_object() {
-		rm "$(sha1_file "$1")"
-	}
-'
+remove_object() {
+	rm "$(sha1_file "$1")"
+}
 
 test_expect_success 'object with bad sha1' '
 	sha=$(echo blob | git hash-object -w --stdin) &&