[v2,07/15] t4011: abstract away SHA-1-specific constants
diff mbox series

Message ID 20191028005907.24985-8-sandals@crustytoothpaste.net
State New
Headers show
Series
  • SHA-256 test fixes, part 6
Related show

Commit Message

brian m. carlson Oct. 28, 2019, 12:58 a.m. UTC
Adjust the test so that it computes variables for object IDs instead of
using hard-coded hashes.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t4011-diff-symlink.sh | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

Comments

Junio C Hamano Oct. 28, 2019, 2:56 a.m. UTC | #1
"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 @@
brian m. carlson Oct. 28, 2019, 11:59 p.m. UTC | #2
On 2019-10-28 at 02:56:14, Junio C Hamano wrote:
> > +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.

Yeah, I can make that change.

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

Can change.

Patch
diff mbox series

diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 5ae19b987d..717034bb50 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -9,11 +9,24 @@  test_description='Test diff of symlinks.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh
 
+# 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"
+}
+
+# Print the short OID of the given file.
+short_oid () {
+	local oid=$(git hash-object "$1") &&
+	git rev-parse --short "$oid"
+}
+
 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
 	--- /dev/null
 	+++ b/frotz
 	@@ -0,0 +1 @@
@@ -21,7 +34,7 @@  test_expect_success 'diff new symlink and file' '
 	\ No newline at end of file
 	diff --git a/nitfol b/nitfol
 	new file mode 100644
-	index 0000000..7c465af
+	index 0000000..$symlink
 	--- /dev/null
 	+++ b/nitfol
 	@@ -0,0 +1 @@
@@ -46,10 +59,10 @@  test_expect_success 'diff unchanged symlink and file'  '
 '
 
 test_expect_success 'diff removed symlink and file' '
-	cat >expected <<-\EOF &&
+	cat >expected <<-EOF &&
 	diff --git a/frotz b/frotz
 	deleted file mode 120000
-	index 7c465af..0000000
+	index $symlink..0000000
 	--- a/frotz
 	+++ /dev/null
 	@@ -1 +0,0 @@
@@ -57,7 +70,7 @@  test_expect_success 'diff removed symlink and file' '
 	\ No newline at end of file
 	diff --git a/nitfol b/nitfol
 	deleted file mode 100644
-	index 7c465af..0000000
+	index $symlink..0000000
 	--- a/nitfol
 	+++ /dev/null
 	@@ -1 +0,0 @@
@@ -90,9 +103,10 @@  test_expect_success 'diff identical, but newly created symlink and file' '
 '
 
 test_expect_success 'diff different symlink and file' '
-	cat >expected <<-\EOF &&
+	new=$(symlink_oid yxyyz) &&
+	cat >expected <<-EOF &&
 	diff --git a/frotz b/frotz
-	index 7c465af..df1db54 120000
+	index $symlink..$new 120000
 	--- a/frotz
 	+++ b/frotz
 	@@ -1 +1 @@
@@ -101,7 +115,7 @@  test_expect_success 'diff different symlink and file' '
 	+yxyyz
 	\ No newline at end of file
 	diff --git a/nitfol b/nitfol
-	index 7c465af..df1db54 100644
+	index $symlink..$new 100644
 	--- a/nitfol
 	+++ b/nitfol
 	@@ -1 +1 @@
@@ -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) &&
+	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 @@