Re: [PATCH] clone --dissociate: avoid locking pack files

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2015, 19:42 UTC
Message-ID: <xmqqlhbn7ky1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150930192814.GA26154@wheezy.local>

Max Kirillov <max@max630.net> writes:

Show 14 quoted lines

>> +	if (option_dissociate) {
>> +		struct packed_git *p;
>> +
>> +		for (p = packed_git; p; p = p->next) {
>> +			close_pack_windows(p);
>> +			close_pack_index(p);
>> +		}
>>  		dissociate_from_references();
>> +	}
>
> This does not seem to close handles to the pack files
> themseves, does Windows still allow removing the files? I
> probably did not tried that, because I started from handles,
> and discovered mapped files only later.

This is trying to (incompletely) mimic what free_pack_by_name() in sha1_file.c is doing but for all packs, I think. I wonder why we do not have to do other things here that are done over there, like clearing delta-base-cache, closing pack_fd and decrementing pack_open_fds, etc.

The right approach may to have a helper in sha1_file.c that closes and cleans up _all_ packs, and call it from here, instead of having builtin/clone.c even know about implementation details such as packed_git is a linked list, etc.

Show 47 quoted lines

>
>>  	junk_mode = JUNK_LEAVE_REPO;
>>  	err = checkout();
>> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
>> index ef1779f..2250ef4 100755
>> --- a/t/t5700-clone-reference.sh
>> +++ b/t/t5700-clone-reference.sh
>> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '
>>  	test_must_fail git -C R fsck &&
>>  	git -C S fsck
>>  '
>> +test_expect_success 'clone, dissociate from partial reference and repack' '
>> +	rm -fr P Q R &&
>> +	git init P &&
>> +	(
>> +		cd P &&
>> +		test_commit one &&
>> +		git repack &&
>> +		test_commit two &&
>> +		git repack
>> +	) &&
>> +	git clone --bare P Q &&
>> +	(
>> +		cd P &&
>> +		git checkout -b second &&
>> +		test_commit three &&
>> +		git repack
>> +	) &&
>> +	git clone --bare --dissociate --reference=P Q R &&
>> +	ls R/objects/pack/*.pack >packs.txt &&
>> +	test_line_count = 1 packs.txt
>> +'
>
> Unless it goes very lowlevel like running lsof of readin
> proc testing this should always pass on Linux, even if the
> issue is not fixed, maybe should be a conditional for
> Windows only?
>
>>  test_done
>> -- 
>> 2.5.3.windows.1.3.gc322723
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html