[PATCH 5 of 5] cleanup: replace False identity testing with an explicit token object

[Augie Fackler]

> On May 11, 2016, at 8:38 AM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> 
>>> -        if tr is False: # not explicitly specified
>>> +        if tr is _token: # not explicitly specified
>> I just saw some code (seriously, I tripped over it about at the same time as I saw this patch) in localrepo.py that explicitly calls dirstate.write(None). Is that a bug? It looks like a bug. I suspect this might want to just look for a truthy value to sniff out clients that are doing the wrong thing?
>> 
>> (if not, we should maybe have some documentation that states why internal users are allowed to skip the transaction here)
> 
> Dirstate change -must- collaborate with the transaction if there is one. However, if there is no currently open transaction, it wont.

So, I think I’ve answered my own question (this response from you did not help in the slightest). Here’s what I think I’m seeing:

localrepo.transaction() needs a way to flush any pending writes in the dirstate before a transaction starts, so that the only things which get rolled back as part of the transaction are things that happen during the transaction. As such, localrepo needs a way to flush the dirstate to disk without a transaction in play (in case it’s dirty), and the way to do that is by calling dirstate.write(None). Is that correct?

If that’s correct, then we should probably just make an _ensureclean() method that the transaction layer can call that does the write without a transaction, and document why it exists. As it is, anyone that wants to bypass a transaction can do so by passing any falsey value into dirstate.write().

(I’ll probably still take this patch, but want to continue this discussion.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160511/b6e6938c/attachment.sig>