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

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

[Johannes Schindelin]

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

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

[Max Kirillov]

On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:
> 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.

Hello. For 1.9.* I used to have some hack for closing files
also. The case was to allow scheduled git gc to remove packs
even if I forgot to quit some less in some console.

> 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();
> +	}

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

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

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

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

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

[PATCH/RFC 0/2] close packs files when they are not needed

[Max Kirillov]

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

Like this?

Note I did not test it to actually work for the current
code. Trying now what I could do with lsof, just to be sure
myself, but probably its use not appropriate for the
project.

Max Kirillov (2):
  sha1_file: close all pack files after running
  sha1_file: set packfile to O_CLOEXEC at open

 builtin/pack-objects.c |  2 +-
 cache.h                |  3 +-
 git.c                  |  2 ++
 pack-bitmap.c          |  2 +-
 sha1_file.c            | 80 +++++++++++++++++++++++++++++++++++---------------
 5 files changed, 63 insertions(+), 26 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

When a builtin has done its job, but waits for pager or not waited
by its caller and still hanging it keeps pack files opened.
This can cause a number of issues, for example on Windows git gc
cannot remove the packs.

Fix this by explicitly closing all pack files and unmapping memory
from the packs after running builtin. Do not die() if the memory region
is still used though, just print a warning, because failure to close
a file should not prevent the currently running program from finishing
its task.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h     |  1 +
 git.c       |  2 ++
 sha1_file.c | 32 +++++++++++++++++++++++++++++---
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 79066e5..153bc46 100644
--- a/cache.h
+++ b/cache.h
@@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
 extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *, int, int);
+extern void close_all_packs(void);
 
 /*
  * Return the SHA-1 of the nth object within the specified packfile.
diff --git a/git.c b/git.c
index 5feba41..ad34680 100644
--- a/git.c
+++ b/git.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
@@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	trace_argv_printf(argv, "trace: built-in: git");
 
 	status = p->fn(argc, argv, prefix);
+	close_all_packs();
 	if (status)
 		return status;
 
diff --git a/sha1_file.c b/sha1_file.c
index 08302f5..62f1dad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
 	return ret;
 }
 
-void close_pack_windows(struct packed_git *p)
+static int close_pack_windows_nodie(struct packed_git *p)
 {
 	while (p->windows) {
 		struct pack_window *w = p->windows;
 
 		if (w->inuse_cnt)
-			die("pack '%s' still has open windows to it",
-			    p->pack_name);
+			return 1;
+
 		munmap(w->base, w->len);
 		pack_mapped -= w->len;
 		pack_open_windows--;
 		p->windows = w->next;
 		free(w);
 	}
+
+	return 0;
+}
+
+void close_pack_windows(struct packed_git *p)
+{
+	if (close_pack_windows_nodie(p))
+		die("pack '%s' still has open windows to it", p->pack_name);
 }
 
 /*
@@ -866,6 +874,24 @@ static int close_one_pack(void)
 	return 0;
 }
 
+void close_all_packs(void)
+{
+	struct packed_git *p = NULL;
+
+	for (p = packed_git; p; p = p->next) {
+		if (close_pack_windows_nodie(p))
+			warning("pack '%s' still has open windows to it", p->pack_name);
+
+		if (p->pack_fd != -1) {
+			if (close(p->pack_fd) != 0)
+				warning("close(%s) failed: %d", p->pack_name, errno);
+			p->pack_fd = -1;
+		}
+
+		close_pack_index(p);
+	}
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
 	struct pack_window *w = *w_cursor;
-- 
2.3.4.2801.g3d0809b

[PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open

[Max Kirillov]

Windows does not support setting O_CLOEXEC by fcntl,
but there is an open flag O_NOINHERIT which results in same
behaviour. Use it in git_open_noatime() and also bring
setting O_CLOEXEC there also to make it consistent. Rename
the function to git_open_noatime_cloexec(), to avoid confusion.

Signed-off-by: Max Kirillov <max@max630.net>
---
 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 sha1_file.c            | 48 ++++++++++++++++++++++++++++--------------------
 4 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1c63f8f..a8052f4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -714,7 +714,7 @@ static off_t write_reused_pack(struct sha1file *f)
 	if (!is_pack_valid(reuse_packfile))
 		die("packfile is invalid: %s", reuse_packfile->pack_name);
 
-	fd = git_open_noatime(reuse_packfile->pack_name);
+	fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
 	if (fd < 0)
 		die_errno("unable to open packfile for reuse: %s",
 			  reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 153bc46..77ef5ca 100644
--- a/cache.h
+++ b/cache.h
@@ -972,7 +972,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open_noatime_cloexec(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 637770a..4a3b23c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -274,7 +274,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
 		return -1;
 
 	idx_name = pack_bitmap_filename(packfile);
-	fd = git_open_noatime(idx_name);
+	fd = git_open_noatime_cloexec(idx_name);
 	free(idx_name);
 
 	if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 62f1dad..2da5de2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -385,7 +385,7 @@ void read_info_alternates(const char * relative_base, int depth)
 	int fd;
 
 	sprintf(path, "%s/%s", relative_base, alt_file_name);
-	fd = git_open_noatime(path);
+	fd = git_open_noatime_cloexec(path);
 	if (fd < 0)
 		return;
 	if (fstat(fd, &st) || (st.st_size == 0)) {
@@ -575,7 +575,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
 	struct pack_idx_header *hdr;
 	size_t idx_size;
 	uint32_t version, nr, i, *index;
-	int fd = git_open_noatime(path);
+	int fd = git_open_noatime_cloexec(path);
 	struct stat st;
 
 	if (fd < 0)
@@ -995,7 +995,6 @@ static int open_packed_git_1(struct packed_git *p)
 	struct pack_header hdr;
 	unsigned char sha1[20];
 	unsigned char *idx_sha1;
-	long fd_flag;
 
 	if (!p->index_data && open_pack_index(p))
 		return error("packfile %s index unavailable", p->pack_name);
@@ -1013,7 +1012,7 @@ static int open_packed_git_1(struct packed_git *p)
 	while (pack_max_fds <= pack_open_fds && close_one_pack())
 		; /* nothing */
 
-	p->pack_fd = git_open_noatime(p->pack_name);
+	p->pack_fd = git_open_noatime_cloexec(p->pack_name);
 	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
 		return -1;
 	pack_open_fds++;
@@ -1026,16 +1025,6 @@ static int open_packed_git_1(struct packed_git *p)
 	} else if (p->pack_size != st.st_size)
 		return error("packfile %s size changed", p->pack_name);
 
-	/* We leave these file descriptors open with sliding mmap;
-	 * there is no point keeping them open across exec(), though.
-	 */
-	fd_flag = fcntl(p->pack_fd, F_GETFD, 0);
-	if (fd_flag < 0)
-		return error("cannot determine file descriptor flags");
-	fd_flag |= FD_CLOEXEC;
-	if (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)
-		return error("cannot set FD_CLOEXEC");
-
 	/* Verify we recognize this pack file format. */
 	if (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
 		return error("file %s is far too short to be a packfile", p->pack_name);
@@ -1515,17 +1504,21 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
 	return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
-int git_open_noatime(const char *name)
+int git_open_noatime_cloexec(const char *name)
 {
+#ifdef __WIN32__
+	static int sha1_file_open_flag = O_NOATIME | O_NOINHERIT;
+#else
 	static int sha1_file_open_flag = O_NOATIME;
+	long fd_flag;
+#endif
+	int fd;
 
 	for (;;) {
-		int fd;
-
 		errno = 0;
 		fd = open(name, O_RDONLY | sha1_file_open_flag);
 		if (fd >= 0)
-			return fd;
+			break;
 
 		/* Might the failure be due to O_NOATIME? */
 		if (errno != ENOENT && sha1_file_open_flag) {
@@ -1535,6 +1528,21 @@ int git_open_noatime(const char *name)
 
 		return -1;
 	}
+
+#ifndef __WIN32__
+	fd_flag = fcntl(fd, F_GETFD, 0);
+	if (fd_flag < 0) {
+		close(fd);
+		return error("cannot determine file descriptor flags");
+	}
+	fd_flag |= FD_CLOEXEC;
+	if (fcntl(fd, F_SETFD, fd_flag) == -1) {
+		close(fd);
+		return error("cannot set FD_CLOEXEC");
+	}
+#endif
+
+	return fd;
 }
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
@@ -1561,7 +1569,7 @@ static int open_sha1_file(const unsigned char *sha1)
 	struct alternate_object_database *alt;
 	int most_interesting_errno;
 
-	fd = git_open_noatime(sha1_file_name(sha1));
+	fd = git_open_noatime_cloexec(sha1_file_name(sha1));
 	if (fd >= 0)
 		return fd;
 	most_interesting_errno = errno;
@@ -1569,7 +1577,7 @@ static int open_sha1_file(const unsigned char *sha1)
 	prepare_alt_odb();
 	for (alt = alt_odb_list; alt; alt = alt->next) {
 		fill_sha1_path(alt->name, sha1);
-		fd = git_open_noatime(alt->base);
+		fd = git_open_noatime_cloexec(alt->base);
 		if (fd >= 0)
 			return fd;
 		if (most_interesting_errno == ENOENT)
-- 
2.3.4.2801.g3d0809b

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

[Max Kirillov]

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?

-- 
Max

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-01 05:29, Max Kirillov wrote:
> When a builtin has done its job, but waits for pager or not waited
> by its caller and still hanging it keeps pack files opened.
> This can cause a number of issues, for example on Windows git gc
> cannot remove the packs.

I did not experience that issue. In any case, closing the packs after the builtin function has returned might not change anything anyway because we are about to `exit()` and that's that.

So I would like to skip this:

> diff --git a/git.c b/git.c
> index 5feba41..ad34680 100644
> --- a/git.c
> +++ b/git.c
> @@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int
> argc, const char **argv)
>  	trace_argv_printf(argv, "trace: built-in: git");
>  
>  	status = p->fn(argc, argv, prefix);
> +	close_all_packs();
>  	if (status)
>  		return status;
>  

Also, I would move the new declaration directly before `close_pack_windows()`:

> diff --git a/cache.h b/cache.h
> index 79066e5..153bc46 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
>  extern void free_pack_by_name(const char *);
>  extern void clear_delta_base_cache(void);
>  extern struct packed_git *add_packed_git(const char *, int, int);
> +extern void close_all_packs(void);
>  
>  /*
>   * Return the SHA-1 of the nth object within the specified packfile.

The convention in Git seems to call things _gently rather than _nodie:

> diff --git a/sha1_file.c b/sha1_file.c
> index 08302f5..62f1dad 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
>  	return ret;
>  }
>  
> -void close_pack_windows(struct packed_git *p)
> +static int close_pack_windows_nodie(struct packed_git *p)
>  {
>  	while (p->windows) {
>  		struct pack_window *w = p->windows;
>  
>  		if (w->inuse_cnt)
> -			die("pack '%s' still has open windows to it",
> -			    p->pack_name);
> +			return 1;
> +
>  		munmap(w->base, w->len);
>  		pack_mapped -= w->len;
>  		pack_open_windows--;
>  		p->windows = w->next;
>  		free(w);
>  	}
> +
> +	return 0;
> +}

And while we're at it, why not teach that function a new parameter `int close_pack_fd`?

There is another problem: when we cannot close the pack window, we cannot really continue, can we? Because if we do, we *still* have the lock, and we'll just fail later, most likely with an unhelpful error message because we do not know where the pack closing failed. I do not think that the warning really helps...

Ciao,
Dscho

Re: [PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open

[Johannes Schindelin]

Hi Max,

On 2015-10-01 05:29, Max Kirillov wrote:
> Windows does not support setting O_CLOEXEC by fcntl,
> but there is an open flag O_NOINHERIT which results in same
> behaviour. Use it in git_open_noatime() and also bring
> setting O_CLOEXEC there also to make it consistent. Rename
> the function to git_open_noatime_cloexec(), to avoid confusion.

Which problem does this solve? I am actually suspecting that this will rather cause problems because now `exec()`ing children might cause bogus file descriptors to be still held on.

So I would recommend to drop this patch. It is not needed to fix the reported bug.

Ciao,
Dscho

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-02 12:05, Johannes Schindelin wrote:

> On 2015-10-01 05:29, Max Kirillov wrote:
>> When a builtin has done its job, but waits for pager or not waited
>> by its caller and still hanging it keeps pack files opened.
>> This can cause a number of issues, for example on Windows git gc
>> cannot remove the packs.

Could you do me another favor? It seems that you want to work on this, so I will step back (I have to take off for the weekend very soon anyway, so I am really glad that you take care of it). But I would really love to see the line

This fixes https://github.com/git-for-windows/git/issues/446

in the commit message, as this will keep the connection between the fix and the original report.

Thanks,
Dscho

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 12:05:55PM +0200, Johannes Schindelin wrote:
> Hi Max,
> 
> On 2015-10-01 05:29, Max Kirillov wrote:
>> When a builtin has done its job, but waits for pager or not waited
>> by its caller and still hanging it keeps pack files opened.
>> This can cause a number of issues, for example on Windows git gc
>> cannot remove the packs.
> 
> I did not experience that issue. In any case, closing the
> packs after the builtin function has returned might not
> change anything anyway because we are about to `exit()`
> and that's that.

Steps to reproduce:
1. install the latest git-for-windows, 2.6.0.windows.1
2. run the script in the end of message
3. remember the directory name, press the enter as it asks
4. in the less scrool down

Then inspect the processes git.exe and less.exe with Process
Explorer of something you find convenient. Both processes
should have opened the pack file, and git.exe should have it
mapped (I found it in the "View"->"Lower Pane views"->"Dlls").

Then, if you run another bash window, cd to the repository
and run "git repack -a -d", it should print:

----
$ git repack -a -d
Counting objects: 3001, done.
Compressing objects: 100% (1003/1003), done.
Writing objects: 100% (3001/3001), done.
Total 3001 (delta 997), reused 3001 (delta 997)
Unlink of file '.git/objects/pack/pack-e1b0e3ac01ff8d79a77648de3370f49b93c58a8b.pack' failed. Should I try again? (y/n)
----

I would like the git gc to succeed, because I run it as a
scheduled task nightly and feel a bit annoyed by the opened
windows which wait for me to say yes (after exiting pager).

So, the fix is needed approximately in that place, after
running any builtin command.

For your case, I think, the same close_all_packs() should be
invoked before the repacking.

> The convention in Git seems to call things _gently rather
> than _nodie:

Thanks, will change it if the idea is welcomed.

>> -void close_pack_windows(struct packed_git *p)
>> +static int close_pack_windows_nodie(struct packed_git *p)
>>  {
>>  	while (p->windows) {
>>  		struct pack_window *w = p->windows;
>>  
>>  		if (w->inuse_cnt)
>> -			die("pack '%s' still has open windows to it",
>> -			    p->pack_name);
>> +			return 1;
>> +
>>  		munmap(w->base, w->len);
>>  		pack_mapped -= w->len;
>>  		pack_open_windows--;
>>  		p->windows = w->next;
>>  		free(w);
>>  	}
>> +
>> +	return 0;
>> +}
> 
> And while we're at it, why not teach that function a new
> parameter `int close_pack_fd`?

I think "close windows" should close windows, if it also
closes pack fd probably should be another name. But current
code seems quite logical. Close the packs, and run closing
windows from it.

> There is another problem: when we cannot close the pack
> window, we cannot really continue, can we?

Yes we can, unlocking of the window is not needed for the
current process to do what it intended to do, it would just
interfere with concurrent git gc.

For the clone case probably die would be appropriate. If you
feel like it worth complicating the code we might search for
some solution.

-- 
Max

The script:
--------- ./t-windows-pack-close.sh ---------
#!/bin/sh

set -e

TEST_DIR=`mktemp -d`

t_git() {
    git --work-tree="$TEST_DIR" --git-dir="$TEST_DIR/.git" "$@"
}

t_git init

for i in $(seq 1000)
do
    echo "commit$i" >"$TEST_DIR/commit.$i"
    t_git add "commit.$i"
    t_git commit -m "commit$i" -q
done

t_git repack

pack_path=$(find "$TEST_DIR/.git/objects/pack" -name "pack-.pack")

echo "dir: $TEST_DIR"
echo press enter to start git log
read

t_git -c core.packedGitWindowSize=100 log
-----------------------------

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:
> Hi Max,
> 
> On 2015-10-02 12:05, Johannes Schindelin wrote:
> 
> > On 2015-10-01 05:29, Max Kirillov wrote:
>>> When a builtin has done its job, but waits for pager or not waited
>>> by its caller and still hanging it keeps pack files opened.
>>> This can cause a number of issues, for example on Windows git gc
>>> cannot remove the packs.
> 
> Could you do me another favor? It seems that you want to
> work on this, so I will step back (I have to take off for
> the weekend very soon anyway, so I am really glad that you
> take care of it). But I would really love to see the line

As I explained in other message, your case is a bit
different.

I could add another call of close_all_packs() as a separate
commit with the "fixes" link, but I'm not so sure about it
if it turns out that additional efforts are required.

-- 
Max

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 10:06:46PM +0300, Max Kirillov wrote:
> for i in $(seq 1000)
> t_git -c core.packedGitWindowSize=100 log

It was 32-bit build.
I cannot promise those exactly numbers will work, because I don not clearly
understand what do they mean. With 100 commits the pack size was 20K, but it
was mapped fully with the window=100.

But I believe with the same build same numbers should reproduce the issue.

For 32-bit builds I can see it on any significantly big repository, like the
git itself. Maybe for 64bit it is less likely.

The code which decides whether to close the pack is in use_pack() in sha1_file.c:
-----------
                        if (!win->offset && win->len == p->pack_size
                                && !p->do_not_close) {
                                close(p->pack_fd);
                                pack_open_fds--;
                                p->pack_fd = -1;
                        }
-----------

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-02 21:21, Max Kirillov wrote:
> On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:
>> On 2015-10-02 12:05, Johannes Schindelin wrote:
>>
>> > On 2015-10-01 05:29, Max Kirillov wrote:
>>>> When a builtin has done its job, but waits for pager or not waited
>>>> by its caller and still hanging it keeps pack files opened.
>>>> This can cause a number of issues, for example on Windows git gc
>>>> cannot remove the packs.
>>
>> Could you do me another favor? It seems that you want to
>> work on this, so I will step back (I have to take off for
>> the weekend very soon anyway, so I am really glad that you
>> take care of it). But I would really love to see the line
> 
> As I explained in other message, your case is a bit
> different.

I guess then we would need two different patches for the two different fixes, at least.

So now I am unsure how to proceed: I do not want to step on your toes, but I also want to see my use case fixed and I want to move forward on this. At the moment, it looks as if we are at an impasse.

Ciao,
Johannes

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:
> I guess then we would need two different patches for the
> two different fixes, at least.
> 
> So now I am unsure how to proceed: I do not want to step
> on your toes, but I also want to see my use case fixed and
> I want to move forward on this. At the moment, it looks as
> if we are at an impasse.

Just continue with your patches then. I do not hurry.

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-05 06:57, Max Kirillov wrote:
> On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:
>> I guess then we would need two different patches for the
>> two different fixes, at least.
>>
>> So now I am unsure how to proceed: I do not want to step
>> on your toes, but I also want to see my use case fixed and
>> I want to move forward on this. At the moment, it looks as
>> if we are at an impasse.
> 
> Just continue with your patches then. I do not hurry.

Okay!
Johannes

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

[Johannes Schindelin]

Hi Max,

On 2015-10-01 06:39, Max Kirillov wrote:
> 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?

That must be the reason, thanks!

Ciao,
Dscho

[PATCH v2 0/4] Fix locking issues on Windows with `git clone --dissociate`

[Johannes Schindelin]

This is version 2, split into multiple commits for easier digestion.

Max, I hope that this helps also your use case!

Johannes Schindelin (4):
  Demonstrate a Windows file locking issue with `git clone --dissociate`
  Consolidate code to close a pack's file descriptor
  Add a function to release all packs
  clone --dissociate: avoid locking pack files

 builtin/clone.c            |  4 +++-
 cache.h                    |  1 +
 sha1_file.c                | 56 ++++++++++++++++++++++++++++------------------
 t/t5700-clone-reference.sh | 21 +++++++++++++++++
 4 files changed, 59 insertions(+), 23 deletions(-)

-- 
2.5.3.windows.1.3.gc322723

[PATCH v2 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate`

[Johannes Schindelin]

On Windows, dissociating from a reference can fail very easily due to
pack files that are still in use when they want to be removed.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5700-clone-reference.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index ef1779f..f7cec85 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_failure MINGW '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

[PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sha1_file.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index d295a32..8c3c913 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p)
 	}
 }
 
+static int close_pack_fd(struct packed_git *p)
+{
+	if (p->pack_fd < 0)
+		return 0;
+
+	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);
 			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
@@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p)
 {
 	if (!open_packed_git_1(p))
 		return 0;
-	if (p->pack_fd != -1) {
-		close(p->pack_fd);
-		pack_open_fds--;
-		p->pack_fd = -1;
-	}
+	close_pack_fd(p);
 	return -1;
 }
 
@@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p,
 					p->pack_name,
 					strerror(errno));
 			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);
 			pack_mmap_calls++;
 			pack_open_windows++;
 			if (pack_mapped > peak_pack_mapped)
-- 
2.5.3.windows.1.3.gc322723

[PATCH v2 3/4] Add a function to release all packs

[Johannes Schindelin]

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 | 20 +++++++++++++++++---
 2 files changed, 18 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..fe823fe 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -798,6 +798,22 @@ 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)
+		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 +922,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)
-- 
2.5.3.windows.1.3.gc322723

[PATCH v2 4/4] clone --dissociate: avoid locking pack files

[Johannes Schindelin]

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

The test case to demonstrate the breakage technically does not need to
be run on Linux or MacOSX. It won't hurt, either, though.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/clone.c            | 4 +++-
 t/t5700-clone-reference.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

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();
+	}
 
 	junk_mode = JUNK_LEAVE_REPO;
 	err = checkout();
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index f7cec85..2250ef4 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' '
 	test_must_fail git -C R fsck &&
 	git -C S fsck
 '
-test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '
+test_expect_success 'clone, dissociate from partial reference and repack' '
 	rm -fr P Q R &&
 	git init P &&
 	(
-- 
2.5.3.windows.1.3.gc322723

Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Junio C Hamano]

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?

It seems to me that it would be a bug if (p->pack_fd < 0) in this
codepath, so in practice nobody will receive a newly invented return
value 0 from this function, but it feels wrong.

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

Also closing "fd" is not the only thing the new helper does, so in
that sense its name is suboptimal, too.

Perhaps a new helper function that is close_pack(), which is
unconditional, with another close_pack_if_open() around it?

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

I'd imagine we'll see the answer in the next patch ;-)

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> 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();
> +	}

Much simpler and looks very good.

Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Johannes Schindelin]

Hi Junio,

On 2015-10-05 22:57, Junio C Hamano wrote:
> 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?

Yes. At this point, `lru_p` can only be non-NULL if lru_p->pack_fd is not larger than 0 (hence the call to `close(lru_p->pack_fd)` does not fail all the time, and hence the `pack_open_fds--` does not result in inconsistent state).

> It seems to me that it would be a bug if (p->pack_fd < 0) in this
> codepath, so in practice nobody will receive a newly invented return
> value 0 from this function, but it feels wrong.

Yes, it would be a bug. And more subtle things would go wrong if that was the case, too.

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

Please note that the `close_pack_fd(p)` call does not invalidate the data. It is the caller (`free_pack_by_name()`) that does. Which is safe.

> Also closing "fd" is not the only thing the new helper does, so in
> that sense its name is suboptimal, too.

Yes, it also decrements the counter. But that is a necessary consequence of closing the file descriptor, otherwise the state would be inconsistent.

> Perhaps a new helper function that is close_pack(), which is
> unconditional, with another close_pack_if_open() around it?

Next patch. And that `close_pack()` actually does do more than closing the file descriptor.

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

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.

Ciao,
Johannes

Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Junio C Hamano]

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

Instead we'd make it absolutely clear what our assumption of this
change and the new helper function is, and a caller that may appear
in the future that wants to have do_not_close bit can complain.  At
that point, maybe we can decide to either loosen it if that caller
has a good reason, or tell the caller to drop do_not_close bit
before calling close-all.

[PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate`

[Johannes Schindelin]

This is version 3, adding that BUG! message if do_not_close was set.

Max, I still hope that this patch series helps also your use case!

Interdiff below the diffstat.

Johannes Schindelin (4):
  Demonstrate a Windows file locking issue with `git clone --dissociate`
  Consolidate code to close a pack's file descriptor
  Add a function to release all packs
  clone --dissociate: avoid locking pack files

 builtin/clone.c            |  4 +++-
 cache.h                    |  1 +
 sha1_file.c                | 59 +++++++++++++++++++++++++++++-----------------
 t/t5700-clone-reference.sh | 21 +++++++++++++++++
 4 files changed, 62 insertions(+), 23 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index fe823fe..ca699d7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -810,7 +810,10 @@ void close_all_packs(void)
 	struct packed_git *p;
 
 	for (p = packed_git; p; p = p->next)
-		close_pack(p);
+		if (p->do_not_close)
+			die("BUG! Want to close pack marked 'do-not-close'");
+		else
+			close_pack(p);
 }
 
 
-- 
2.6.1.windows.1

[PATCH v3 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate`

[Johannes Schindelin]

On Windows, dissociating from a reference can fail very easily due to
pack files that are still in use when they want to be removed.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5700-clone-reference.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index ef1779f..f7cec85 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_failure MINGW '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.6.1.windows.1

[PATCH v3 2/4] Consolidate code to close a pack's file descriptor

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sha1_file.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index d295a32..8c3c913 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p)
 	}
 }
 
+static int close_pack_fd(struct packed_git *p)
+{
+	if (p->pack_fd < 0)
+		return 0;
+
+	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);
 			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
@@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p)
 {
 	if (!open_packed_git_1(p))
 		return 0;
-	if (p->pack_fd != -1) {
-		close(p->pack_fd);
-		pack_open_fds--;
-		p->pack_fd = -1;
-	}
+	close_pack_fd(p);
 	return -1;
 }
 
@@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p,
 					p->pack_name,
 					strerror(errno));
 			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);
 			pack_mmap_calls++;
 			pack_open_windows++;
 			if (pack_mapped > peak_pack_mapped)
-- 
2.6.1.windows.1

[PATCH v3 3/4] Add a function to release all packs

[Johannes Schindelin]

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)
-- 
2.6.1.windows.1

[PATCH v3 4/4] clone --dissociate: avoid locking pack files

[Johannes Schindelin]

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

The test case to demonstrate the breakage technically does not need to
be run on Linux or MacOSX. It won't hurt, either, though.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/clone.c            | 4 +++-
 t/t5700-clone-reference.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

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();
+	}
 
 	junk_mode = JUNK_LEAVE_REPO;
 	err = checkout();
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index f7cec85..2250ef4 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' '
 	test_must_fail git -C R fsck &&
 	git -C S fsck
 '
-test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '
+test_expect_success 'clone, dissociate from partial reference and repack' '
 	rm -fr P Q R &&
 	git init P &&
 	(
-- 
2.6.1.windows.1

Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor

[Johannes Schindelin]

Hi Junio,

On 2015-10-06 00:15, Junio C Hamano wrote:
> 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."

I just sent another iteration out that adds this sanity check.

Ciao,
Dscho

Re: [PATCH v3 3/4] Add a function to release all packs

[Junio C Hamano]

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.  

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

Re: [PATCH v3 3/4] Add a function to release all packs

[Johannes Schindelin]

Hi Junio,

On 2015-10-07 19:49, Junio C Hamano wrote:
> 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.

This time I was more clever and checked `pu` before sending off a fixed patch, seeing that you patched this already.

Thanks,
Dscho

Re: [PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate`

[Max Kirillov]

On Tue, Oct 06, 2015 at 03:17:36PM +0200, Johannes Schindelin wrote:
> This is version 3, adding that BUG! message if do_not_close was set.
> 
> Max, I still hope that this patch series helps also your use case!

Thanks, this mostly makes gone one of my commits. I only
need to invoke the function after builtin is done. And maybe
spend some time to check that dies do not happen.

The other one with cloexec I think I will still need.

It migh take some time before I return to it.

-- 
Max