In D8725#130162, @mjacob wrote:

In D8725#130160, @martinvonz wrote:

Could you include in the commit message which commit added the code and which commit made it unnecessary? "the previous patch/commit" is an okay way of saying that , IMO, even though it requires the reviewer to queue the two patches right after each other for the description to make sense.

In this case, I could use the Differential revision (D8724) to refer to the other patch. Would that also make sense? (I wonder why you didn’t mention this possibility — is it because the repository should be self-contained?)

Could you include in the commit message which commit added the code and which commit made it unnecessary? "the previous patch/commit" is an okay way of saying that , IMO, even though it requires the reviewer to queue the two patches right after each other for the description to make sense.

Should we have an entry in the release notes about this or have we given up on documenting API changes? I'm fine with either, as long as we have some policy.

Reminder that this is intended for stable (since that's where the py3-only fix is)

We have not made this change before because there is no hg update --abort. Do we have that feature yet (I haven't followed all the changes recently)?

In D8587#128844, @marmoute wrote:

In D8587#128843, @martinvonz wrote:

In D8587#128842, @marmoute wrote:

In D8587#128841, @martinvonz wrote:

Worth mentioning in relnotes? (This has been queued, so that would be a followup.)

I don't think so, what would you mention ?

We have a section for API changes. This patch affects many functions. (I suppose you might argue that we don't need to mention changes in private APIs (name prefixed by _), but these are not all private.)

These are private API used by experimental feature. I really do not expect that to affect anyone.

In D8587#128842, @marmoute wrote:

In D8587#128841, @martinvonz wrote:

Worth mentioning in relnotes? (This has been queued, so that would be a followup.)

I don't think so, what would you mention ?

Worth mentioning in relnotes? (This has been queued, so that would be a followup.)

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

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 ?

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 ?

In D7177#128757, @marmoute wrote:

In D7177#128756, @martinvonz wrote:

In D7177#128740, @marmoute wrote:

In D7177#128389, @martinvonz wrote:

Maybe another option is to allow multiple -d arguments for this case? Something like hg rebase -r C -d B -d D. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support hg rebase -r C -d 'B + D' for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).

That's interresting. How would that works with a practical case ?

The example I have was meant to be a practical example for how to do it in the case given in the patch description :) But let me know if you meant some other aspect of "how it works". As I said, I'm not sure ant the BC bit of it at least.

I meant how that would work in general, esepcially case more complicated than the basic base shown by @joerg.sonnenberger.

In D8596#128758, @marmoute wrote:

In D8596#128750, @martinvonz wrote:

In D8596#128704, @marmoute wrote:

In D8596#128319, @martinvonz wrote:

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

I had marked it "Changes Planned" but I've fixed the issues and the version I uploaded a day or two ago is ready for review .

Okay, (It would help if you reply to your own comment to state this clearly, which you now did)

In D7177#128740, @marmoute wrote:

In D7177#128389, @martinvonz wrote:

Maybe another option is to allow multiple -d arguments for this case? Something like hg rebase -r C -d B -d D. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support hg rebase -r C -d 'B + D' for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).

That's interresting. How would that works with a practical case ?

In D8599#128724, @marmoute wrote:

This patch seems related to D8596 which @martinvonz pointed as "not meant to be merged". What should we do with this diff ? is it intended for landing ? If not could get we it out of need-review and marke it RFC ?

In D8598#128720, @marmoute wrote:

This patch seems related to D8596 which @martinvonz pointed as "not meant to be merged". What should we do with this diff ? is it intended for landing ? If not could get we it out of need-review and marke it RFC ?

In D8597#128716, @marmoute wrote:

This patch seems related to D8596 which @martinvonz pointed as "not meant to be merged". What should we do with this diff ? is it intended for landing ? If not could get we it out of need-review and marke it RFC ?

In D8616#128712, @marmoute wrote:

This patch seems related to D8596 which @martinvonz pointed as "not meant to be merged". What should we do with this diff ? is it intended for landing ? If not could get we it out of need-review and marke it RFC ?

In D8595#128708, @marmoute wrote:

This patch seems related to D8596 which @martinvonz pointed as "not meant to be merged". What should we do with this diff ? is it intended for landing ? If not could get we it out of need-review and marke it RFC ?

In D8596#128704, @marmoute wrote:

In D8596#128319, @martinvonz wrote:

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

In D8563#128618, @marmoute wrote:

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

In D8552#128605, @marmoute wrote:

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

Maybe another option is to allow multiple -d arguments for this case? Something like hg rebase -r C -d B -d D. I haven't thought through BC, but I think that's what I'd prefer if we were writing rebase from scratch. I know we can't support hg rebase -r C -d 'B + D' for backward-compatibility reasons (because we already support that -- it rebases to the highest revnum in the set).

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

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.

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

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.

Also update the release notes to mention the API change?

Thanks for the quick fix and the added test case!