Page MenuHomePhabricator

marmoute (Pierre-Yves David)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 29 2017, 2:56 PM (143 w, 5 d)

Recent Activity

Today

marmoute added a comment to D8336: tests: use `f --hexdump` to print file content.

sure

Wed, Apr 1, 4:18 AM
D8352: extensions: don't crash if __file__ not defined now requires changes to proceed.
Wed, Apr 1, 4:17 AM
marmoute added a comment to D8351: hgcli: customize for Mercurial.

Seems overall good. I added a request for comments.

Wed, Apr 1, 4:15 AM
marmoute accepted D8350: hgcli: add stub PyOxidizer project.
Wed, Apr 1, 4:11 AM
marmoute added a comment to D8349: hgcli: remove legacy project.

sure

Wed, Apr 1, 4:08 AM

Sun, Mar 29

marmoute accepted D8172: notify: optional mail threading based on obsmarker.
Sun, Mar 29, 1:33 PM

Fri, Mar 27

marmoute added a comment to D8172: notify: optional mail threading based on obsmarker.

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

Fri, Mar 27, 10:09 AM

Thu, Mar 26

marmoute updated the diff for D8193: nodemap: automatically "vacuum" the persistent nodemap when too sparse.
Thu, Mar 26, 9:13 AM
marmoute updated the diff for D8316: testlib: adjust wait-on-file timeout according to the global test timeout.
Thu, Mar 26, 9:13 AM

Wed, Mar 25

D8324: py3: use integer division in histedit now requires changes to proceed.
Wed, Mar 25, 5:59 AM

Sat, Mar 21

marmoute added a comment to D8281: narrow: escape includepats/excludepats when sending over the wire.

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.

Sat, Mar 21, 12:16 AM
marmoute accepted D8318: setup: build C extensions with -Werror=declaration-after-statement.
Sat, Mar 21, 12:02 AM

Fri, Mar 20

marmoute created D8316: testlib: adjust wait-on-file timeout according to the global test timeout.
Fri, Mar 20, 7:08 PM
marmoute updated the diff for D8189: testlib: add a small scrip to help process to synchronise using file.
Fri, Mar 20, 7:07 PM
marmoute added a comment to D8190: nodemap: test that concurrent process don't see the pending transaction.

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.

Fri, Mar 20, 7:05 PM
marmoute updated subscribers of D8189: testlib: add a small scrip to help process to synchronise using file.

[…]
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).

Fri, Mar 20, 7:05 PM
marmoute added a comment to D8190: nodemap: test that concurrent process don't see the pending transaction.

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
-
Fri, Mar 20, 3:28 PM
marmoute updated subscribers of D8189: testlib: add a small scrip to help process to synchronise using file.

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

Fri, Mar 20, 11:49 AM
marmoute added a comment to D8304: cext: move variable declaration to the top of the block for C89 support.

Great, @mharbison72 can you submit a patch ?

Fri, Mar 20, 11:19 AM
marmoute added a comment to D8304: cext: move variable declaration to the top of the block for C89 support.
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.

Fri, Mar 20, 10:54 AM
marmoute added a comment to D8304: cext: move variable declaration to the top of the block for C89 support.

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

Fri, Mar 20, 10:15 AM

Thu, Mar 19

D7967: exchange: recognize changegroup3 bundles in `getbundlespec()` now requires changes to proceed.

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

Thu, Mar 19, 9:33 PM
marmoute added a comment to D8189: testlib: add a small scrip to help process to synchronise using file.

Gentle ping on this patch.

Thu, Mar 19, 8:47 PM
marmoute added a comment to D8244: copies: fix the changeset based algorithm regarding merge.

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.

Thu, Mar 19, 8:44 PM
marmoute added a comment to D8243: copies: stop recording buggy file merge when new file overwrite an old one.

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?

Thu, Mar 19, 8:21 PM
marmoute added a comment to D8115: rebase: show bug when rebasing merge with obsoleted revs on both sides.

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.

Thu, Mar 19, 8:16 PM
D8290: morestatus: recommend `hg resolve --clear` when appropriate now requires changes to proceed.

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

Thu, Mar 19, 8:02 PM
D8172: notify: optional mail threading based on obsmarker now requires changes to proceed.

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

Thu, Mar 19, 8:00 PM
marmoute added a comment to D8289: resolve: add a --clear option for clearing the merge state.

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?

Thu, Mar 19, 7:43 PM
marmoute added a comment to D8281: narrow: escape includepats/excludepats when sending over the wire.

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.

Thu, Mar 19, 7:35 PM
marmoute added a comment to D8233: phabricator: allow multiple DREVSPEC args to phabread|phabimport|phabupdate.

The code change seems simple and covered by tests.

Thu, Mar 19, 7:25 PM
marmoute added a comment to D6846: packaging: script the building of a MacOS installer using a custom python.

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

Thu, Mar 19, 7:17 PM
marmoute added a comment to D7296: pycompat: kludge around pytype being confused by __new__.

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

Thu, Mar 19, 7:14 PM
marmoute added a comment to D7576: hg-core: add configparser to library.

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

Thu, Mar 19, 7:07 PM
marmoute added a comment to D7575: hg-core: vendor Facebook's configparser crate.

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

Thu, Mar 19, 7:05 PM
marmoute added a comment to D7574: hg-core: add utils::path to project.

Looks like there are unresolved discussion about error handling here.

Thu, Mar 19, 7:04 PM
marmoute committed rHGbc9a9016467d: byteify-string: resolve symlink before byteifying.
byteify-string: resolve symlink before byteifying
Thu, Mar 19, 4:37 PM
marmoute added a comment to D8172: notify: optional mail threading based on obsmarker.

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

Thu, Mar 19, 5:07 AM

Mon, Mar 16

marmoute closed D8276: cext-index: propagate inline_scan error in `index_deref`.
Mon, Mar 16, 11:04 PM
marmoute closed D8275: heptapod-ci: fix test paths in the listing file.
Mon, Mar 16, 11:04 PM
marmoute committed rHG864e9534d3d4: cext-index: propagate inline_scan error in `index_deref`.
cext-index: propagate inline_scan error in `index_deref`
Mon, Mar 16, 11:04 PM
marmoute committed rHGdaf083140b5b: heptapod-ci: fix test paths in the listing file.
heptapod-ci: fix test paths in the listing file
Mon, Mar 16, 11:04 PM
marmoute added a comment to D8284: fix: disallow `hg fix --all --working-dir`.

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.

Mon, Mar 16, 4:15 PM

Sat, Mar 14

marmoute added a comment to D8295: rebase: mention -r argument in synopsis.

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.

Sat, Mar 14, 5:34 AM
marmoute accepted D8291: rebase: remove unused defaults argument values from _definedestmap().
Sat, Mar 14, 5:31 AM
marmoute added a comment to D8284: fix: disallow `hg fix --all --working-dir`.

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

Sat, Mar 14, 4:41 AM
marmoute added a comment to D8288: fix: mark -r as advanced.

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

Sat, Mar 14, 4:40 AM

Fri, Mar 13

marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

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.

Fri, Mar 13, 1:10 PM
marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

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

Fri, Mar 13, 12:49 PM
marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

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

Fri, Mar 13, 11:38 AM
marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

(like imports and includes in other languages)

Fri, Mar 13, 11:29 AM
marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

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

Fri, Mar 13, 10:15 AM
D8281: narrow: escape includepats/excludepats when sending over the wire now requires changes to proceed.

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

Fri, Mar 13, 4:58 AM
D8282: tests: consistently put #testcases at beginning of file now requires changes to proceed.
Fri, Mar 13, 4:51 AM
marmoute added a comment to D8282: tests: consistently put #testcases at beginning of file.

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

Fri, Mar 13, 4:51 AM
marmoute added a comment to D8280: tests: make test-doctest.t module list match reality.

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

Fri, Mar 13, 4:48 AM

Thu, Mar 12

marmoute accepted D8277: tests: fix rebase test broken by earlier cleanup.
Thu, Mar 12, 9:43 PM
marmoute accepted D8278: rust-core: add missing `Debug` traits.
Thu, Mar 12, 9:06 PM
marmoute created D8276: cext-index: propagate inline_scan error in `index_deref`.
Thu, Mar 12, 2:26 PM
marmoute created D8275: heptapod-ci: fix test paths in the listing file.
Thu, Mar 12, 2:26 PM

Wed, Mar 11

marmoute updated the diff for D8243: copies: stop recording buggy file merge when new file overwrite an old one.
Wed, Mar 11, 6:03 PM
marmoute added a comment to D8189: testlib: add a small scrip to help process to synchronise using file.

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.

Wed, Mar 11, 2:20 PM
marmoute updated the diff for D8192: nodemap: display percentage of unused in `hg debugnodemap`.
Wed, Mar 11, 2:20 PM
marmoute updated the diff for D8193: nodemap: automatically "vacuum" the persistent nodemap when too sparse.
Wed, Mar 11, 2:20 PM
marmoute updated the diff for D8191: nodemap: make sure on disk change get rolled back with the transaction.
Wed, Mar 11, 2:20 PM
marmoute added a comment to D8189: testlib: add a small scrip to help process to synchronise using file.

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?

Wed, Mar 11, 2:18 PM
marmoute accepted D8270: run-tests: restrict Rust thread pool to 3 threads during tests.
Wed, Mar 11, 11:56 AM
marmoute closed D8184: nodemap: track the tip_node for validation.
Wed, Mar 11, 11:44 AM
marmoute closed D8187: nodemap: make sure hooks have access to an up-to-date version.
Wed, Mar 11, 11:44 AM
marmoute committed rHG448d700e0d27: nodemap: make sure the nodemap docket is updated after the changelog.
nodemap: make sure the nodemap docket is updated after the changelog
Wed, Mar 11, 11:44 AM
marmoute closed D8188: nodemap: make sure the nodemap docket is updated after the changelog.
Wed, Mar 11, 11:44 AM
marmoute closed D8163: nodemap: use data from the index in debugnodemap --dump-new.
Wed, Mar 11, 11:44 AM
marmoute committed rHG64e2f603de9d: nodemap: make sure hooks have access to an up-to-date version.
nodemap: make sure hooks have access to an up-to-date version
Wed, Mar 11, 11:43 AM
marmoute closed D8164: rust-nodemap: automatically use the rust index for persistent nodemap.
Wed, Mar 11, 11:43 AM
marmoute committed rHG6c906eaedd0d: nodemap: track the tip_node for validation.
nodemap: track the tip_node for validation
Wed, Mar 11, 11:43 AM
marmoute closed D8181: nodemap: add a todo list for getting out of experimental.
Wed, Mar 11, 11:43 AM
marmoute committed rHG15a033cabc19: nodemap: add a todo list for getting out of experimental.
nodemap: add a todo list for getting out of experimental
Wed, Mar 11, 11:43 AM
marmoute committed rHGe7fff9c3cdac: rust-nodemap: automatically use the rust index for persistent nodemap.
rust-nodemap: automatically use the rust index for persistent nodemap
Wed, Mar 11, 11:43 AM
marmoute committed rHGfebe88a6f7f7: nodemap: use data from the index in debugnodemap --dump-new.
nodemap: use data from the index in debugnodemap --dump-new
Wed, Mar 11, 11:43 AM
marmoute closed D8174: nodemap: refresh the persistent data on nodemap creation.
Wed, Mar 11, 11:42 AM
marmoute committed rHG87b327de772c: nodemap: refresh the persistent data on nodemap creation.
nodemap: refresh the persistent data on nodemap creation
Wed, Mar 11, 11:41 AM

Tue, Mar 10

marmoute added a comment to D8272: archive: fix crash when archiving to gzip file with Python 3.8.2+.

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?

Tue, Mar 10, 9:42 PM
marmoute added a comment to D8192: nodemap: display percentage of unused in `hg debugnodemap`.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:37 PM
marmoute added a comment to D8184: nodemap: track the tip_node for validation.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:37 PM
marmoute added a comment to D8193: nodemap: automatically "vacuum" the persistent nodemap when too sparse.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:37 PM
marmoute added a comment to D8187: nodemap: make sure hooks have access to an up-to-date version.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:36 PM
marmoute added a comment to D8191: nodemap: make sure on disk change get rolled back with the transaction.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:36 PM
marmoute added a comment to D8188: nodemap: make sure the nodemap docket is updated after the changelog.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:36 PM
marmoute added a comment to D8187: nodemap: make sure hooks have access to an up-to-date version.

No longer applies on top of @.

Tue, Mar 10, 9:36 PM
marmoute added a comment to D8164: rust-nodemap: automatically use the rust index for persistent nodemap.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:35 PM
marmoute added a comment to D8186: nodemap: deal with the "debugupdatecache" case using a "fake" transaction.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:35 PM
marmoute added a comment to D8185: changelog: change the implementation of `_divertopenener`.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:35 PM
marmoute added a comment to D8183: nodemap: test that an outdated nodemap can catch up.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:35 PM
marmoute added a comment to D8152: revlog: using two new functions in C capsule from Rust code.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:35 PM
marmoute added a comment to D8174: nodemap: refresh the persistent data on nodemap creation.

rebase above latest default up to landed-D8182

Tue, Mar 10, 9:34 PM
marmoute added a comment to D8183: nodemap: test that an outdated nodemap can catch up.

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

Tue, Mar 10, 8:53 PM
marmoute added inline comments to D8270: run-tests: restrict Rust thread pool to 3 threads during tests.
Tue, Mar 10, 8:22 PM
marmoute added inline comments to D7932: debugbackupbundle: introduce command to interact with strip backups.
Tue, Mar 10, 8:22 PM
marmoute added a comment to D8189: testlib: add a small scrip to help process to synchronise using file.

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

Tue, Mar 10, 8:20 PM
marmoute closed D8186: nodemap: deal with the "debugupdatecache" case using a "fake" transaction.
Tue, Mar 10, 4:48 PM