This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11000

windows: add windows behavior on broken pager
ClosedPublic

Authored by Alphare on Jul 7 2021, 9:14 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG9aad229a773a: windows: add windows behavior on broken pager
Summary

Apparently, Windows has "better" behavior than Unix in this case. This is an
edge case that led me down a rabbit hole, only to find a bug in the Python
documentation... I am not planning on trying to reproduce the same behavior
on Unix systems since it's not really useful, but other people are welcome to!

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jul 7 2021, 9:14 AM
marmoute accepted this revision.Jul 7 2021, 11:57 AM
mharbison72 added a subscriber: mharbison72.Jul 8 2021, 12:01 PM

Is there any more context you can/want to supply about the bug, what's better about Windows, the rabbit hole, etc?

marmoute added a comment.Jul 8 2021, 12:02 PM
In D11000#168698, @mharbison72 wrote:

Is there any more context you can/want to supply about the bug, what's better about Windows, the rabbit hole, etc?

My understanding (@Alphare can correct me) is that windows do -not- have the bug. The test is showing and documenting a bug. And This works fine on Windows, so I don't think we need extra documentation.

Alphare added a comment.Jul 9 2021, 4:40 AM
In D11000#168698, @mharbison72 wrote:

Is there any more context you can/want to supply about the bug, what's better about Windows, the rabbit hole, etc?

Sure. The expected behavior is apparently to not paginate if the pager is broken, but still show the contents. I didn't understand that going in, so I was trying to figure out why Windows' behavior was differing from Linux'.
The pager code makes use of os.dup, whose documentation says "On Windows, when duplicating a standard stream (0: stdin, 1: stdout, 2: stderr), the new file descriptor is inheritable.", which is not true (at least in 3.9) as os.get_inheritable(os.dup(1)) evaluates to False. This is made worse by the fact that os.dup and its sibiling os.dup2 work differently on Python 2 and 3... but also in different versions of Python 3 because of bugfixes, which should explain why the doc is wrong (see https://bugs.python.org/issue37267 and related bugs). I had help from Victor Stinner who initially championed the Python 3 changes.

But turns out this had nothing to do with the problem since Windows "behaves properly", maybe by accident?

I hope that makes sense.

mharbison72 added inline comments.Jul 9 2021, 11:08 AM
tests/test-pager.t
223

I have no idea why, but now this output is being removed on Windows with py3.8.1 and py3.9.0. I *know* I was seeing this output on this system before, but I can't figure out what's changed. I'm running this on top of 57bdecf4322c.

baymax updated this revision to Diff 29073.Jul 9 2021, 9:21 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 29093.Jul 9 2021, 11:41 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare added inline comments.Jul 13 2021, 11:42 AM
tests/test-pager.t
223

It appears that this is flaky somehow, but I've had many runs with the text appearing. We can deal with the flakyness later I think, once we're sure about how frequent it is.

Alphare added a commit: rHG9aad229a773a: windows: add windows behavior on broken pager.Jul 16 2021, 1:04 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG9aad229a773a: windows: add windows behavior on broken pager (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtests/test-pager.t (24 lines)

Diff 29297

tests/test-pager.t

\x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc) \x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc)
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: modify a 8 summary: modify a 8
#endif #endif
A complicated pager command gets worse behavior. Bonus points if you can A complicated pager command gets worse behavior. Bonus points if you can
improve this. improve this. Windows apparently does this better?
#if windows
mharbison72Unsubmitted
Not Done

I have no idea why, but now this output is being removed on Windows with py3.8.1 and py3.9.0. I *know* I was seeing this output on this system before, but I can't figure out what's changed. I'm running this on top of 57bdecf4322c.

mharbison72: I have no idea why, but now this output is being removed on Windows with py3.8.1 and py3.9.0.
AlphareAuthorUnsubmitted
Done

It appears that this is flaky somehow, but I've had many runs with the text appearing. We can deal with the flakyness later I think, once we're sure about how frequent it is.

Alphare: It appears that this is flaky somehow, but I've had many runs with the text appearing. We can…
$ hg log --limit 3 \ $ hg log --limit 3 \
> --config pager.pager='this-command-better-never-exist --seriously' \ > --config pager.pager='this-command-better-never-exist --seriously' \
> 2>/dev/null || true > 2>/dev/null || true
\x1b[0;33mchangeset: 10:46106edeeb38\x1b[0m (esc)
tag: tip
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: modify a 10
\x1b[0;33mchangeset: 9:6dd8ea7dd621\x1b[0m (esc)
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: modify a 9
\x1b[0;33mchangeset: 8:cff05a6312fe\x1b[0m (esc)
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: modify a 8
#else
$ hg log --limit 3 \
> --config pager.pager='this-command-better-never-exist --seriously' \
> 2>/dev/null || true
#endif
Pager works with shell aliases. Pager works with shell aliases.
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [alias] > [alias]
> echoa = !echo a > echoa = !echo a
> EOF > EOF