On 11/22/16 8:19 PM, Ryan McElroy wrote:
On 11/10/16 5:11 PM, Bryan O'Sullivan wrote:
On Thu, Nov 10, 2016 at 2:29 AM, Durham Goode <dur...@fb.com
<mailto:dur...@fb.com>> wrote:
We worked out a solution to Bryan’s main concern, but there
hasn’t been any discussion of my main proposal. Internally we
worked around it by forcing bookmarks and the changelog to load
in the right order very early during repo setup, but that’s a
hack and doesn’t address the greater problem.
For the Real Problem (TM), I'd rather try the simpler solution first,
of reading the bookmarks file in the changelog constructor, then
splitting it into lines in the bmstore constructor. I'd hope that
could be both lightweight enough that it could avoid a performance
hit, and unlikely enough to introduce bugs later on that we wouldn't
have to worry about a more heavyweight "make bugs impossible"
solution. If I turn out to be wrong and we need something more formal
(which we've gotten away without for years with revlogs, FWIW), we
haven't really lost much.
I think that fixing the read open ordering is not sufficient.
I was just chatting with Jun Wu (quark) about this, and after that
discussion, it seems that we also have to ensure the write order is
correct. Specifically, bookmarks must be written only after the
changelog is written, otherwise, regardless of the order that we open
files for read, we can have a bookmarks file that points to commits
that no reader can see yet.
Based on a very quick look at the transaction code, it looks like
filegenerators in transactions are stored in a dict, so we have no
guarantees about what order they will be called in, so it's very
likely that we often call the write of the bookmarks file before the
write of the changelog, making it so that no read-side fix alone can
be sufficient to solve this problem
Well, I looked a little more deeply and actually filegenerators have an
order parameter (sorry I missed that the first time).
However, the order parameter is unset by the bmstore when it calls
addfilegenerator.
However (and now I'm remembering more of this), I see that we have a set
called "postfinalizegenerators" which are forced to be after the
finalize steps which is where the changelog is written (introduced in
a5009789960c by durham). So it seems we're already okay here, assuming
all that code it working as intended.
Sorry for forgetting about this.
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
