dirstate.write() API (was: Re: [PATCH 5 of 5] cleanup: replace False identity testing with an explicit token object)

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Wed May 11 11:44:21 EDT 2016


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 at 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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


More information about the Mercurial-devel mailing list