| Message ID | 20191028005907.24985-8-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SHA-256 test fixes, part 6 | expand |
"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 @@
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.
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 @@
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(-)