The opened packs seem to stay open forever.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

I'm very unsure about the solution, though: it is really horrible code
to debug...

 builtin-pack-objects.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
[ message continues ]

In my MinGW port I have the patch below that avoids that t5303 fails
because of a pack file that remains open. (Open files cannot be replaced
on Windows.) I had hoped that your patch would help, but it does not.
Something else still keeps the pack file open. Can anything be done about
that?

-- Hannes

From: Johannes Sixt <j6t@kdbg.org>
Date: Mon, 17 Nov 2008 09:25:19 +0100
Subject: [PATCH] t5303: Do not overwrite an existing pack

This test corrupts a pack file, then repacks the objects. The consequence
is that the repacked pack file has the same name as the original file
(that has been corrupted).

During its operation, git-pack-objects opens the corrupted file and keeps
it open at all times. On Windows, this is a problem because a file that is
open in any process cannot be delete or replaced, but that is what we do
in some of the test cases, and so they fail.

The work-around is to write the repacked objects to a file of a different
name, and replace the original after git-pack-objects has terminated.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t5303-pack-corruption-resilience.sh |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t5303-pack-corruption-resilience.sh
b/t/t5303-pack-corruption-resilience.sh
index 5132d41..41c83e3 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -43,8 +43,11 @@ create_new_pack() {

 do_repack() {
     pack=`printf "$blob_1\n$blob_2\n$blob_3\n" |
-          git pack-objects $@ .git/objects/pack/pack` &&
-    pack=".git/objects/pack/pack-${pack}"
+          git pack-objects $@ .git/objects/pack/packtmp` &&
+    packtmp=".git/objects/pack/packtmp-${pack}" &&
+    pack=".git/objects/pack/pack-${pack}" &&
+    mv "${packtmp}.pack" "${pack}.pack" &&
+    mv "${packtmp}.idx" "${pack}.idx"
 }

 do_corrupt_object() {
-- 
1.6.0.4.1683.g35125


--

[ message continues ]

Do this _and_ the other handle-leak-patch together help?

BTW, you better report such things. Even though it is typical
for this platform brokenness, leaking open files is never good.
--

[ message continues ]

No, that doesn't help, either. :-(

-- Hannes
--

[ message continues ]

Nicolas
--

[ message continues ]

Thanks, but I should have mentioned that at this time this patch was just
meant for exposition, not inclusion. [It's just one of many, many patches
needed to make the test suite pass on MinGW.]

I'd prefer a solution to the problem that the pack file remains open. Do
you have an idea where git-pack-objects keeps the pack file open, even
with Alex's two patches applied?

-- Hannes

--

[ message continues ]

Well, I'd include it right away since it is fundamentally the right 

That's not the issue.

The test is using pack-objects to overwrite a pack file, but to do so 
pack-objects has to open and read from the pack file about to be 
overwritten.  This is just wrong even if it happens to work by luck on 
Linux.

It is on purpose that the pack files are kept open.  This is to minimize 
the number of open() and mmap()/read() operations between successive 
object reads.  Those are not leaked file handles since they get closed 
when new packs are opened and the number of concurrent opened packs 
reaches a certain limit.


Nicolas
--

[ message continues ]

Are you sure? Will it work in a real repository? Were noone does
rename the previous pack files into packtmp-something?
--

[ message continues ]

Oh, the patch only works around the failure in the test case. In a real
repository there is usually no problem because the destination pack file
does not exist.

The unusual case is where you do this:

 $ git rev-list -10 HEAD | git pack-objects foobar

twice in a row: In this case the second invocation fails on Windows
because the destination pack file already exists *and* is open. But not
even git-repack does this even if it is called twice. OTOH, the test case
*does* exactly this.

-- Hannes

--

[ message continues ]

Still bad...

BTW, the patch _does_ fix the "unusual case" for me. IOW, I plainly copied
your rev-list|pack-objects into command line. As expected, it fails for Junio's
master (after the second run) and works with the patch:

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  82864ec14cd06e6089543a1419762e4cd40f7988
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  fatal: unable to rename temporary pack file: Permission denied

  $ git cherry-pick fix-fd-leak
  Finished one cherry-pick.
  [master]: created e629465: "Fix handle leak in builtin-pack-objects"
   1 files changed, 1 insertions(+), 0 deletions(-)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 9 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)

  $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar
  Counting objects: 10, done.
  Compressing objects: 100% (9/9), done.
  0e43d919a2a8336fd740d8d9b8f9f78d49e855e5
  Writing objects: 100% (10/10), done.
  Total 10 (delta 1), reused 10 (delta 1)
--

[ message continues ]

OK.... Well, despite my earlier assertion, I think the above should be a 
valid operation.

I'm looking at it now.  I'm therefore revoking my earlier ACK as well 
(better keep that test case alive).


Nicolas
--

[ message continues ]

Hold on a moment: When I tested the above sequence, I was fooled by a flaw
in mingw_rename() (it doesn't replace read-only files). With that fixed,
it works as expected in repeated invocations (note that foobar is outside
the .git/objects/pack directory).

If I use .git/objects/pack/foobar instead, then I get the failures on
Windows, and I won't argue that this should be "fixed". ;)

-- Hannes
--

[ message continues ]

From: Johannes Sixt <j6t@kdbg.org>

On POSIX, rename() can replace files that are not writable. On Windows,
however, read-only files cannot be replaced without additional efforts:
We have to make the destination writable first.

Since the situations where the destination is read-only are rare, we do not
make the destination writable on every invocation, but only if the first
try to rename a file failed with an "access denied" error.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---

Would you please clarify which operation you exactly mean? As is written

Here is this fix.

-- Hannes

 compat/mingw.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b534a8a..3dbe6a7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -819,6 +819,8 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	DWORD attrs;
+
 	/*
 	 * Try native rename() first to get errno right.
 	 * It is based on MoveFile(), which cannot overwrite existing files.
@@ -830,12 +832,19 @@ int mingw_rename(const char *pold, const char *pnew)
 	if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
 		return 0;
 	/* TODO: translate more errors */
-	if (GetLastError() == ERROR_ACCESS_DENIED) {
-		DWORD attrs = GetFileAttributes(pnew);
-		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) {
+	if (GetLastError() == ERROR_ACCESS_DENIED &&
+	    (attrs = GetFileAttributes(pnew)) != INVALID_FILE_ATTRIBUTES) {
+		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
 			errno = EISDIR;
 			return -1;
 		}
+		if ((attrs & FILE_ATTRIBUTE_READONLY) &&
+		    SetFileAttributes(pnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {
+			if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
+				return 0;
+			/* revert file attributes on failure */
+			SetFileAttributes(pnew, attrs);
+		}
 	}
 	errno = EACCES;
 	return -1;
-- 
1.6.0.4.1694.g07ac

--

[ message continues ]

Any news here?
--

[ message continues ]

Yes: my hard disk crashed a couple hours after I mentioned this, so most 
of my time has been spent on recovery since then.  I'll come back to it 
eventually.


Nicolas
--

[ message continues ]

Heh... Good luck!

--

[ message continues ]

Especially on Windows where an opened file cannot be replaced, make
sure pack-objects always close packs it is about to replace. Even on
non Windows systems, this could save potential bad results if ever
objects were to be read from the new pack file using offset from the old
index.

This should fix t5303 on Windows.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---


OK, here it is at last.  Please confirm it works on Windows before Junio 
merges it.

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 67eefa2..cedef52 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -535,6 +535,7 @@ static void write_pack_file(void)
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
+			free_pack_by_name(tmpname);
 			if (adjust_perm(pack_tmp_name, mode))
 				die("unable to make temporary pack file readable: %s",
 				    strerror(errno));
diff --git a/cache.h b/cache.h
index f15b3fc..231c06d 100644
--- a/cache.h
+++ b/cache.h
@@ -820,6 +820,7 @@ extern int open_pack_index(struct packed_git *);
 extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
+extern void free_pack_by_name(const char *);
 extern struct packed_git *add_packed_git(const char *, int, int);
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
 extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
diff --git a/sha1_file.c b/sha1_file.c
index 6c0e251..0e021c5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -673,6 +673,37 @@ void unuse_pack(struct pack_window **w_cursor)
 }
 
 /*
+ * This is used by git-repack in case a newly created pack happens to
+ * contain the same set of objects as an existing one.  In that case
+ * the resulting file might be different even if its name would be the
+ * same.  It is best to close any reference to the ...
[ message continues ]

Will do in about 16 hours.

--

[ message continues ]

I can confirm that this patch fixes t5303 on Windows (MinGW).

-- Hannes
--

[ message continues ]

Thanks; it is a bit too late for tonight, but it will appear in tomorrow's
'master'.
--

[ message continues ]

And it does that for me, FWIW.
--

[ message continues ]

What is the actual problem?  Pack windows are left open on purpose.


Nicolas
--

[ message continues ]

* expecting success: do_repack &&
     git prune-packed -v &&
     git verify-pack ${pack}.pack &&
     git cat-file blob $blob_1 > /dev/null &&
     git cat-file blob $blob_2 > /dev/null &&
     git cat-file blob $blob_3 > /dev/null
Counting objects: 3, done.
error: unknown object type 0 at offset 2032 in
.git/objects/pack/pack-8d1e04fc992cfbdb3ab72cf7abbf08cce8b1900.pack
Compressing objects: 100% (2/2), done.
fatal: unable to rename temporary pack file
.git/objects/pack/tmp_pack_s9Gijm->.git/objects/pack/pack-b8d1e04fc992cfbdbab72cf7abbf08cce8b1900.pack:
Permission denied
* FAIL 10: ... and then a repack "clears" the corruption
        do_repack &&
             git prune-packed -v &&
             git verify-pack ${pack}.pack &&
             git cat-file blob $blob_1 > /dev/null &&
             git cat-file blob $blob_2 > /dev/null &&
             git cat-file blob $blob_3 > /dev/null
--

[ message continues ]