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

[Martin von Zweigbergk]

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://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist).

> 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 http://paste.debian.net/402058/) 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?