Page MenuHomePhabricator

fix: use context to fetch mergestate instead of loading it directly
Needs ReviewPublic

Authored by durin42 on Mon, May 18, 6:07 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

durin42 created this revision.Mon, May 18, 6:07 PM

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

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.