In D11444#175525, @Alphare wrote:

@martinvonz clangformat complains:

@@ -8,3 +8,18 @@
   >   cmp $f $f.formatted || diff -u $f $f.formatted
   >   rm $f.formatted
   > done
+  mercurial/cext/parsers.c mercurial/cext/parsers.c.formatted differ: char 3857, line 147
+  --- mercurial/cext/parsers.c        2021-09-17 15:05:49.010806214 +0000
+  +++ mercurial/cext/parsers.c.formatted      2021-09-17 15:06:34.139000016 +0000
+  @@ -144,8 +144,9 @@
+
+   static inline bool dirstate_item_c_added(dirstateItemObject *self)
+   {
+  -   unsigned char mask = (dirstate_flag_wc_tracked | dirstate_flag_p1_tracked |
+  -                dirstate_flag_p2_tracked);
+  +   unsigned char mask =
+  +       (dirstate_flag_wc_tracked | dirstate_flag_p1_tracked |
+  +        dirstate_flag_p2_tracked);
+      unsigned char target = dirstate_flag_wc_tracked;
+      return (self->flags & mask) == target;
+   }

I can just amend this if you want.

In D11445#175503, @marmoute wrote:

We should just drop the if :

self->flags |= dirstate_flag_possibly_dirty;
Py_RETURN_NONE

I noticed that this broke hg fix in some cases. I wrote a test case to show the difference (replace set_possibly_dirty by normallookup to see the different behavior)

In D10893#172904, @marmoute wrote:

In D10893#172447, @martinvonz wrote:

In D10893#171439, @martinvonz wrote:

In D10893#170300, @marmoute wrote:

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

Ping on this.

I don't have a simple sentence (nor a simple idea) about this ready to use. I did not have a single second to even think about this discussion as I have been busy with 5.9-freeze related stuff for the past 3 weeks. Since 5.9 is unfortunately not out of the woods yet, do not hold you breath on this.

In D11237#172500, @valentin.gatienbaron wrote:

I have been assuming that changes can still be submitted during a change freeze, but they are just going to get ignored for a few days. None of my changes are particularly intended to go on 5.9.

This patch seems to be for default. I think we're supposed to be in code freeze (though lots of patches have still been queued). Should we mark patches (like this one) that are not for stable as "Changes Requested" so they're not accidentally queued during the freeze?

In D10893#171439, @martinvonz wrote:

In D10893#170300, @marmoute wrote:

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

Actually, do you mind sharing at least sentence or so about what you think might be the right UX, even if it's not completely clear to you yet? I just want to get a sense of what you have in mind.

This is slower, but probably doesn't matter?

In D10893#170300, @marmoute wrote:

This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

In D10893#170300, @marmoute wrote:

Thanks for starting this discussion "early" (before sending more code) so that we could start discussing this early. These discussion around the corner case are very insightful about the use-cases and what might/should be done to solve them.
This last round of discussion make me think that this might not be the right UX to achieve our goal here. However I won't have time to elaborate much more because (1) I need some quiet time to think about it more (2) writing the ideas down will probably cost me multiple hours, hours that I currently do not have as the Windows/py3 testing and issue6528/6538 have been time consuming. I'll come back to you about that.

In D10893#170283, @Alphare wrote:

Sorry I forgot to post this before the week-end.
@martinvonz broader picture question: what is your expected timeline for this getting into core? I'm seeing new questions coming up and it's looking like at least the UX for this new feature needs to be discussed some more.
I wanted to get this feature in before the freeze, but now that I see that it would be rushed at best, I'm hoping we can move this discussion to early August after the freeze to actually give the underlying questions a real shot. We've decided the same for the new dirstate-v2 format internally at Octobus for example.

In D10893#170047, @marmoute wrote:

In D10893#166902, @martinvonz wrote:

I'll start with not even having the flag :P When automatically rebasing the descendants, it seems more clear that the changes should not be left in the working copy. I was planning on explicitly rebasing them and not leaving them in the working copy, not relying on them disappearing only because the changes were already present in an ancestor afterwards. To clarify, I was planning for it to do this:

1. Create temporary commit from working copy
2. Rebase temporary commit to target (not using --keep)
3. Fold the rebased commit into the target
4. Rebase descendants, not including the temporary commit from step 1

One could imagine using --keep in step two and include the temporary commit in step 4 and just rely on it becoming empty. However, if there were conflicts to resolve in step 2, then you'd run into conflicts again in step 4. It seems better to avoid that.

Looks like we could have a --no-evolve flag that stop after step 3 and be good.

In D11039#169655, @Alphare wrote:

Can we test that further descendants (potentially not linear) are not rebased automatically? This can be done in a separate, simpler test case as an additional changeset.

I won't actually need this. I'm going to make the command update to the old working copy parent before doing the rebasing, which will result in rebase taking care of the update for us.

I'll add tests for amending into siblings and descendants too, along with code changes necessary to make that work.

This series is now ready for review again

In D10917#168352, @martinvonz wrote:

In D10917#168351, @marmoute wrote:

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

In D10917#168351, @marmoute wrote:

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

In D10917#166942, @marmoute wrote:

In D10917#166941, @martinvonz wrote:

In D10917#166940, @marmoute wrote:

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

No, given the information returned by find_new_divergence_from() it seems unavoidable that filtered revision might be returned as the "common-predecessor" value. (sidenote: that the common-precessors should probably be a list, and the return of find_new_divergence_from and _find_new_divergence should probably be list too).
My point is that we are looking for divergence within the current filter and that we should not pass unfiltered repository to the function that looks for that divergence. Otherwise it might compute things on the wrong set of revisions.
However, function along the chain should be prepared for the "common-predecessor" nodes to be potentially filtered. In this case this means turning this line

return (repo[r], repo[div[0]], repo[div[1]])

to

return (repo[r], repo[div[0]], unfi[div[1]])

In D10917#166940, @marmoute wrote:

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

In D10893#166901, @marmoute wrote:

In D10893#166899, @martinvonz wrote:

In D10893#166881, @marmoute wrote:

In D10893#166669, @martinvonz wrote:

In D10893#166668, @Alphare wrote:

In D10893#166666, @martinvonz wrote:

With --no-evolve, This can simply be:

• hg amend -i --rev A --no-evolve → no conflict resolution yet

I imagine that the selected changes are removed from the working copy after this step. Is that what you were thinking as well? Or were you thinking that they would remain in the working copy as well (more like hg rebase --keep if you think of the working copy contents as a commit)?

That is a very good question, that I have not been able to have a clear opinion on so far. (I did not though about it too much either).
I can see argument for both, "if it is integrated somewhere else, it should no longer be in the wdir() diff" and "This is asking for trouble, we better leave it around".
In the usecase were a small type fix is cast-away in an ancestor and the change is not really relevant to the WC, dropping it is "cleaner" and won't introduce conflict.
On the other hand If the usecase is to send something more conflictful and/or more relevant to the WC, keeping it around might make sense ?
I suggest we start with the path of least resistance (probably keeping it in the WC then) and add a # XXX TODO comment near the experimental config so that we don't forget about that before getting the feature out of experiment.

In D10893#166881, @marmoute wrote:

In D10893#166669, @martinvonz wrote:

In D10893#166668, @Alphare wrote:

In D10893#166666, @martinvonz wrote:

Sure, I can add a --no-rebase flag too. When would you want that?

This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.

I'm not sure I follow. Let's say you have this history:

@ C
|
o B
|
o A

You then make some change and run hg amend -r B --no-rebase. You get this:

@ C
|
x B
|
| B'
|/
o A

Presumably the working copy is now clean again? However, that means that you can't make changes that build on top of the previous amend.
You then make some further changes in the working copy. What do you do now? If you do hg amend -r B, you would presumably get divergence. Should we allow hg amend -r B'? I had been thinking that we restrict hg amend -r to only amend into an ancestor just to keep it simple and because that seems like what people want in 99% of cases.

There is a bunch of small usecase for --no-rebase (that should probably be named --no-evolve
In simple case, the user knows he will amend various bits of the current working copy in different commit of the stack and prefer to run the evolution only once (to reduce conflict resolution and/or to reduce markers creation).
In more advanced case, this might be actually needed to avoid a loop of complicated and unnecessary conflict resolution. Lets imagine the following scenario:

@ C
|
o B
|
o A

• There are change to amend in A, but they will need adjustement to actually work in A and they will conflict with some content in B (then C)
• There are change to amend in B, but they will conflict with the change in A and with some change in C

Without --no-evolve, this would mean the following operation

• hg amend -i --rev A → this requires conflict resolution when evolving B, then C; conflict is resolved with non final content
• hg amend -i --rev B → this requires conflict resolution when evolving C; conflict is resolved with non final content
• hg update A; hack hack hack; hg amend; hg next this requires conflict resolution when evolving B,
• hack hack hack; hg amend; hg next this requires conflict resolution when evolving C,

With --no-evolve, This can simply be:

• hg amend -i --rev A --no-evolve → no conflict resolution yet

I'll update this patch and add several more after it in a day or two. (I should have marked it RFC to start with.)

In D10893#166756, @mharbison72 wrote:

In D10893#166684, @martinvonz wrote:

In D10893#166678, @mharbison72 wrote:

Also, adding the feature to hg fold is going to be more work because it's not in core.

Hmm, yeah. Are you planning on implementing the --from abc --into xyz case in the near term? I probably just wasn't clear enough that I thought this (currently out of scope) functionality is what might belong elsewhere.

In D10893#166678, @mharbison72 wrote:

In D10893#166666, @martinvonz wrote:

In D10893#166662, @Alphare wrote:

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

+1

• I remember sprint discussions wherein people warned that the semantics of --rev were not clear in the context of amend and suggested using something like --into-rev. I personally think --rev feels natural to me and I don't have strong objections to using it, but we should maybe ask the community (with a poll?) and keep the option open while implementing it.

I initially called it hg amend --into, but then someone (in an unrelated discussion) mentioned hg amend -i yesterday and I thought that users might confuse hg amend --into and hg amend -i. I think most users think of the feature as hg amend -r, so I switched to that.
My reason for initially calling it --into is that the command works on two revisions: the working copy and the target commit. Most hg commands default to working on the working copy and use -r to change that (for example, hg branch and hg topic change the branch name in the working copy by default, but you can change the branch/topic of other commits by passing -r). hg amend defaults to amending from the working copy and into the parent of the working copy. It seems unlikely that we'll add support for amending from an arbitrary commit into an ancestor of that commit. If we ever do that, we can make that hg amend --from abc123 --into abc123^^ and keep -r as a deprecated alias for --into.
Sounds okay?

-r seems reasonable to me. The --from abc --into xyz form sounds more like a non contiguous hg fold to me. I haven't given any thought to if adding that functionality there would make the UI too awkward, given that you can select multiple source revs to fold.

In D10893#166668, @Alphare wrote:

In D10893#166666, @martinvonz wrote:

Thanks for the quick review. I sent this out early to get feedback on the idea -- particularly on the fact that we'll have hg amend --continue and hg amend --abort -- before I wasted time going too far in the wrong direction.

Now that I think of it I'm not entirely certain what the intermediate step for amend will be used for. Will it just be in case of rebase conflicts or is there more to it?

In D10893#166662, @Alphare wrote:

This is indeed a very important quality-of-life feature and I'm +1 on adding it to Mercurial.

In D10894#166664, @Alphare wrote:

Doesn't this lose the ui.silent feature?