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

Basic diff output #238

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

Basic diff output #238

wants to merge 2 commits into from

Conversation

nc7s
Copy link
Contributor

@nc7s nc7s commented Oct 14, 2023

As proposed in #208 (comment), produce diffs of changes, instead of printing entire files.

This is a rough WIP that copies the terminal-inline example of similar and barely works. I intend to make it produce patches that are actually applicable.

example diff (note that coloring is provided by bat) example diff (note that coloring is provided by bat)

@CosmicHorrorDev CosmicHorrorDev marked this pull request as draft October 16, 2023 22:51
@CosmicHorrorDev
Copy link
Collaborator

Thanks for the PR! I really like how the output looks. I've gone ahead and marked this as a draft while this is still a WIP, but just let me know if you need anything

As for early feedback I think my only concern is that it shouldn't replace the existing output, but should be added as another option. Maybe through a --preview-diff flag or by changing --preview to take take a value like --preview <plain|diff>?

@nc7s
Copy link
Contributor Author

nc7s commented Oct 17, 2023

Thanks for the PR! I really like how the output looks. I've gone ahead and marked this as a draft while this is still a WIP, but just let me know if you need anything

Didn't know you can do that ;)

As for early feedback I think my only concern is that it shouldn't replace the existing output, but should be added as another option. Maybe through a --preview-diff flag or by changing --preview to take take a value like --preview <plain|diff>?

Indeed, the word preview has a meaning. Maybe just --diff? Since with such a flag it seems semantically incorrect to overwrite the file anyway.

@CosmicHorrorDev
Copy link
Collaborator

Sure --diff seems good!

@nc7s

This comment was marked as resolved.

@CosmicHorrorDev
Copy link
Collaborator

Hmmm, I think I would be all right with that. Just push changes and ping me whenever you want feedback

I am planning on pushing for a release over the next few days

@nc7s nc7s mentioned this pull request Nov 2, 2023
@nc7s nc7s changed the title WIP: diff output for --preview WIP: diff output Nov 3, 2023
@nc7s

This comment was marked as outdated.

@alexilyaev
Copy link

Can you share an example of how it would look like?

Got here after searching how to make --preview only show the matched strings or lines that would change and not print the whole file.

Example

This will print every single line of the file and then only at the end prints the change in color.
In large files, this makes --preview unusable.

echo "111\n222\n333\n444\n555\n666\n777\n888\n999" | sd -p '(999)' '000'
image

So, I'm wondering if the diff mode will help with that scenario?

Similar open issues

I suppose one of these would cover the large file use case as well

@nc7s
Copy link
Contributor Author

nc7s commented Dec 19, 2023

Can you share an example of how it would look like?

You can click the triangle on the left of "example" in the topic. Later updates differ a little in detail, but largely it's the same. The Wikipedia entry on diff is probably also worth a read.

Got here after searching how to make --preview only show the matched strings or lines that would change and not print the whole file.

So, I'm wondering if the diff mode will help with that scenario?

Probably. I never thought of diffs in this perspective, but it does work in this way (showing only changes, and limited surrounding context).

@alexilyaev
Copy link

@nc7s Oh, didn't notice the ▶ next to the example diff ☺️👍🏼
How did you get it to show colors? (I'm on macOS, using Zsh in iTerm)

Ok, so I'm testing -d with a change on consecutive lines and it does show me only the changed lines.
But, it doesn't show me the change for each line, rather a big chunk of "before" lines and then a big chunk of "after" lines:

echo "123\n123\n123\n123\n123\n123\n123\n123\n123\n123\n" | cargo run -- -d '2' '9'
image

If the changes are not in consecutive line, then it works as expected:

echo "123\n000\n123\n000\n123\n000\n123\n000\n123\n000\n" | cargo run -- -d '2' '9'
image

I still think that -p should have a flag to show only the affected lines and not print the whole file.

@nc7s
Copy link
Contributor Author

nc7s commented Dec 21, 2023

@alexilyaev:

How did you get it to show colors? (I'm on macOS, using Zsh in iTerm)

bat. Now spelled in the <summary>.

Ok, so I'm testing -d with a change on consecutive lines and it does show me only the changed lines. But, it doesn't show me the change for each line, rather a big chunk of "before" lines and then a big chunk of "after" lines:

I still think that -p should have a flag to show only the affected lines and not print the whole file.

This is expected behavior of diffs. Recommended read: https://en.wikipedia.org/wiki/Diff

Sometimes you won't know where and what the changed lines are if no surrounding context is provided. That's why diffs have contexts - to match surrounding lines. They may otherwise update an unrelated but identical line, which is totally possible for short lines of common words.

If you are confident, context radius could be reduced, down to zero. I'm considering an option to set that, but haven't decided on its naming yet.

@CosmicHorrorDev: copied over from earlier, maybe covered in noise:

Although, it lacks a context radius argument. I tend to not use a generic name like --context for a diff. But --diff-context is too long. I also don't know how to name the "file" when input is through stdin, so it's currently empty; maybe the user knows better, so we also provide an argument like --diff-filename?

@chmln
Copy link
Owner

chmln commented Dec 23, 2023

This is great as a prototype, excellent work. I have long wanted to implement something like this.

Not sure if this is actually possible but it would be great if we could integrate one of the rust pagers as a library so that users don't necessarily need to have e.g. bat installed and piped into, to get a nice looking diff output

@nc7s
Copy link
Contributor Author

nc7s commented Dec 23, 2023

@chmln:

Not sure if this is actually possible but it would be great if we could integrate one of the rust pagers as a library so that users don't necessarily need to have e.g. bat installed and piped into, to get a nice looking diff output

My thought then was that it feels unnecessary for every CLI program to implement its own colorization, esp. when the output is in a pretty common format. Feels like bloat.

Copy link
Owner

@chmln chmln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits :)

@@ -20,6 +20,11 @@ pub struct Options {
/// format are likely to change in the future).
pub preview: bool,

#[arg(short, long)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think diff is short enough to not warrant a short flag, -d might be more useful for other things in the future

Copy link
Contributor Author

@nc7s nc7s Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about -D? Even sed doesn't have many flags. (I impl'd this after all ;)

And what do you think about an option specifying context radius? And filename for piped diffing?

path: Option<&Path>,
) -> Result<String> {
let diff = TextDiff::from_lines(old.as_ref(), new.as_ref());
let path = if let Some(path) = path {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just convert to a lossy string here, not sure its worth erroring out. Having the user see squareish symbols is better than not being able to see a diff at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed when acting as an end user interface, not so when the output is used as a proper diff. Or I'm overestimating the latter use case?

@CosmicHorrorDev
Copy link
Collaborator

For me the big thing is that an integrated pager can be more aware of its contents. An external pager is usually very naïve when it comes to searching or even determining terminal width

@chmln
Copy link
Owner

chmln commented Dec 23, 2023

My thought then was that it feels unnecessary for every CLI program to implement its own colorization, esp. when the output is in a pretty common format.

That's a valid point, although somewhat more of a philosphical one. Ultimately, the users want to see a pretty diff. If we can make that happen without having them install bat and configure a good looking theme..I think that's a clear win.

And if someone really wants to use their own diff program, we can detect that our diff output is being piped and just produce the raw version as you suggest.

Addendum: nevertheless, shipping this 'raw' diff mode is still a big win for all the power users. Just wanted to provide some ideas and food for thought.

@CosmicHorrorDev
Copy link
Collaborator

Also sorry for taking so long to give feedback. I've been stretched pretty thin lately. I likely won't have time to take a look till the new year

@nc7s
Copy link
Contributor Author

nc7s commented Dec 23, 2023

@CosmicHorrorDev:

For me the big thing is that an integrated pager can be more aware of its contents. An external pager is usually very naïve when it comes to searching or even determining terminal width

I'm no expert in this field, but fear that properly setting up an integrated one requires serious work (you gotta know the ins and outs). Plus, we've already seen GIT_PAGER and QUILT_PAGER, to name a few.

Also sorry for taking so long to give feedback. I've been stretched pretty thin lately. I likely won't have time to take a look till the new year

No worries ;) we all have a bunch of work at year end.

@chmln:

My thought then was that it feels unnecessary for every CLI program to implement its own colorization, esp. when the output is in a pretty common format.

That's a valid point, although somewhat more of a philosphical one. Ultimately, the users want to see a pretty diff. If we can make that happen without having them install bat and configure a good looking theme..I think that's a clear win.

Diff coloring is almost too easy to not implement, certainly can be made to work. The "do one thing" principle sometimes has a vague boundary…

And if someone really wants to use their own diff program, we can detect that our diff output is being piped and just produce the raw version as you suggest.

Yeah, configurable colorization was talked about in #268.

So, combined, maybe we can have colorless diffs first, then implement proper colorization later in one go.

@dev-ardi
Copy link
Contributor

Hello @chmln, @nc7s

With @CosmicHorrorDev stepping down what's the status on this? I think it's possitive if either noctis or me would have merge permissions. As I said before don't want to wholely maintain but I wouldn't mind reviewing PRs and doing basic fixes at least until another maintainer shows up.

@nc7s nc7s marked this pull request as ready for review January 17, 2024 22:51
@nc7s nc7s changed the title WIP: diff output Basic diff output Jan 17, 2024
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.

5 participants