Re: Git Test Coverage Report (v2.27.0-rc0)

["brian m. carlson" <sandals@crustytoothpaste.net>]

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

On 2020-05-19 at 12:11:15, Derrick Stolee wrote:
> On 5/15/2020 1:22 PM, Derrick Stolee wrote:
> > brian m. carlson	13e7ed6a builtin/checkout: compute checkout metadata for checkouts
> > builtin/checkout.c
> > 13e7ed6a 625)        is_null_oid(&info->oid) ? &tree->object.oid :
> 
> This is part of the following hunk:
> 
> @@ -619,6 +620,11 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>         opts.verbose_update = o->show_progress;
>         opts.src_index = &the_index;
>         opts.dst_index = &the_index;
> +       init_checkout_metadata(&opts.meta, info->refname,
> +                              info->commit ? &info->commit->object.oid :
> +                              is_null_oid(&info->oid) ? &tree->object.oid :
> +                              &info->oid,
> +                              NULL);
>         parse_tree(tree);
>         init_tree_desc(&tree_desc, tree->buffer, tree->size);
>         switch (unpack_trees(1, &tree_desc, &opts)) {
> 
> The double-nested ternary definitely complicates the coverage here.
> It also points out that all tests have `info->commit` a non-NULL.
> 
> This certainly looks safe, but I don't know.

This is me trying to be defensive.  I think the code path that is not
covered here that can be is the info->oid code path; I don't technically
believe the other case (using the tree) is possible, although the
checkout code is complex enough that I can't be certain.

It would look like this, I believe:

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4bfffa9c31..a69ec3c4b7 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -408,6 +408,25 @@ test_expect_success PERL 'required process filter should filter data' '
 		EOF
 		test_cmp_exclude_clean expected.log debug.log &&
 
+		# Make sure that the file appears dirty, so checkout below has to
+		# run the configured filter.
+		META="treeish=$MASTER" &&
+		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&
+
+		filter_git checkout --quiet --no-progress $(git rev-parse master) &&
+		cat >expected.log <<-EOF &&
+			START
+			init handshake complete
+			IN: smudge test.r $META blob=$M $S [OK] -- OUT: $S . [OK]
+			IN: smudge test2.r $META blob=$M2 $S2 [OK] -- OUT: $S2 . [OK]
+			IN: smudge test4-empty.r $META blob=$EMPTY 0 [OK] -- OUT: 0  [OK]
+			IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $META blob=$M3 $S3 [OK] -- OUT: $S3 . [OK]
+			STOP
+		EOF
+		test_cmp_exclude_clean expected.log debug.log &&
+
+		git checkout empty-branch &&
+		META="ref=refs/heads/master treeish=$MASTER" &&
 		filter_git checkout --quiet --no-progress master &&
 		cat >expected.log <<-EOF &&
 			START


I'm about to go make dinner, but I'll try to get a patch written up with
this tonight or tomorrow.  If it's more urgent than that, you may add my
sign-off.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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