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

[FUJIWARA Katsunori]

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

(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 at lares.dti.ne.jp