In D6196#90521, @yuja wrote:

4   org.python.python             	0x00000001000eb761 va_build_value + 737
5   org.python.python             	0x00000001000eb837 _Py_BuildValue_SizeT + 167

Maybe we shouldn't trust the Python doc too much, which says 's#' of
Py_BuildValue() takes an int.

https://docs.python.org/2/c-api/arg.html#c.Py_BuildValue

Can you test if this patch fixes the problem?

diff --git a/mercurial/cext/parsers.c b/mercurial/cext/parsers.c
--- a/mercurial/cext/parsers.c
+++ b/mercurial/cext/parsers.c
@@ -184,7 +184,8 @@ static PyObject *parse_dirstate(PyObject
 		goto quit;
 	}
 
-	parents = Py_BuildValue(PY23("s#s#", "y#y#"), str, 20, str + 20, 20);
+	parents = Py_BuildValue(PY23("s#s#", "y#y#"), str, (Py_ssize_t)20,
+	                        str + 20, (Py_ssize_t)20);
 	if (!parents) {
 		goto quit;
 	}
diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -366,7 +366,7 @@ static PyObject *index_get(indexObject *
 
 	entry = Py_BuildValue(tuple_format, offset_flags, comp_len, uncomp_len,
 	                      base_rev, link_rev, parent_1, parent_2, c_node_id,
-	                      20);
+	                      (Py_ssize_t)20);
 
 	if (entry) {
 		PyObject_GC_UnTrack(entry);
@@ -3017,8 +3017,9 @@ void revlog_module_init(PyObject *mod)
 	PyModule_AddObject(mod, "nodetree", (PyObject *)&nodetreeType);
 
 	if (!nullentry) {
-		nullentry = Py_BuildValue(PY23("iiiiiiis#", "iiiiiiiy#"), 0, 0,
-		                          0, -1, -1, -1, -1, nullid, 20);
+		nullentry =
+		    Py_BuildValue(PY23("iiiiiiis#", "iiiiiiiy#"), 0, 0, 0, -1,
+		                  -1, -1, -1, nullid, (Py_ssize_t)20);
 	}
 	if (nullentry)
 		PyObject_GC_UnTrack(nullentry);

I don't have time to look into this, but this makes the mac builds very unhappy. (The parent runs fine.) Somehow the bot is still running, but recording various segfaults. When I tried to investigate, the build process fails because python hg version is segfaulting. Here's the output of that, with the initial python frames removed:

In D6140#89771, @martinvonz wrote:

In D6140#89770, @martinvonz wrote:

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

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.

The docstring does say "in the set" :) But I agree that it's not very clear. I copied the pattern from other functions. I would probably have said "in the input set" otherwise. Do you think that would have been clearer? We could make that change to all the existing cases of plain "set" referring to the input.

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?

Is there something that can be done to repair this, that should be suggested in the hint? I've seen fncache errors in the context of a corrupt repo, but I'm not sure what an end user would do with this message.

I don't know anything about PowerShell or AWS, but this series looks nice for making Windows easier to deal with.

In D6082#88981, @Kwan wrote:

In D6082#88852, @mharbison72 wrote:

In D6082#88851, @pulkit wrote:

@mharbison72 thanks for tips on adding test. Will add tests in next iteration.

I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php

Seems worth a shot. I don’t know anything about it, but presumably this would be rendered specially in the web UI, like the test plan, etc. That sounds better than as a follow up comment. It also seems natural enough that maybe it can be done unconditionally, instead of needing the argument.

Yeah, it shows in the Diff Detail pane, like here. Unfortunately I think it can be only set when using the creatediff endpoint, which is what I had to change my fork to do (though I could be wrong, the conduit docs are too sparse to be sure).

In D6082#88851, @pulkit wrote:

@mharbison72 thanks for tips on adding test. Will add tests in next iteration.

I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php

To create a test, you need to pip install pytest-vcr, edit auth.hgphab.phabtoken at the top of the test, and use --test-vcr with your hg phabsend command. See a641fd1a1196 for some background, and remember to purge your phabtoken from the changes before posting. FWIW, I used a VirtualBox VM running Phabricator from Bitnami to figure things out before generating the test on phab.m-s.o to avoid extra noise.

There are a handful of other places in branchmap and elsewhere that also log or format repo.filtername.

In D5981#87343, @martinvonz wrote:

In D5981#87341, @mharbison72 wrote:

The tests run clean with this.

But I'm a bit confused. The test harness has been doing '\' -> '/' conversion without (glob) now for a little over a year, and it complains if there's a trailing (glob) and no '\' -> '/' conversion. That's not been happening here, and made me suspicious. These globs predate that functionality slightly, so I'm not sure the meaning of the referenced commit (which seems to say nothing will change because ui.slash is set).

You're right, and even if I go back to bdcaf612e75a (where you added these globs), it seems like it should have been converted to slashes already there (because the test runner set ui.slash back then, too, and the code seemed to respect that). Do you have time to go back and see if the globbing was never needed?

The tests run clean with this.

In D5801#87302, @martinvonz wrote:

In D5801#87300, @mharbison72 wrote:

In D5801#87296, @martinvonz wrote:

I noticed another bug and sent D5978. Maybe your test failure is because you're using the eol extension? I don't know what else would cause the \r in contrib.perf. I have no idea how that's related to this patch, though.

The \r is how output normally is on Windows. The test harness accounts for this when matching lines, but displays the actual output (with \r) when there are differences. The eol extension isn’t configured on this machine, but the custom HGRCPATH content from the test harness would override that anyway.

Ah, so the only difference is the extra "import newer module separately in try clause for early Mercurial" in contrib/perf.py then. This patch didn't change that file. Can you check again that it was this patch that caused that and that it's not just flaky?

In D5801#87296, @martinvonz wrote:

I noticed another bug and sent D5978. Maybe your test failure is because you're using the eol extension? I don't know what else would cause the \r in contrib.perf. I have no idea how that's related to this patch, though.

This seems to have converted a test from dos to unix style. I'm not sure if that was intentional, or if this feature is still a WIP. (Sorry, I've been busy lately, and haven't had time to follow this closely)

For some reason, this breaks test-contrib-perf.t:

Maybe a py3 thing?

In D5749#84825, @durin42 wrote:

In D5749#84719, @mharbison72 wrote:

In D5749#84640, @indygreg wrote:

@durin42 and I were both able to produce intermittent failures with this change. Why, I'm not sure.

Does the Windows regression occur with this patch or with D5751? I ask because the behavior of socket.shutdown() is documented to vary by platform. I wouldn't at all be surprised if D5751 caused the regression seen here.

It was this patch.

Like I said, I’ve seen connection resets in the non-daemonized serve output (don’t recall if it was push, pull, or both). I thought it was weird, but there were no other signs of problems, so I wrote it off as some odd Windows thing. (Not sure that I’ve seen it on Linux or OS X too)

Does D5764 help the situation? It made a big difference for me.

Since the tag command's code is already looking at repo.tagtype(), I wonder if it would be better to just inline the global tag lookup and nullid comparison right there instead of updating the interface.

In D5749#84640, @indygreg wrote:

@durin42 and I were both able to produce intermittent failures with this change. Why, I'm not sure.

Does the Windows regression occur with this patch or with D5751? I ask because the behavior of socket.shutdown() is documented to vary by platform. I wouldn't at all be surprised if D5751 caused the regression seen here.

While this is better, it triggers a few failures on Windows. I've seen these connection resets with hg serve outside of the test harness. Any ideas? It almost seems like the client is closing the socket before all of the data is sent. (This failure occurs on py2)

In D5496#82908, @angel.ezquerra wrote:

In D5496#82671, @yuja wrote:

Do you mean that the flag should only indicate whether you want to hide the same branch merges? I guess that is OK too, since the main use case for this flag is to hide the merge from the same branch. However I think we should change the flag name then. Perhaps "hidesame"? Or "includesame" or "includeself", defaulting to True? Any ideas?

In D5495#82416, @angel.ezquerra wrote:

I've sent an updated set of patches, following your recommendations. There are 2 patches now, since each includes its own tests. This means that the 3rd patch on the original patch set is no longer needed. However I don't know what is the best way to tell that to phabricator...

This works for me on Windows in that narrow window where this was constantly happening, thanks!