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

Moving around diffs #50

Closed
ahmedelgabri opened this issue Nov 2, 2019 · 13 comments · Fixed by #217
Closed

Moving around diffs #50

ahmedelgabri opened this issue Nov 2, 2019 · 13 comments · Fixed by #217

Comments

@ahmedelgabri
Copy link

I'd like to do something like this diff-so-fancy moving around diff

I tried this, but it only worked with git show but not git diff

pager = delta --dark --hunk-style plain --theme OneHalfDark | less --pattern '^(Date|added|deleted|modified|renamed): '
@dandavison
Copy link
Owner

Cool, that's a neat trick! So, here's what's going on:

  • git show displays the commit metadata (which includes Date: ), so it's working for the Date anchors, at least. (git log -p also works in that sense)
  • git diff does not show commit metadata, so the only thing git diff will anchor on is added|deleted|modified|renamed
  • The thing is, delta does not currently print out the word "modified". I made that decision because one of my aims for delta was to be a "cleaned up" / "minimal" diff view, and in practice, in a git repo, one is nearly always looking at modified files, so it didn't seem necessary to print the word "modified".
  • So git diff will work for you if you have added/deleted/renamed files, but if there are none of those it fails with "pattern not found".

OK, so how can we make this work better?

One idea is to match the horizontal line separator delta uses at each new file. E.g. something like this:

'^(Date|added|deleted|──────────────────────────────────────────────────────────────────────────────|renamed)'

Unfortunately, every time it jumps to the match, it leaves the actual file name off screen by one line, which isn't helpful. If we could use a regexp that matches two lines it could work, but it looks like that that's just not going to be possible.

Here's something that does work, at the cost of slightly uglier output:

git log -p | delta --file-style plain | less -R --pattern '^(Date: |added: |deleted: |renamed: |diff --git)'

So -- what do you think? Is it worth adding an option for delta to emit a searchable anchor along with the modified file name? (And if so would you like to work on it? :) ) In general I would say that the part of delta involving the various options for changing the way different sections of the diff output are styled, is probably an area that could benefit from some more attention.

@dandavison
Copy link
Owner

We could add back the word "modified" and be done with it, but I wonder whether more in keeping with delta's aims would be to use a set of single-character symbols, instead of the full words, e.g.

→ added
← removed
Δ modified
♻ renamed

@ahmedelgabri
Copy link
Author

Here's something that does work, at the cost of slightly uglier output:

git log -p | delta --file-style plain | less -R --pattern '^(Date: |added: |deleted: |renamed: |diff --git)'

That's not as bad as I thought it would be, to be honest 😄

So -- what do you think? Is it worth adding an option for delta to emit a searchable anchor along with the modified file name? (And if so would you like to work on it? :) ) In general I would say that the part of delta involving the various options for changing the way different sections of the diff output are styled, is probably an area that could benefit from some more attention.

I think it's useful or it's just my muscle memory, I also don't mind working on it but I have zero Rust experience so I will need lots of help 😉

We could add back the word "modified" and be done with it, but I wonder whether more in keeping with delta's aims would be to use a set of single-character symbols, instead of the full words, e.g.

→ added
← removed
Δ modified
♻ renamed

I honestly think bringing back "modified" is the best option, adding icons should be optional IMO.

@dandavison
Copy link
Owner

dandavison commented May 24, 2020

I think it's been too long without a solution for this (#199 calls for this feature also cc @driesg). One good thing is that delta starts its own less process, so it can potentially set the regular expression itself.

@driesg
Copy link

driesg commented May 25, 2020

I honestly think bringing back "modified" is the best option, adding icons should be optional IMO.

I tend to agree with this, text is usually more clear than icons.

In general I would say that the part of delta involving the various options for changing the way different sections of the diff output are styled, is probably an area that could benefit from some more attention.

@dandavison where would you start?

@awerlang
Copy link

dotfiles log -p | delta | less -R -j2 --pattern '^(Date|added|deleted|──────────────────────────────────────────────────────────────────────────────|renamed)'

@dandavison
Copy link
Owner

Hi everyone, this should be addressed now in the master branch. If you're able to try it out, that would be great (instructions for building delta).

Here's what I've done. Basically there are now two ways to activate diff navigation in delta:

  1. Use new option delta --navigate. This will set the less --pattern regexp automatically, according to the values of the new options described below.

  2. Use delta --file-modified-label="XXXX" and set the less --pattern regexp yourself (e.g. using one of the environment variables LESS, BAT_PAGER, PAGER)

New option Default value
--file-modified-label ""
--file-removed-label "removed:"
--file-added-label "added:"
--file-renamed-label "renamed:"

I know that some people suggested prefixing every file path with modified: by default but I couldn't bring myself to do that :) One of the aims of Delta is visually clean output by default, and 95% of the time one is looking at a modified file, so that 9-character English word felt noisy to me.

By default, delta --navigate uses the unicode character Δ, so here's what it looks like on my current white background.1

image

But that's just the default, you can do

delta --navigate --file-modified-label="modified:"

In general it shouldn't be necessary to pipe delta into less: delta starts its own less process automatically. (This is what bat does also, but with diff-so-fancy you have to pipe it explicitly to less.)

Feedback and testing on master very welcome!


1 Just to be totally explicit for anyone reading this, Δ is used in math to mean "a change to a quantity" so the idea is that one thinks of it here as meaning "change to a file".

@dsifford
Copy link

@dandavison Any idea when the next release is going to be slated? Would love to not have to build directly from source to use this. 🙏

@dandavison
Copy link
Owner

Hi @dsifford, soon! I'm working on it. There have been a huge number of changes related to how we configure stylable elements (see #205) and in master we can now use ~/.gitconfig to configure delta. So no more features to be added before next release! But I need to do a bit more work and add tests to make sure all this is solid.

@driesg
Copy link

driesg commented Jun 22, 2020

Thanks @dandavison! I got back from leave and have a few things to catch up on. Once that's done, I can have a go at building from source and report any findings

@dandavison
Copy link
Owner

--navigate is available in delta v0.2.0 which was released today.

@dsifford
Copy link

dsifford commented Jul 2, 2020

Works like a charm. Thanks @dandavison 👍

@dandavison
Copy link
Owner

The latest release of delta allows navigate to be activated via an environment variable, e.g.:

DELTA_NAVIGATE=1 git diff

This is motivated by the fact that leaving navigate active all the time is problematic; please see the README section https://github.com/dandavison/delta#navigation-keybindings-for-large-diffs

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 a pull request may close this issue.

5 participants