[Outreachy PATCH 0/2] t0410: fix exit code and hash assumptions in the missing-tree test

[Outreachy PATCH 0/2] t0410: fix exit code and hash assumptions in the missing-tree test

[Dorine Tipo]

This series cleans up the 'rev-list stops traversal at missing and
promised tree' test in t0410:

 - Patch 1 replaces the 'git ls-tree | grep | head | cut' pipeline that
   finds the OID of a_dir with 'git rev-parse HEAD:a_dir'. The pipeline
   drops git's exit code, and its cut assumes a SHA-1 length, so it
   yields an abbreviated OID under SHA-256.

 - Patch 2 stores the output of 'git rev-parse' in variables before
   passing it to test_grep, so a failure in git stops the && chain.

I ran the full test suite with both SHA-1 and SHA-256, and it passes.

There are more places in t0410 that pass "$(git ...)" directly as an
argument. If this approach looks good, I'm happy to convert the rest
in a follow-up.

This is my microproject for Outreachy.

Dorine Tipo (2):
  t0410: use rev-parse to get the subtree OID
  t0410: keep git's exit code in test_grep calls

 t/t0410-partial-clone.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.34.1

[Outreachy PATCH 1/2] t0410: use rev-parse to get the subtree OID

[Dorine Tipo]

The 'rev-list stops traversal at missing and promised tree' test gets
the OID of a_dir by piping 'git ls-tree' through grep, head and cut.

The pipeline goes against t/README which says not to pipe the output
of a git command, since only the last command's exit code is kept.
'cut -b13-52' also assumes a 40-character SHA-1 name, so under SHA-256
TREE2 holds an abbreviated OID.

Use 'git rev-parse HEAD:a_dir' instead. It returns the full OID in a
single command, the same way TREE is computed with
'git rev-parse foo^{tree}' just above.

Signed-off-by: Dorine Tipo <dorine.a.tipo@gmail.com>
---
 t/t0410-partial-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 788e9a1631..a24b663f73 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -405,7 +405,7 @@ test_expect_success 'rev-list stops traversal at missing and promised tree' '
 	TREE=$(git -C repo rev-parse foo^{tree}) &&
 
 	# a tree referenced by HEAD^{tree} (tree referenced from tree)
-	TREE2=$(git -C repo ls-tree HEAD^{tree} | grep " tree " | head -1 | cut -b13-52) &&
+	TREE2=$(git -C repo rev-parse HEAD:a_dir) &&
 
 	promise_and_delete "$TREE" &&
 	promise_and_delete "$TREE2" &&
-- 
2.34.1

[Outreachy PATCH 2/2] t0410: keep git's exit code in test_grep calls

[Dorine Tipo]

The 'rev-list stops traversal at missing and promised tree' test passes
"$(git -C repo rev-parse ...)" directly to test_grep. t/README says
not to use command substitution in a way that discards git's exit
code: the && chain only sees test_grep's exit code, so a failure in
git is not caught there.

Assign the rev-parse output to variables first and pass those to
test_grep instead.

Signed-off-by: Dorine Tipo <dorine.a.tipo@gmail.com>
---
 t/t0410-partial-clone.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index a24b663f73..57da830105 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -413,9 +413,13 @@ test_expect_success 'rev-list stops traversal at missing and promised tree' '
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&
-	test_grep $(git -C repo rev-parse foo) out &&
+
+	FOO=$(git -C repo rev-parse foo) &&
+	HEAD_OID=$(git -C repo rev-parse HEAD) &&
+
+	test_grep "$FOO" out &&
 	test_grep ! $TREE out &&
-	test_grep $(git -C repo rev-parse HEAD) out &&
+	test_grep "$HEAD_OID" out &&
 	test_grep ! $TREE2 out
 '
 
-- 
2.34.1

Re: [Outreachy PATCH 1/2] t0410: use rev-parse to get the subtree OID

[Junio C Hamano]

Dorine Tipo <dorine.a.tipo@gmail.com> writes:

> The 'rev-list stops traversal at missing and promised tree' test gets
> the OID of a_dir by piping 'git ls-tree' through grep, head and cut.
>
> The pipeline goes against t/README which says not to pipe the output
> of a git command, since only the last command's exit code is kept.
> 'cut -b13-52' also assumes a 40-character SHA-1 name, so under SHA-256
> TREE2 holds an abbreviated OID.
>
> Use 'git rev-parse HEAD:a_dir' instead. It returns the full OID in a
> single command, the same way TREE is computed with
> 'git rev-parse foo^{tree}' just above.
>
> Signed-off-by: Dorine Tipo <dorine.a.tipo@gmail.com>
> ---
>  t/t0410-partial-clone.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 788e9a1631..a24b663f73 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -405,7 +405,7 @@ test_expect_success 'rev-list stops traversal at missing and promised tree' '
>  	TREE=$(git -C repo rev-parse foo^{tree}) &&
>  
>  	# a tree referenced by HEAD^{tree} (tree referenced from tree)
> -	TREE2=$(git -C repo ls-tree HEAD^{tree} | grep " tree " | head -1 | cut -b13-52) &&
> +	TREE2=$(git -C repo rev-parse HEAD:a_dir) &&
>  
>  	promise_and_delete "$TREE" &&
>  	promise_and_delete "$TREE2" &&

[devil's advocate hat on]

That may avoid piping, but are they chekcing the same thing?  If
HEAD had a tree that sorts before a_dir due to a bug, the original
test would have used that tree instead of the tree of a_dir.  Later
tests may have noticed that the tree object is not what is expected
for a_dir and led to the discovery of such a bug.  The new test goes
directly to a_dir so it wouldn't catch such a bug.

[devil's advoicate hat off]

Arguably, the use of "head -1" in the original pipeline is
unnecessary as 'repo' prepared there only has a_dir/something and
nothing else, and if the extra tree that was added by a bug sorted
after a_dir, it would have hidden such a bug, instead of catching
it.  So in the sense, it would be better to assume that the tree
would be created in a shape that we expect and explicitly name the
directory we are interested in i.e., "HEAD:a_dir".

So, one thing we may want to add to the second paragraph of the
proposed log message is that the original assumed silently that the
first tree object in ls-tree output is the tree we are intrerested
in, without even validating what path the tree was stored at, and it
would be an improvement to name 'a_dir' explicitly in the test.