[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#1035354: marked as done (unblock: fish/3.6.0-3.1)



Your message dated Sun, 07 May 2023 07:48:52 +0000
with message-id <E1pvZ8W-009eWi-FI@respighi.debian.org>
and subject line unblock fish
has caused the Debian Bug report #1035354,
regarding unblock: fish/3.6.0-3.1
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
1035354: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1035354
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message ---
Package: release.debian.org
Severity: normal
User: release.debian.org@packages.debian.org
Usertags: unblock
X-Debbugs-Cc: fish@packages.debian.org, Mo Zhou <lumin@debian.org>
Control: affects -1 + src:fish

Please unblock package fish

As described in #1000351, mc ships fragile prompt extraction code which
was broken by a change in fish 3.3.0, leaving fish unusable in
conjunction with mc. I had hoped that this bug would be fixed in mc by
the time of bookworm release, but this didn’t happen. Instead, the
upstream developers of fish proposed a workaround and shipped it
in the bugfix release 3.6.1.

I intend to either upload an NMU or let Mo Zhou do a maintainer’s
upload.

This fix has very limited impact, as it specifically checks for the
presence of an mc-specific environment variable, and is a no-op
otherwise. The workaround itself is also straightforward.

The impact of not shipping this patch is that all users of both fish and
mc (like myself) will have to put fish on hold and stay on the version
shipped in bullseye.

[ Checklist ]
  [x] all changes are documented in the d/changelog
  [x] I reviewed all changes and I approve them
  [x] attach debdiff against the package in testing


Links:

[1]: https://bugs.debian.org/1035353: the original mc bug
[2]: https://bugs.debian.org/1000351: a clone of the above for the
     package fish
[3]: https://github.com/fish-shell/fish-shell/pull/9540: a pull request
     in the upstream package.

unblock fish/3.6.0-3.1
diff --git a/debian/changelog b/debian/changelog
index 51eef54d7c12..8e24670893ef 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+fish (3.6.0-3.1) unstable; urgency=medium
+
+  * Non-maintainer upload.
+  * Apply an upstream workaround for Midnight Commander's issue with prompt.
+    See https://github.com/fish-shell/fish-shell/pull/9540 and #1000351.
+
+ -- Andrej Shadura <andrewsh@debian.org>  Mon, 01 May 2023 19:01:01 +0200
+
 fish (3.6.0-3) unstable; urgency=medium
 
   * Cherry-pick upstream fixes from the v3.6.1 branch.
diff --git a/debian/patches/0003-workaround-for-Midnight-Commander.patch b/debian/patches/0003-workaround-for-Midnight-Commander.patch
new file mode 100644
index 000000000000..a744b9498963
--- /dev/null
+++ b/debian/patches/0003-workaround-for-Midnight-Commander.patch
@@ -0,0 +1,91 @@
+From d9f08d1e12a6d4dc66fdabe576bc79feb499abfd Mon Sep 17 00:00:00 2001
+From: Fabian Boehm <FHomborg@gmail.com>
+Date: Sat, 4 Feb 2023 18:57:41 +0100
+Subject: Add workaround for Midnight Commander's issue with prompt extraction
+
+When we draw the prompt, we move the cursor to the actual
+position *we* think it is by issuing a carriage return (via
+`move(0,0)`), and then going forward until we hit the spot.
+
+This helps when the terminal and fish disagree on the width of the
+prompt, because we are now definitely in the correct place, so we can
+only overwrite a bit of the prompt (if it renders longer than we
+expected) or leave space after the prompt. Both of these are benign in
+comparison to staircase effects we would otherwise get.
+
+Unfortunately, midnight commander ("mc") tries to extract the last
+line of the prompt, and does so in a way that is overly naive - it
+resets everything to 0 when it sees a `\r`, and doesn't account for
+cursor movement. In effect it's playing a terminal, but not committing
+to the bit.
+
+Since this has been an open request in mc for quite a while, we hack
+around it, by checking the $MC_SID environment variable.
+
+If we see it, we skip the clearing. We end up most likely doing
+relative movement from where we think we are, and in most cases it
+should be *fine*.
+
+Origin: upstream, https://github.com/fish-shell/fish-shell/pull/9540
+---
+
+diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp
+index 28282f13ebd..9c3882ce02e 100644
+--- a/src/env_dispatch.cpp
++++ b/src/env_dispatch.cpp
+@@ -484,6 +484,14 @@ static void initialize_curses_using_fallbacks(const environment_t &vars) {
+ // Apply any platform-specific hacks to cur_term/
+ static void apply_term_hacks(const environment_t &vars) {
+     UNUSED(vars);
++    // Midnight Commander tries to extract the last line of the prompt,
++    // and does so in a way that is broken if you do `\r` after it,
++    // like we normally do.
++    // See https://midnight-commander.org/ticket/4258.
++    if (auto var = vars.get(L"MC_SID")) {
++        screen_set_midnight_commander_hack();
++    }
++
+     // Be careful, variables like "enter_italics_mode" are #defined to dereference through cur_term.
+     // See #8876.
+     if (!cur_term) {
+diff --git a/src/screen.cpp b/src/screen.cpp
+index b6e1ea8c38a..ef8fbf16ff3 100644
+--- a/src/screen.cpp
++++ b/src/screen.cpp
+@@ -75,6 +75,12 @@ static size_t try_sequence(const char *seq, const wchar_t *str) {
+     return 0;  // this should never be executed
+ }
+
++static bool midnight_commander_hack = false;
++
++void screen_set_midnight_commander_hack() {
++    midnight_commander_hack = true;
++}
++
+ /// Returns the number of columns left until the next tab stop, given the current cursor position.
+ static size_t next_tab_stop(size_t current_line_width) {
+     // Assume tab stops every 8 characters if undefined.
+@@ -905,7 +911,11 @@ void screen_t::update(const wcstring &left_prompt, const wcstring &right_prompt,
+
+     // Also move the cursor to the beginning of the line here,
+     // in case we're wrong about the width anywhere.
+-    this->move(0, 0);
++    // Don't do it when running in midnight_commander because of
++    // https://midnight-commander.org/ticket/4258.
++    if (!midnight_commander_hack) {
++        this->move(0, 0);
++    }
+
+     // Clear remaining lines (if any) if we haven't cleared the screen.
+     if (!has_cleared_screen && need_clear_screen && clr_eol) {
+diff --git a/src/screen.h b/src/screen.h
+index 9c90fa44af8..26bbb452b62 100644
+--- a/src/screen.h
++++ b/src/screen.h
+@@ -330,4 +330,6 @@ class layout_cache_t : noncopyable_t {
+ };
+
+ maybe_t<size_t> escape_code_length(const wchar_t *code);
++
++void screen_set_midnight_commander_hack();
+ #endif
diff --git a/debian/patches/series b/debian/patches/series
index cfda4f9b28f2..09cc9a1baab2 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
 0001-reader-make-Escape-during-history-search-restore-com.patch
 0002-reader-Remove-assert-in-history-search.patch
+0003-workaround-for-Midnight-Commander.patch
>From d9f08d1e12a6d4dc66fdabe576bc79feb499abfd Mon Sep 17 00:00:00 2001
From: Fabian Boehm <FHomborg@gmail.com>
Date: Sat, 4 Feb 2023 18:57:41 +0100
Subject: [PATCH] Add workaround for Midnight Commander's issue with prompt
 extraction

When we draw the prompt, we move the cursor to the actual
position *we* think it is by issuing a carriage return (via
`move(0,0)`), and then going forward until we hit the spot.

This helps when the terminal and fish disagree on the width of the
prompt, because we are now definitely in the correct place, so we can
only overwrite a bit of the prompt (if it renders longer than we
expected) or leave space after the prompt. Both of these are benign in
comparison to staircase effects we would otherwise get.

Unfortunately, midnight commander ("mc") tries to extract the last
line of the prompt, and does so in a way that is overly naive - it
resets everything to 0 when it sees a `\r`, and doesn't account for
cursor movement. In effect it's playing a terminal, but not committing
to the bit.

Since this has been an open request in mc for quite a while, we hack
around it, by checking the $MC_SID environment variable.

If we see it, we skip the clearing. We end up most likely doing
relative movement from where we think we are, and in most cases it
should be *fine*.
---
 src/env_dispatch.cpp |  8 ++++++++
 src/screen.cpp       | 12 +++++++++++-
 src/screen.h         |  2 ++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp
index 28282f13ebd..9c3882ce02e 100644
--- a/src/env_dispatch.cpp
+++ b/src/env_dispatch.cpp
@@ -484,6 +484,14 @@ static void initialize_curses_using_fallbacks(const environment_t &vars) {
 // Apply any platform-specific hacks to cur_term/
 static void apply_term_hacks(const environment_t &vars) {
     UNUSED(vars);
+    // Midnight Commander tries to extract the last line of the prompt,
+    // and does so in a way that is broken if you do `\r` after it,
+    // like we normally do.
+    // See https://midnight-commander.org/ticket/4258.
+    if (auto var = vars.get(L"MC_SID")) {
+        screen_set_midnight_commander_hack();
+    }
+
     // Be careful, variables like "enter_italics_mode" are #defined to dereference through cur_term.
     // See #8876.
     if (!cur_term) {
diff --git a/src/screen.cpp b/src/screen.cpp
index b6e1ea8c38a..ef8fbf16ff3 100644
--- a/src/screen.cpp
+++ b/src/screen.cpp
@@ -75,6 +75,12 @@ static size_t try_sequence(const char *seq, const wchar_t *str) {
     return 0;  // this should never be executed
 }
 
+static bool midnight_commander_hack = false;
+
+void screen_set_midnight_commander_hack() {
+    midnight_commander_hack = true;
+}
+
 /// Returns the number of columns left until the next tab stop, given the current cursor position.
 static size_t next_tab_stop(size_t current_line_width) {
     // Assume tab stops every 8 characters if undefined.
@@ -905,7 +911,11 @@ void screen_t::update(const wcstring &left_prompt, const wcstring &right_prompt,
 
     // Also move the cursor to the beginning of the line here,
     // in case we're wrong about the width anywhere.
-    this->move(0, 0);
+    // Don't do it when running in midnight_commander because of
+    // https://midnight-commander.org/ticket/4258.
+    if (!midnight_commander_hack) {
+        this->move(0, 0);
+    }
 
     // Clear remaining lines (if any) if we haven't cleared the screen.
     if (!has_cleared_screen && need_clear_screen && clr_eol) {
diff --git a/src/screen.h b/src/screen.h
index 9c90fa44af8..26bbb452b62 100644
--- a/src/screen.h
+++ b/src/screen.h
@@ -330,4 +330,6 @@ class layout_cache_t : noncopyable_t {
 };
 
 maybe_t<size_t> escape_code_length(const wchar_t *code);
+
+void screen_set_midnight_commander_hack();
 #endif

--- End Message ---
--- Begin Message ---
Unblocked.

--- End Message ---

Reply to: