[MERGE/RFC] Reconcile now rewrites inventory sha1sums

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
| Aaron Bentley wrote:
|
|> This patch updates reconcile to rewrite inventory sha1 sums, which
|> Robert has identified as a prerequisite to making rich-root-pack our
|> default format.
|
| bb:tweak
|
|> +    def _fix_inventory_sha1s(self):
|> +        """Copy revision texts and update their inventory sha1s."""
|> +        def replace_revision(revision_id, parent_ids, lines):
|> +            store = self.repo._revision_store.text_store
|> +            for name in store._id_to_names(revision_id, None):
|> +                try:
|> +                    store._transport.delete(name)
|> +                except errors.NoSuchFile:
|> +                    pass
|> +            revision_file = StringIO(''.join(lines))
|> +            store.add(revision_file, revision_id)
|> +        _rewrite_inventory_sha1s(self.repo, replace_revision)
|
| I'd appreciate it if someone else more familiar with pre-knit formats
| reviewed this block of code. John perhaps?

I'm missing some context, but assuming this is the flat-text store of revision
texts, this seems fine. It is a little "invasive" in that it peeks behind the
scenes a lot. But it is an old format that we haven't used for quite a while
(weaves, I believe) so I'm okay with a bit of cruft here.

I assume _rewrite_inventory_sha1s is a helper to figure out what needs to be
rewritten, and then calls the parameter for each entry that is actually new. For
Knits, you can just add a new entry which will end up superseding the existing
ones. Here, the path on disk is important, so you have to nuke it before adding one.

I'm a little concerned about race-conditions, etc. But if someone does ^C while
this is running, they *will* lose data. A better solution would be to write the
new data to a temporary location, and then "rename()" it into place over the
existing data. (fancy_rename on Windows, but rename() nonetheless.)

As this is a *rarely* used format, I don't know if I can muster enough will to
care, but maybe at least an XXX or TODO here to indicate that we know there is a
data loss race, but we are willing to accept it for a format that has not been
recommended (and we've been giving upgrade warnings) for practically a year.

John
=:->


|
|> +        base_index = getattr(self.new_pack, index_name + '_index')
|> +        if file_id is not None:
|> +            index = GraphIndexPrefixAdapter(
|> +                base_index,
|> +                (file_id, ), 1,
|> +                add_nodes_callback=base_index.add_nodes)
|> +        else:
|> +            index = base_index
|> +        knit_index = KnitGraphIndex(index,
|> +            add_callback=index.add_nodes,
|> +            deltas=deltas, parents=parents)
|> +        return knit.KnitVersionedFile('reconcile-revisions',
|
| I think you want ...
|
|   'reconcile %ss' % (index_name,)
|
| there instead of 'reconcile-revisions'.
|
|> +            self._pack_collection.transport,
|> +            index=knit_index,
|> +            delta=deltas,
|> +            access_method=_PackAccess(
|> +                {base_index:self.new_pack.access_tuple()},
|> +                (self.new_pack._writer, index)),
|
| Did you mean ...
|
|   {index:...
|
| instead of {base_index:... there?
|
| Otherwise, looks good to me.
|
| Ian C.
|
|

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgxmCoACgkQJdeBCYSNAAMbxACeI/ZaMsr7rF4nmRRLu0RlZt37
dE0AoMenHry4gT6P0WYYc1ZljpwJHPU9
=Nefg
-----END PGP SIGNATURE-----