patchclone --dissociate: avoid locking pack files

> When `git clone` is asked to dissociate the repository from the
> reference repository whose objects were used, it is quite possible that
> the pack files need to be repacked. In that case, the pack files need to
> be deleted that were originally hard-links to the reference repository's
> pack files.

> On platforms where a file cannot be deleted if another process still
> holds a handle on it, we therefore need to take pains to release all
> pack files and indexes before dissociating.
> 
> This fixes https://github.com/git-for-windows/git/issues/446
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/clone.c            |  9 ++++++++-
>  t/t5700-clone-reference.sh | 21 +++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 578da85..223adc4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	transport_unlock_pack(transport);
>  	transport_disconnect(transport);
>  
> -	if (option_dissociate)
> +	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();
> +	}

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

>  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

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

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

> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:
>> -	if (option_dissociate)
>> +	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();
>> +	}

> On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote:
>> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:
>>> -	if (option_dissociate)
>>> +	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.
> 
> Apparently, pack file is closed just after mapping if it's
> smaller than core.packedGitWindowSize. Could it be the
> reason that this patch worked in you test case?

> +	close(p->pack_fd);
> +	pack_open_fds--;
> +	p->pack_fd = -1;
> +
> +	return 1;
> +}
> +
>  /*
>   * The LRU pack is the one with the oldest MRU window, preferring packs
>   * with no used windows, or the oldest mtime if it has no windows allocated.
> @@ -853,12 +865,8 @@ static int close_one_pack(void)
>  		find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
>  	}
>  
> -	if (lru_p) {
> -		close(lru_p->pack_fd);
> -		pack_open_fds--;
> -		lru_p->pack_fd = -1;
> -		return 1;
> -	}
> +	if (lru_p)
> +		return close_pack_fd(lru_p);

>  	return 0;
>  }
> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)
>  		if (strcmp(pack_name, p->pack_name) == 0) {
>  			clear_delta_base_cache();
>  			close_pack_windows(p);
> -			if (p->pack_fd != -1) {
> -				close(p->pack_fd);
> -				pack_open_fds--;
> -			}
> +			close_pack_fd(p);

>  			if (!win->offset && win->len == p->pack_size
> -				&& !p->do_not_close) {
> -				close(p->pack_fd);
> -				pack_open_fds--;
> -				p->pack_fd = -1;
> -			}
> +				&& !p->do_not_close)
> +				close_pack_fd(p);

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> There was a lot of repeated code to close the file descriptor of
>> a given pack. Let's just refactor this code into a single function.
> 
> That is a very good idea, but...
> 
>> +static int close_pack_fd(struct packed_git *p)
>> +{
>> +	if (p->pack_fd < 0)
>> +		return 0;
> 
> Is this "return 0" compatible with ...
> 
>> +	close(p->pack_fd);
>> +	pack_open_fds--;
>> +	p->pack_fd = -1;
>> +
>> +	return 1;
>> +}
>> +
>>  /*
>>   * The LRU pack is the one with the oldest MRU window, preferring packs
>>   * with no used windows, or the oldest mtime if it has no windows allocated.
>> @@ -853,12 +865,8 @@ static int close_one_pack(void)
>>  		find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
>>  	}
>>
>> -	if (lru_p) {
>> -		close(lru_p->pack_fd);
>> -		pack_open_fds--;
>> -		lru_p->pack_fd = -1;
>> -		return 1;
>> -	}
>> +	if (lru_p)
>> +		return close_pack_fd(lru_p);
> 
> ... what is returned from here?

> 
>>  	return 0;
>>  }
>> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)
>>  		if (strcmp(pack_name, p->pack_name) == 0) {
>>  			clear_delta_base_cache();
>>  			close_pack_windows(p);
>> -			if (p->pack_fd != -1) {
>> -				close(p->pack_fd);
>> -				pack_open_fds--;
>> -			}
>> +			close_pack_fd(p);
> 
> And here, the closer _must_ be (and it currently is) very aware that
> the pack chosen may not be open anymore.  By giving a function that
> conditionally closes if the pack is still open too bland a name,
> that distinction is lost at these two call sites.

>>  			if (!win->offset && win->len == p->pack_size
>> -				&& !p->do_not_close) {
>> -				close(p->pack_fd);
>> -				pack_open_fds--;
>> -				p->pack_fd = -1;
>> -			}
>> +				&& !p->do_not_close)
>> +				close_pack_fd(p);
> 
> I wonder how this do_not_close bit should interact with "we are
> shutting down (or we are spawning another to do the real work, while
> we won't do anything useful anymore), so close everything down".

>>> +				&& !p->do_not_close)
>>> +				close_pack_fd(p);
>> 
>> I wonder how this do_not_close bit should interact with "we are
>> shutting down (or we are spawning another to do the real work, while
>> we won't do anything useful anymore), so close everything down".
>
> The `close_all_packs()` function is meant for the use case where you
> really close all the packs, no question asked.
>
> I cannot think of a use case where it would make sense to try to be
> gentle, yet still ask for *all* of the packs to be closed. Maybe you
> can think of one to convince me that it might make sense to respect
> the `do_not_close` flag in such a function? Because so far, I am
> totally unconvinced.

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>>>> +				&& !p->do_not_close)
>>>> +				close_pack_fd(p);
>>>
>>> I wonder how this do_not_close bit should interact with "we are
>>> shutting down (or we are spawning another to do the real work, while
>>> we won't do anything useful anymore), so close everything down".
>>
>> The `close_all_packs()` function is meant for the use case where you
>> really close all the packs, no question asked.
>>
>> I cannot think of a use case where it would make sense to try to be
>> gentle, yet still ask for *all* of the packs to be closed. Maybe you
>> can think of one to convince me that it might make sense to respect
>> the `do_not_close` flag in such a function? Because so far, I am
>> totally unconvinced.
> 
> Do not wait for being convinced forever, as I am not interested in
> convincing you either way.  Decision by made-up examples or lack of
> imagination is a waste of time ;-).
> 
> My earlier "I wonder" leads me to totally different conclusion,
> which is "we declare that it is a bug for any caller to call
> close-all-packs while marking any open pack with do_not_close bit".
> 
> Which merely means that while iterating over packs in the
> "close-all" loop, we should throw a die("BUG") at the caller if that
> bit is on.  That way, we won't have to rely on "we did not think of
> a good use case so we unconditionally closed the packs without
> telling the remainder of the system, hopefully we broke nothing."

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 578da85..cc896e2 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	transport_unlock_pack(transport);
>  	transport_disconnect(transport);
>  
> -	if (option_dissociate)
> +	if (option_dissociate) {
> +		close_all_packs();
>  		dissociate_from_references();
> +	}

> On Windows, files that are in use cannot be removed or renamed. That
> means that we have to release pack files when we are about to, say,
> repack them. Let's introduce a convenient function to close them
> pack files.
>
> While at it, we consolidate the close windows/close fd/close index
> stanza in `free_pack_by_name()` into the `close_pack()` function that
> is used by the new `close_all_packs()` function to avoid repeated code.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

>  cache.h     |  1 +
>  sha1_file.c | 23 ++++++++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 752031e..57f6a74 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *);
>  
>  extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);
>  extern void close_pack_windows(struct packed_git *);
> +extern void close_all_packs(void);
>  extern void unuse_pack(struct pack_window **);
>  extern void free_pack_by_name(const char *);
>  extern void clear_delta_base_cache(void);
> diff --git a/sha1_file.c b/sha1_file.c
> index 8c3c913..ca699d7 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -798,6 +798,25 @@ static int close_pack_fd(struct packed_git *p)
>  	return 1;
>  }
>  
> +static void close_pack(struct packed_git *p)
> +{
> +	close_pack_windows(p);
> +	close_pack_fd(p);
> +	close_pack_index(p);
> +}
> +
> +void close_all_packs(void)
> +{
> +	struct packed_git *p;
> +
> +	for (p = packed_git; p; p = p->next)
> +		if (p->do_not_close)
> +			die("BUG! Want to close pack marked 'do-not-close'");
> +		else
> +			close_pack(p);
> +}
> +
> +
>  /*
>   * The LRU pack is the one with the oldest MRU window, preferring packs
>   * with no used windows, or the oldest mtime if it has no windows allocated.
> @@ -906,9 +925,7 @@ void free_pack_by_name(const char *pack_name)
>  		p = *pp;
>  		if (strcmp(pack_name, p->pack_name) == 0) {
>  			clear_delta_base_cache();
> -			close_pack_windows(p);
> -			close_pack_fd(p);
> -			close_pack_index(p);
> +			close_pack(p);
>  			free(p->bad_object_sha1);
>  			*pp = p->next;
>  			if (last_found_pack == p)

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> On Windows, files that are in use cannot be removed or renamed. That
>> means that we have to release pack files when we are about to, say,
>> repack them. Let's introduce a convenient function to close them
>> pack files.
>>
>> While at it, we consolidate the close windows/close fd/close index
>> stanza in `free_pack_by_name()` into the `close_pack()` function that
>> is used by the new `close_all_packs()` function to avoid repeated code.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
> 
> This is the only one that was updated among the four and the change
> looks sensible.  I'd reword the end of the first paragraph of the
> proposed log message, though, to replace "close them pack files"
> with "close all the pack files and their idx files".
> 
> Thanks.