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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 36 additions & 23 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ memmap2 = "0.9.0"
tempfile = "3.8.0"
thiserror = "1.0.50"
clap.workspace = true
similar = { version = "2.3.0", features = ["inline", "bytes"] }

[dev-dependencies]
assert_cmd = "2.0.12"
Expand Down
5 changes: 5 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?

/// Show changes as a unified diff. You may want to use a rich
/// color pager to colorize it.
pub diff: bool,

#[arg(
short = 'F',
long = "fixed-strings",
Expand Down
28 changes: 28 additions & 0 deletions src/diff.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use similar::TextDiff;
use std::path::Path;

use crate::error::{Error, Result};

pub(crate) fn create_udiff<UO: AsRef<[u8]>, UN: AsRef<[u8]>>(
old: UO,
new: UN,
context: usize,
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?

if let Some(spath) = path.to_str() {
spath
} else {
return Err(Error::InvalidPath(path.into()));
}
} else {
""
};

let mut udiff = diff.unified_diff();
udiff.header(path, path);
udiff.context_radius(context);

Ok(udiff.to_string())
}
20 changes: 19 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ mod cli;
mod error;
mod input;

pub(crate) mod diff;
pub(crate) mod replacer;

use clap::Parser;
Expand Down Expand Up @@ -69,7 +70,9 @@ fn try_main() -> Result<()> {
.collect()
};

if options.preview || sources.first() == Some(&Source::Stdin) {
if options.preview
|| sources.first() == Some(&Source::Stdin) && !options.diff
{
let mut handle = stdout().lock();

for (source, replaced) in sources.iter().zip(replaced) {
Expand All @@ -78,6 +81,21 @@ fn try_main() -> Result<()> {
}
handle.write_all(&replaced)?;
}
} else if options.diff {
use crate::diff::create_udiff;

let mut handle = stdout().lock();

for ((source, mmap), replaced) in
sources.iter().zip(&mmaps).zip(replaced)
{
let path = match source {
Source::Stdin => None,
Source::File(path) => Some(path.as_path()),
};
// TODO: custom context radius
write!(handle, "{}", create_udiff(mmap, replaced, 3, path)?)?;
}
} else {
// Windows requires closing mmap before writing:
// > The requested operation cannot be performed on a file with a user-mapped section open
Expand Down