Support unified diff format #56

m-lima · 2019-11-17T15:19:09Z

As discussed on #53, I wasn't able to see the behavior reported. However, there is an assumption that the file name will be prepended by a/ and b/ as it comes from git diff.

I ended up generating a new state to represent that the output is coming from git diff, since I didn't want to assume that the user wans't comparing files inside directories a and b, respectively

Some cargo fmt came in as well.. I can undo these, if you wish

dandavison

This looks great! One question: I did this:

$ git show HEAD~1:./src/delta.rs > /tmp/was.rs
$ diff -u /tmp/was.rs src/delta.rs | delta

and see this

I don't think it makes sense to include the "renamed/modified/added" line when processing non-git input, would you agree?

cf raw diff -u output:

--- /tmp/was.rs 2019-11-19 17:11:36.372066306 -0500
+++ src/delta.rs        2019-11-18 13:23:37.574575743 -0500
@@ -7,6 +7,7 @@                                                                                                        
 use crate::bat::assets::HighlightingAssets;

Seeing as this is a new feature, it would be fantastic if you could add an "end-to-end" style test with a name like test_unified_diff in mod tests in src/delta.rs, following the same style as those other tests. This would involve adding a new constant with a name like UNIFIED_DIFF_INPUT and UNIFIED_DIFF_EXPECTED_OUTPUT containing some example unified diff output and corresponding delta output, and making at least one assertion about the colors applied. Let me know if that makes sense (and whether you have time to do it.)

dandavison · 2019-11-19T22:04:19Z

src/delta.rs

-            state = State::FileMeta;
+            state = State::GitFileMeta;
+            painter.set_syntax(parse::get_file_extension_from_diff_line(&line));
+        } else if line.starts_with("diff -u ") || line.starts_with("diff -U") {


Shall we include the trailing space after both?

The -U parameter takes an argument (the number of context lines)
So something like diff -U3 ... is a valid command

I avoided a regex matcher because -U is the only parameter that begins with a capital U, so it seemed unnecessary overhead.

Let me know what you think. I'm happy to either extend the check and look for spaces or digits, or make a regex pattern

The -U parameter takes an argument

Oh, I see. Makes sense then, I like your simple and efficient starts_with implementation.

But, sorry if I'm being stupid, I don't see a string like diff -u in unified diff output, do you?

$ diff -u <(echo a) <(echo b) --- /dev/fd/63 2019-11-20 10:08:51.973225512 -0500 +++ /dev/fd/62 2019-11-20 10:08:51.973585959 -0500 @@ -1 +1 @@ -a +b

$ diff --version diff (GNU diffutils) 3.6

However, the output pasted by @fdcds in the intro of #53 does show a string like that, so I'm a bit confused. Is there a different version of diff that outputs such a string? (On a Mac the system diff and the one from the coreutils brew package seem to be the same.) Seems the same on linux:

$ docker run ubuntu bash -c 'diff -u <(echo a) <(echo b); uname -a; diff --version' --- /dev/fd/63 2019-11-20 15:17:14.345904000 +0000 +++ /dev/fd/62 2019-11-20 15:17:14.345904000 +0000 @@ -1 +1 @@ -a +b Linux 7c3c98a876c6 4.9.184-linuxkit #1 SMP Tue Jul 2 22:58:16 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux diff (GNU diffutils) 3.6

Yeah.. This is what I commented on #53, it wasn't very clear what was happening and I couldn't replicate it. I was very confused at first. But
I did manage to find out what he meant, though. You need to diff -u <directory1> <directory2>, this will output diff -u ... before each file, or Only in <directory1> for files that do not have a name match

Right, I see, thanks for explaining.

m-lima · 2019-11-20T05:22:20Z

Hi Dan, thanks for the review. I totally agree that some tests are needed. Just found a couple more new corner cases. I'll go ahead and plop in this different scenarios to be tested

As for the "renamed/modified/added", what about something like:
comparing: /tmp/was.rs 2019-11-19 ⟶ ./src/delta.rs 2019-11-18
To give the context of what "added to" and "removed from" lines mean?

* Detect the input and passthrough if not a diff * Show better header for diff -u between files

dandavison · 2019-11-20T19:41:26Z

src/delta.rs

+#[derive(Debug, PartialEq)]
+pub enum Source {
+    GitDiff,     // Coming from a `git diff` command
+    DiffUnified, // Coming from a `git -u` command


Little typo here: s/git/diff/

dandavison · 2019-11-20T19:48:00Z

src/delta.rs

-            if config.opt.commit_style != cli::SectionStyle::Plain {
+    for raw_line in lines_peekable {
+        if source == Source::Unknown {
+            writeln!(painter.writer, "{}", raw_line)?;


Do you think a continue statement here would make the code more readable?

* Fix typo

dandavison

This is a really nice PR. With the unit tests and careful coding style you've made it very little work for me here! I just have two comments here for you and then I think we'll be ready to merge.

dandavison · 2019-11-22T18:12:34Z

src/delta.rs

@@ -252,6 +313,28 @@ fn handle_hunk_line(painter: &mut Painter, line: &str, state: State, config: &Co
    }
 }

+fn handle_file_uniqueness(


When I read the code I struggled a bit to understand this function name. The other analogous functions are called handle_commit_meta_header_line, handle_file_meta_header_line, handle_hunk_meta_line, handle_hunk_line.

WDYT about handle_unified_diff_unique_file_name? And would you mind adding a brief docstring explaining what it does?

Or handle_directory_diff_unique_file_name?

dandavison · 2019-11-22T18:15:23Z

src/delta.rs

+        // Header
+        assert_eq!(
+            lines.nth(1).unwrap(),
+            "comparing: one.rs\t2019-11-20 ⟶   src/two.rs\t2019-11-18"


I'm hesitant about including these dates with the file names. What's your thinking on this? I'm kind of thinking that timestamps are orthogonal to the question of how two files differ and we should use

comparing: one.rs ⟶ src/two.rs.

dandavison · 2019-11-22T18:16:46Z

src/parse.rs

+    comparing: bool,
+) -> String {
+    if comparing {
+        format!("comparing: {} ⟶   {}", minus_file, plus_file)


Thanks, I like your style decision here to make this harmonious with the git diff "renamed" output. (I just have that concern above about not including the date timestamps).

dandavison · 2019-11-22T18:30:53Z

Here's an example and a question about the output. This creates two directories that have two comparable files, and also one unique file:

mkdir /tmp/z1 /tmp/z2
git show HEAD~10:./src/delta.rs > /tmp/z1/a.rs
git show HEAD:./src/delta.rs > /tmp/z2/a.rs
touch /tmp/z2/b.rs
git show HEAD~10:./src/paint.rs > /tmp/z1/c.rs
git show HEAD:./src/paint.rs > /tmp/z2/c.rs
diff -u /tmp/z1 /tmp/z2 | delta

At the end of the diff we get

Now, did we expect the Only in text to be blue? And would it be appropriate for it to have the same underline styling as the other file headers sections?

m-lima · 2019-11-22T19:06:04Z

Thanks for the input, Dan. This is great feedback :)

To address some of your points:

Yes! It should be blue! It was suppose to have the same markers as a new file block. I'll look into it
I kind of liked to have the date, because it allows the user to know which file is more recent. But I see your point and I also know that delta is about a clean and simple output. I'll fix this
Because of your later comment, I suppose you thought more about it. So I'll go for your last suggestion: handle_directory_diff_unique_file_name

* Change diff -u function name and add doc * Handle file uniqueness after a hunk change

m-lima · 2019-11-22T20:05:58Z

src/delta.rs

@@ -90,11 +114,20 @@ where
                handle_hunk_meta_line(&mut painter, &line, config)?;
                continue;
            }
+        } else if source == Source::DiffUnified && line.starts_with("Only in ") {
+            state = State::FileMeta;


I want to avoid generating a new state, and since this section should behave like a new file block, I used the same state
Let me know if you'd rather have a new state for it

Reusing the same state here makes sense to me.

dandavison · 2019-11-25T15:36:47Z

Thanks very much @m-lima, this is a great contribution.

Incidentally, I wonder about the package name git-delta that I've been using (due to delta being unavailable on the standard package repositories), since you've now made delta genuinely support a non-git diff format.

m-lima · 2019-11-26T07:38:05Z

It was a pleasure picking up an issue with this project and a great first experience contributing to OSS :)

Allow ignoring git diff virtual path

ee8aee1

dandavison mentioned this pull request Nov 18, 2019

Make sure rename only shows up once #57

Closed

m-lima added 2 commits November 18, 2019 18:28

If a +++ or --- block is found, make sure to set state

f93ffbe

Create a new state for "diff -u" file header

200f97e

dandavison requested changes Nov 19, 2019

View reviewed changes

m-lima added 2 commits November 20, 2019 07:12

Add unified diff test cases

29bba74

* Parse diff -u between directories

5313df1

* Detect the input and passthrough if not a diff * Show better header for diff -u between files

dandavison reviewed Nov 20, 2019

View reviewed changes

* Make main delta loop more readable

5daa9ba

* Fix typo

dandavison changed the title ~~Allow ignoring git diff virtual path~~ Support unified diff format Nov 22, 2019

dandavison mentioned this pull request Nov 22, 2019

Make Delta an official Debian package #26

Closed

dandavison requested changes Nov 22, 2019

View reviewed changes

* Trim out the date from diff -u file name

b6a9a63

* Change diff -u function name and add doc * Handle file uniqueness after a hunk change

m-lima commented Nov 22, 2019

View reviewed changes

dandavison merged commit 98e8243 into dandavison:master Nov 25, 2019

dandavison mentioned this pull request Nov 25, 2019

Support diff -u ... | delta #53

Closed

m-lima deleted the 53-allow-pipe-from-diff branch November 26, 2019 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support unified diff format #56

Support unified diff format #56

m-lima commented Nov 17, 2019

dandavison left a comment •

edited

Loading

dandavison Nov 19, 2019

m-lima Nov 20, 2019

dandavison Nov 20, 2019

m-lima Nov 20, 2019

dandavison Nov 20, 2019

m-lima commented Nov 20, 2019

dandavison Nov 20, 2019

dandavison Nov 20, 2019

dandavison left a comment

dandavison Nov 22, 2019

dandavison Nov 22, 2019

dandavison Nov 22, 2019

dandavison Nov 22, 2019

dandavison commented Nov 22, 2019

m-lima commented Nov 22, 2019

m-lima Nov 22, 2019

dandavison Nov 25, 2019

dandavison commented Nov 25, 2019

m-lima commented Nov 26, 2019

Support unified diff format #56

Support unified diff format #56

Conversation

m-lima commented Nov 17, 2019

dandavison left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-lima commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison commented Nov 22, 2019

m-lima commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison commented Nov 25, 2019

m-lima commented Nov 26, 2019

dandavison left a comment •

edited

Loading