Hi,
On Fri, 8 Aug 2008, Pieter de Bie wrote:
On 8 aug 2008, at 18:19, Shawn O. Pearce wrote:
quoted
The unpack-objects process will fail when it finds this bad object, and
everything after that in the pack file will be dropped on the floor and
not get unpacked.
Even with the -r switch?
-r When unpacking a corrupt packfile, the command dies at the first
corruption. This flag tells it to keep going and make
the best effort to recover as many objects as possible.
In any case, the pack is too large for me to let my computer repack
everything, when only one object needs repacking.
Ciao,
Dscho
On Fri, 8 Aug 2008, Johannes Schindelin wrote:
In any case, the pack is too large for me to let my computer repack
everything, when only one object needs repacking.
By that you mean you cannot/don't want to use repack -f, right?
There _could_ be a way to hack pack-objects so not to reuse bad objects.
However I don't want that to impact the code too much for an
event that hopefully should almost never happens, especially if using -f
does work around it already.
Well, let's see.
[...]
OK, here's what the patch to allow repacking without -f and still using
redundant objects in presence of pack corruption might look like.
Please tell me if that works for you.
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..88e73f3 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -277,6 +277,7 @@ static unsigned long write_object(struct sha1file *f,
*/
if (!to_reuse) {
+ no_reuse:
if (!usable_delta) {
buf = read_sha1_file(entry->idx.sha1, &type, &size);
if (!buf)@@ -364,14 +365,28 @@ static unsigned long write_object(struct sha1file *f,
reused_delta++;
}
hdrlen = encode_header(type, entry->size, header);
+
offset = entry->in_pack_offset;
revidx = find_pack_revindex(p, offset);
datalen = revidx[1].offset - offset;
if (!pack_to_stdout && p->index_version > 1 &&
- check_pack_crc(p, &w_curs, offset, datalen, revidx->nr))
- die("bad packed object CRC for %s", sha1_to_hex(entry->idx.sha1));
+ check_pack_crc(p, &w_curs, offset, datalen, revidx->nr)) {
+ error("bad packed object CRC for %s", sha1_to_hex(entry->idx.sha1));
+ if (entry->delta)
+ reused_delta--;
+ goto no_reuse;
+ }
+
offset += entry->in_pack_header_size;
datalen -= entry->in_pack_header_size;
+ if (!pack_to_stdout && p->index_version == 1 &&
+ check_pack_inflate(p, &w_curs, offset, datalen, entry->size)) {
+ die("corrupt packed object for %s", sha1_to_hex(entry->idx.sha1));
+ if (entry->delta)
+ reused_delta--;
+ goto no_reuse;
+ }
+
if (type == OBJ_OFS_DELTA) {
off_t ofs = entry->idx.offset - entry->delta->idx.offset;
unsigned pos = sizeof(dheader) - 1;@@ -394,10 +409,6 @@ static unsigned long write_object(struct sha1file *f,
return 0;
sha1write(f, header, hdrlen);
}
-
- if (!pack_to_stdout && p->index_version == 1 &&
- check_pack_inflate(p, &w_curs, offset, datalen, entry->size))
- die("corrupt packed object for %s", sha1_to_hex(entry->idx.sha1));
copy_pack_data(f, p, &w_curs, offset, datalen);
unuse_pack(&w_curs);
reused++;
Nicolas
Nicolas Pitre [off-list ref] wrote:
OK, here's what the patch to allow repacking without -f and still using
redundant objects in presence of pack corruption might look like.
Please tell me if that works for you.
Aside from goto being considered harmful by some really smart people,
this patch makes a lot of sense. Its only downside is a backwards
goto within this function, but the code is actually still quite
clear to me.
If this allows git to magically fix Dscho's bad pack, it may be
worth including in the core tree.
quoted hunk
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..88e73f3 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -277,6 +277,7 @@ static unsigned long write_object(struct sha1file *f,
*/
if (!to_reuse) {
+ no_reuse:
if (!usable_delta) {
buf = read_sha1_file(entry->idx.sha1, &type, &size);
if (!buf)@@ -364,14 +365,28 @@ static unsigned long write_object(struct sha1file *f,
reused_delta++;
}
hdrlen = encode_header(type, entry->size, header);
+
offset = entry->in_pack_offset;
revidx = find_pack_revindex(p, offset);
datalen = revidx[1].offset - offset;
if (!pack_to_stdout && p->index_version > 1 &&
- check_pack_crc(p, &w_curs, offset, datalen, revidx->nr))
- die("bad packed object CRC for %s", sha1_to_hex(entry->idx.sha1));
+ check_pack_crc(p, &w_curs, offset, datalen, revidx->nr)) {
+ error("bad packed object CRC for %s", sha1_to_hex(entry->idx.sha1));
+ if (entry->delta)
+ reused_delta--;
+ goto no_reuse;
+ }
+
offset += entry->in_pack_header_size;
datalen -= entry->in_pack_header_size;
+ if (!pack_to_stdout && p->index_version == 1 &&
+ check_pack_inflate(p, &w_curs, offset, datalen, entry->size)) {
+ die("corrupt packed object for %s", sha1_to_hex(entry->idx.sha1));
+ if (entry->delta)
+ reused_delta--;
+ goto no_reuse;
+ }
+
if (type == OBJ_OFS_DELTA) {
off_t ofs = entry->idx.offset - entry->delta->idx.offset;
unsigned pos = sizeof(dheader) - 1;@@ -394,10 +409,6 @@ static unsigned long write_object(struct sha1file *f,
return 0;
sha1write(f, header, hdrlen);
}
-
- if (!pack_to_stdout && p->index_version == 1 &&
- check_pack_inflate(p, &w_curs, offset, datalen, entry->size))
- die("corrupt packed object for %s", sha1_to_hex(entry->idx.sha1));
copy_pack_data(f, p, &w_curs, offset, datalen);
unuse_pack(&w_curs);
reused++;
Nicolas
--
Shawn.
On Sun, 10 Aug 2008, Shawn O. Pearce wrote:
Nicolas Pitre [off-list ref] wrote:
quoted
OK, here's what the patch to allow repacking without -f and still using
redundant objects in presence of pack corruption might look like.
Please tell me if that works for you.
Aside from goto being considered harmful by some really smart people,
Well, other really smart people consider gotos perfectly fine when used
judiciously. So this ends up being a question of belief and taste.
this patch makes a lot of sense. Its only downside is a backwards
goto within this function, but the code is actually still quite
clear to me.
The actual downside I see with this patch is the fact that real data
corruptions might be "fixed" automagically with user unaware of it.
This could be a serious sign that the hardware is going bad and
requiring the user to consciously use -f to fix things is good. However
it is most unlikely that redundant objects will be kept around in the
normal case, hence manual intervention will be needed anyway to bring a
copy of bad object into the repository. So not having to use -f might
not be such an issue.
If this allows git to magically fix Dscho's bad pack, it may be
worth including in the core tree.
Yep.
Nicolas
Nicolas Pitre [off-list ref] wrote:
The actual downside I see with this patch is the fact that real data
corruptions might be "fixed" automagically with user unaware of it.
This could be a serious sign that the hardware is going bad and
requiring the user to consciously use -f to fix things is good. However
it is most unlikely that redundant objects will be kept around in the
normal case, hence manual intervention will be needed anyway to bring a
copy of bad object into the repository. So not having to use -f might
not be such an issue.
Yup, I agree completely.
Duplicates should be rare, and likely are only the fault of a
dumb transport fetch, or the user trying to fix their repository
by placing copies of corrupt objects obtained from elsewhere.
Requiring -f to fix such cases is heavy-handed. Some trees can
take many hours to repack with -f; think gcc or OOo.
--
Shawn.
Hi,
On Sun, 10 Aug 2008, Nicolas Pitre wrote:
On Fri, 8 Aug 2008, Johannes Schindelin wrote:
quoted
In any case, the pack is too large for me to let my computer repack
everything, when only one object needs repacking.
By that you mean you cannot/don't want to use repack -f, right?
Right. However, I had a relatively fast machine standing nearby today,
so that scp was not too painful.
There _could_ be a way to hack pack-objects so not to reuse bad objects.
However I don't want that to impact the code too much for an event that
hopefully should almost never happens, especially if using -f does work
around it already.
Well, let's see.
[...]
OK, here's what the patch to allow repacking without -f and still using
redundant objects in presence of pack corruption might look like.
Please tell me if that works for you.
The testing took quite a while unfortunately, mainly because I followed
Shawn's advice, and added not only a loose object, but also a single pack
with the single object in it, and a newer timestamp.
This resulted in my CPU being hogged when Git tried to read the object. I
do not know exactly what is happening, but I suspect an infinite loop due
to the funny interaction between a valid and a corrupt pack containing the
same object. Or maybe the issue described later in this mail.
Only when I removed the pack did things actually go further, so there is
still a bug lurking.
Your patch worked _almost_:
offset += entry->in_pack_header_size;
datalen -= entry->in_pack_header_size;
+ if (!pack_to_stdout && p->index_version == 1 &&
+ check_pack_inflate(p, &w_curs, offset, datalen, entry->size)) {
+ die("corrupt packed object for %s", sha1_to_hex(entry->idx.sha1));
This needs to be an error(), obviously.
+ if (entry->delta)
+ reused_delta--;
+ goto no_reuse;
+ }
+
if (type == OBJ_OFS_DELTA) {
off_t ofs = entry->idx.offset - entry->delta->idx.offset;
unsigned pos = sizeof(dheader) - 1;
With that, it took quite a while, then it told me about the corrupt
object.
And then it hangs in the loop sha1_file.c:1511. The function inflate()
returns Z_BUF_ERROR, and nothing is read.
Oh, and it still tries to access the same corrupt pack.
Thanks,
Dscho
P.S.: I have to wrap up my work at my current (interim) job, and will be
moving in the next days, so do not expect too much from my side before
Monday.