git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 00/10] Hash-independent tests, part 4
@ 2019-06-16 18:53 brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 01/10] t: add helper to convert object IDs to paths brian m. carlson
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

This is an additional series of fixes for tests to make them work with
SHA-256.

This series makes use of several constants, such as $ZERO_OID, which
will be replaced with appropriate values based on "test_oid" with later
work.

There is one test (t1410) which adds an SHA1 prerequisite. I wasn't able
to get the math to work out when trying to compute the proper values for
SHA-256, and the test doesn't test what it's supposed to test without
changes, so I opted to mark it with a prerequisite. Suggestions on how
to make it functionally useful under SHA-256 would be appreciated.

brian m. carlson (10):
  t: add helper to convert object IDs to paths
  t1410: make hash size independent
  t1450: make hash size independent
  t5000: make hash independent
  t6030: make test work with SHA-256
  t0027: make hash size independent
  t0090: make test pass with SHA-256
  t1007: remove SHA1 prerequisites
  t1710: make hash independent
  t2203: avoid hard-coded object ID values

 t/t0027-auto-crlf.sh                          |   6 +-
 t/t0090-cache-tree.sh                         |   4 +-
 t/t1007-hash-object.sh                        |  58 ++++++++++--------
 t/t1410-reflog.sh                             |  16 ++---
 t/t1450-fsck.sh                               |  41 ++++++++-----
 t/t1700-split-index.sh                        |  51 ++++++++++-----
 t/t2203-add-intent.sh                         |   6 +-
 t/t5000-tar-tree.sh                           |  16 +++--
 ...8938e6999cb59b3ff66739902a => huge-object} | Bin
 t/t6030-bisect-porcelain.sh                   |  31 +++++-----
 t/test-lib-functions.sh                       |   6 ++
 11 files changed, 142 insertions(+), 93 deletions(-)
 rename t/t5000/{19f9c8273ec45a8938e6999cb59b3ff66739902a => huge-object} (100%)


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-17 19:05   ` Johannes Schindelin
  2019-06-16 18:53 ` [PATCH v2 02/10] t1410: make hash size independent brian m. carlson
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

There are several places in our testsuite where we want to insert a
slash after an object ID to make it into a path we can reference under
.git/objects, and we have various ways of doing so.  Add a helper to
provide a standard way of doing this that works for all size hashes.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/test-lib-functions.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8270de74be..11a6abca2e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1314,6 +1314,12 @@ test_oid () {
 	eval "printf '%s' \"\${$var}\""
 }
 
+# Insert a slash into an object ID so it can be used to reference a location
+# under ".git/objects".  For example, "deadbeef..." becomes "de/adbeef..".
+test_oid_to_path () {
+	echo "$1" | sed -e 's!^..!&/!'
+}
+
 # Choose a port number based on the test script's number and store it in
 # the given variable name, unless that variable already contains a number.
 test_set_port () {

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 02/10] t1410: make hash size independent
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 01/10] t: add helper to convert object IDs to paths brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 03/10] t1450: " brian m. carlson
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Instead of parsing object IDs using fixed-length shell patterns, use cut
to extract the first two characters of an object ID in addition to the
test helper for object paths.  Update another test to look up an
appropriate object ID fragment from the all-zeros object ID instead of
hardcoding the value.

Although the test for parsing reflogs at BUFSIZ boundaries passes, mark
it with the SHA1 prerequisite, as it doesn't currently usefully test
anything when using a hash longer than 20 bytes.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1410-reflog.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 79f731db37..82950c0282 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -30,14 +30,13 @@ check_fsck () {
 }
 
 corrupt () {
-	aa=${1%??????????????????????????????????????} zz=${1#??}
-	mv .git/objects/$aa/$zz .git/$aa$zz
+	mv .git/objects/$(test_oid_to_path $1) .git/$1
 }
 
 recover () {
-	aa=${1%??????????????????????????????????????} zz=${1#??}
+	aa=$(echo $1 | cut -c 1-2)
 	mkdir -p .git/objects/$aa
-	mv .git/$aa$zz .git/objects/$aa/$zz
+	mv .git/$1 .git/objects/$(test_oid_to_path $1)
 }
 
 check_dont_have () {
@@ -55,6 +54,7 @@ check_dont_have () {
 }
 
 test_expect_success setup '
+	test_oid_init &&
 	mkdir -p A/B &&
 	echo rat >C &&
 	echo ox >A/D &&
@@ -313,12 +313,12 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
 # Each line is 114 characters, so we need 75 to still have a few before the
 # last 8K. The 89-character padding on the final entry lines up our
 # newline exactly.
-test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '
+test_expect_success SHA1 'parsing reverse reflogs at BUFSIZ boundaries' '
 	git checkout -b reflogskip &&
-	z38=00000000000000000000000000000000000000 &&
+	zf=$(test_oid zero_2) &&
 	ident="abc <xyz> 0000000001 +0000" &&
 	for i in $(test_seq 1 75); do
-		printf "$z38%02d $z38%02d %s\t" $i $(($i+1)) "$ident" &&
+		printf "$zf%02d $zf%02d %s\t" $i $(($i+1)) "$ident" &&
 		if test $i = 75; then
 			for j in $(test_seq 1 89); do
 				printf X
@@ -329,7 +329,7 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '
 		printf "\n"
 	done >.git/logs/refs/heads/reflogskip &&
 	git rev-parse reflogskip@{73} >actual &&
-	echo ${z38}03 >expect &&
+	echo ${zf}03 >expect &&
 	test_cmp expect actual
 '
 

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 03/10] t1450: make hash size independent
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 01/10] t: add helper to convert object IDs to paths brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 02/10] t1410: make hash size independent brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 04/10] t5000: make hash independent brian m. carlson
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Replace several hard-coded full and partial object IDs with variables or
computed values.  Create junk data to stuff inside an invalid tree that
can be either 20 or 32 bytes long.  Compute a binary all-zeros object ID
instead of hard-coding a 20-byte length.

Additionally, compute various object IDs by using test_oid and
$EMPTY_BLOB so that this test works with multiple hash algorithms.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1450-fsck.sh | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 0f268a3664..b36e0528d0 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -9,6 +9,7 @@ test_description='git fsck random collection of tests
 . ./test-lib.sh
 
 test_expect_success setup '
+	test_oid_init &&
 	git config gc.auto 0 &&
 	git config i18n.commitencoding ISO-8859-1 &&
 	test_commit A fileA one &&
@@ -54,8 +55,8 @@ test_expect_success 'setup: helpers for corruption tests' '
 
 test_expect_success 'object with bad sha1' '
 	sha=$(echo blob | git hash-object -w --stdin) &&
-	old=$(echo $sha | sed "s+^..+&/+") &&
-	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	old=$(test_oid_to_path "$sha") &&
+	new=$(dirname $old)/$(test_oid ff_2) &&
 	sha="$(dirname $new)$(basename $new)" &&
 	mv .git/objects/$old .git/objects/$new &&
 	test_when_finished "remove_object $sha" &&
@@ -84,7 +85,7 @@ test_expect_success 'branch pointing to non-commit' '
 test_expect_success 'HEAD link pointing at a funny object' '
 	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
 	mv .git/HEAD .git/SAVED_HEAD &&
-	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
+	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
 	cat out &&
@@ -244,10 +245,16 @@ test_expect_success 'tree object with duplicate entries' '
 '
 
 test_expect_success 'unparseable tree object' '
+	test_oid_cache <<-\EOF &&
+	junk sha1:twenty-bytes-of-junk
+	junk sha256:twenty-bytes-of-junk-twelve-more
+	EOF
+
 	test_when_finished "git update-ref -d refs/heads/wrong" &&
 	test_when_finished "remove_object \$tree_sha1" &&
 	test_when_finished "remove_object \$commit_sha1" &&
-	tree_sha1=$(printf "100644 \0twenty-bytes-of-junk" | git hash-object -t tree --stdin -w --literally) &&
+	junk=$(test_oid junk) &&
+	tree_sha1=$(printf "100644 \0$junk" | git hash-object -t tree --stdin -w --literally) &&
 	commit_sha1=$(git commit-tree $tree_sha1) &&
 	git update-ref refs/heads/wrong $commit_sha1 &&
 	test_must_fail git fsck 2>out &&
@@ -275,8 +282,9 @@ test_expect_success 'tree entry with type mismatch' '
 '
 
 test_expect_success 'tag pointing to nonexistent' '
-	cat >invalid-tag <<-\EOF &&
-	object ffffffffffffffffffffffffffffffffffffffff
+	badoid=$(test_oid deadbeef) &&
+	cat >invalid-tag <<-EOF &&
+	object $badoid
 	type commit
 	tag invalid
 	tagger T A Gger <tagger@example.com> 1234567890 -0000
@@ -386,8 +394,8 @@ test_expect_success 'rev-list --verify-objects' '
 
 test_expect_success 'rev-list --verify-objects with bad sha1' '
 	sha=$(echo blob | git hash-object -w --stdin) &&
-	old=$(echo $sha | sed "s+^..+&/+") &&
-	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	old=$(test_oid_to_path $sha) &&
+	new=$(dirname $old)/$(test_oid ff_2) &&
 	sha="$(dirname $new)$(basename $new)" &&
 	mv .git/objects/$old .git/objects/$new &&
 	test_when_finished "remove_object $sha" &&
@@ -402,7 +410,7 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '
 
 	test_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&
 	cat out &&
-	test_i18ngrep -q "error: hash mismatch 63ffffffffffffffffffffffffffffffffffffff" out
+	test_i18ngrep -q "error: hash mismatch $(dirname $new)$(test_oid ff_2)" out
 '
 
 test_expect_success 'force fsck to ignore double author' '
@@ -417,13 +425,12 @@ test_expect_success 'force fsck to ignore double author' '
 '
 
 _bz='\0'
-_bz5="$_bz$_bz$_bz$_bz$_bz"
-_bz20="$_bz5$_bz5$_bz5$_bz5"
+_bzoid=$(printf $ZERO_OID | sed -e 's/00/\\0/g')
 
 test_expect_success 'fsck notices blob entry pointing to null sha1' '
 	(git init null-blob &&
 	 cd null-blob &&
-	 sha=$(printf "100644 file$_bz$_bz20" |
+	 sha=$(printf "100644 file$_bz$_bzoid" |
 	       git hash-object -w --stdin -t tree) &&
 	  git fsck 2>out &&
 	  cat out &&
@@ -434,7 +441,7 @@ test_expect_success 'fsck notices blob entry pointing to null sha1' '
 test_expect_success 'fsck notices submodule entry pointing to null sha1' '
 	(git init null-commit &&
 	 cd null-commit &&
-	 sha=$(printf "160000 submodule$_bz$_bz20" |
+	 sha=$(printf "160000 submodule$_bz$_bzoid" |
 	       git hash-object -w --stdin -t tree) &&
 	  git fsck 2>out &&
 	  cat out &&
@@ -586,7 +593,7 @@ test_expect_success 'fsck --connectivity-only' '
 		# its type. That lets us see that --connectivity-only is
 		# not actually looking at the contents, but leaves it
 		# free to examine the type if it chooses.
-		empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
+		empty=.git/objects/$(test_oid_to_path $EMPTY_BLOB) &&
 		blob=$(echo unrelated | git hash-object -w --stdin) &&
 		mv -f $(sha1_file $blob) $empty &&
 
@@ -631,10 +638,12 @@ test_expect_success 'fsck --name-objects' '
 
 test_expect_success 'alternate objects are correctly blamed' '
 	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
+	name=$(test_oid numeric) &&
+	path=$(test_oid_to_path "$name") &&
 	git init --bare alt.git &&
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
-	mkdir alt.git/objects/12 &&
-	>alt.git/objects/12/34567890123456789012345678901234567890 &&
+	mkdir alt.git/objects/$(dirname $path) &&
+	>alt.git/objects/$(dirname $path)/$(basename $path) &&
 	test_must_fail git fsck >out 2>&1 &&
 	test_i18ngrep alt.git out
 '

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 04/10] t5000: make hash independent
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (2 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 03/10] t1450: " brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 05/10] t6030: make test work with SHA-256 brian m. carlson
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

This test uses a stub of a very large (64 GB) object to test our
generation of tar archives.  In doing so, it uses the object ID of the
object so it can insert it into the database properly.  Look up these
values using test_oid.  Restructure the test slightly to use
test_oid_in_path.

Since we care about the object, not how it is named in a particular hash
algorithm, rename it to "huge-object", which is shorter and more
descriptive.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t5000-tar-tree.sh                             |  16 +++++++++++-----
 ...5a8938e6999cb59b3ff66739902a => huge-object} | Bin
 2 files changed, 11 insertions(+), 5 deletions(-)
 rename t/t5000/{19f9c8273ec45a8938e6999cb59b3ff66739902a => huge-object} (100%)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 602bfd9574..37655a237c 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,6 +94,13 @@ check_tar() {
 	'
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOF
+	obj sha1:19f9c8273ec45a8938e6999cb59b3ff66739902a
+	obj sha256:3c666f798798601571f5cec0adb57ce4aba8546875e7693177e0535f34d2c49b
+	EOF
+'
+
 test_expect_success \
     'populate workdir' \
     'mkdir a &&
@@ -369,11 +376,10 @@ test_lazy_prereq TAR_HUGE '
 '
 
 test_expect_success LONG_IS_64BIT 'set up repository with huge blob' '
-	obj_d=19 &&
-	obj_f=f9c8273ec45a8938e6999cb59b3ff66739902a &&
-	obj=${obj_d}${obj_f} &&
-	mkdir -p .git/objects/$obj_d &&
-	cp "$TEST_DIRECTORY"/t5000/$obj .git/objects/$obj_d/$obj_f &&
+	obj=$(test_oid obj) &&
+	path=$(test_oid_to_path $obj) &&
+	mkdir -p .git/objects/$(dirname $path) &&
+	cp "$TEST_DIRECTORY"/t5000/huge-object .git/objects/$path &&
 	rm -f .git/index &&
 	git update-index --add --cacheinfo 100644,$obj,huge &&
 	git commit -m huge
diff --git a/t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a b/t/t5000/huge-object
similarity index 100%
rename from t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a
rename to t/t5000/huge-object

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 05/10] t6030: make test work with SHA-256
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (3 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 04/10] t5000: make hash independent brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 06/10] t0027: make hash size independent brian m. carlson
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Compute several object ID values instead of hard-coding them, and use
test_oid_to_path to cleanly produce a path for an object.

Note that the bisect code which is tested here remains sensitive to the
hash algorithm in use because it uses the object ID to disambiguate
between two equidistant commits.  Fortunately, SHA-1 and SHA-256
disambiguate identically in the cases we care about, so there is no need
to modify the test to accommodate this situation.  However, if a further
hash algorithm change occurs, this test may require some restructuring.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t6030-bisect-porcelain.sh | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 49a394bd75..bdc42e9440 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -615,6 +615,7 @@ test_expect_success 'broken branch creation' '
 	git add missing/MISSING &&
 	git commit -m "6(broken): Added file that will be deleted" &&
 	git tag BROKEN_HASH6 &&
+	deleted=$(git rev-parse --verify HEAD:missing) &&
 	add_line_into_file "7(broken): second line on a broken branch" hello2 &&
 	git tag BROKEN_HASH7 &&
 	add_line_into_file "8(broken): third line on a broken branch" hello2 &&
@@ -622,12 +623,12 @@ test_expect_success 'broken branch creation' '
 	git rm missing/MISSING &&
 	git commit -m "9(broken): Remove missing file" &&
 	git tag BROKEN_HASH9 &&
-	rm .git/objects/39/f7e61a724187ab767d2e08442d9b6b9dab587d
+	rm .git/objects/$(test_oid_to_path $deleted)
 '
 
 echo "" > expected.ok
 cat > expected.missing-tree.default <<EOF
-fatal: unable to read tree 39f7e61a724187ab767d2e08442d9b6b9dab587d
+fatal: unable to read tree $deleted
 EOF
 
 test_expect_success 'bisect fails if tree is broken on start commit' '
@@ -713,12 +714,12 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
 "
 
 cat > expected.bisect-log <<EOF
-# bad: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
-# good: [7b7f204a749c3125d5224ed61ea2ae1187ad046f] Add <2: A new day for git> into <hello>.
-git bisect start '32a594a3fdac2d57cf6d02987e30eec68511498c' '7b7f204a749c3125d5224ed61ea2ae1187ad046f'
-# good: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
-git bisect good 3de952f2416b6084f557ec417709eac740c6818c
-# first bad commit: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
+# bad: [$HASH4] Add <4: Ciao for now> into <hello>.
+# good: [$HASH2] Add <2: A new day for git> into <hello>.
+git bisect start '$HASH4' '$HASH2'
+# good: [$HASH3] Add <3: Another new day for git> into <hello>.
+git bisect good $HASH3
+# first bad commit: [$HASH4] Add <4: Ciao for now> into <hello>.
 EOF
 
 test_expect_success 'bisect log: successful result' '
@@ -731,14 +732,14 @@ test_expect_success 'bisect log: successful result' '
 '
 
 cat > expected.bisect-skip-log <<EOF
-# bad: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
-# good: [7b7f204a749c3125d5224ed61ea2ae1187ad046f] Add <2: A new day for git> into <hello>.
-git bisect start '32a594a3fdac2d57cf6d02987e30eec68511498c' '7b7f204a749c3125d5224ed61ea2ae1187ad046f'
-# skip: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
-git bisect skip 3de952f2416b6084f557ec417709eac740c6818c
+# bad: [$HASH4] Add <4: Ciao for now> into <hello>.
+# good: [$HASH2] Add <2: A new day for git> into <hello>.
+git bisect start '$HASH4' '$HASH2'
+# skip: [$HASH3] Add <3: Another new day for git> into <hello>.
+git bisect skip $HASH3
 # only skipped commits left to test
-# possible first bad commit: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
-# possible first bad commit: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
+# possible first bad commit: [$HASH4] Add <4: Ciao for now> into <hello>.
+# possible first bad commit: [$HASH3] Add <3: Another new day for git> into <hello>.
 EOF
 
 test_expect_success 'bisect log: only skip commits left' '

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 06/10] t0027: make hash size independent
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (4 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 05/10] t6030: make test work with SHA-256 brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 07/10] t0090: make test pass with SHA-256 brian m. carlson
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Several parts of this test generate files that have specific hard-coded
object IDs in them.  We don't really care about what the object ID in
question is, so we turn them all to zeros.

However, because some of these values are fixed and some are generated,
they can be of different lengths, which causes problems when running
with SHA-256.  Furthermore, some assertions in this test use only fixed
object IDs and some use both fixed and generated ones, so converting
only the expected results fixes some tests while breaking others.
Convert both actual and expected object IDs to the all-zeros object ID
of the appropriate length to ensure that the test passes when using
SHA-256.

The astute observer will notice that both tr and sed are used here.
Converting the tr call to a sed y/// command looks logical at first, but
it isn't possible because POSIX doesn't allow escapes in y/// commands
other than "\\" and "\n".

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t0027-auto-crlf.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 3587e454f1..959b6da449 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -15,8 +15,10 @@ compare_ws_file () {
 	pfx=$1
 	exp=$2.expect
 	act=$pfx.actual.$3
-	tr '\015\000abcdef0123456789' QN00000000000000000 <"$2" >"$exp" &&
-	tr '\015\000abcdef0123456789' QN00000000000000000 <"$3" >"$act" &&
+	tr '\015\000abcdef0123456789' QN00000000000000000 <"$2" |
+		sed -e "s/0000*/$ZERO_OID/" >"$exp" &&
+	tr '\015\000abcdef0123456789' QN00000000000000000 <"$3" |
+		sed -e "s/0000*/$ZERO_OID/" >"$act" &&
 	test_cmp "$exp" "$act" &&
 	rm "$exp" "$act"
 }

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 07/10] t0090: make test pass with SHA-256
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (5 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 06/10] t0027: make hash size independent brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 08/10] t1007: remove SHA1 prerequisites brian m. carlson
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

One assertion of this test checks for a shrinking cache tree.  The
initial index contains a cache tree with two directory names but no
object ID, and the second index contains a cache tree with an object ID
but no directory name.

With SHA-1, the second index is smaller than the first, because the
directory information stored takes more than the 20 bytes of an SHA-1
hash, but with SHA-256, the hash is longer, and the test fails the
assertion that the second index is smaller than the first.

To address this issue, increase the length of the subdirectory name to
ensure that the cache tree does indeed shrink in size regardless of the
algorithm in use.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t0090-cache-tree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 504334e552..ce9a4a5f32 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -162,8 +162,8 @@ test_expect_success PERL 'commit --interactive gives cache-tree on partial commi
 '
 
 test_expect_success PERL 'commit -p with shrinking cache-tree' '
-	mkdir -p deep/subdir &&
-	echo content >deep/subdir/file &&
+	mkdir -p deep/very-long-subdir &&
+	echo content >deep/very-long-subdir/file &&
 	git add deep &&
 	git commit -m add &&
 	git rm -r deep &&

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 08/10] t1007: remove SHA1 prerequisites
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (6 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 07/10] t0090: make test pass with SHA-256 brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 09/10] t1710: make hash independent brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 10/10] t2203: avoid hard-coded object ID values brian m. carlson
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Update this test to use test_oid_cache to specify the object IDs for
both SHA-1 and SHA-256.  Since this test now works with both algorithms,
remove the SHA1 prerequisite.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1007-hash-object.sh | 58 +++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 7099d33508..64b340f227 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -9,22 +9,19 @@ echo_without_newline() {
 }
 
 test_blob_does_not_exist() {
-	test_expect_success SHA1 'blob does not exist in database' "
+	test_expect_success 'blob does not exist in database' "
 		test_must_fail git cat-file blob $1
 	"
 }
 
 test_blob_exists() {
-	test_expect_success SHA1 'blob exists in database' "
+	test_expect_success 'blob exists in database' "
 		git cat-file blob $1
 	"
 }
 
 hello_content="Hello World"
-hello_sha1=5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689
-
 example_content="This is an example"
-example_sha1=ddd3f836d3e3fbb7ae289aa9ae83536f76956399
 
 setup_repo() {
 	echo_without_newline "$hello_content" > hello
@@ -44,7 +41,16 @@ pop_repo() {
 	rm -rf $test_repo
 }
 
-setup_repo
+test_expect_success 'setup' '
+	setup_repo &&
+	test_oid_cache <<-EOF
+	hello sha1:5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689
+	hello sha256:1e3b6c04d2eeb2b3e45c8a330445404c0b7cc7b257e2b097167d26f5230090c4
+
+	example sha1:ddd3f836d3e3fbb7ae289aa9ae83536f76956399
+	example sha256:b44fe1fe65589848253737db859bd490453510719d7424daab03daf0767b85ae
+	EOF
+'
 
 # Argument checking
 
@@ -73,23 +79,23 @@ test_expect_success "Can't use --path with --no-filters" '
 
 push_repo
 
-test_expect_success SHA1 'hash a file' '
-	test $hello_sha1 = $(git hash-object hello)
+test_expect_success 'hash a file' '
+	test "$(test_oid hello)" = $(git hash-object hello)
 '
 
-test_blob_does_not_exist $hello_sha1
+test_blob_does_not_exist "$(test_oid hello)"
 
-test_expect_success SHA1 'hash from stdin' '
-	test $example_sha1 = $(git hash-object --stdin < example)
+test_expect_success 'hash from stdin' '
+	test "$(test_oid example)" = $(git hash-object --stdin < example)
 '
 
-test_blob_does_not_exist $example_sha1
+test_blob_does_not_exist "$(test_oid example)"
 
-test_expect_success SHA1 'hash a file and write to database' '
-	test $hello_sha1 = $(git hash-object -w hello)
+test_expect_success 'hash a file and write to database' '
+	test "$(test_oid hello)" = $(git hash-object -w hello)
 '
 
-test_blob_exists $hello_sha1
+test_blob_exists "$(test_oid hello)"
 
 test_expect_success 'git hash-object --stdin file1 <file0 first operates on file0, then file1' '
 	echo foo > file1 &&
@@ -161,11 +167,11 @@ pop_repo
 for args in "-w --stdin" "--stdin -w"; do
 	push_repo
 
-	test_expect_success SHA1 "hash from stdin and write to database ($args)" '
-		test $example_sha1 = $(git hash-object $args < example)
+	test_expect_success "hash from stdin and write to database ($args)" '
+		test "$(test_oid example)" = $(git hash-object $args < example)
 	'
 
-	test_blob_exists $example_sha1
+	test_blob_exists "$(test_oid example)"
 
 	pop_repo
 done
@@ -173,22 +179,22 @@ done
 filenames="hello
 example"
 
-sha1s="$hello_sha1
-$example_sha1"
+oids="$(test_oid hello)
+$(test_oid example)"
 
-test_expect_success SHA1 "hash two files with names on stdin" '
-	test "$sha1s" = "$(echo_without_newline "$filenames" | git hash-object --stdin-paths)"
+test_expect_success "hash two files with names on stdin" '
+	test "$oids" = "$(echo_without_newline "$filenames" | git hash-object --stdin-paths)"
 '
 
 for args in "-w --stdin-paths" "--stdin-paths -w"; do
 	push_repo
 
-	test_expect_success SHA1 "hash two files with names on stdin and write to database ($args)" '
-		test "$sha1s" = "$(echo_without_newline "$filenames" | git hash-object $args)"
+	test_expect_success "hash two files with names on stdin and write to database ($args)" '
+		test "$oids" = "$(echo_without_newline "$filenames" | git hash-object $args)"
 	'
 
-	test_blob_exists $hello_sha1
-	test_blob_exists $example_sha1
+	test_blob_exists "$(test_oid hello)"
+	test_blob_exists "$(test_oid example)"
 
 	pop_repo
 done

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 09/10] t1710: make hash independent
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (7 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 08/10] t1007: remove SHA1 prerequisites brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  2019-06-16 18:53 ` [PATCH v2 10/10] t2203: avoid hard-coded object ID values brian m. carlson
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

This test uses several index hashes, which necessarily depend on the
version of the index and the hash algorithm in use.  Use test_oid_cache
to provide values for these for both SHA-1 and SHA-256.  Also, compute
an object ID and use $EMPTY_BLOB to make the remainder of the tests
independent of the hash algorithm in use.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1700-split-index.sh | 51 ++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 4f2f84f309..12a5568844 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -20,6 +20,22 @@ create_non_racy_file () {
 	test-tool chmtime =-5 "$1"
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOF
+	own_v3 sha1:8299b0bcd1ac364e5f1d7768efb62fa2da79a339
+	own_v3 sha256:38a6d2925e3eceec33ad7b34cbff4e0086caa0daf28f31e51f5bd94b4a7af86b
+
+	base_v3 sha1:39d890139ee5356c7ef572216cebcd27aa41f9df
+	base_v3 sha256:c9baeadf905112bf6c17aefbd7d02267afd70ded613c30cafed2d40cb506e1ed
+
+	own_v4 sha1:432ef4b63f32193984f339431fd50ca796493569
+	own_v4 sha256:6738ac6319c25b694afa7bcc313deb182d1a59b68bf7a47b4296de83478c0420
+
+	base_v4 sha1:508851a7f0dfa8691e9f69c7f055865389012491
+	base_v4 sha256:3177d4adfdd4b6904f7e921d91d715a471c0dde7cf6a4bba574927f02b699508
+	EOF
+'
+
 test_expect_success 'enable split index' '
 	git config splitIndex.maxPercentChange 100 &&
 	git update-index --split-index &&
@@ -29,11 +45,11 @@ test_expect_success 'enable split index' '
 	# NEEDSWORK: Stop hard-coding checksums.
 	if test "$indexversion" = "4"
 	then
-		own=432ef4b63f32193984f339431fd50ca796493569
-		base=508851a7f0dfa8691e9f69c7f055865389012491
+		own=$(test_oid own_v4)
+		base=$(test_oid base_v4)
 	else
-		own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339
-		base=39d890139ee5356c7ef572216cebcd27aa41f9df
+		own=$(test_oid own_v3)
+		base=$(test_oid base_v3)
 	fi &&
 
 	cat >expect <<-EOF &&
@@ -99,17 +115,18 @@ test_expect_success 'enable split index again, "one" now belongs to base index"'
 
 test_expect_success 'modify original file, base index untouched' '
 	echo modified | create_non_racy_file one &&
+	file1_blob=$(git hash-object one) &&
 	git update-index one &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<-EOF &&
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0	one
+	100644 $file1_blob 0	one
 	EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
 	q_to_tab >expect <<-EOF &&
 	$BASE
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0Q
+	100644 $file1_blob 0Q
 	replacements: 0
 	deletions:
 	EOF
@@ -121,7 +138,7 @@ test_expect_success 'add another file, which stays index' '
 	git update-index --add two &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<-EOF &&
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0	one
+	100644 $file1_blob 0	one
 	100644 $EMPTY_BLOB 0	two
 	EOF
 	test_cmp ls-files.expect ls-files.actual &&
@@ -129,7 +146,7 @@ test_expect_success 'add another file, which stays index' '
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
 	q_to_tab >expect <<-EOF &&
 	$BASE
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0Q
+	100644 $file1_blob 0Q
 	100644 $EMPTY_BLOB 0	two
 	replacements: 0
 	deletions:
@@ -141,14 +158,14 @@ test_expect_success 'remove file not in base index' '
 	git update-index --force-remove two &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<-EOF &&
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0	one
+	100644 $file1_blob 0	one
 	EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
 	q_to_tab >expect <<-EOF &&
 	$BASE
-	100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0Q
+	100644 $file1_blob 0Q
 	replacements: 0
 	deletions:
 	EOF
@@ -237,9 +254,9 @@ test_expect_success 'set core.splitIndex config variable to true' '
 	git update-index --add three &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<-EOF &&
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	three
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+	100644 $EMPTY_BLOB 0	one
+	100644 $EMPTY_BLOB 0	three
+	100644 $EMPTY_BLOB 0	two
 	EOF
 	test_cmp ls-files.expect ls-files.actual &&
 	BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
@@ -257,8 +274,8 @@ test_expect_success 'set core.splitIndex config variable to false' '
 	git update-index --force-remove three &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<-EOF &&
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+	100644 $EMPTY_BLOB 0	one
+	100644 $EMPTY_BLOB 0	two
 	EOF
 	test_cmp ls-files.expect ls-files.actual &&
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
@@ -285,7 +302,7 @@ test_expect_success 'set core.splitIndex config variable back to true' '
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<-EOF &&
 	$BASE
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	four
+	100644 $EMPTY_BLOB 0	four
 	replacements:
 	deletions:
 	EOF
@@ -309,7 +326,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '
 	test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<-EOF &&
 	$BASE
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	six
+	100644 $EMPTY_BLOB 0	six
 	replacements:
 	deletions:
 	EOF

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 10/10] t2203: avoid hard-coded object ID values
  2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
                   ` (8 preceding siblings ...)
  2019-06-16 18:53 ` [PATCH v2 09/10] t1710: make hash independent brian m. carlson
@ 2019-06-16 18:53 ` brian m. carlson
  9 siblings, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-16 18:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Duy Nguyen, Johannes Schindelin, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

In order to make this test work with multiple hash algorithms, compute
the object ID used in this test instead of hard-coding it.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t2203-add-intent.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 68e54d5c44..5bbe8dcce4 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -247,12 +247,14 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
 test_expect_success '"diff HEAD" includes ita as new files' '
 	git reset --hard &&
 	echo new >new-ita &&
+	oid=$(git hash-object new-ita) &&
+	oid=$(git rev-parse --short $oid) &&
 	git add -N new-ita &&
 	git diff HEAD >actual &&
-	cat >expected <<-\EOF &&
+	cat >expected <<-EOF &&
 	diff --git a/new-ita b/new-ita
 	new file mode 100644
-	index 0000000..3e75765
+	index 0000000..$oid
 	--- /dev/null
 	+++ b/new-ita
 	@@ -0,0 +1 @@

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-16 18:53 ` [PATCH v2 01/10] t: add helper to convert object IDs to paths brian m. carlson
@ 2019-06-17 19:05   ` Johannes Schindelin
  2019-06-18  1:29     ` brian m. carlson
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-17 19:05 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Jeff King, Duy Nguyen, Jonathan Tan, Eric Sunshine,
	Junio C Hamano

Hi brian,

On Sun, 16 Jun 2019, brian m. carlson wrote:

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8270de74be..11a6abca2e 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1314,6 +1314,12 @@ test_oid () {
>  	eval "printf '%s' \"\${$var}\""
>  }
>
> +# Insert a slash into an object ID so it can be used to reference a location
> +# under ".git/objects".  For example, "deadbeef..." becomes "de/adbeef..".
> +test_oid_to_path () {
> +	echo "$1" | sed -e 's!^..!&/!'
> +}

I guess it does not *really* matter all that much, but this does spawn a
new process (and I think it actually spawns 4 on Windows, for reasons, and
spawning processes is super expensive on Windows).

We might actually want to think about using something like this instead
(which admittedly looks a bit like gobbledygook to the uninitiated, but it
definitely avoids any spawned process):

test_oid_to_path () {
	echo "${1%${1#??}}/${1#??}"
}

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-17 19:05   ` Johannes Schindelin
@ 2019-06-18  1:29     ` brian m. carlson
  2019-06-18  6:14       ` Johannes Sixt
  0 siblings, 1 reply; 20+ messages in thread
From: brian m. carlson @ 2019-06-18  1:29 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: git, Jeff King, Duy Nguyen, Jonathan Tan, Eric Sunshine,
	Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On 2019-06-17 at 19:05:03, Johannes Schindelin wrote:
> I guess it does not *really* matter all that much, but this does spawn a
> new process (and I think it actually spawns 4 on Windows, for reasons, and
> spawning processes is super expensive on Windows).
> 
> We might actually want to think about using something like this instead
> (which admittedly looks a bit like gobbledygook to the uninitiated, but it
> definitely avoids any spawned process):
> 
> test_oid_to_path () {
> 	echo "${1%${1#??}}/${1#??}"
> }

I'm fine making that change. The original design was because we had
other code that used that technique and I didn't see an obviously better
solution. Now you've provided one and a good justification.

I think the complexity isn't too terrible since it's a one-line function
and the comment adequately explains the behavior so people don't have to
parse it to understand it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18  1:29     ` brian m. carlson
@ 2019-06-18  6:14       ` Johannes Sixt
  2019-06-18 16:15         ` Johannes Schindelin
  2019-06-18 23:03         ` brian m. carlson
  0 siblings, 2 replies; 20+ messages in thread
From: Johannes Sixt @ 2019-06-18  6:14 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Johannes Schindelin, git, Jeff King, Duy Nguyen, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Am 18.06.19 um 03:29 schrieb brian m. carlson:
> On 2019-06-17 at 19:05:03, Johannes Schindelin wrote:
>> I guess it does not *really* matter all that much, but this does spawn a
>> new process (and I think it actually spawns 4 on Windows, for reasons, and
>> spawning processes is super expensive on Windows).
>>
>> We might actually want to think about using something like this instead
>> (which admittedly looks a bit like gobbledygook to the uninitiated, but it
>> definitely avoids any spawned process):
>>
>> test_oid_to_path () {
>> 	echo "${1%${1#??}}/${1#??}"
>> }
> 
> I'm fine making that change. The original design was because we had
> other code that used that technique and I didn't see an obviously better
> solution. Now you've provided one and a good justification.

Regardless of how it is implemented, I have another gripe with this
helper: the way it must be used requires a process: $(test_out_to_path $foo)

And looking through this patch series, I see a gazillion of *new*
process substitutions $(test_something...) and $(basename $whatever).
Can't we do something about it?

-- Hannes

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18  6:14       ` Johannes Sixt
@ 2019-06-18 16:15         ` Johannes Schindelin
  2019-06-18 16:55           ` SZEDER Gábor
  2019-06-18 17:04           ` Jeff King
  2019-06-18 23:03         ` brian m. carlson
  1 sibling, 2 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-18 16:15 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: brian m. carlson, git, Jeff King, Duy Nguyen, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Hi Hannes,

On Tue, 18 Jun 2019, Johannes Sixt wrote:

> Am 18.06.19 um 03:29 schrieb brian m. carlson:
> > On 2019-06-17 at 19:05:03, Johannes Schindelin wrote:
> >> I guess it does not *really* matter all that much, but this does spawn a
> >> new process (and I think it actually spawns 4 on Windows, for reasons, and
> >> spawning processes is super expensive on Windows).
> >>
> >> We might actually want to think about using something like this instead
> >> (which admittedly looks a bit like gobbledygook to the uninitiated, but it
> >> definitely avoids any spawned process):
> >>
> >> test_oid_to_path () {
> >> 	echo "${1%${1#??}}/${1#??}"
> >> }
> >
> > I'm fine making that change. The original design was because we had
> > other code that used that technique and I didn't see an obviously better
> > solution. Now you've provided one and a good justification.
>
> Regardless of how it is implemented, I have another gripe with this
> helper: the way it must be used requires a process: $(test_out_to_path
> $foo)

Indeed.

> And looking through this patch series, I see a gazillion of *new*
> process substitutions $(test_something...) and $(basename $whatever).
> Can't we do something about it?

I wish there was. Unix shell scripting has not evolved much in the past,
what, 3 decades? So I don't really see a way to "pass variables by
reference" to shell functions, short of calling `eval` (which buys
preciously little as it _also_ has to spawn a new process [*1*]).

Of course, if we would slowly get away from depending so much on shell
scripting, then we would reap quite a few other benefits, too, not only
speed, but also much easier debugging, code coverage, etc.

Ciao,
Dscho

Footnote *1*: Theoretically, it could be a *ton* faster by using threads
on Windows. But threads are pretty much an afterthought on Unix/Linux, so
no mainstream POSIX shell supports this. They all `fork()` to interpret an
`eval` as far as I can tell.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18 16:15         ` Johannes Schindelin
@ 2019-06-18 16:55           ` SZEDER Gábor
  2019-06-19 19:56             ` Johannes Schindelin
  2019-06-18 17:04           ` Jeff King
  1 sibling, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2019-06-18 16:55 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Sixt, brian m. carlson, git, Jeff King, Duy Nguyen,
	Jonathan Tan, Eric Sunshine, Junio C Hamano

On Tue, Jun 18, 2019 at 06:15:46PM +0200, Johannes Schindelin wrote:
> > Regardless of how it is implemented, I have another gripe with this
> > helper: the way it must be used requires a process: $(test_out_to_path
> > $foo)
> 
> Indeed.
> 
> > And looking through this patch series, I see a gazillion of *new*
> > process substitutions $(test_something...) and $(basename $whatever).
> > Can't we do something about it?
> 
> I wish there was. Unix shell scripting has not evolved much in the past,
> what, 3 decades? So I don't really see a way to "pass variables by
> reference" to shell functions, short of calling `eval` (which buys
> preciously little as it _also_ has to spawn a new process [*1*]).

> Footnote *1*: Theoretically, it could be a *ton* faster by using threads
> on Windows. But threads are pretty much an afterthought on Unix/Linux, so
> no mainstream POSIX shell supports this. They all `fork()` to interpret an
> `eval` as far as I can tell.

'eval' doesn't fork().  It can't possibly fork(), because if it did,
then any variables set in the eval-ed code snippet couldn't be visible
outside the 'eval'.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18 16:15         ` Johannes Schindelin
  2019-06-18 16:55           ` SZEDER Gábor
@ 2019-06-18 17:04           ` Jeff King
  2019-06-19 19:55             ` Johannes Schindelin
  1 sibling, 1 reply; 20+ messages in thread
From: Jeff King @ 2019-06-18 17:04 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Sixt, brian m. carlson, git, Duy Nguyen, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

On Tue, Jun 18, 2019 at 06:15:46PM +0200, Johannes Schindelin wrote:

> > And looking through this patch series, I see a gazillion of *new*
> > process substitutions $(test_something...) and $(basename $whatever).
> > Can't we do something about it?
> 
> I wish there was. Unix shell scripting has not evolved much in the past,
> what, 3 decades? So I don't really see a way to "pass variables by
> reference" to shell functions, short of calling `eval` (which buys
> preciously little as it _also_ has to spawn a new process [*1*]).

Really? An eval can impact the caller's state, so it _can't_ happen in a
sub-process in most cases.

E.g., if I run this:

-- >8 --
#!/bin/sh

# usage: test_oid_to_path <var> <oid>
# to set the variable <var> in the caller's environment to the path of <oid>
test_oid_to_path() {
	path="${2%${2#??}}/${2#??}"
	eval "$1=\$path"
}

test_oid_to_path foo 1234abcd
echo foo: $foo
-- >8 --

it all happens in a single process, under both bash and dash.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18  6:14       ` Johannes Sixt
  2019-06-18 16:15         ` Johannes Schindelin
@ 2019-06-18 23:03         ` brian m. carlson
  1 sibling, 0 replies; 20+ messages in thread
From: brian m. carlson @ 2019-06-18 23:03 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin, git, Jeff King, Duy Nguyen, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

On 2019-06-18 at 06:14:03, Johannes Sixt wrote:
> Am 18.06.19 um 03:29 schrieb brian m. carlson:
> > On 2019-06-17 at 19:05:03, Johannes Schindelin wrote:
> > I'm fine making that change. The original design was because we had
> > other code that used that technique and I didn't see an obviously better
> > solution. Now you've provided one and a good justification.
> 
> Regardless of how it is implemented, I have another gripe with this
> helper: the way it must be used requires a process: $(test_out_to_path $foo)
> 
> And looking through this patch series, I see a gazillion of *new*
> process substitutions $(test_something...) and $(basename $whatever).
> Can't we do something about it?

A command substitution need not invoke another process. In fact, all of
the test_oid calls operate only within the shell. The test_oid_cache and
test_oid_init calls require spawning only expr, and they are typically
limited to one or two per test.

Within reason, I'm happy to try to make things easier for Windows folks
if I can, but it's still the case that process creation is more
expensive on Windows and shell scripting is designed around process
creation. My hope is that Microsoft's work on WSL will help them improve
their Win32 process creation time so it's faster and this becomes less
of an issue.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18 17:04           ` Jeff King
@ 2019-06-19 19:55             ` Johannes Schindelin
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-19 19:55 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, brian m. carlson, git, Duy Nguyen, Jonathan Tan,
	Eric Sunshine, Junio C Hamano

Hi Peff,

On Tue, 18 Jun 2019, Jeff King wrote:

> On Tue, Jun 18, 2019 at 06:15:46PM +0200, Johannes Schindelin wrote:
>
> > > And looking through this patch series, I see a gazillion of *new*
> > > process substitutions $(test_something...) and $(basename $whatever).
> > > Can't we do something about it?
> >
> > I wish there was. Unix shell scripting has not evolved much in the past,
> > what, 3 decades? So I don't really see a way to "pass variables by
> > reference" to shell functions, short of calling `eval` (which buys
> > preciously little as it _also_ has to spawn a new process [*1*]).
>
> Really? An eval can impact the caller's state, so it _can't_ happen in a
> sub-process in most cases.
>
> E.g., if I run this:
>
> -- >8 --
> #!/bin/sh
>
> # usage: test_oid_to_path <var> <oid>
> # to set the variable <var> in the caller's environment to the path of <oid>
> test_oid_to_path() {
> 	path="${2%${2#??}}/${2#??}"
> 	eval "$1=\$path"
> }
>
> test_oid_to_path foo 1234abcd
> echo foo: $foo
> -- >8 --
>
> it all happens in a single process, under both bash and dash.

Oops. I think I may have read too much into the name `eval.c` in Dash's
source code, I assumed that it was all about the `eval` command. But now
that I look at this again, I see:

/*
 * Execute a command inside back quotes.  If it's a builtin command, we
 * want to save its output in a block obtained from malloc.  Otherwise
 * we fork off a subprocess and get the output of the command via a pipe.
 * Should be called with interrupts off.
 */

void
evalbackcmd(union node *n, struct backcmd *result)
{
	[...]

I saw this at
https://git.kernel.org/pub/scm/utils/dash/dash.git/tree/src/eval.c#n608

So this is actually about `$(...)` (or the old, non-nestable backtick
version of it) and not about `eval`.

And of course I was also making another incorrect connection to the Perl
command `eval` which allows you to catch code that `die()`s (which *must*
run in a subprocess).

So yeah, I guess `eval` would work here to avoid the `$(...)` constructs.

Sorry for the noise,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 01/10] t: add helper to convert object IDs to paths
  2019-06-18 16:55           ` SZEDER Gábor
@ 2019-06-19 19:56             ` Johannes Schindelin
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2019-06-19 19:56 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Johannes Sixt, brian m. carlson, git, Jeff King, Duy Nguyen,
	Jonathan Tan, Eric Sunshine, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

Hi,

On Tue, 18 Jun 2019, SZEDER Gábor wrote:

> On Tue, Jun 18, 2019 at 06:15:46PM +0200, Johannes Schindelin wrote:
> > > Regardless of how it is implemented, I have another gripe with this
> > > helper: the way it must be used requires a process: $(test_out_to_path
> > > $foo)
> >
> > Indeed.
> >
> > > And looking through this patch series, I see a gazillion of *new*
> > > process substitutions $(test_something...) and $(basename $whatever).
> > > Can't we do something about it?
> >
> > I wish there was. Unix shell scripting has not evolved much in the past,
> > what, 3 decades? So I don't really see a way to "pass variables by
> > reference" to shell functions, short of calling `eval` (which buys
> > preciously little as it _also_ has to spawn a new process [*1*]).
>
> > Footnote *1*: Theoretically, it could be a *ton* faster by using threads
> > on Windows. But threads are pretty much an afterthought on Unix/Linux, so
> > no mainstream POSIX shell supports this. They all `fork()` to interpret an
> > `eval` as far as I can tell.
>
> 'eval' doesn't fork().  It can't possibly fork(), because if it did,
> then any variables set in the eval-ed code snippet couldn't be visible
> outside the 'eval'.

Good point. My brain must have made a couple of incorrect associations
there.

Sorry,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-06-19 19:56 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-16 18:53 [PATCH v2 00/10] Hash-independent tests, part 4 brian m. carlson
2019-06-16 18:53 ` [PATCH v2 01/10] t: add helper to convert object IDs to paths brian m. carlson
2019-06-17 19:05   ` Johannes Schindelin
2019-06-18  1:29     ` brian m. carlson
2019-06-18  6:14       ` Johannes Sixt
2019-06-18 16:15         ` Johannes Schindelin
2019-06-18 16:55           ` SZEDER Gábor
2019-06-19 19:56             ` Johannes Schindelin
2019-06-18 17:04           ` Jeff King
2019-06-19 19:55             ` Johannes Schindelin
2019-06-18 23:03         ` brian m. carlson
2019-06-16 18:53 ` [PATCH v2 02/10] t1410: make hash size independent brian m. carlson
2019-06-16 18:53 ` [PATCH v2 03/10] t1450: " brian m. carlson
2019-06-16 18:53 ` [PATCH v2 04/10] t5000: make hash independent brian m. carlson
2019-06-16 18:53 ` [PATCH v2 05/10] t6030: make test work with SHA-256 brian m. carlson
2019-06-16 18:53 ` [PATCH v2 06/10] t0027: make hash size independent brian m. carlson
2019-06-16 18:53 ` [PATCH v2 07/10] t0090: make test pass with SHA-256 brian m. carlson
2019-06-16 18:53 ` [PATCH v2 08/10] t1007: remove SHA1 prerequisites brian m. carlson
2019-06-16 18:53 ` [PATCH v2 09/10] t1710: make hash independent brian m. carlson
2019-06-16 18:53 ` [PATCH v2 10/10] t2203: avoid hard-coded object ID values brian m. carlson

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).