Fix handle leak in builtin-pack-objects
PATCH 1/1 Fix handle leak in builtin-pack-objects
The opened packs seem to stay open forever.
---
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(-)
From d0fbfd05e5c1f41f58281d85d68c705c5f036e7d Mon Sep 17 00:00:00 2001
From: Alex Riesen <[email protected]>
Date: Wed, 19 Nov 2008 11:04:48 +0100
Subject: [PATCH] Fix handle leak in builtin-pack-objects
The opened packs seem to stay open forever.
--- builtin-pack-objects.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
builtin-pack-objects.c +1 -0
@@ -532,6 +532,7 @@ static void write_pack_file(void) idx_tmp_name =write_idx_file (NULL , written_list, nr_written, sha1); +release_pack_memory (-1 ,-1 );snprintf (tmpname,sizeof (tmpname),"%s-%s.pack" , base_name,sha1_to_hex (sha1));
Signature · 1 line
1.6.0.4.644.gb619a
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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 <[email protected]>
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.
--- t/t5303-pack-corruption-resilience.sh | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/t/t5303-pack-corruption-resilience.sh
index 5132d41..41c83e3 100755
t/t5303-pack-corruption-resilience.sh +5 -2
@@ -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() {
Signature · 1 line
1.6.0.4.1683.g35125
2008/11/19 Johannes Sixt <[email protected]>:
Quoted 9 lines
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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?
Do this _and_ the other handle-leak-patch together help? (I think the second should)
Quoted 2 lines
From: Johannes Sixt <[email protected]>
Date: Mon, 17 Nov 2008 09:25:19 +0100
BTW, you better report such things. Even though it is typical for this platform brokenness, leaking open files is never good.
Quoted 10 lines · Alex Riesen
2008/11/19 Johannes Sixt <[email protected]>:
Quoted 8 lines
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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?
Quoted 2 lines
Do this _and_ the other handle-leak-patch together help? (I think the second should)
No, that doesn't help, either. :-(
-- Hannes
Nicolas Pitre
Acked-byQuoted 24 lines · Johannes Sixt
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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 <[email protected]>
Date: Mon, 17 Nov 2008 09:25:19 +0100
Subject: [PATCH] t5303: Do not overwrite an existing packThis 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.
Quoted 4 lines
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 <[email protected]>
Quoted 27 lines
--- t/t5303-pack-corruption-resilience.sh | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)t/t5303-pack-corruption-resilience.sh +5
-2@@ -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() {Signature · 1 line
1.6.0.4.1683.g35125
Nicolas
Quoted 16 lines · Nicolas Pitre
Quoted 14 lines · Johannes Sixt
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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 <[email protected]>
Date: Mon, 17 Nov 2008 09:25:19 +0100
Subject: [PATCH] t5303: Do not overwrite an existing pack
...
Quoted 1 line
Acked-by Nicolas Pitre <[email protected]>
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
Quoted 17 lines · Johannes Sixt
Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Johannes Sixt
Quoted 1 line · Alex Riesen
The opened packs seem to stay open forever.
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?
Quoted 6 lines
Quoted 6 lines
-- Hannes
From: Johannes Sixt <[email protected]>
Date: Mon, 17 Nov 2008 09:25:19 +0100
Subject: [PATCH] t5303: Do not overwrite an existing pack
Quoted 5 lines
...
Quoted 1 line
Acked-by Nicolas Pitre <[email protected]>Thanks, but I should have mentioned that at this time this patch was just meant for exposition, not inclusion.
Well, I'd include it right away since it is fundamentally the right thing to do.
Quoted 3 lines
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?
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
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 8 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>
Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
Quoted 10 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 6 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
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
2008/11/19 Johannes Sixt <[email protected]>:
Quoted 16 lines
Quoted 10 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 6 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
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.
Quoted 9 lines
The unusual case is where you do this:
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
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)
Quoted 16 lines · Johannes Sixt
Quoted 10 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 6 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
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.
Quoted 8 lines
The unusual case is where you do this:
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
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
Quoted 10 lines · Nicolas Pitre
Quoted 8 lines · Johannes Sixt
The unusual case is where you do this:
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
Quoted 5 lines
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).
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
PATCH 1/1 compat/mingw.c: Teach mingw_rename() to replace read-only files
From: Johannes Sixt <[email protected]>
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.
---
Quoted 4 lines · Johannes Sixt
Quoted 3 lines · Nicolas Pitre
Quoted 2 lines · Johannes Sixt
The unusual case is where you do this:
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
OK.... Well, despite my earlier assertion, I think the above should be a valid operation.
Would you please clarify which operation you exactly mean? As is written above, or with .git/objects/pack/foobar instead like in the test case?
Quoted 5 lines
Quoted 2 lines
I'm looking at it now. I'm therefore revoking my earlier ACK as well (better keep that test case alive).
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...
Here is this fix.
-- Hannes
compat/mingw.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-)
compat/mingw.c +12 -3
@@ -819,6 +819,8 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)#undef renameint 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 ;
Signature · 1 line
1.6.0.4.1694.g07ac
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 26 lines · Johannes Sixt
Quoted 16 lines
Quoted 10 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 6 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
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.
Quoted 8 lines
The unusual case is where you do this:
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
Quoted 6 lines
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).
Any news here?
Quoted 27 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 26 lines · Johannes Sixt
Quoted 24 lines
Quoted 10 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 6 lines · Johannes Sixt
Quoted 4 lines
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 <[email protected]>Acked-by Nicolas Pitre <[email protected]>Are you sure? Will it work in a real repository? Were noone does rename the previous pack files into packtmp-something?
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 foobartwice 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.
Quoted 8 lines
Quoted 6 lines
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).
Any news here?
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
Nicolas Pitre, Wed, Nov 26, 2008 15:33:51 +0100:
Quoted 11 lines · Alex Riesen
Quoted 6 lines
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 3 lines
I'm looking at it now. I'm therefore revoking my earlier ACK as well (better keep that test case alive).
Any news here?
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.
Heh... Good luck!
PATCH 1/1 make sure packs to be replaced are closed beforehand
In reply to Nicolas PitreEspecially 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.
---
Quoted 23 lines · Nicolas Pitre
Quoted 22 lines · Alex Riesen
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 20 lines · Johannes Sixt
Quoted 4 lines
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.
Quoted 8 lines
The unusual case is where you do this:
$ git rev-list -10 HEAD | git pack-objects foobartwice 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.
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).
Quoted 5 lines
Quoted 1 line
Any news here?
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.
OK, here it is at last. Please confirm it works on Windows before Junio merges it.
builtin-pack-objects.c +1 -0
@@ -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));
cache.h +1 -0
@@ -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 );
sha1_file.c +31 -0
@@ -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 old pack before it is +* replaced on disk. Of course no index pointers nor windows for given pack +* must subsist at this point. If ever objects from this pack are requested +* again, the new version of the pack will be reinitialized through +* reprepare_packed_git(). +*/ +void free_pack_by_name (const char *pack_name) +{ +struct packed_git *p, **pp = &packed_git; + +while (*pp) { + p = *pp; +if (strcmp (pack_name, p->pack_name) ==0 ) { +close_pack_windows (p); +if (p->pack_fd !=-1 ) +close (p->pack_fd); +if (p->index_data) +munmap ((void *)p->index_data, p->index_size); +free (p->bad_object_sha1); + *pp = p->next; +free (p); +return ; + } + pp = &p->next; + } +} + +/* * Do not call this directly as this leaks p->pack_fd on error return; * call open_packed_git() instead. */
Re: [PATCH] make sure packs to be replaced are closed beforehand
Nicolas Pitre, Tue, Dec 09, 2008 20:26:52 +0100:
Quoted 10 lines
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 <[email protected]>---
...
Quoted 3 lines
OK, here it is at last. Please confirm it works on Windows before Junio merges it.
Will do in about 16 hours.
Re: [PATCH] make sure packs to be replaced are closed beforehand
In reply to Nicolas PitreQuoted 7 lines · Nicolas Pitre
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.
...
Quoted 2 lines
OK, here it is at last. Please confirm it works on Windows before Junio merges it.
I can confirm that this patch fixes t5303 on Windows (MinGW).
-- Hannes
Re: [PATCH] make sure packs to be replaced are closed beforehand
Quoted 7 lines · Johannes Sixt
Quoted 6 lines · Nicolas Pitre
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.
Quoted 6 lines
Quoted 1 line
This should fix t5303 on Windows.
...
Quoted 2 lines
OK, here it is at last. Please confirm it works on Windows before Junio merges it.
I can confirm that this patch fixes t5303 on Windows (MinGW).
Thanks; it is a bit too late for tonight, but it will appear in tomorrow's 'master'.
Re: [PATCH] make sure packs to be replaced are closed beforehand
In reply to Johannes Sixt2008/12/10 Johannes Sixt <[email protected]>:
Quoted 7 lines
Quoted 6 lines · Nicolas Pitre
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.
Quoted 7 lines
Quoted 1 line
This should fix t5303 on Windows.
...
Quoted 2 lines
OK, here it is at last. Please confirm it works on Windows before Junio merges it.
I can confirm that this patch fixes t5303 on Windows (MinGW).
And it does that for me, FWIW.
Quoted 7 lines · Alex Riesen
The opened packs seem to stay open forever.
Signed-off-by Alex Riesen <[email protected]>---
I'm very unsure about the solution, though: it is really horrible code to debug...
What is the actual problem? Pack windows are left open on purpose.
Nicolas
2008/11/19 Nicolas Pitre <[email protected]>:
Quoted 12 lines · Alex Riesen
Quoted 8 lines
The opened packs seem to stay open forever.
Signed-off-by Alex Riesen <[email protected]>---
I'm very unsure about the solution, though: it is really horrible code to debug...
What is the actual problem? Pack windows are left open on purpose.
* 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/nullCounting 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