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

🚀Extended plain mode coloring options #168

Open
da-x opened this issue May 1, 2020 · 9 comments
Open

🚀Extended plain mode coloring options #168

da-x opened this issue May 1, 2020 · 9 comments

Comments

@da-x
Copy link
Contributor

da-x commented May 1, 2020

I have a working draft (branch) for plain-mode extensions which adds coloring for further aspects of the diff. Hopefully I did not butcher the parsing code too much. Hoping to open a discussion about this to see that I'm taking the right direction.

image

This makes it look more like my old fancydiff, which I want to retire. I added 4 options:

        --commit-meta-header-background <commit-meta-header-bg>
            Optional background color for the first line of the commit header
        --commit-meta-other-background <commit-meta-other-bg>
            Optional background color for the other lines of the commit header. Otherwise, `commit-meta-header-bg` is
        --file-background <file-bg>
            Optional background color for the file section of git output
        --hunk-background <hunk-bg>
            Optional background color for the hunk-marker section of git output
@0xC0FFEE
Copy link

0xC0FFEE commented May 1, 2020

Hi @da-x, I really like the idea of coloring the commit messages! However I'm not totally sure, whether this is really delta's job?

git's support for pretty formats[1] is quite rich. It is even possible to print arbitrary ANSI escape sequences via %x.

Here is a first quick attempt to match your example (I guess extending the background color to the end of the line should also somehow work):
git log --format=format:"%C(white #880088)commit %H%n%C(white #440044)Author: %an <%ae>%n%C(white #440044)Date: %ad%n%n%s%n%n%b%n" -p

image

IMHO it will be quite hard (aka. impossible) for delta to parse arbitrarily complex git pretty formats. So IMO delta should just print the existing lines unmodified (a first step in this direction has been done in #163).

git lacks support for custom formatting for hunks. So at least technically I do not see a problem when delta tries to apply custom formatting to hunks, as the hunk format is fixed.

[1] https://git-scm.com/docs/pretty-formats

@da-x
Copy link
Contributor Author

da-x commented May 1, 2020

@0xC0FFEE, yeah I agree that if Git was a bit more featureful we wouldn't need to parse and recolor its output. Your suggestion looks cool, but I still prefer the indented commit body message (no alternative to %b I guess), and whole line background color, so I'll continue to use delta for now. It's easier than contributing features to Git mainline.

BTW, in my draft changes I modified the parsing a slight bit - detecting where the commit header ends upon detecting a blank line. As it stands delta is not trying to do much parsing to the commit message (unlike in fancydiff where I've also highlighted Signed-off-by). Also, this led me to think that that in my draft changes, the original ANSI should not be stripped if commit message recoloring is not enabled, so that it can be future compatible to the possibility of newer Git client version with more coloring capabilities.

@dandavison
Copy link
Owner

@da-x great, I'm happy to have some other collaborators on the code! Modulo the current discussion, I have one initial question about the code changes: is there some refactoring of the state machine parser that you see that could be done first, to make the introduction of the new features as clean as possible?

@da-x @0xC0FFEE Do you think there's a good (and not too complex) solution where we can have both? Delta's purpose is to give people a lot of possibilities in designing their own git output styling, and I think it's OK to implement some things that overlap with what advanced users can achieve with git formatting operations. Defaults is another question of course.

So doing both would be something like

  • If Delta cannot parse something, then it should emit the raw line, which might include ANSI escapes.

  • But if it can parse it then Delta may have an option allowing that element to be styled, even if that styling is something that could have been achieved with native git functionality.

If I'm understanding correctly, the concern is how we handle the conflict between people who are using git's native formatting and want Delta to pass on their input unchanged, vs people who are using default git formatting and will be happy for Delta to apply its own colors (stripping any ANSI sequences from the input in the process). But is that not basically resolved by the assumption

If you want ANSI sequences to be passed through unchanged for <element> then you will not use --<element>-fg-color or --<element>-bg-color

?

I still prefer the indented commit body message (no alternative to %b I guess), and whole line background color, so I'll continue to use delta for now.

Well I hope so, you'd lose a lot of features otherwise! To make the discussion completely clear for all readers though, I believe @0xC0FFEE was suggesting that you use delta, but that the coloring is achieved by (a) a git log format, and (b) delta being good about passing on ANSI sequences unchanged wherever possible.

@da-x
Copy link
Contributor Author

da-x commented May 1, 2020

@dandavison while editing the code I had to copy some logic pertaining to the whole-line background handing in diff hunks. Apart from that, I saw no need for any per-line state machine change besides commit meta-data header end detection. As of today, I started to use delta :).

I agree that we should let Git's color styling to pass through as long as the user did not specify otherwise. However my draft is not supporting it, as it strips colors in draw::write_colored by default... Also to complicate things, as a side effect it strips colors from Git log's ref decoration

image
image

Only Git knows what is the type of the ref (e.g. tag or branch), and it is colored accordingly, so we must preserve these colors. Depending on what the user specified, we may need to do a composite of delta's commit-color around Git's own provided decorated ref colors (controlled via color.decorate.<slot>), meaning we need to parse Git's output instead of just stripping, and carefully combine the colors onto the commit line from both programs.

@0xC0FFEE
Copy link

0xC0FFEE commented May 1, 2020

@da-x @0xC0FFEE Do you think there's a good (and not too complex) solution where we can have both?

I think the easiest and least complicated solution would be to let the user specify the desired functionality (e.g. via a command line switch). So basically delta would have two distinct modes:

  1. Work with the standard git log format
  2. Just pass everything through

Maybe (and just maybe) this decision could be made automatically based on the parsed text. However parsing things, were the input can be arbitrary text, might be (too) complicated.

If I'm understanding correctly, the concern is how we handle the conflict [...]

I think supporting both use cases is desirable. However as we do not have any context, except for the text-based diff itself, it will be quite hard to support both use cases automatically IMHO. That is why I thought maybe we can get away with only implementing one of the use cases in delta and do the rest via git pretty format.

@da-x

Your suggestion looks cool, but I still prefer the indented commit body message (no alternative to %b I guess), and whole line background color, so I'll continue to use delta for now.

Indentation can be achieved via the %w modifier:

git log --format=format:"%C(white #880088)commit %H%n%C(white #440044)Author: %an <%ae>%n%C(white #440044)Date:   %ad%n%n%w(,4,4)%s%n%n%w(,4,4)%b%n" -p

And as I've written before, arbitrary ANSI escape sequences can be printed via %x. So coloring the whole line might be possible with standard git after all (haven't had a look at this yet).

@dandavison
Copy link
Owner

dandavison commented May 29, 2020

@da-x I haven't forgotten about the additions in your branch. I'd like to incorporate them to make more elements stylable. I did however completely change the master branch as you may have noticed. I hope you'll approve of the more consistent style CLI and internal data structures, but sorry to make your changes need recasting! I'll be happy to help out; one task is decide on what new styles we want to add I think. And I also need to circle back and think about the issue we discussed in this ticket of when delta does native color pass-through.

@da-x
Copy link
Contributor Author

da-x commented May 29, 2020

Thanks for working on this. The changes look cool. We'd still need to see whether --color-only should mean overriding colors set by Git, like my branch does (I tried to use --color-only with --file-style in master and it still does not apply the colors).

@dandavison
Copy link
Owner

@da-x: the master branch is getting closer to supporting what you want here. Would you be interested in adding the remaining missing pieces from your branch to master? (Basically: background-color right-fill and new option --commit-meta-style)

@da-x, @0xC0FFEE: all stylable elements now support --foo-style=raw meaning to pass on the input from git unchanged, with original ANSI colors. (That can be combined with a decoration.)

If raw is not in effect, then we strip out git's colors.

Here's an attempt at recreating the screenshot in this ticket using the new features in delta master:

git -c color.diff.meta='white #333333' \
    -c core.pager="delta --commit-style 'bold white "#880088"'
                         --hunk-header-style '"#aaaaaa" blue'
                         --hunk-header-decoration-style none
                         --file-style raw
                         --file-decoration-style none" \
    show 5b2620981
image

The main things we're missing are:

  1. Background colors in commit/file/hunk-header don't fill right currently (we don't have your function paint_whole_line)
  2. Need to add a new stylable element for non-hash commit lines (Author:, Date:, Merge:). I suggest the name --commit-meta-style (--commit-style currently refers to the first line with the hash).

Comments:

  • Other stylable elements in delta have a --foo-decoration-style but it's not clear that --commit-meta-decoration-style would make sense?
  • I think I'm on board with right-filling background colors by default. It might be nice for users to be able to control that -- perhaps there could be a special style attribute with a name like nofill? -- but honestly the main thing is just that we don't release any command line options that we subsequently want to retract or change the meaning of.

@da-x: sorry if I haven't understood yet what you were saying about --color-only. Hopefully this makes it clearer what my current thinking is and we can continue the discussion.

@da-x
Copy link
Contributor Author

da-x commented Jun 3, 2020

Yes, I meant that with --color-only, it's really a shortcut to passing raw on all elements, so that non-ANSI output of Git before delta matches the non-ANSI output of delta. I'll try to rebase my branch master to see how we narrowed the difference. For reference, this is how I currently run my branch:

exec delta \
    --color-only \
    --24-bit-color always \
    --highlight-removed \
    --file-color '#cccccc' \
    --file-background '#222222' \
    --commit-color '#ffffff' \
    --commit-meta-header-background '#880088' \
    --commit-meta-other-background '#440044' \
    --hunk-color '#999999' \
    --hunk-background '#000088' \
    "$@"

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

No branches or pull requests

3 participants