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

login
register
mail settings
Submitter Pierre-Yves David
Date May 11, 2016, 7:38 a.m.
Message ID <b62cf5924c3b5684cb06.1462952333@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/15009/
State Accepted
Commit 0e9ed09f5fe9c390e60a98426f64bc3a231f7aa0
Headers show

Comments

Pierre-Yves David - May 11, 2016, 7:38 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1462469571 -7200
#      Thu May 05 19:32:51 2016 +0200
# Node ID b62cf5924c3b5684cb0693fe4186dc607dcc5b50
# Parent  685102426f222a6746960a905e2392cc2c4607da
# EXP-Topic cleanup
cleanup: replace False identity testing with an explicit token object

The recommended way to check default value (when None is not as option) is a
token object. Identity testing to integer is less explicit and not guaranteed to
work in all implementations.
timeless - May 11, 2016, 12:24 p.m.
The series itself is ok.

I'm not a huge fan of "x uses y instead of z", it's moderately painful to read.
But that shouldn't stop this from going in.

On Wed, May 11, 2016 at 3:38 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462469571 -7200
> #      Thu May 05 19:32:51 2016 +0200
> # Node ID b62cf5924c3b5684cb0693fe4186dc607dcc5b50
> # Parent  685102426f222a6746960a905e2392cc2c4607da
> # EXP-Topic cleanup
> cleanup: replace False identity testing with an explicit token object
>
> The recommended way to check default value (when None is not as option) is a
> token object. Identity testing to integer is less explicit and not guaranteed to
> work in all implementations.
>
> diff -r 685102426f22 -r b62cf5924c3b mercurial/dirstate.py
> --- a/mercurial/dirstate.py     Wed May 11 09:31:47 2016 +0200
> +++ b/mercurial/dirstate.py     Thu May 05 19:32:51 2016 +0200
> @@ -74,6 +74,8 @@ def _trypending(root, vfs, filename):
>                  raise
>      return (vfs(filename), False)
>
> +_token = object()
> +
>  class dirstate(object):
>
>      def __init__(self, opener, ui, root, validate):
> @@ -688,12 +690,12 @@ class dirstate(object):
>          self._pl = (parent, nullid)
>          self._dirty = True
>
> -    def write(self, tr=False):
> +    def write(self, tr=_token):
>          if not self._dirty:
>              return
>
>          filename = self._filename
> -        if tr is False: # not explicitly specified
> +        if tr is _token: # not explicitly specified
>              self._ui.deprecwarn('use dirstate.write with '
>                                 'repo.currenttransaction()',
>                                 '3.9')
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - May 11, 2016, 12:34 p.m.
> On May 11, 2016, at 3:38 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462469571 -7200
> #      Thu May 05 19:32:51 2016 +0200
> # Node ID b62cf5924c3b5684cb0693fe4186dc607dcc5b50
> # Parent  685102426f222a6746960a905e2392cc2c4607da
> # EXP-Topic cleanup
> cleanup: replace False identity testing with an explicit token object
> 
> The recommended way to check default value (when None is not as option) is a
> token object. Identity testing to integer is less explicit and not guaranteed to
> work in all implementations.
> 
> diff -r 685102426f22 -r b62cf5924c3b mercurial/dirstate.py
> --- a/mercurial/dirstate.py	Wed May 11 09:31:47 2016 +0200
> +++ b/mercurial/dirstate.py	Thu May 05 19:32:51 2016 +0200
> @@ -74,6 +74,8 @@ def _trypending(root, vfs, filename):
>                 raise
>     return (vfs(filename), False)
> 
> +_token = object()
> +
> class dirstate(object):
> 
>     def __init__(self, opener, ui, root, validate):
> @@ -688,12 +690,12 @@ class dirstate(object):
>         self._pl = (parent, nullid)
>         self._dirty = True
> 
> -    def write(self, tr=False):
> +    def write(self, tr=_token):
>         if not self._dirty:
>             return
> 
>         filename = self._filename
> -        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)

>             self._ui.deprecwarn('use dirstate.write with '
>                                'repo.currenttransaction()',
>                                '3.9')
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 11, 2016, 12:38 p.m.
On 05/11/2016 02:34 PM, Augie Fackler wrote:
>> On May 11, 2016, at 3:38 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1462469571 -7200
>> #      Thu May 05 19:32:51 2016 +0200
>> # Node ID b62cf5924c3b5684cb0693fe4186dc607dcc5b50
>> # Parent  685102426f222a6746960a905e2392cc2c4607da
>> # EXP-Topic cleanup
>> cleanup: replace False identity testing with an explicit token object
>>
>> The recommended way to check default value (when None is not as option) is a
>> token object. Identity testing to integer is less explicit and not guaranteed to
>> work in all implementations.
>>
>> diff -r 685102426f22 -r b62cf5924c3b mercurial/dirstate.py
>> --- a/mercurial/dirstate.py	Wed May 11 09:31:47 2016 +0200
>> +++ b/mercurial/dirstate.py	Thu May 05 19:32:51 2016 +0200
>> @@ -74,6 +74,8 @@ def _trypending(root, vfs, filename):
>>                  raise
>>      return (vfs(filename), False)
>>
>> +_token = object()
>> +
>> class dirstate(object):
>>
>>      def __init__(self, opener, ui, root, validate):
>> @@ -688,12 +690,12 @@ class dirstate(object):
>>          self._pl = (parent, nullid)
>>          self._dirty = True
>>
>> -    def write(self, tr=False):
>> +    def write(self, tr=_token):
>>          if not self._dirty:
>>              return
>>
>>          filename = self._filename
>> -        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.
Augie Fackler - May 11, 2016, 12:46 p.m.
> On May 11, 2016, at 8:38 AM, Pierre-Yves David <pierre-yves.david@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.)
Pierre-Yves David - May 11, 2016, 1:34 p.m.
On 05/11/2016 02:46 PM, Augie Fackler wrote:
>> On May 11, 2016, at 8:38 AM, Pierre-Yves David <pierre-yves.david@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)
Augie Fackler - May 11, 2016, 2:04 p.m.
> On May 11, 2016, at 09:34, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
>> (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)

It's related in my mind, because I was having a lot of trouble understanding why we're using a sentinel object instead of just requiring callers to always pass a transaction. Now that I understand, this patch is a strict improvement (so I'll take it), but there's still some work to be done to make this a clean API (which I'll probably do sometime this week.)

Thanks for bearing with me.
Pierre-Yves David - May 11, 2016, 2:17 p.m.
On 05/11/2016 04:04 PM, Augie Fackler wrote:
>> On May 11, 2016, at 09:34, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>> (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 repo._phasecache.invalidate(not sure why this discussion would delay them.
>> (discussion in itself it useful, but unrelated)
> It's related in my mind, because I was having a lot of trouble understanding why we're using a sentinel object instead of just requiring callers to always pass a transaction. Now that I understand, this patch is a strict improvement (so I'll take it), but there's still some work to be done to make this a clean API (which I'll probably do sometime this week.)

The sentinel object is here for deprecation purpose, the argument will 
become mandatory after 3.9.

Your last statement is a bit confusing to me, especially I'm not sure 
what you find "unclear" in the current API. Here is a wide explanation 
of the situation as an attempt to clarify.

None is valid value and will remain a valid value because dirstate 
change do not requires a transaction. They just have to collaborate with 
one if present. For example `hg update` or `hg add` do not use a 
transaction. As you pointed out in your previous email, there is also 
case around the transaction code that explicitly call the write with 
None because it is the transaction code dirstate is collaborating with, 
so it is supposed to know what it is doing.
Augie Fackler - May 11, 2016, 2:26 p.m.
> On May 11, 2016, at 10:17, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> None is valid value and will remain a valid value because dirstate change do not requires a transaction. They just have to collaborate with one if present. For example `hg update` or `hg add` do not use a transaction. As you pointed out in your previous email, there is also case around the transaction code that explicitly call the write with None because it is the transaction code dirstate is collaborating with, so it is supposed to know what it is doing.

Aha! This is the missing piece of understanding for me: it's valid for update() (et al) codepaths to adjust dirstate without a transaction - it's only required to pass a transaction if one is in play.[1] In summary, today we have three cases:

dirstate.write(), no active transaction -> fine
dirstate.write(), a transaction is active -> buggy
dirstate.write(tr) -> always fine

This series is merely about disabling that middle case more formally.

I think, given that, the direction I'd actually rather go (but won't block this patch on), is make tr an optional argument to dirstate.write()[0], and if devel-warnings are enabled, check to see if there's a current transaction and log if so. Does that sound like a workable future approach?

(My concern here is that None as a magic value for "I know what I'm doing" is going to be prone to misuse and/or abuse, and it makes the API a little weird.)

0: It'll also be legal to pass a falsey value for tr, which is another way of explicitly saying "don't use a transaction", but that's still Wrong if there's a transaction in flight, right?
Pierre-Yves David - May 11, 2016, 2:43 p.m.
On 05/11/2016 04:26 PM, Augie Fackler wrote:
>> On May 11, 2016, at 10:17, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> None is valid value and will remain a valid value because dirstate change do not requires a transaction. They just have to collaborate with one if present. For example `hg update` or `hg add` do not use a transaction. As you pointed out in your previous email, there is also case around the transaction code that explicitly call the write with None because it is the transaction code dirstate is collaborating with, so it is supposed to know what it is doing.
> Aha! This is the missing piece of understanding for me: it's valid for update() (et al) codepaths to adjust dirstate without a transaction - it's only required to pass a transaction if one is in play.[1] In summary, today we have three cases:
>
> dirstate.write(), no active transaction -> fine
> dirstate.write(), a transaction is active -> buggy
> dirstate.write(tr) -> always fine
>
> This series is merely about disabling that middle case more formally.

The first case is also problematic. Unless you are in a very narrow set 
of code (pretty much, the strip code that explicitly forbid transaction 
to be in place). There is no way for any code to know if it will be use 
within a transaction at some point. So all code using dirstate.write() 
should call it using dirstate.write(repo.currenttransaction()) the 
currenttransaction bit will take are of returning None or the active 
transaction.

This construct of "dirstate.write(repo.currenttransaction())" is the 
result of the long discussion in the middle of last year with at least 
Foozy and I. I do not think it is worth revisiting, but if you really 
feel like doing so, please go dig that these discussion out first.

> I think, given that, the direction I'd actually rather go (but won't block this patch on), is make tr an optional argument to dirstate.write()[0], and if devel-warnings are enabled, check to see if there's a current transaction and log if so. Does that sound like a workable future approach?

as pointed a bit earlier in this message, because you never now if code 
will eventually be wrapped within a transaction, all normal code should 
be called with "dirstate.write(repo.currenttransaction())" all the time. 
So making this argument mandatory is the way to go to me. We should 
improve the documentation to make that clear.

> (My concern here is that None as a magic value for "I know what I'm doing" is going to be prone to misuse and/or abuse, and it makes the API a little weird.)

"None" is not just a magic value for dirstate.write it is the official 
return of repo.currenttransaction() when none exist, various other part 
of the code rely on this and I see his as a valid python idiom.

The case that explicitly pass None are narrow and we should document 
them properly. This is python, if programmer decide to ignore the 
warning sign they are responsible adult. We should make sure they see 
the sign.

> 0: It'll also be legal to pass a falsey value for tr, which is another way of explicitly saying "don't use a transaction", but that's still Wrong if there's a transaction in flight, right?

Nope, the current code make it legal but we should enforce a None 
checking here. I don't see a reason for a legal falsey value here.
Katsunori FUJIWARA - May 11, 2016, 3:42 p.m.
At Wed, 11 May 2016 15:34:30 +0200,
Pierre-Yves David wrote:
> 
> On 05/11/2016 02:46 PM, Augie Fackler wrote:
> >> On May 11, 2016, at 8:38 AM, Pierre-Yves David <pierre-yves.david@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.

(I'm just writing reply to Augie :-))

Yes, that is correct. But strictly speaking, using 'None' as 'tr' is
not for "we want to bypass transaction" but for "we can ensure there
is no transaction in flight" in cases below:

  1. before writing journal files out
     https://selenic.com/repo/hg/file/df838803c1d4/mercurial/localrepo.py#l1016
  2. after successful closing transaction
     https://selenic.com/repo/hg/file/df838803c1d4/mercurial/localrepo.py#l1036
  3. at releasing wlock
     https://selenic.com/repo/hg/file/df838803c1d4/mercurial/localrepo.py#l1360

(In fact, in corner case of #3, we can't assume that. But it isn't
related to this series. I'll post it as new topic)

BTW, largefilesdirstate class derived from dirstate also invokes
dirstate.write(None). This should be replaced, too, if we introduce
_ensureclean().


> > 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
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - May 11, 2016, 3:44 p.m.
At Wed, 11 May 2016 10:26:37 -0400,
Augie Fackler wrote:
> 
> > On May 11, 2016, at 10:17, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > 

> > None is valid value and will remain a valid value because dirstate
> > change do not requires a transaction. They just have to
> > collaborate with one if present. For example `hg update` or `hg
> > add` do not use a transaction. As you pointed out in your previous
> > email, there is also case around the transaction code that
> > explicitly call the write with None because it is the transaction
> > code dirstate is collaborating with, so it is supposed to know
> > what it is doing.
>
> Aha! This is the missing piece of understanding for me: it's valid
> for update() (et al) codepaths to adjust dirstate without a
> transaction - it's only required to pass a transaction if one is in
> play.[1] In summary, today we have three cases:
> 
> dirstate.write(), no active transaction -> fine
> dirstate.write(), a transaction is active -> buggy
> dirstate.write(tr) -> always fine
> 
> This series is merely about disabling that middle case more formally.
> 
> I think, given that, the direction I'd actually rather go (but won't
> block this patch on), is make tr an optional argument to
> dirstate.write()[0], and if devel-warnings are enabled, check to see
> if there's a current transaction and log if so. Does that sound like
> a workable future approach?

Unfortunately, there is no easy way on dirstate.write() side to
examine whether transaction is running, because dirstate doesn't have
reference to repo object (even though thread local variable or so
might help that idea).


> (My concern here is that None as a magic value for "I know what I'm
> doing" is going to be prone to misuse and/or abuse, and it makes the
> API a little weird.)
> 
> 0: It'll also be legal to pass a falsey value for tr, which is
> another way of explicitly saying "don't use a transaction", but
> that's still Wrong if there's a transaction in flight, right?

Right. We can't easily assume there is no transaction in flight. It is
reason why we should recommend to pass the result of
repo.currenttransaction() to dirstate.write() (wlock, slock and
transaction scope is easily nested :-<)


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - May 11, 2016, 3:51 p.m.
At Wed, 11 May 2016 16:43:39 +0200,
Pierre-Yves David wrote:
> 
> On 05/11/2016 04:26 PM, Augie Fackler wrote:
> >> On May 11, 2016, at 10:17, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> >>
> >> None is valid value and will remain a valid value because dirstate change do not requires a transaction. They just have to collaborate with one if present. For example `hg update` or `hg add` do not use a transaction. As you pointed out in your previous email, there is also case around the transaction code that explicitly call the write with None because it is the transaction code dirstate is collaborating with, so it is supposed to know what it is doing.
> > Aha! This is the missing piece of understanding for me: it's valid for update() (et al) codepaths to adjust dirstate without a transaction - it's only required to pass a transaction if one is in play.[1] In summary, today we have three cases:
> >
> > dirstate.write(), no active transaction -> fine
> > dirstate.write(), a transaction is active -> buggy
> > dirstate.write(tr) -> always fine
> >
> > This series is merely about disabling that middle case more formally.
> 
> The first case is also problematic. Unless you are in a very narrow set 
> of code (pretty much, the strip code that explicitly forbid transaction 
> to be in place). There is no way for any code to know if it will be use 
> within a transaction at some point. So all code using dirstate.write() 
> should call it using dirstate.write(repo.currenttransaction()) the 
> currenttransaction bit will take are of returning None or the active 
> transaction.
> 
> This construct of "dirstate.write(repo.currenttransaction())" is the 
> result of the long discussion in the middle of last year with at least 
> Foozy and I. I do not think it is worth revisiting, but if you really 
> feel like doing so, please go dig that these discussion out first.

For future reference:

    https://www.mercurial-scm.org/wiki/DirstateTransactionPlan

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff -r 685102426f22 -r b62cf5924c3b mercurial/dirstate.py
--- a/mercurial/dirstate.py	Wed May 11 09:31:47 2016 +0200
+++ b/mercurial/dirstate.py	Thu May 05 19:32:51 2016 +0200
@@ -74,6 +74,8 @@  def _trypending(root, vfs, filename):
                 raise
     return (vfs(filename), False)
 
+_token = object()
+
 class dirstate(object):
 
     def __init__(self, opener, ui, root, validate):
@@ -688,12 +690,12 @@  class dirstate(object):
         self._pl = (parent, nullid)
         self._dirty = True
 
-    def write(self, tr=False):
+    def write(self, tr=_token):
         if not self._dirty:
             return
 
         filename = self._filename
-        if tr is False: # not explicitly specified
+        if tr is _token: # not explicitly specified
             self._ui.deprecwarn('use dirstate.write with '
                                'repo.currenttransaction()',
                                '3.9')