I noticed inconsistencies with this too. I was wondering if we should populate __index__ with the full help text to avoid that. (The other inconsistency that comes to mind is printing out the disabled extension list in hg help extensions.) While it is apparently only used for py2exe based stuff, it might be handy with PyOxidizer too, until that gains the ability to enumerate and read embedded *.py files and directories as resources.

I suspect the Windows process code in Python doesn't accept bytes and the POSIX code does?

In D8337#124542, @yuja wrote:

On Windows, my assumption was os.fsencode() == .encode("mbcs") if
sys._enablelegacywindowsfsencoding(). So this looks good to me.
Perhaps, the "ignore" error mode would match the legacy Windows behavior.

This looks like it also fixes the phabricator test on Windows, which diverged by creating a different hash for the create alpha for phabricator test € commit.

The test harness *should* match existing \n output as a fallback, which got me to wondering if it was the (esc) at the end screwing it up. I tried this patch:

In D8311#124447, @mharbison72 wrote:

In D8311#124387, @Alphare wrote:

Could you add a comment about what happens if you hg split either the base or the tip of the range?

I think what happens is each newnode key maps to a single precursor, so it's like a regular amend case. It just happens that multiple newnode keys have the same oldnode in their value tuple. But all we care about is the first and last newnode in the range.
That said, I tried creating a simple test where I amended in a new file to the last commit of the last test, split it with the internal extension, and... Somehow I ended up with a weird state where the first half of the split is pruned...

In D8311#124387, @Alphare wrote:

Could you add a comment about what happens if you hg split either the base or the tip of the range?

Duplicate of 0424a9134bcf

In D8306#124380, @Alphare wrote:

I am very curious as to how the UI will look on a folded series.

This was a test run of phabsend --fold to show what the UI looks like. It's a fold of D8317 and D8318 (with the URLs removed).

In D8304#124121, @marmoute wrote:

Great, @mharbison72 can you submit a patch ?

In D6846#124054, @marmoute wrote:

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

In D8281#124246, @spectral wrote:

• I'm *not* expecting to do anything about \ -> / conversion.

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.

In D8232#124097, @yuja wrote:

+def _getdrevs(ui, stack, *specs):
+ """convert user supplied DREVSPECs into "Differential Revision" dicts
+
+ See `hg help phabread` for how to specify each DREVSPEC.
+ """
+ if len(*specs) > 0:

^^^^^^

Fixed bad argument expansion since I had to rebase this. Please let me
know if that's wrong.

Never mind. Maybe specs is a list containing a single list, in which case,
the code is valid.

In D8304#124109, @marmoute wrote:

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

This is needed on stable to fix the build.

Thanks, I just hit this myself.

In D8284#123877, @martinvonz wrote:

In D8284#123873, @marmoute wrote:

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.

If the intend is to inform, maybe we could issue a warning?

Makes sense. I suppose we should do the same for --all and --rev REV in that case. We currently consider that an error. I don't care much about this issue, so I'll just move this patch to the side for now. That way the rest of the stack (which I care about) can be queued independently.

In D6735#123875, @khanchi97 wrote:

In D6735#100642, @mharbison72 wrote:

Here's a case I stumbled upon that is a problem. It looks like it thinks it isn't in the middle of an update, but .hgsubstate isn't put back to the pre-update state.

diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -1027,10 +1027,38 @@ filesystem (see also issue4583))
   > [extensions]
   > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py
   > EOF
+  $ hg -R repo st -S
+  ? s/b
+  $ hg -R repo diff -S
+  $ hg -R repo log -r .
+  changeset:   0:c4018e9aea1b
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     1
+
+  $ cat repo/.hgsubstate
+  f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
   $ hg -R repo update
   b: untracked file differs
   abort: untracked files in working directory differ from files in requested revision (in subrepository "s")

@mharbison72 I am bit confused here.

1. Since hg -R repo update resulted with an abort then why we have dirty working directory now? Isn't transaction rollback worked correctly?

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.

Thanks for this!

In D8232#123673, @Alphare wrote:

This seems like something that can easily have a few simple unit tests

In D8281#123625, @spectral wrote:

In D8281#123621, @mharbison72 wrote:

The Windows path changes seem like a good idea.
Would quoting paths with commas eliminate the need for custom escaping? I don't feel strongly about it, but custom escaping always feels weird to me. (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)

Let me play with that a bit, I think it'll work and be detectable on the server since the first character can't currently be a double-quote, so there wouldn't really be any BC issues aside from the pconvert (which wouldn't be as important anymore, but still probably a good idea?)

The Windows path changes seem like a good idea.

It looks like test-run-tests.py and i18n/check-translation.py have a test too. (Not sure if we care about them; I mostly care about Windows.)

In D8189#123526, @marmoute wrote:

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.

Do you have a link to the bug ?

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?

In D7631#123318, @mjacob wrote:

I like the name --from for the option. It would also make sense in combination with a possible future --into option.