In D9789#149346, @indygreg wrote:

I'm -0 on this change. Caches are caches and IMO should only be populated on demand.

In D9516#149033, @mharbison72 wrote:

Big picture, why do we need to be able to tell namespace parts from command parts?

To make sure the user understand they are stepping in an entire different realm with different rules.
In the standard namespace (no prefix) we garantee (or aims at) the lack of footgun and excellent backward compatibility.
The debug namespace should not be somewhere any normal user ever have to go (at least without a responsible staff member)
The admin namespace will have its own rules and its own "be careful, this is not a "normal" situation to be there.
So we want something more distincting than just a simple - that we could have for "command group" or readability like phab-send, phab-read, etc.

So it sounds like more of a convention for these behavior properties than say, commands in the admin group requiring --yes-i-know-this-may-eat-my-data. We'll have to have to figure out how to convey that somehow. I see admin, for example, and think "I'm in charge of making this work for my group, and it sounds like it does what I want, so it's for me and just some extra characters to type". I don't have any suggestions on how to highlight these behaviors ATM.

In D9780#149093, @joerg.sonnenberger wrote:

In D9780#149086, @marmoute wrote:

I like the fact we now take a rev as argument. However two discussion remains:

• could/should we stop passing the changelogrevision as argument ?

The performance critical path is IMO in during changegroup application (unbundle) and that one is already doing most of the work.
There are some good chances that consumers of the API will look at older revisions in at least some use cases, so the simple
last-use-cache won't work as well in that case. As such I think it is both simpler and more predictable to do the work once.

I like the fact we now take a rev as argument. However two discussion remains:

In D9843#149035, @mharbison72 wrote:

Can we get space for extra flags for things not supported by the filesystem? I'm thinking specifically +x and symlink support on Windows.
https://www.mercurial-scm.org/wiki/DirState#Proposed_extensions

Sad but useful.

Should this go on stable instead to make the test fixed on stable too?

@joerg.sonnenberger and I had a discussion on IRC about this API and the result is that Joerg prefers the higher level API for reason I now understand better without necessarly finding them decisive while I still prefer the lower level API for reason that (hopefully) Joerg understand better without finding them more decisive. So we are doing to need a bit more time to thing about that (and probably about the broader picture of the full API of the involved object) with probably more people.

In D9780#148752, @joerg.sonnenberger wrote:

In D9780#148725, @marmoute wrote:

• it pass around quite high level object where I think rev would be more appropriate,

I don't disagree, but that's a more general change IMO. I really dislike D9826 as it introduces new hidden assumptions about how a revlog backend works.

So I am still not fan of this API:

I phabsent this in behalf of Victor to avoid him premature exposure to phabricator crapiness.

(this is otherwise great), and we can probably turn this in a property cache as a follow up.

This looks good, thanks.

Still not convinced with the API, so poking at what else would be possible.
I nothing obvious emerge we should move forward with that series before the freeze, the change final change is quite valuable.

This is clearer. For extra clarity I would probably use something like:

This looks good. The documentation could be a bit better. However this can be dealt with during the freeze. So lets focus on the implementation/behavior side for now.

The requested distinction happen as a follow up in D9824. It seems fine to me.

Am I fine with the required fix being a follow up and such follow up now exist. So I think we are good to go.

This looks "fine" as it fix the previous race window. However do not hesitate to follow up with another patch to reduce the code duplication.

Great, thanks.

Can you add a small in place comment to explain the requirements ?

Same as before, we will have two different option as one case (probably upgrade) migh be expected within an organization but not the other.

We need a distinct option for upgrading and downgrading. The "risk" associated with each operation is quite different and should be dealt with differently.

I gave a shot at a --confirm option. An alternative series to this one can be found in D9818 D9819 and D98120

note: I am fine with D9783 being taken in its current forms and the locking being fixed as a follow up. Overall the series is still an improvement.