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

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

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.