From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 13 Mar 2022 19:41:21 GMT
Subject: Re: [PATCH] partial-clone: add a partial-clone test case
Message-ID: <xmqq4k41vdwe.fsf@gitster.g>
In-Reply-To: <pull.1175.git.1647193162570.gitgitgadget@gmail.com>

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

> 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.

> +	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?

> +'
> +
>  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).