Re: [PATCH v2 3/4] Add 'promisor-remote' capability to protocol v2

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2024, 05:47 UTC
Message-ID: <xmqqserboqot.fsf@gitster.g>
In-Reply-To: <20240910163000.1985723-4-christian.couder@gmail.com>

I was looking at test breakages caused by this topic (in 'seen', t5710 fails leak checking).

Then I noticed something strange. Next to the "$(TRASH_DIRECTORY)", running this script leaves a few garbage files under the "t/" directory.

I think the culprit is this helper function.

Show 10 quoted lines

> +initialize_server () {
> +	# Repack everything first
> +	git -C server -c repack.writebitmaps=false repack -a -d &&
> +
> +	# Remove promisor file in case they exist, useful when reinitializing
> +	rm -rf server/objects/pack/*.promisor &&
> +
> +	# Repack without the largest object and create a promisor pack on server
> +	git -C server -c repack.writebitmaps=false repack -a -d \
> +	    --filter=blob:limit=5k --filter-to="$(pwd)" &&

This --filter-to="$(pwd)" expands to $(TRASH_DIRECTORY), which is "..../t/trash-directory.t5710-promisor-remote-capability". I think that is the cause for two extra trash files that are _OUTSIDE_ the trash directory, which is an absolute no-no for tests to be safely runnable. Next to the trash directory, this ends up creating three files

trash directory.t5710-...-980d3ff591aae1651cdd52f7dfad4fb6319ee3c2.idx trash directory.t5710-...-980d3ff591aae1651cdd52f7dfad4fb6319ee3c2.pack trash directory.t5710-...-980d3ff591aae1651cdd52f7dfad4fb6319ee3c2.rev

> +	promisor_file=$(ls server/objects/pack/*.pack | sed "s/\.pack/.promisor/") &&
> +	touch "$promisor_file" &&

Style: don't "touch" a single file to create it.  Instead >redirect_into_it.

The first failure in leak check seems to be

not ok 5 - fetch with promisor.advertise set to 'false' # # git -C server config promisor.advertise false && # # # Clone from server to create a client # GIT_NO_LAZY_FETCH=0 git clone -c remote.server2.promisor=true \ # -c remote.server2.fetch="+refs/heads/*:refs/remotes/server2/*" \ # -c remote.server2.url="file://$(pwd)/server2" \ # -c promisor.acceptfromserver=All \ # --no-local --filter="blob:limit=5k" server client && # test_when_finished "rm -rf client" && # # # Check that the largest object is not missing on the server # check_missing_objects server 0 "" && # # # Reinitialize server so that the largest object is missing again # initialize_server

but I didn't dig further. Can you take a look? I'll eject the topic from 'seen' in the meantime to unblock the CI.

Thanks.