Seems to need rebased?

Something in this stack (that I didn't push) breaks the phabricator extension. See phabricator.py line 170:

@eh.wrapfunction(localrepo, "loadhgrc")
def _loadhgrc(orig, ui, wdirvfs, hgvfs, requirements):

So you'll need to do....something there. Maybe look for other wrappers in extensions too while you're there...

I queued a mailing list version of this series. Should I drop it?

Yeah, if you’re seeing these changes I’m pretty sure you have a too-old black...

In D8691#129807, @graemenail wrote:

Thanks for the prompt review.

In D8691#129796, @durin42 wrote:

(If anything, ~/Library/Preferences/org.mercurial-scm.hgrc might make sense, but that seems silly to me.)

I agree that ~/Library/Preferences would be the default XDG analogy in macOS. However, the patch was more motivated by relocating the configuration file.
Am I to understand that setting HGRCPATH is the preferred approach for relocating hgrc on macOS?

Yes: XDG isn't a thing on macOS, and we won't respect it there. Perhaps the help text should be improved instead.

LGTM, but needs rebased.

My feelings on this are "obviously yes" but I thought that so quickly I lack confidence that I'm not missing something. :)

I'm a fan of this, but for default, not stable. Is there a specific reason you need a new feature on the stable branch?

This is still valid.

Doesn't apply?

Okay, now localrepo correctly uses the same context across whole operations. This almost certainly avoids some bugs, given that committing a memctx was (surprise!) reading the mergestate from disk.

In D8563#128829, @marmoute wrote:

In D8563#128768, @martinvonz wrote:

In D8563#128630, @marmoute wrote:

Is your concern about the creation of new "independant" wctc (by each call of repo[None]) instead of reusing the same wctx for the full operation ?

Yes. I guess I'd rather have the pattern wctx.mergestate() indicate that it works with either a workingctx or a overlayworkingctx, while direct access to mergestate.read() indicates that we care about the actual working copy.

Is there any reason to distinct the two ? From afar, it would be better for the mergestate module to become an implementation details and every access to happen throught the context for consistency.
Do you have concrêt example where distincting the two seems better ?

In D8552#128827, @marmoute wrote:

In D8552#128826, @martinvonz wrote:

In D8552#128780, @durin42 wrote:

In D8552#128779, @durin42 wrote:

In D8552#128767, @martinvonz wrote:

In D8552#128629, @marmoute wrote:

In D8552#128621, @martinvonz wrote:

In D8552#128605, @marmoute wrote:

@martinvonz can you clarify why mergestate on an overlayworkingctx would never make sense ?

That makes sense. What I said does not make sense is to check for unresolved merge conflicts (before starting an operation).

Checking for unresolved merge conflict on an overlayworkingctx does not make sense because the overlayctx is new and therefor will always be conflict free ?

I view overlayworkingctx as something that exists for creating commits without touching the working copy and without caring about the working copy state. In the code here, we explicitly care about the working copy state (because we're going to modify it).

Ah, interesting. That angle hadn't occurred to me. I don't think I'm wholly convinced, but maybe we need an explicit mechanism for "get the wdir" as contrasted with "get a context for the wdir".
I think I'll at least remove this patch from the stack for now...

Question @martinvonz : do you think overlayworkingctx should also not read the local mergestate, but instead should use an in-memory one? I'm not sure how you're envisioning the abstraction boundary there...

Oh, definitely. That's also what you did in later patches, right? It would seem like a bug to me if it read the local mergestate.

+1

Sadly I have no idea on that.

Best guess (given I've never hit an AV problem) is that the AV engine would lose its lunch on the EICAR file and alert the user. I figure if the AV engine isn't picking up on it after 2 seconds then it's probably also not a performance issue for us.

I don't have concrete numbers, but it avoids some function calls and uses only internal details instead of part of the public API.

In D8566#128623, @marmoute wrote:

how well, I see why you want an explicite method to avoid unrolling the iterator. Could we return an object with sensible bool and len methods?

In D8515#128625, @marmoute wrote:

Is this to represent conflict for in-memory merge ? Or to help with the "merge diff" display ? Because I am not sure how it help (or is used) in the first case, but I can see it tremendously useful for mergediff (since it show how conflict got resolved)

In D8552#128779, @durin42 wrote:

In D8552#128767, @martinvonz wrote:

In D8552#128629, @marmoute wrote:

In D8552#128621, @martinvonz wrote:

In D8552#128605, @marmoute wrote:

@martinvonz can you clarify why mergestate on an overlayworkingctx would never make sense ?

That makes sense. What I said does not make sense is to check for unresolved merge conflicts (before starting an operation).

Checking for unresolved merge conflict on an overlayworkingctx does not make sense because the overlayctx is new and therefor will always be conflict free ?

I view overlayworkingctx as something that exists for creating commits without touching the working copy and without caring about the working copy state. In the code here, we explicitly care about the working copy state (because we're going to modify it).

Ah, interesting. That angle hadn't occurred to me. I don't think I'm wholly convinced, but maybe we need an explicit mechanism for "get the wdir" as contrasted with "get a context for the wdir".
I think I'll at least remove this patch from the stack for now...

In D8552#128767, @martinvonz wrote:

In D8552#128629, @marmoute wrote:

In D8552#128621, @martinvonz wrote:

In D8552#128605, @marmoute wrote:

@martinvonz can you clarify why mergestate on an overlayworkingctx would never make sense ?

That makes sense. What I said does not make sense is to check for unresolved merge conflicts (before starting an operation).

Checking for unresolved merge conflict on an overlayworkingctx does not make sense because the overlayctx is new and therefor will always be conflict free ?

I view overlayworkingctx as something that exists for creating commits without touching the working copy and without caring about the working copy state. In the code here, we explicitly care about the working copy state (because we're going to modify it).

This is now much closer! I'm down to a handful of failures, all like this:

ha!