I missed the debug one too, sorry. (Although I’m not sure why that also isn’t devel., so maybe it isn’t a big deal. Curious what others think.)

Should we hide devel.* like we do experimental when verbose isn’t applied? Presumably we don’t want regular users to know about them.

Maybe I’m looking at something wrong, but it looks like a couple references to “the shelve” snuck in after the last edit in diff 16017.

In D6643#97250, @valentin.gatienbaron wrote:

In D6643#97247, @mharbison72 wrote:

I ran this on the repo that was giving me fits last summer, and it stayed consistent up until just after the point where there was an octopus fixup merge. I suspect the logic in that part of convert is too loose (i.e. it should be dropping files in the fixup merge that were already in the first merge if there weren't changes in the 3rd branch), but wasn't able to write a simple test. (The original repo was in bzr, converted to hg, and then I had to reconvert to lfs.) I've been swamped at work, so I don't have time to dig into it. But no sense in holding this up, as there doesn't seem to be a regression here.

I didn't quite understand. Are you saying your convert call cause hashes to change after this octopus merge, but it's not caused by my change, i.e. hg behaves this way even before?

In D6613#97136, @valentin.gatienbaron wrote:

@mharbison72 I dealt with the problem of convert in the previous commit. I didn't add a config option to disable the new code path, because it's really awkward to document, and I think it would be annoying to use if I made more fixes (the other case where files are spuriously listed, involving exec bits, probably has little impact on repo size/pull performance but it may be worth fixing anyway because it causes confusion). But if there are downsides to what I did, the config option is still on the table.

The update worked for me, thanks.

Don't take this as a complete list, but it looks like repository.py, remotefilelog, eol, and probably keyword need to be taught the new kwarg. Note that the keyword wrapper is called kwcommitctx, so it isn't enough to grep for def commitctx.

In D6613#96822, @valentin.gatienbaron wrote:

Ok. Maybe it would be simpler or more robust to do the direct thing: optionally treat the files list in the input commit as input and reuse them blindly in the resulting commit, when doing a hg->hg conversion without filemap.

In D6613#96749, @valentin.gatienbaron wrote:

In D6613#96743, @mharbison72 wrote:

If this goes forward, can this be gated by a config option (even if it is on by default)? IIUC, this will change the hash for merges, and then cascade. That seems problematic for convert operations.
Most converts will do things that change hashes anyway, but the default options don't change anything. This is a convenient behavior for easily migrating to/from LFS, for example. (In theory, anyway. In practice, I've seen repos where a convert alters the changelog.) If there is a config option, convert should probably disable it by default to maintain the current behavior. I can see a use case for enabling it though, to clean up the repo.

Given the implementation, it's easy to add a knob to disable the change. But that doesn't seem enough: If you have a repository where commits have been created with a mix of hg before and after this change, there is no single value of the knob that would make convert be the identity.

If this goes forward, can this be gated by a config option (even if it is on by default)? IIUC, this will change the hash for merges, and then cascade. That seems problematic for convert operations.

In D6490#95686, @khanchi97 wrote:

In D6490#95355, @markand wrote:

In D6490#95328, @mharbison72 wrote:

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

1. maybe a more specific flag name like --allow-close-branch or something like that
2. a config option instead of a flag name

Fair point. Specific is better.
I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

Just my $0.02.
I think that hg commit already has many options that do not even belong to committing (such as -A, --close-branch). Let's not add an option that is not related to commit. To my opinion closing a branch should be done with hg branch not commit (unfortunately this was never the case). Anyway if you really want to do it with commit, I would suggest that --close-branch takes an optional argument rather than adding a new option. This makes UX a bit more convenient
e.g.

hg commit --close-branch --force-close-branch (not that)
hg commit --close-branch forced (or force or something like that)

I think it's a little hard to make user understand (by providing a hint like "use force arg with --close-branch") that you need to provide an argument as compared to just providing a standalone flag --force-close-branch as @mharbison72 suggested. I do agree with you that it would make +1 in the no. of flags which don't make sense with hg commit. What do you say?

I worked around the same bug in Windows in platform.posixfile [1]. Should this be done in the posix layer (which is currently only an alias to open())? It looks like there are uses of posixfile outside of vfs.

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

1. maybe a more specific flag name like --allow-close-branch or something like that
2. a config option instead of a flag name

This needs some fixups to keep test-help.t and the Windows installer happy:

LGTM. Thanks for doing this.

Minor nit that I don't care too much about, but I'm wondering what others think of not-head changeset vs non-head changeset. The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.

In D6355#92614, @mharbison72 wrote:

Should this abort if subrepos are present, since you can't shelve a subrepo? No -S is available on update, but maybe looking for .hgsub with any contents is good enough?

Should this abort if subrepos are present, since you can't shelve a subrepo? No -S is available on update, but maybe looking for .hgsub with any contents is good enough?

In D5981#92414, @martinvonz wrote:

I had forgotten about this patch and it's not very important anyway. I don't know how I'm supposed to fix it, so I'll just drop. @mharbison72, let me know if there was something else I should have done.

This might break --pure without --local in the annotate tests. No idea if that's a valid combination, but the buildbots (mostly) use that. In fairness, it seems that this combination had an error where _filecommit() was given too many arguments in the direct ancestors, so maybe the real breakage occurred in there. But there seems to be extra output here.

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: