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

avoid the display issue with git add -p without losing CR #1000

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yoichi
Copy link
Contributor

@yoichi yoichi commented Mar 3, 2022

Solve #664 without losing CRs in delta's output.
This will also show other control characters in the output of git add-p.

It partially fixes #754
There remains a problem on preserving CRs in delta's input.

@yoichi
Copy link
Contributor Author

yoichi commented Mar 3, 2022

I found that git diff --color=always does not guarantee the escape sequence between CR and LF on each line.
So this It wasn't enough for #754.

I think we should deal with it by extending bytelines or not using bytelines.

is needed...

@dandavison
Copy link
Owner

Ho @yoichi, I think patching bytelines, or not using bytelines, are good ideas. The bytelines author was very helpful when I contacted them about an issue.

Not using it does make sense though, since it's not a mainstream library. I think the reason I started using it was to support non-utf8 encoded input. I'll find the relevant issues.

@dandavison
Copy link
Owner

See #188 for encoding discussion. I think I never finished the work discussed.

@dandavison
Copy link
Owner

bytelines allows us to output lossy decoding. Before bytelines I was doing this, which meant an error message would appear in the middle of user's output: 9d1aafe

We should look how other Rust projects deal with non utf8 input; do they not use stdin.lock().lines()?

@norisio
Copy link
Contributor

norisio commented Mar 4, 2022

There seems need to a new issue (or #754 probably) to discuss how to preserve CR.

@dandavison
Copy link
Owner

I have to say my instinct is that this doesn't feel like the right solution. But I don't think I have the problem clear in my mind so I'm sorry to say something vague like that. I don't really want to make delta's instructions for git add -p more complicated (that's my fault because the name --color-only was never a very good name!)

What is this there about this problem that is specific to git add -p?

@yoichi
Copy link
Contributor Author

yoichi commented Mar 5, 2022

The problem specfic to git add -p is that if we preserve CRs (and other control characters) and want to show them,
we must translate them to something printable (e.g. caret notation), since bare control characters will be handled
by the terminal.

After I've read the following comment from @dandavison on #754

I do think it's worth bearing in mind that delta's output is for humans to look at, not for machines to parse

then I came up with an idea: we can choose delta's bahavior that it always translate control characters in its output
to printables, without introducing new option, and not limited to git add -p.

@yoichi yoichi changed the title New option caret-encode to avoid the display issue with git add -p avoid the display issue with git add -p without losing CR Mar 5, 2022
@yoichi
Copy link
Contributor Author

yoichi commented Mar 5, 2022

In 4ac5035, I've removed the option caret-encode added in this PR.

Note

  • The replacement process can be placed earlier, but it makes the input caret notation and control characters indistinguishable.
  • This change allows us to use reverse video to distinguish between them.

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.

🐛 CRLF line endings are stripped from diff
3 participants