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

[Pierre-Yves David]

On 05/11/2016 02:46 PM, Augie Fackler wrote:
>> 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?

That seems like a plausible explanation. I think foozy is the one who 
did this, I've CCed him for validation.

> 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().

It is not about falsey value, it is about passing None, the official way 
for "write outside of a transaction". I agree with you that we should 
should update the documentation to make it clear, but the "write" method 
seems to be the place were it belong. I'm not sure what would be the 
value of a dedicated "_ensureclean" and why it would have to be private.

> (I’ll probably still take this patch, but want to continue this discussion.)

That patch is not related to these dirstate.write(None) call in 
localrepo I'm not sure why this discussion would delay them.
(discussion in itself it useful, but unrelated)

-- 
Pierre-Yves David