Re: [PATCH v2 07/15] t4011: abstract away SHA-1-specific constants

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2019, 02:56 UTC
Message-ID: <xmqqk18pa88h.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191028005907.24985-8-sandals@crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

It does make sense to introduce these two additional helpers, one that takes the contents to be hashed, and the other that takes the name of the file that stores the contents to be hashed. The former is handy when you need to compute an object name for a symbolic link, and the latter is handy when you have contents already stored in a file. But if you have a short contents in a variable, nothing prevents you from using the former to compute an object name for a file that would have the contents you have in the variable without having to first create a temporary file.

It crossed my mind that it may make more logical sense to call the former "oid_from_contents" and the latter "oid_from_file", but I'll shortly change my position ;-)

Show 5 quoted lines

> +# Print the short OID of a symlink with the given name.
> +symlink_oid () {
> +	local oid=$(printf "%s" "$1" | git hash-object --stdin) &&
> +	git rev-parse --short "$oid"
> +}

This is good.

> +# Print the short OID of the given file.

To contrast with the above, s/given/contents of the/ may make the distinction clearer, I think.

> +short_oid () {
> +	local oid=$(git hash-object "$1") &&
> +	git rev-parse --short "$oid"
> +}

Given that we do not use the former helper to compute a regular file object name without having a concrete file on the filesystem, I do not mind calling it symlink_oid at all. But then this may want to be called file_oid to make the contrast better, and it would match the use of these two in a test near the end of this patch.

Show 8 quoted lines

>  test_expect_success 'diff new symlink and file' '
> -	cat >expected <<-\EOF &&
> +	symlink=$(symlink_oid xyzzy) &&
> +	cat >expected <<-EOF &&
>  	diff --git a/frotz b/frotz
>  	new file mode 120000
> -	index 0000000..7c465af
> +	index 0000000..$symlink

It is a mistake to name the variable "symlink", even though symlink_oid is a good name for this helper function.

If you were showing a change that updates the symlink RelNotes from pointing at Documentation/RelNotes/2.0.0.txt to Documentation/RelNotes/2.1.0.txt, for example, you would say

	old=$(symlink_oid Documentation/RelNotes/2.0.0.txt) &&
	new=$(symlink_oid Documentation/RelNotes/2.1.0.txt) &&
	cat expect <<-EOF &&
	diff --git a/RelNotes b/RelNotes
	index $old..$new
	...
	EOF

so I'd expect $new (on the right hand side of ..) would read more natural.

Show 7 quoted lines

> @@ -137,14 +151,16 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '
>  '
>  
>  test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '
> -	cat >expect <<-\EOF &&
> +	file=$(short_oid file.bin) &&
> +	link=$(symlink_oid file.bin) &&

Here, I do think $file and $link are good names, and that leads me to say s/short_oid/file_oid/ would be a good idea.

Show 13 quoted lines

> +	cat >expect <<-EOF &&
>  	diff --git a/file.bin b/file.bin
>  	new file mode 100644
> -	index 0000000..d95f3ad
> +	index 0000000..$file
>  	Binary files /dev/null and b/file.bin differ
>  	diff --git a/link.bin b/link.bin
>  	new file mode 120000
> -	index 0000000..dce41ec
> +	index 0000000..$link
>  	--- /dev/null
>  	+++ b/link.bin
>  	@@ -0,0 +1 @@