Merge lp://staging/~xaav/bzr-git/bugfix into lp://staging/bzr-git

Proposed by xaav
Status: Rejected
Rejected by: Jelmer Vernooij
Proposed branch: lp://staging/~xaav/bzr-git/bugfix
Merge into: lp://staging/bzr-git
Diff against target: 15 lines (+4/-1)
1 file modified
object_store.py (+4/-1)
To merge this branch: bzr merge lp://staging/~xaav/bzr-git/bugfix
Reviewer Review Type Date Requested Status
Jelmer Vernooij Disapprove
Review via email: mp+59160@code.staging.launchpad.net

Description of the change

Fixed related bug.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This exception is a sign of a genuine bug or broken data elsewhere; ignoring it is a bad idea (and will cause other issues down the line).

review: Disapprove
Revision history for this message
xaav (xaav) wrote :

> This exception is a sign of a genuine bug or broken data elsewhere; ignoring
> it is a bad idea (and will cause other issues down the line).

The problem isn't with this git software; it's with the repository. However, according to the official git developers, this should be a warning - Not a fatal crash.

This fix prints the exception (warns the user), but avoids a fatal crash.

If you have another suggestion, I would like to hear it. This is a genuine problem, but does not warrant a crash. In fact, crashing is causing issues with launchpad (see related bug). Simply ignoring this is not going to fix the problem.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

On Wed, 2011-04-27 at 22:41 +0000, Geoff wrote:
> > This exception is a sign of a genuine bug or broken data elsewhere; ignoring
> > it is a bad idea (and will cause other issues down the line).
>
> The problem isn't with this git software; it's with the repository. However, according to the official git developers, this should be a warning - Not a fatal crash.
>
> This fix prints the exception (warns the user), but avoids a fatal crash.
>
> If you have another suggestion, I would like to hear it. This is a genuine problem, but does not warrant a crash. In fact, crashing is causing issues with launchpad (see related bug). Simply ignoring this is not going to fix the problem.
A git object not matching its expected checksum is an error in Git too.

bzr-git does not keep a copy of the original git objects around after it
does a fetch, as that would require twice as much disk space as a
regular Bazaar or Git repository. Instead, it creates the Git objects
again from scratch based on the Bazaar data it has, when it needs the
Git objects. The Git objects are mostly used as delta base during a
smart server fetch operation.

The way in which bzr-git regenerates these Git objects from the Bazaar
repository is deterministic and there is currently no way to remember in
what way a particular git object was originally misformatted if it was.
The error this MP comments out occurs when bzr-git fails to recreate a
broken Git object. Simply ignoring the error means that you'll probably
end up with corrupt data in the repository.

The proper fix is to store in the bzr data somehow in what way the
original git object was broken so that when bzr-git tries to reconstruct
the original git object it knows how.

Cheers,

Jelmer

Unmerged revisions

1213. By xaav <email address hidden>

Fixed SHA1 issue

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: