[Commented On] D8552: fix: use context to fetch mergestate instead of loading it directly
marmoute (Pierre-Yves David)
phabricator at mercurial-scm.org
Wed Jun 10 08:02:25 UTC 2020
marmoute added a comment.
In D8552#128826 <https://phab.mercurial-scm.org/D8552#128826>, @martinvonz wrote:
> In D8552#128780 <https://phab.mercurial-scm.org/D8552#128780>, @durin42 wrote:
>
>> In D8552#128779 <https://phab.mercurial-scm.org/D8552#128779>, @durin42 wrote:
>>
>>> In D8552#128767 <https://phab.mercurial-scm.org/D8552#128767>, @martinvonz wrote:
>>>
>>>> In D8552#128629 <https://phab.mercurial-scm.org/D8552#128629>, @marmoute wrote:
>>>>
>>>>> In D8552#128621 <https://phab.mercurial-scm.org/D8552#128621>, @martinvonz wrote:
>>>>>
>>>>>> In D8552#128605 <https://phab.mercurial-scm.org/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
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8552/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D8552
To: durin42, #hg-reviewers, marmoute
Cc: marmoute, martinvonz, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20200610/e9b47e6b/attachment-0002.html>
More information about the Mercurial-patches
mailing list