sure

Seems overall good. I added a request for comments.

sure

Looks good to me, thanks for the update. Sorry for the delay, it totally fell into the cracks.

In D8281#124246, @spectral wrote:

In D8281#124129, @durin42 wrote:

In D8281#124058, @marmoute wrote:

Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.

+0, honestly. I won't require it, but I'd really rather we shaved this yak _now_ rather than when narrow has even more users.

I'm getting a bit frustrated with how much time I've spent on this, made worse by the fact that I agree with everything everyone's saying and so it's not like I'm frustrated at the review process, just how slow I've been at accomplishing this.

In D8190#124147, @durin42 wrote:

In D8190#124146, @marmoute wrote:

In D8190#124141, @martinvonz wrote:

This new patch works, but I don't like the idea of not testing two fully independant process. Running a process through hooks is quite different and who know how that semantic could evolve.

Can we wrap the hg invocation in the script in enough env -u VAR business that we can know it won't see the pending transaction? That seems like it ought to be eminently doable. In fact, it might even merit an environment variable of its own, eg HG_IGNORE_PENDING_TXN=1 hg ... for use in other scripts.

In D8189#123522, @marmoute wrote:

In D8189#123323, @durin42 wrote:

[…]
I'm also not happy about the 1-second floor this puts on the step. Doesn't sleep(1) support sub-second sleeps on all platforms at this point?

I am extremly sad too. But last time I checked, it was still not the case. We detect plateform and use small increment on better plateform (but I would rather follow up for that).

In D8190#124141, @martinvonz wrote:

The parent patch (D8189) was a bit controversial. We don't need that patch if we apply the following patch on top of this one (making this test sleep-free):

diff --git a/tests/test-persistent-nodemap.t b/tests/test-persistent-nodemap.t
--- a/tests/test-persistent-nodemap.t
+++ b/tests/test-persistent-nodemap.t
@@ -320,17 +320,20 @@ An up to date nodemap should be availabl
 Another process does not see the pending nodemap content during run.
-  $ PATH=$RUNTESTDIR/testlib/:$PATH
+  $ PATH=$PWD:$RUNTESTDIR/testlib/:$PATH
+  $ cat >> log_nodemap <<EOS
+  > unset HG_PENDING
+  > hg debugnodemap --metadata > "\$1"
+  > EOS
+  $ chmod +x log_nodemap
   $ echo qpoasp > a
   $ hg ci -m a2 \
-  > --config "hooks.pretxnclose=wait-on-file 20 sync-repo-read sync-txn-pending" \
-  > --config "hooks.txnclose=touch sync-txn-close" > output.txt 2>&1 &
+  > --config "hooks.pretxnclose=log_nodemap pretxn-output" \
+  > --config "hooks.txnclose=log_nodemap posttxn-output"
 (read the repository while the commit transaction is pending)
-  $ wait-on-file 20 sync-txn-pending && \
-  > hg debugnodemap --metadata && \
-  > wait-on-file 20 sync-txn-close sync-repo-read
+  $ cat pretxn-output
   uid: ???????????????? (glob)
   tip-rev: 5004
   tip-node: ba87cd9559559e4b91b28cb140d003985315e031
@@ -340,7 +343,7 @@ Another process does not see the pending
   data-unused: 192 (pure !)
   data-unused: 192 (rust !)
   data-unused: 0 (no-pure no-rust !)
-  $ hg debugnodemap --metadata
+  $ cat posttxn-output
   uid: ???????????????? (glob)
   tip-rev: 5005
   tip-node: bae4d45c759e30f1cb1a40e1382cf0e0414154db
@@ -350,6 +353,3 @@ Another process does not see the pending
   data-unused: 448 (pure !)
   data-unused: 448 (rust !)
   data-unused: 0 (no-pure no-rust !)
-
-  $ cat output.txt
-

In D8189#124122, @durin42 wrote:

I'm still -0 on this: I'd rather we found an approach that didn't require sleeping for so long. Perhaps a Python script would be a better fit here?
(I won't block this landing, but I won't push it.)

Great, @mharbison72 can you submit a patch ?

In D8304#124116, @yuja wrote:

Could we enable some kind of warning to catch this in the tests ?

-Wdeclaration-after-statement for gcc and maybe clang.

Could we enable some kind of warning to catch this in the tests ?

It looks like futher change will be necessary on this patch, moving it out of yadaa

Gentle ping on this patch.

In D8244#123560, @martinvonz wrote:

In D8244#122821, @marmoute wrote:

In D8244#122806, @martinvonz wrote:

In 99ebde4fec99, we changed the list of files stored into the files field.
This lead to the changeset centric copy algorithm to break in various merge
situation involving merge.

Could you explain why it broke? It's hard to review this patch without really knowing what the problem or the solution is.

Outdated information from p1 could overwrite newer information from p2.

The new implementation focus on correctness. Performance improvement will come
later.

How much slower is it? Could you run some of those benchmarks you have run on previous patches touching this code? How do you plan to improve it?

There are two new calls that might degrade performance. This isancestor call that is easy to cache in memory. And the "ismerged" logic that is easy to cache on disk with the rest of the copy related information (the case is rare).
There is some win to have in python, but the main win will be the move the Rust algorithm (that need to be updated with the new logic). Moving to rust give a very large performance boost on the slow cases (usually over 10x, sometime 100x IIRC). That is the one I care about.

I'll make the performance impact more concrete myself. I picked two quite arbitrary tags in the mozilla-unified repo and this is what I saw:
Before this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 5.279230 comb 5.270000 user 5.250000 sys 0.020000 (best of 3)

After this patch:

$ python3 ~/hg/hg perfpathcopies FIREFOX_BETA_44_END FIREFOX_BETA_54_END
! wall 8.277523 comb 8.280000 user 8.170000 sys 0.110000 (best of 3)

Could you share some more benchmarking data? I know you had a set of commits that you've used before (and that you've asked me to use for benchmarking my patches to copies.py against). It's quite a significant slowdown for the case I tested above, but I'm fine with it since it fixes a bug. I'd just like to see how it behaves in other cases.

In D8243#123559, @martinvonz wrote:

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

In D8115#121345, @martinvonz wrote:

We already have tests for this (grepping for "unwanted changes" in tests/ should be enough to find them).
It's not easy to omit revisions 8 and 10 from the rebase. The key is to realize that rebase is done by repeated grafting of diffs and that the diffs are always between two commits. We can therefore rebase revisions 9 and 11 without a problem (we take the diff from revisions 8 and 10, respectively). But when rebasing the merge commit, which diff do we graft? If we use 9 as base (i.e. we try to graft the diff between 9 and 12), we'll get unwanted changes from commit 10. Conversely, if we use 11 as base, we'll get unwanted changes from commit 8.
I have been thinking a bit about how to solve it by doing the rebase in smaller steps, but it's pretty complicated and maybe I should write it down in a plan page instead.

This on seems need to be updated with hg update --continue/hg continue

The feature looks interresting, but I had a couple of feedback.

In D8289#124009, @pulkit wrote:

I like the idea. IIRC, Ryan from FB hit similar issues in a sprint some years ago and came up with hg up --finish or something like that.
Maybe we should not let user clear the mergestate and suggest continue/<cmd-name> --continue if it's not result of update command. Thoughts?

Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.

The code change seems simple and covered by tests.

What's the status of this ? @mharbison72 id the linking problem got solved ?

This is the last series stuck at the bottom of yadda.

@Alphare What's your current opinion on this diff, can you mark it accepted or request change appropriately ?

@Alphare What's your current opinion on this diff, can you mark it accepted or request change appropriatly ?

Looks like there are unresolved discussion about error handling here.

I am planning to have a look at this by the end of the week. SOrry for the delay

In D8284#123872, @martinvonz wrote:

In D8284#123838, @marmoute wrote:

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

The idea was to inform users that they're doing something that's a little weird, in case they were hoping for it to do something else. I don't care much and I'm fine with dropping this patch if that's the consensus.

In D8295#123832, @mharbison72 wrote:

Weird, I thought this was dependent on evolve too. But I guess if that were really the case, it would have be implemented in a wrapper in evolve. I suppose it works without evolve if using --keep, but I never use that.

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

For the record, I always use hg fix with --rev.

In D8282#123677, @martinvonz wrote:

In D8282#123675, @marmoute wrote:

Can you drop your change to tests/test-push-race.t from this patch ?

That change is consistent with the other changes in putting #requires and #testcases first. I'll update the commit message.

Can you drop your change to tests/test-push-race.t from this patch ?

That's a good point, doing it for #require too would be more consistent.

In D8282#123661, @martinvonz wrote:

(like imports and includes in other languages)

I would rather have the large title (===\ntitle\n===) be the very first things in the test for clarify.

The escaping scheme is a bit puzzling to me. Coudl we use something more standard for this ? (like urlencode).

The other change looks good and I would take them if they were not in the same changeset as the change in tests/test-push-race.t

Should we try to move to some automatic detection of file with doctest? That would be more reliable.

In D8189#123521, @mharbison72 wrote:

In D8189#123323, @durin42 wrote:

I think it might be better to poll more often in the script and not even take a timeout: sleep forever waiting for the condition, and if it never comes let the test timeout at the runner level. Thoughts?

I filed a bug about this that self-archived, but:

$ echo '  $ sleep 10' > test-timeout.t
$ time ./run-tests.py --local test-timeout.t -t 5
running 1 tests using 1 parallel processes
t
Failed test-timeout.t: timed out
# Ran 1 tests, 0 skipped, 1 failed.
python hash seed: 204038743
real    0m10.363s
user    0m0.000s
sys     0m0.030s

So it looks like tests never timeout, but then the result is discarded afterward if the timeout period elapsed. I can reproduce it on Windows and macOS.

In D8189#123323, @durin42 wrote:

In D8189#123280, @marmoute wrote:

By using explicit wait on signal (through the fs) that each process reached the appropriate file. We avoid flackyness. There are already multiple use of this approach in the test suite, that does not suffer from flackyness (unlike the wheelbarrow of flaky test relying on sleep for sync).

You avoid flakyness iff the test manages to finish this step in under 20 seconds (in the next change, as an example). Which is to say, this is still a flake waiting to happen, you've just made it less likely. I think it might be better to poll more often in the script and not even take a timeout: sleep forever waiting for the condition, and if it never comes let the test timeout at the runner level. Thoughts?

In D8272#123252, @ludovicchabant wrote:

The _write_gzip_header function is now getting passed the compression level in the latest Python release.
It does seem brittle to me to override some Python internal method anyway? Can we get rid of this altogether?

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

In D8187#123258, @durin42 wrote:

No longer applies on top of @.

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

rebase above latest default up to landed-D8182

This got cherry picked before the other patches that implement more of the nodemap in rust. As a result test-persistent-nodemap.t breaks.

By using explicit wait on signal (through the fs) that each process reached the appropriate file. We avoid flackyness. There are already multiple use of this approach in the test suite, that does not suffer from flackyness (unlike the wheelbarrow of flaky test relying on sleep for sync).