tpope / vim-fugitive

fugitive.vim: A Git wrapper so awesome, it should be illegal
https://www.vim.org/scripts/script.php?script_id=2975
20.01k stars 1.01k forks source link

`:Git <any-command> --help` mishandles formatting #2300

Closed finite-state-machine closed 5 months ago

finite-state-machine commented 5 months ago

Running e.g. :Git commit --help displays the relevant man page in a new window (as expected), but various formatting is mishandled, as shown in the following screenshot: Screenshot 2024-05-09 at 19 27 43

IMO it would be more helpful to show a correctly formatted man page, but it would also be reasonable and preferable to suspend VIm and run the man process in the foreground.

My quick read [edit: confirmed by an answer on StackOverflow]: the manpage viewer is using ^H (backspace) to double-strike some characters for emphasis [edit: and overlay underscores with characters to simulate underlining] as might make sense on e.g. a dot matrix printer. For example, the sequence:

D^HDE^HES^HSC^HCR^HRI^HIP^HPT^HTI^HIO^HON^HN

...prints D, moves the carriage back one character, then prints DE, and so on. This ultimately renders the word DESCRIPTION by double-striking each non-control character. The slight misalignment between the first strike and the second (no printer is that precise) would result in a bold rendition even on a plain-text printer.

Debugging observations and platform details - VIm recognizes the document as a man page (`set ft?` → `ft=man`) - I'm running on macOS 14 ("Sonoma") with Fugitive `0f693bf` - `man` and `less` are supplied by the OS (at least, when invoked from the command-line) - `man` seems to have no version-reporting facility - `less` reports its version (omitting the warranty statement) as ``` less 643 (PCRE2 regular expressions) Copyright (C) 1984-2023 Mark Nudelman ``` - VIm is installed via homebrew: ``` VIM - Vi IMproved 9.1 (2024 Jan 02, compiled Apr 18 2024 20:05:12) macOS version - arm64 Included patches: 1-350 Compiled by Homebrew Huge version without GUI. Features included (+) or not (-): +acl +file_in_path +mouse_urxvt -tag_any_white +arabic +find_in_path +mouse_xterm -tcl +autocmd +float +multi_byte +termguicolors +autochdir +folding +multi_lang +terminal -autoservername -footer -mzscheme +terminfo -balloon_eval +fork() +netbeans_intg +termresponse +balloon_eval_term +gettext +num64 +textobjects -browse -hangul_input +packages +textprop ++builtin_terms +iconv +path_extra +timers +byte_offset +insert_expand +perl +title +channel +ipv6 +persistent_undo -toolbar +cindent +job +popupwin +user_commands -clientserver +jumplist +postscript +vartabs +clipboard +keymap +printer +vertsplit +cmdline_compl +lambda +profile +vim9script +cmdline_hist +langmap -python +viminfo +cmdline_info +libcall +python3 +virtualedit +comments +linebreak +quickfix +visual +conceal +lispindent +reltime +visualextra +cryptv +listcmds +rightleft +vreplace +cscope +localmap +ruby +wildignore +cursorbind +lua +scrollbind +wildmenu +cursorshape +menu +signs +windows +dialog_con +mksession +smartindent +writebackup +diff +modify_fname +sodium -X11 +digraphs +mouse +sound -xattr -dnd -mouseshape +spell -xfontset -ebcdic +mouse_dec +startuptime -xim +emacs_tags -mouse_gpm +statusline -xpm +eval -mouse_jsbterm -sun_workshop -xsmp +ex_extra +mouse_netterm +syntax -xterm_clipboard +extra_search +mouse_sgr +tag_binary -xterm_save -farsi -mouse_sysmouse -tag_old_static system vimrc file: "$VIM/vimrc" user vimrc file: "$HOME/.vimrc" 2nd user vimrc file: "~/.vim/vimrc" 3rd user vimrc file: "~/.config/vim/vimrc" user exrc file: "$HOME/.exrc" defaults file: "$VIMRUNTIME/defaults.vim" fall-back for $VIM: "/opt/homebrew/share/vim" Compilation: clang -c -I. -Iproto -DHAVE_CONFIG_H -DMACOS_X -DMACOS_X_DARWIN -g -O2 -D_REENTRANT -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 Linking: clang -o vim -lm -lncurses -lsodium -liconv -lintl -framework AppKit -L/opt/homebrew/opt/lua/lib -llua5.4 -mmacosx-version-min=14.2 -fstack-protector-strong -L/opt/homebrew/opt/perl/lib/perl5/5.38/darwin-thread-multi-2level/CORE -lperl -L/opt/homebrew/opt/python@3.12/Frameworks/Python.framework/Versions/3.12/lib/python3.12/config-3.12-darwin -lpython3.12 -framework CoreFoundation -lruby.3.3 -L/opt/homebrew/Cellar/ruby/3.3.0/lib ```
finite-state-machine commented 5 months ago

Relevant, and perhaps helpful: https://stackoverflow.com/questions/60106514/

tpope commented 5 months ago

This works fine on Linux, and I could have sworn it used to work fine on macOS. I wonder if something changed.

https://unix.stackexchange.com/questions/13267/how-to-disable-styling-on-man-pages demonstrates the behavior I expect: ASCII style formatting is disabled when TERM=dumb. Making Fugitive responsible for cleaning it up is inconvenient, to say the least.

finite-state-machine commented 5 months ago

I agree it doesn't make sense for Fugitive to mess around with control bytes.

I can see three possible fixes:

Obviously some of this doesn't make sense on Windows or other non-POSIXy platforms.

The wrapper script might look like this:

#!/usr/bin/env sh

TERM=dumb /path/to/real/man $@

(My shell script is rusty; take this with a grain of salt.)

I'm willing to help in principle, but I'm not experienced with VIm scripting.

tpope commented 5 months ago

I agree it doesn't make sense for Fugitive to mess around with control bytes.

I can see three possible fixes:

  • [always] set TERM=dumb or pipe through col -b, if this doesn't cause other problems

We do always set TERM=dumb. The problem is that on macOS, TERM=dumb is ignored.

Always setting col -b feels like a bad idea. For other commands, I don't want to be perpetually unsure if the output is mangled or not.

  • [always] wrap man in a shell script, put the wrapper in a temp dir, and put the temp dir at the front of the $PATH when invoking git

This is probably the least worst option. Inconvenient but not impossible. There's also man.man.path that could be pointed at the wrapper script to avoid getting PATH involved.

  • [conditional] detect --help (but presumably not after --) and conditionally use TERM or col

We already detect --help, so that part's fine. But conditionally using col means we can't just directly execute git, we need to involve the shell. This is more disruptive than the last one.

Obviously some of this doesn't make sense on Windows or other non-POSIXy platforms.

This complicates the implementation, and is part of the "inconvenient", but not a deal breaker.

The wrapper script might look like this:

#!/usr/bin/env sh

TERM=dumb /path/to/real/man $@

What is /path/to/real/man here? If there's another man we could call that does the right thing, we could do away with the wrapper script entirely, and it gets a lot less annoying.

finite-state-machine commented 5 months ago

What is /path/to/real/man here? If there's another man we could call that does the right thing, we could do away with the wrapper script entirely, and it gets a lot less annoying.

The idea of using a separate script was that we could set TERM (or pipe man output through col -b) without impacting output from git itself – if git doesn't invoke man, the script is never executed and nothing changes.

If you can pass git a config setting (you mentioned man.man.path) to invoke a wrapper script by path, there's no need to "spell out" the path to man in the script, in which case the 2nd line would become TERM=dumb man $@ rather than TERM=dumb /path/to/man $@.

Taking all of that into account, it sounds like the best approach might be (pseudo-code):

$script_path := new_temporary_file_path()
write this literal to $script_path:
    #!/usr/bin/env sh
    TERM=dumb man $@ | col -b  # presumably, on any given platform, either `TERM` or `col -b` will do the trick
when it's time to invoke git:
    run "git -c man.man.path=" + shell_escape($script_path) + "  <usual git arguments>"
tpope commented 5 months ago

Looks like man uses MANPAGER even when not connected to a terminal, which isn't what I would expect, but works to our advantage as a straightforward solution.

finite-state-machine commented 5 months ago

That works beautifully! Thank you, @tpope!

benknoble commented 2 months ago

Hm. It would be convenient if Git commit --help and similar leaned on :help :Man; you would't need to use col -b or similar, then. Probably a larger change, though.