Re: [PATCH] partial-clone: add a partial-clone test case

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2022, 19:41 UTC
Message-ID: <xmqq4k41vdwe.fsf@gitster.g>
In-Reply-To: <pull.1175.git.1647193162570.gitgitgadget@gmail.com>

"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>
> In a blobless-cloned repo, `git log --follow -- <path>` (`<path>` have
> an exact OID rename) doesn't download blob of the file from where the
> new file is renamed.

Is this "doesn't" (documenting current behaviour, without saying if it is wrong or is desired) or "shouldn't" (documenting the desired behaviour, which the current implementation may or may not satisfy)?

> +test_expect_success 'git log --follow does not download blobs if an exact OID rename found (blobless clone)' '

That's mouthful.

Show 12 quoted lines

> +	rm -rf repo partial.git &&
> +	test_create_repo repo &&
> +	content="some dummy content" &&
> +	test_commit -C repo create-a-file file.txt "$content" &&
> +	git -C repo mv file.txt new-file.txt &&
> +	git -C repo commit -m rename-the-file &&
> +	test_config -C repo uploadpack.allowfilter 1 &&
> +	test_config -C repo uploadpack.allowanysha1inwant 1 &&
> +
> +	git clone --filter=blob:none "file://$(pwd)/repo" partial.git &&
> +	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> +		git -C partial.git log --follow -- new-file.txt > "$(pwd)/trace.txt" &&

Lose SP after '>'.

		git -C partial.git log --follow -- new-file.txt >"$(pwd)/trace.txt" &&

> +	! test_subcommand_inexact fetch <trace.txt

Looking at the implementation of the helper, it seems to be prepared to handle negation itself. Shouldn't this be

	test_subcommand_inexact ! fetch <trace.txt

instead?

Show 15 quoted lines

> +'
> +
>  test_expect_success 'lazy-fetch when accessing object not in the_repository' '
>  	rm -rf full partial.git &&
>  	test_create_repo full &&
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0f439c99d61..07a2b60c103 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1811,7 +1811,7 @@ test_subcommand_inexact () {
>  		shift
>  	fi
>  
> -	local expr=$(printf '"%s".*' "$@")
> +	local expr=$(printf '.*"%s".*' "$@")

The original wanted to make sure that the arguments to the helper are initial items of a comma separated list, and an existing caller, for example, i.e.

  test_subcommand_inexact git pack-objects --honor-pack-keep <trace

is relying on the behaviour to make sure 'git', 'pack-objects', ... appear at the beginning of "[...]" enclosed list. This change breaks its ability to notice that an insertion of unrelated token before 'git' as an error.

In other words, it looks like an uncalled-for selfish change.

Why can't you specify what should NOT come before "fetch" in your use of this helper?

>  	expr="${expr%,}"

The preimage already has this problem, but the stripping of trailing comma here is a result of mistaken copy-and-paste from the exact variant, I think. test_subcommand uses

	local expr=$(printf '"%s",' "$@")

to concatenate "$@" into a single comma-separated string, so it perfectly makes sense to drop the last one here, but with or without your change here, neither is adding a comma that need to be stripped.

It is not _your_ theme, but I think this helper is poorly designed, especially compared to the original "exact" variant.

        test_subcommand_inexact () {
                local negate=
                if test "$1" = "!"
                then
                        negate=t
                        shift
                fi

                local expr=$(printf '"%s".*' "$@")
                expr="${expr%,}"

                if test -n "$negate"
                then
                        ! grep "\"event\":\"child_start\".*\[$expr\]"
                else
                        grep "\"event\":\"child_start\".*\[$expr\]"
                fi
        }

I've already touched that "${expr%,}" there is a totally useful statement that will always be a no-op.

When "test_subcommand_inexact git pack-objects" is run, the printf assigns to $expr:

		expr='"git".*"pack-objects".*'

and the actual grep command invoked becomes

	grep '"event":"child_start".*\["git".*"pack-objects".*\]'

I am not sure if that is what we really want.

I wonder if it was more like this that the original wanted to grep for:

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

in which case the two lines there should be more like

	local expr=$(printf '"%s",' "$@")
	expr="${expr%,}.*"

I would think. This comes from Derrick's e4d0c11c (repack: respect kept objects with '--write-midx -b', 2021-12-20).