This is an archive of the discontinued Mercurial Phabricator instance.

localrepo: get mergestate via context
AbandonedPublic

Authored by durin42 on May 18 2020, 6:08 PM.

Details

Reviewers
marmoute
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.May 18 2020, 6:08 PM

It looks like the latter two hunks in this file will need to be able to read the merge state from an overlayworkingctx. Maybe that's coming in a later patch, but I think it would be easier to follow if this code was changed only then (very much related to my comment on an earlier patch).

Yeah, I'm happy to reorder patches, this is still a bit "work-in-progress unfortunately.

durin42 updated this revision to Diff 21509.May 28 2020, 4:26 PM

@martinvonz do you mean that these patch break normal behavior if taken without other patch ?

@martinvonz do you mean that these patch break normal behavior if taken without other patch ?

No, but I think those two places should be able to accept an overlayworkingctx, so it might be better to leave them unchanged for now and to switch them directly to use wctx.mergestate() once we have a wctx passed in here.

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 ?

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

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 ?

Should we have a brief email thread or IRC discussion about this? We seem to be conflating workingctx and overlayworkingctx here, and I'm not sure we're all on the same page.

durin42 updated this revision to Diff 21609.Jun 11 2020, 12:07 PM

I can probably do a VC tomorrow (US morning time) if that help

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.

marmoute requested changes to this revision.Jul 31 2020, 12:02 PM

According to IRC discussion, this series is not ready for review, so moving it out of yadda.

This revision now requires changes to proceed.Jul 31 2020, 12:02 PM
durin42 updated this revision to Diff 23026.Oct 2 2020, 2:45 PM
durin42 planned changes to this revision.Oct 2 2020, 2:47 PM
durin42 abandoned this revision.Jan 19 2021, 1:50 PM