In D6168#89874, @martinvonz wrote:

In D6168#89872, @jeffpc wrote:

FWIW, I tested this on both Unleashed (an illumos fork) and FreeBSD. It fixes the issue for me on both.

Thanks for testing, Jeff. I'll queue based on that.

In D6168#89872, @jeffpc wrote:

FWIW, I tested this on both Unleashed (an illumos fork) and FreeBSD. It fixes the issue for me on both.

In D6140#89770, @martinvonz wrote:

Josef 'Jeff' Sipek <jeffpc@josefsipek.net> sent this to mercurial-devel. I'm adding it here for reference.

"x::x" is a useful trick for making a range contiguous, but it gets
annoying if "x" is a long expression. Let's provide a simple function
that helps with that. It also makes it the trick more discoverable.

...

+@predicate('contiguous(set)', safe=True, takeorder=True)
+def contiguous(repo, subset, x, order):
+ """Changesets that have both ancestors and descendants in the set. This
+ effectively fills in gaps in the set to make it contiguous, without adding
+ new common ancestors or common descendants.
+
+ "contiguous(x)" is identical to "x::x".

I read this doc string and the patch intro several times, and every time I
concluded that this function was useless. Only after reading some of the
other replies, did I realize that "x" here can be a set.

Josef 'Jeff' Sipek <jeffpc@josefsipek.net> sent this to mercurial-devel. I'm adding it here for reference.

In D6140#89749, @mharbison72 wrote:

I've only used X::X where X was trivial, so I'm still trying to get my mind around this. Out of curiosity, what are the scenarios where a nontrivial X is useful?

When the test suite is run with --extra-config-opt to use the new copy
tracing, all tests pass, besides test-copies.t (which fails in the
same way as you can see in this patch).

In D6124#89743, @pulkit wrote:

Amended the following in flight:

In D6140#89733, @marmoute wrote:

I spend some more time thinking about it, especially about the semantic
I would expect from a contiguous function. My conclusion is that I'm
seeing something call contiguous more as a filter than something
adding element to the set.

Good point. Isn't there a debug command for rebuilding it? We can add a
hint about that.

In D6140#89474, @yuja wrote:

I think "contiguous" is good as it stands for the main use case. "closure"
seems confusing unless we have stronger math background than computer science.
The other candidates would require more knowledge about the theory.

In D6140#89469, @gracinet wrote:

I thought of "closure" as well, but I fear it has too many possible meanings, "transitive closure" being one of them in that context (certainly related, but not the same thing), and of course the closures in functional programming.

I think "hull" could be appropriate, if not too pedantic.

I couldn't find out quickly if people dealing with partial ordered sets theory (another way to think of DAGs) actually use "hull", but here's the analogy:
for the convex hull, you add [a, b] (line segment) to the set whenever a and b belong to it, for this "poset hull" you do the same with a::b (which in poset theory would be called the interval [a, b])

Can you wrap the commit message to 80 columns? I didn't find anything on https://www.mercurial-scm.org/wiki/ContributingChanges about it, but that's how almost everyone else does it.

In D6140#89440, @marmoute wrote:

I chatted a bit with Georges about this. He suggested something along
the line of fill or complete.

In D2010#89436, @indygreg wrote:

If we loosen the naming requirement, I think a good convention would be to have new files use the *modern* convention and for existing code/files to generally stick to the old convention.

That being said, if someone were to introduce a new function into an existing file and wanted to use the modern names, I wouldn't mind.

I would not like to see global, API breaking rewrites for the sake of rewrites. If we wanted to do a global search and replace on variables inside functions, I'd be OK with that (that won't break API compat). But I'm in no rush to do it.

I would also not like to see patches introducing mixed naming conventions within functions. I think we should try to keep things consistent at definitely the function level and possibly the file level.

I understand that it's not at all clear if we want to make this change by default, but I'd appreciate if we can queue this patch as is so I and others can easily start testing it (it's pretty deep inside _performrevert(), so it's hard to do it cleanly in an extension). I don't believe I'm changing the behavior for anyone who has not set the new config.

In D6140#89400, @marmoute wrote:

I am a fan of this function, I need this on a regular basis. Having an
explicit function for this also open the way to various optimization.
For example we know that a set already has this property we could skip
all computation.

I am ambivalent about the naming however. It feels a bit odd. There are
case where it could be misleading.

Lets look at the following case:

c e
| |
b d
|/
a

the revset (b+c+d+e)::(b+c+d+e) returns the same b+c+d+e, however
the set is not "contiguous" as b+c and d+e as not connected.

In D2010#89337, @indygreg wrote:

Should we queue this patch or abandon it?

In D6005#89211, @pulkit wrote:

In D6005#89182, @martinvonz wrote:

Can you update the commit message to explain how this works? I think I saw somewhere else that it first uncommits everything and then does an interactive amend. Is that correct?

Yes, this is correct.

Also, I just tried to use this feature and it failed (depending on how many lines were added). I added two lines to a file, committed, then ran hg uncommit -i and selected the first line. Then I got this:

starting interactive selection
patching file z
Hunk #1 succeeded at 2 with fuzz 2 (offset -1 lines).

Yes this is also expected with the current implementation. Sometimes uncommit can fail and it will report Hunk Failed like this. There is definitely lot to improve in this implementation and I am fine with having this in core first.

In D6115#89208, @taapas1128 wrote:

Earlier it didn't but now it can I suppose it should after the modification.

Does hg unamend accept patterns as arguments?

Could you choose the "Abandon Revision" action on this patch so it's clear that it's not meant to be queued?

Can you update the commit message to explain how this works? I think I saw somewhere else that it first uncommits everything and then does an interactive amend. Is that correct?

Sorry, I forgot (again) to manually add the parent revision. That's done now.

In D6117#89132, @taapas1128 wrote:

In the documentation part don't you think there should be information about the matcher as there was before D971

Sorry, I was actually about to send a different patch for updating its documentation. I'll send it in a few minutes and you can let me know what you think.

unamend: made match optional in firdirstate()

In D5963#88980, @khanchi97 wrote:

Gentle ping for review.

Here are some little comments to start with. I haven't even started reviewing the three new functions, but I need to take a break and work on other things a bit now.

As I said a while ago on IRC, it feels like _fixdirstate() should not need an interactive flag, and it should not even need to be specifically for uncommit. It feels like it should work the same for any case where we move to a different commit without changing the working copy, such as uncommit, unamend, amend, fix (though we may want optimized code paths for some of them). So I took this patch and tried the most naive attempt at unifying the interactive version with the non-interactive version, namely to just drop the code for the interactive path (see patch below). Surprisingly, all tests pass :) So maybe you can just apply the below patch and clean it up (drop the if True), or maybe you need to add tests.