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

[Junio C Hamano <gitster@pobox.com>]

"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 ;-)

> +# 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.

>  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.

> @@ -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.

> +	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 @@