dirstate.write() API

[Pierre-Yves David]

On 05/11/2016 04:26 PM, 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.

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.

-- 
Pierre-Yves