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 >> >> 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.