[PATCH] treemanifest: don't use a manifest in the cache if it's dirty

Tony Tung tonytung at instagram.com
Mon Feb 22 17:42:30 EST 2016


> On Feb 20, 2016, at 9:52 PM, Martin von Zweigbergk <martinvonz at google.com> wrote:
> 
> On Thu, Feb 18, 2016 at 3:46 PM, Tony Tung <tonytung at fb.com> wrote:
>> # HG changeset patch
>> # User Tony Tung <tonytung at merly.org>
>> # Date 1455839061 28800
>> #      Thu Feb 18 15:44:21 2016 -0800
>> # Node ID a7cd652b0f4398e939ebc10c6ceff86c4c119a72
>> # Parent  c4ee1010595a7cae4aad0c7dbcd152719f780e6a
>> treemanifest: don't use a manifest in the cache if it's dirty.
> 
> Drop the trailing period and add " (issue5076)" instead (see
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_ContributingChanges-23Submission-5Fchecklist&d=CwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nX46-h6uPFfl2aGGFhBQbg&m=kj1Em7qqtj4fSbjVd3KC92KDCH1vKBgo45lrg82b8zI&s=XGXEMdVOTsQ6ObOYMeSjsuEn-gDSsEHfkIeppaPaL7E&e= ).

Thanks, will do.

>> When doing hg convert, we may encounter subrepo merges.  In that case, the
>> working directory subrepo will be dirty (i.e., differ from the repository
>> state), which will cause the manifest to be written to.  Subsequent reads
>> of this manifest entry will note that it's dirty, and calls to node() will
>> fail.
> 
> Nice find!
> 
> However, it wasn't clear to me why this would be specific to
> treemanifests, so I added dirty checking to the flat manifest code as
> well (see patch on https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.debian.net_402058_&d=CwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nX46-h6uPFfl2aGGFhBQbg&m=kj1Em7qqtj4fSbjVd3KC92KDCH1vKBgo45lrg82b8zI&s=GQhm80laTpyUYP6IY5JZG3FvWnv7io_Q7f1r29o4iKE&e= ) and ran the test
> in the issue tracker (probably the same as in your patch) *without*
> treemanifests. Sure enough, it fails there too. So it seems like the
> only reason this is failing only with treemanifests is that only
> treemanifests check that they are clean in some cases. Please test it
> for yourself to make sure I didn't just see what I wanted to see when
> I tested it.
> 
> As I said on the issue tracker, I've spent a lot of time trying to
> understand how manifestmerge() interacts with submerge(). Perhaps the
> reading of dirty manifests is the reason I never understood what was
> happening.
> 
> Anyway, what do we do now? I think the test case should be moved out
> of test-treemanifest.t at least. Is the fix to add dirty-checking in
> manifestdict as well and not bypass the cache there too? Or perhaps
> the bug is in the subrepo code?

I think the fundamental issue (bearing in mind that I’m pretty inexperienced in mercurial internals :) ) is that the convert code is doing a manifestmerge into a working context.  This is not actually the case, as it does not update the working directory (which is why the subrepo is considered dirty).

I had an alternate fix that went deep into merge.py, but was pretty ugly.  It would behave differently if called with a special argument indicating that it was not a working context.  However, it may be more correct.

Happy to move the test case out, and to add dirty checking for alternate manifest implementations.

Love to hear your thoughts.

Thanks,
Tony


More information about the Mercurial-devel mailing list