Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use modify label on diffs & add right-arrow option #780

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

WayneD
Copy link
Contributor

@WayneD WayneD commented Nov 17, 2021

This branch was inspired by my using delta to filter unified diffs and noticing that it was using a hard-wired "comparing:" label instead of the normal file-modified-label. This was aggravated by that heading not being in the default --navigate regex, so my n/N commands weren't stopping at it. I also noticed that having delta do the diff generation resulted in no label at all, not even with --navigate, which could not be fixed by a regex tweak. So, instead of adding the "comparing:" string to the navigate regex default, I decided to change the diff headings to use the modified label. While twiddling things I also decided to make the right-arrow configurable because I don't like how the ⟶ has 2 extra spaces on the right side (which might be needed by some shells?).

This branch has 2 commits on it:

  1. Change the diff headings to always use file-modified-label. Whoever wants the old heading can set that option to "comparing:" (which is what one of the modified tests now does, while the other test uses --navigate to have it output 2 Δ headings).
  2. Add the handling for --right-arrow=FOO and associated git config parsing.

I've tested the results and things look good to me. I've also run my changes through clippy and fmt to ensure that they're in good shape.

When filtering a diff and when directly diffing files/dirs, use the
modified label that is output when interpreting git diffs.  This gets
rid of the "comparing:" prefix (which was otherwise missing from the
--navigate regex) and ensures that a direct diff has a label to stop at
when --navigate is enabled.  One of the unified-diff tests now uses
--file-modified-label=comparing: to generate the same output as before,
while the other now uses --navigate to get a delta char label.
This allows the file1 -> file2 arrow to be configured to whatever
characters the user desires.  The default is the same unicode arrow as
before with 2 spaces after it, since this allows someone to remove the
extra spaces that are in the current output.
@@ -550,6 +550,10 @@ pub struct Opt {
/// Text to display in front of a renamed file path.
pub file_renamed_label: String,

#[structopt(long = "right-arrow", default_value = "⟶ ")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: there are two trailing spaces in this default value in order to match what was previously hard-coded in various places (whether or not the previous treatment was correct is a separate issue).

@dandavison
Copy link
Owner

dandavison commented Nov 17, 2021

Hi @WayneD, thanks very much for this. I agree with both your changes. For example, having navigate work when comparing directory trees with delta dir1 dir2 or diff -u dir1 dir2 seems like a nice improvement.

Regarding the spaces after the right arrow, I don't know what's going on. I develop on MacOS using iTerm2 and I think that its handling of that right-arrow unicode character may be a little buggy.

For example, if I paste in to a shell prompt, my line cursor is rendered in the middle of the arrow stem:

image

Now, if I enter two space characters, it looks like this:

image

However, I also see similar behavior using the default MacOS Terminal.app emulator:

On paste (where has the head gone?!)

image

After entering two spaces:

image

I'm guessing you see more reasonable behavior with your terminal emulator(s)?

@dandavison dandavison merged commit 90fd9c6 into dandavison:master Nov 17, 2021
@WayneD
Copy link
Contributor Author

WayneD commented Nov 17, 2021

When editing in vim, my cursor position is off-by-one on the line after the double-wide arrow char. So, moving the arrow into a single spot in the source files has the nice benefit of making the various format lines that output it easier to edit. That said, the arrow lines render fine (even in the editor), being shown with all the following characters after the arrow.

I'm using the ChromeOS terminal into my Linux container. Here's the output of the old code vs the new where I've changed the right-arrow config to have no trailing spaces:

image image

@dandavison
Copy link
Owner

OK great. Not entirely sure whether we should change defaults, I guess it depends on the proportion of users' terminal emulators that are handling this correctly. Neither iTerm2 nor Terminal.app seem to. I've opened https://gitlab.com/gnachman/iterm2/-/issues/10058

(Is that vim as a desktop app, rather than a terminal app?)

Thanks again for the PR!

@WayneD
Copy link
Contributor Author

WayneD commented Nov 17, 2021

I use vim as an in-terminal editor (though it does have a gui mode as well). It's for people who like to edit in the old "vi" modal style but with lots of modern features (and I toss in some xemacs line editing into edit mode for good measure).So, it just seems to be unaware that some chars can render double-wide instead of single-wide (and thus it positions the cursor in the wrong spot).

https://en.wikipedia.org/wiki/Vim_(text_editor)

@dandavison
Copy link
Owner

Incidentally, the iTerm2 author gave a very helpful explanation in https://gitlab.com/gnachman/iterm2/-/issues/10058:

image

@WayneD
Copy link
Contributor Author

WayneD commented Dec 13, 2021

Since it's such a problematical character, it seems better to switch to one that renders consistently. How about changing the default to be 🡒 (U+1F852)? I've switched to that one in my personal ~/.gitconfig.

@dandavison
Copy link
Owner

Hm, neither Chrome nor iTerm2 can render that character on my Mac. (Are you using Linux?)

image
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants