From: Jim Porter <jporterbugs@gmail.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 72721@debbugs.gnu.org, gautier@gautierponsinet.xyz
Subject: bug#72721: 31.0.50; Visual-wrap-prefix-mode breaks Magit log buffers
Date: Tue, 20 Aug 2024 10:33:06 -0700 [thread overview]
Message-ID: <36584786-6af4-c59f-bb3e-f3459b2904be@gmail.com> (raw)
In-Reply-To: <86jzgbwgud.fsf@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]
On 8/20/2024 4:53 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Aug 2024 17:46:18 -0700
>> Cc: eliz@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> On 8/19/2024 2:39 PM, Gautier Ponsinet wrote:
>>> Hello everyone,
>>>
>>> The new visual-wrap-prefix-mode breaks the rendering of the Magit Log
>>> buffers.
>>>
>>> In emacs -Q:
>>> * Install Magit and its dependencies and load Magit.
>>> * Go to a local repository (via M-x dired or M-x cd).
>>> * M-x global-visual-wrap-prefix-mode
>>> * M-x magit-log-current
>>>
>>> Could someone please confirm/reproduce?
>>
>> I can confirm this. I'm not quite sure of all the details, but it seem
>> that this is due to a bad interaction between overlays and the
>> 'min-width' display spec. The end result was that we were calling
>> 'get-text-property' with a (large-ish) buffer position when the OBJECT
>> arg was a string of length 1. That can happen in magit-log on the
>> mostly-blank line where it's making the ASCII art just below a merge
>> commit. (The leading whitespace makes 'visual-wrap-prefix-mode' do its
>> thing.)
>>
>> I'm not super familiar with how the display engine works, but I think we
>> don't want to call 'display_min_width' when we're working with an
>> overlay. See the attached patch.
>
> I'd appreciate a reproducer without Magit, as I don't have it
> installed and would prefer not to have to.
Me too... I haven't been able to get a reduced test case yet since Magit
is pretty complex and I haven't figured out what it's doing exactly. It
*seems* to be due to overlays, but I only know that from examining
things in GDB. I haven't deciphered the relevant Magit code yet.
>> Eli, I'm sure you understand this code much better than me. Does the
>> above make sense? I can also try to improve the commentary in the code,
>> but I'm just making some educated guesses as to what's happening here.
>
> It looks like you are breaking min-width support for display strings?
> They are used on the mode line and also in other places, and in
> general, min-width should treat buffers and strings alike. Can you
> explain the motivation for the proposed changes, and describe what you
> saw with the current code in this case? Where's the call to
> get-text-property and why did it use a buffer position instead of a
> string position?
You're probably right. I think my patch was a little over-aggressive
(see attached for a more-surgical one). This patch may still be wrong,
but hopefully it gets a bit closer to what we want.
I think this is what's happening, in a bit more detail:
magit-log-current uses overlays (I think to set up the right margin
text?). When visual-wrap-prefix-mode ("vwpm") is enabled, the display
engine goes through the buffer, finds the 'min-width' display property
from vwpm and holds onto it. Next, it starts processing an overlay.
Eventually, that calls 'handle_display_prop' for the overlay which calls
'display_min_width'. At this point, we have an object stored in
'it->min_width_property' (thanks to vwpm), the local variable 'object'
is the overlay string, and 'bufpos' is the actual buffer position.
Finally we call 'get_display_property' with the bufpos and object (which
calls 'Fget_text_property'), and kaboom: 'object' is a string of length
1, but bufpos is much larger (~400 in my test).
I've also attached a backtrace, though I'm not sure how informative it
is on its own.
[-- Attachment #2: 0001-Fix-bad-interaction-between-min-width-display-spec-a.patch --]
[-- Type: text/plain, Size: 3421 bytes --]
From 13fe68ebc2eb2fe7ca0ee4ac4733b3abc3ed0cab Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 19 Aug 2024 17:38:47 -0700
Subject: [PATCH] Fix bad interaction between 'min-width' display spec and
overlays
Previously, when iterating over overlays, we would pass the overlay
string and the buffer position to 'display_min_width', which would use
those values to try to get the display property. However, the buffer
position is very likely out of bounds for the overlay string!
* src/xdisp.c (get_display_property): Rename BUFPOS to CHARPOS.
(display_min_width): Take CHARPOS instead of BUFPOS, and get BUFPOS on
our own. This way, we can be sure that when calling
'get_display_property', we provide it with the correct kind of position.
(handle_display_prop): Pass the character pos of OBJECT's position to
'display_min_width' (bug#72721).
---
src/xdisp.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/xdisp.c b/src/xdisp.c
index 30771a1c83d..af93a824bee 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5633,17 +5633,19 @@ find_display_property (Lisp_Object disp, Lisp_Object prop)
}
static Lisp_Object
-get_display_property (ptrdiff_t bufpos, Lisp_Object prop, Lisp_Object object)
+get_display_property (ptrdiff_t charpos, Lisp_Object prop, Lisp_Object object)
{
- return find_display_property (Fget_text_property (make_fixnum (bufpos),
+ return find_display_property (Fget_text_property (make_fixnum (charpos),
Qdisplay, object),
prop);
}
static void
-display_min_width (struct it *it, ptrdiff_t bufpos,
+display_min_width (struct it *it, ptrdiff_t charpos,
Lisp_Object object, Lisp_Object width_spec)
{
+ ptrdiff_t bufpos = CHARPOS (it->current.pos);
+
/* We're being called at the end of the `min-width' sequence,
probably. */
if (!NILP (it->min_width_property)
@@ -5658,9 +5660,9 @@ display_min_width (struct it *it, ptrdiff_t bufpos,
get_display_property (0, Qmin_width, object)))
/* In a buffer -- check that we're really right after the
sequence of characters covered by this `min-width'. */
- || (bufpos > BEGV
+ || (bufpos > BEGV && charpos > 0
&& EQ (it->min_width_property,
- get_display_property (bufpos - 1, Qmin_width, object))))
+ get_display_property (charpos - 1, Qmin_width, object))))
{
Lisp_Object w = Qnil;
double width;
@@ -5713,9 +5715,9 @@ display_min_width (struct it *it, ptrdiff_t bufpos,
&& !EQ (it->min_width_property,
get_display_property (0, Qmin_width, object)))
/* Buffer. */
- || (bufpos > BEGV
+ || (bufpos > BEGV && charpos > 0
&& !EQ (width_spec,
- get_display_property (bufpos - 1, Qmin_width, object))))
+ get_display_property (charpos - 1, Qmin_width, object))))
{
it->min_width_property = width_spec;
it->min_width_start = it->current_x;
@@ -5795,10 +5797,10 @@ handle_display_prop (struct it *it)
if (!STRINGP (it->string))
object = it->w->contents;
- /* Handle min-width ends. */
+ /* Handle min-width ends, except when processing an overlay. */
if (!NILP (it->min_width_property)
&& NILP (find_display_property (propval, Qmin_width)))
- display_min_width (it, bufpos, object, Qnil);
+ display_min_width (it, CHARPOS (*position), object, Qnil);
if (NILP (propval))
return HANDLED_NORMALLY;
--
2.25.1
[-- Attachment #3: backtrace.txt --]
[-- Type: text/plain, Size: 2987 bytes --]
#0 raise (sig=sig@entry=5) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00005555555e78aa in display_min_width
(it=it@entry=0x7fffffff7500, bufpos=bufpos@entry=440, object=object@entry=0x5555572a2f14, width_spec=width_spec@entry=0x0) at ../../src/xdisp.c:5653
#2 0x00005555555e9fe0 in handle_display_prop (it=0x7fffffff7500)
at ../../src/lisp.h:1178
#3 0x00005555555e2f85 in handle_stop (it=0x7fffffff7500)
at ../../src/xdisp.c:4162
#4 0x00005555555f0998 in next_element_from_buffer (it=0x7fffffff7500)
at ../../src/xdisp.c:9701
#5 0x00005555555ee675 in get_next_display_element (it=it@entry=0x7fffffff7500)
at ../../src/xdisp.c:8267
#6 0x00005555555f4ea8 in display_line
(it=0x7fffffff7500, cursor_vpos=<optimized out>) at ../../src/xdisp.c:25395
#7 0x00005555555f98f1 in try_window
(window=window@entry=0x5555572e1ecd, pos=..., flags=flags@entry=1)
at ../../src/xdisp.c:21214
#8 0x0000555555618790 in redisplay_window
(window=0x5555572e1ecd, just_this_one_p=<optimized out>)
at ../../src/xdisp.c:20594
#9 0x000055555561ade3 in redisplay_window_0
(window=window@entry=0x5555572e1ecd) at ../../src/xdisp.c:18077
#10 0x00005555557676fc in internal_condition_case_1
(bfun=bfun@entry=0x55555561adb0 <redisplay_window_0>, arg=arg@entry=0x5555572e1ecd, handlers=<optimized out>, hfun=hfun@entry=0x5555555d2380 <redisplay_window_error>) at ../../src/eval.c:1622
#11 0x00005555555ceff9 in redisplay_windows (window=0x5555572e1ecd)
at ../../src/xdisp.c:18046
#12 0x00005555555cf01d in redisplay_windows (window=0x5555572e1cad)
at ../../src/xdisp.c:18040
#13 0x00005555556019c6 in redisplay_internal () at ../../src/xdisp.c:17445
#14 0x00005555556eb5b6 in read_char
(commandflag=1, map=0x7fffe74a7ad3, prev_event=0x0, used_mouse_menu=0x7fffffffdd8b, end_time=0x0) at ../../src/keyboard.c:2673
#15 0x00005555556eded5 in read_key_sequence
(keybuf=0x7fffffffdee0, prompt=0x0, dont_downcase_last=<optimized out>, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=<optimized out>, disable_text_conversion_p=false) at ../../src/keyboard.c:10747
#16 0x00005555556efccf in command_loop_1 () at ../../src/lisp.h:1178
#17 0x0000555555767667 in internal_condition_case
(bfun=bfun@entry=0x5555556efb00 <command_loop_1>, handlers=handlers@entry=0x90, hfun=hfun@entry=0x5555556e2f90 <cmd_error>) at ../../src/eval.c:1598
#18 0x00005555556db6ea in command_loop_2 (handlers=handlers@entry=0x90)
at ../../src/keyboard.c:1163
#19 0x0000555555767559 in internal_catch
(tag=tag@entry=0x12450, func=func@entry=0x5555556db6c0 <command_loop_2>, arg=arg@entry=0x90) at ../../src/eval.c:1277
#20 0x00005555556db686 in command_loop () at ../../src/lisp.h:1178
#21 0x00005555556e2ae7 in recursive_edit_1 () at ../../src/keyboard.c:749
#22 0x00005555556e2ea4 in Frecursive_edit () at ../../src/keyboard.c:832
#23 0x00005555555b28c3 in main (argc=<optimized out>, argv=<optimized out>)
at ../../src/emacs.c:2624
next prev parent reply other threads:[~2024-08-20 17:33 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-19 21:39 bug#72721: 31.0.50; Visual-wrap-prefix-mode breaks Magit log buffers Gautier Ponsinet
2024-08-20 0:46 ` Jim Porter
2024-08-20 11:53 ` Eli Zaretskii
2024-08-20 17:33 ` Jim Porter [this message]
2024-08-20 19:01 ` Eli Zaretskii
2024-08-21 3:15 ` Jim Porter
2024-08-21 5:18 ` Jim Porter
2024-08-21 19:12 ` Jim Porter
2024-08-22 12:54 ` Eli Zaretskii
2024-08-22 9:59 ` Eli Zaretskii
2024-08-22 9:53 ` Eli Zaretskii
2024-08-22 16:19 ` Jim Porter
2024-08-25 7:29 ` Eli Zaretskii
2024-08-25 16:26 ` Jim Porter
2024-08-25 17:39 ` Eli Zaretskii
2024-08-25 18:41 ` Jim Porter
2024-08-29 11:47 ` Eli Zaretskii
2024-08-30 5:01 ` Jim Porter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=36584786-6af4-c59f-bb3e-f3459b2904be@gmail.com \
--to=jporterbugs@gmail.com \
--cc=72721@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=gautier@gautierponsinet.xyz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.