I think I'd prefer to leave code that doesn't use overlayworkingctx unchanged. I don't see any reason to get the merge state from the context if we know that the context is a regular workingctx.

In D8552#127895, @martinvonz wrote:

I think I'd prefer to leave code that doesn't use overlayworkingctx unchanged. I don't see any reason to get the merge state from the context if we know that the context is a regular workingctx.

I'd rather it was globally-consistent that if you need a mergestate, you load it via a context, so that more code will implicitly be right for in-memory operations.

In D8552#127900, @durin42 wrote:

In D8552#127895, @martinvonz wrote:

I think I'd prefer to leave code that doesn't use overlayworkingctx unchanged. I don't see any reason to get the merge state from the context if we know that the context is a regular workingctx.

I'd rather it was globally-consistent that if you need a mergestate, you load it via a context, so that more code will implicitly be right for in-memory operations.

As long as we do repo[None].mergestate() it will not be more or less right than before. We'd at least want to refactor the code so it creates a wctx instance early on and then reuse that as much as possible, so we could later replace the wctx = repo[None] by wctx = overlayworkingctx(...). In cases like this one (checking for unresolved merge conflicts), it will never make sense to do that on an overlayworkingctx. I'm very much in favor of making more things work "in memory" where that's reasonable, but I'd prefer to leave callers unchanged until we actually try that in earnest. I think we have a similar situation with the dirstate and I don't think we've tried to hide all access to the dirstate bethind the context objects.

I lean toward @durin42 here. Having one clear official way to do it in all case would be better. @martinvonz can you clarify why mergestate on an overlayworkingctx would never make sense ?

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

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 ?

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

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

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.

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

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

I had to look at my own patches: overlayworkingctx uses the in-memory mergestate, workingctx uses the on-disk one. repo[None] returns a _workingctx_, not an overlay one, so I'm confused what the issue is here.

@durin42 what's the state of this series? Can the bottom part of it be reviewed or do you plan to revisit the entire series? Perhaps a rebase would be in order, just in case?

This is stalled. I've got some bugs in the logic I can't track down and
this is second on my priority list at work. I do intend to come back to it,
but it'd be fair to mark it all as changes requested.

In D8552#133369, @durin42 wrote:

This is stalled. I've got some bugs in the logic I can't track down and
this is second on my priority list at work. I do intend to come back to it,
but it'd be fair to mark it all as changes requested.

I'm thinking of working on improving support for having in-memory rebase not fall back to on-disk rebase. This series will be useful for that. I'll review it again and will queue at least some of the patches.