Calculate line numbers correctly in raw/color-only mode #272
Conversation
- call handle_hunk_header_line() if line number option is set - insert empty line before hunk header line only if decoration style is set
src/delta.rs
Outdated
| @@ -131,7 +131,7 @@ where | |||
| painter.paint_buffered_minus_and_plus_lines(); | |||
| state = State::HunkHeader; | |||
| painter.set_highlighter(); | |||
| if should_handle(&state, config) { | |||
| if config.line_numbers || should_handle(&state, config) { | |||
There was a problem hiding this comment.
Do you think it would make sense to incorporate the if config.line_numbers condition into should_handle so that all the logic is in one place?
|
Thanks very much for this work @yoichi. And @alfredbez for testing and reviewing. I added one failing test in master that will pass when this branch is merged: 0e98077 (asserting that line numbers start at 1, not 0, under color-only) |
|
Now let's figure out what to do about side-by-side and then I'll get these fixes released, since |
| writeln!(painter.writer)?; | ||
| if config.hunk_header_style.decoration_style != DecorationStyle::NoDecoration { | ||
| writeln!(painter.writer)?; | ||
| } |
There was a problem hiding this comment.
@yoichi do you recall the logic here -- why do we want a newline only if hunk_header_style.decoration_style != DecorationStyle::NoDecoration? I'm sorry, I should have asked you to add a comment at the time. Given that hunk-header-style=omit is failing (#274), I'm thinking we need to do more work to characterize the conditions under which the newline and other alterations are permitted. If you have any thoughts then great, but if not don't worry!
There was a problem hiding this comment.
In the README configuration example, we instruct to use color-only option with interactive.diffFilter.
On the other hand, as a limitation of git, interactive.diffFilter requires one-to-one correspondence between input and output lines.
I guessed that we originally inserted the line break here because of the layout of the header, so I thought it would be reasonable to skip the layout change of inserting the line when there is no style. And it resolve the break of one-to-one correspondence noted above.
hunk-header-style=omit removes the header line so it also breaks one-to-one correspondence by another reason. Not to omit header in raw/color-only mode may solve that problem, I think.
There was a problem hiding this comment.
Thanks @yoichi! I wonder whether I should get rid of that extra newline character completely, at least in order to get the code into an easy-to-understand state. And then consider re-introducing it. I think I added it in the big v0.2 update because I thought that the extra space made the diff easier to read.
ghost
mentioned this pull request
fix #258