On Thu, Jul 28, 2011 at 3:56 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Dmitry Ivankov wrote:
>
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1514,6 +1514,9 @@ static int tree_content_set(
>> if (e->tree)
>> release_tree_content_recursive(e->tree);
>> e->tree = subtree;
>> + if (S_ISDIR(mode)) {
>> + hashclr(e->versions[0].sha1);
>> + }
>> hashclr(root->versions[1].sha1);
>> return 1;
>> }
>
> Based on your later explanations, it looks like this hit the nail on
> the head. The idea is that normally e->tree includes entries for both
> e->versions[0] and e->versions[1] and that in the "M 040000 ..." case,
> this codepath at least currently doesn't have enough information to
> decide on a good delta base.
>
> Just doing a "hashclr(e->versions[0].sha1)" will change the preimage
> tree object for the parent directory, causing invalid deltas _there_.
> So that's not quite right.
>
> Possible fixes:
>
> a. invalidate preimage tree names all the way up the hierarchy.
> This is what I misread the code as doing at first. It would
> be a cop-out; I think we can do better.
>
> b. merge the preimage and postimage trees in e->tree, and use
> the existing preimage tree as delta basis. This would produce
> an unnecessarily large delta, but it should work.
That's what I was trying to do in [7/7] fast-import: fix data
corruption in load_tree.
But [7/7] covers only subtree == NULL case.
Delta size shouldn't be too large for the top level tree, if of course
preimage and postimage are similar. Another bad thing is that changes
go deep down the tree (while in a. they go up the tree).
> c. add a bit to "struct tree_content" (or abuse delta_depth) to
> say "my parent entry's versions[0].sha1 has nothing to do with
> me; please do not write me out as a delta against it"
>
> d. invalidate preimage tree object names up the hierarchy, and allow
> tree_content_set callers to pass a "const struct tree_entry_ms *"
> argument indicating a preimage tree object name for the new
> subtree.
not much better than a.
> e. instead of replacing a tree entry, delete it and add it again as a
> new tree entry. Make sure internal APIs can deal with multiple
> tree entries with the same name.
There is a risk that trees will grow twice in ram :)
> I find (c) tempting.
The best thing about it is that it's local. And it looks like it
disables deltas for the minimum number of objects among a-d, and e.
isn't too much better.
> Like this, vaguely. What do you think?
Tested it on my dump - no fsck complaints.
[skipped part of the patch]
> @@ -1470,6 +1477,7 @@ static void tree_content_replace(
> {
> if (!S_ISDIR(mode))
> die("Root cannot be a non-directory");
> + hashclr(root->versions[0].sha1);
> hashcpy(root->versions[1].sha1, sha1);
> if (root->tree)
> release_tree_content_recursive(root->tree);
Should it be root->versions[0].mode |= NO_DELTA instead?
I see that it's only called for the tree root, but still. This way
versions[0].sha1
will be changed only in store tree to discard the preimage and in
load_tree or other
entry creation place (in the end of tree_content_set).
Will give it more testing, thanks for a small patch for this problem :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html