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

Create a more compact diff format #2400

Closed
wants to merge 5 commits into from
Closed

Create a more compact diff format #2400

wants to merge 5 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 20, 2022

An example showcasing all the possible render combinations

Capture

@bors
Copy link
Contributor

bors commented Jul 20, 2022

☔ The latest upstream changes (presumably #2373) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

Some "N lines skipped" have "..." after them and some not -- what's up with that?

Is it possible to always print a few lines around where things changed, for context (the way diff -u does)?

@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 22, 2022
@oli-obk oli-obk force-pushed the diffy_mc_diffface branch from 0f66d93 to 62e6df0 Compare July 25, 2022 14:45
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 25, 2022

Is it possible to always print a few lines around where things changed, for context (the way diff -u does)?

done

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jul 25, 2022
@RalfJung
Copy link
Member

RalfJung commented Jul 27, 2022

I think I'm going to have to just trust you on this one, since that row function would be a lot of effort to review properly. I don't know the API it interacts with and it has very few comments. print_skip has no comments at all.

Can you show how an example diff look like now?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 27, 2022

I think I'm going to have to just trust you on this one

My plan was to just see how it goes in practice and adjust it if we see problems. Worst case is just some badly rendered diffs

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 27, 2022

two random missing lines in a stderr cause:

image

at the two line skip point:

image

when only one line would be skipped:

image

when just some chars change in a line:

image

when you both add and remove chars from a line:

image

when you do larger diffs:

image

the last one is not great... I could make it like "more than half of the chars must be the same"

@RalfJung
Copy link
Member

Nice!

I think the one thing I am missing is whether - or + is the reference output vs actual output. Regular diff starts with a header like

--- build.rs    2022-07-25 12:43:20.977864910 -0400
+++ Cargo.toml  2022-07-25 12:43:20.977864910 -0400

that makes this quite clear; should we have something similar?

@RalfJung
Copy link
Member

Also, does this always print the first and last line? Not that I object, just a bit surprising. In particular it seems to be the first two lines and the last one line, which is strangely asymmetric?

@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Aug 3, 2022
@bors
Copy link
Contributor

bors commented Aug 25, 2022

☔ The latest upstream changes (presumably #2449) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2022

continued in oli-obk/ui_test#8

@oli-obk oli-obk closed this Aug 30, 2022
@oli-obk oli-obk deleted the diffy_mc_diffface branch August 30, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants