Add `EofNewLine` enum on `ModifiedFile`

[xphoniex]

Currently we're getting diffs like this when EOF is missing new line:

diff --git a/HomebrewFormula b/HomebrewFormula
index 4ee8025..1ffaf04 120000
--- a/HomebrewFormula
+++ b/HomebrewFormula
@@ -1 +1 @@
-pkg/brew/radicle-tools.rb
\ No newline at end of file
+pkg/brew
\ No newline at end of file

So we remove the "\ No newline at end of file" message and a flag to Hunk instead.

[FintanH]

Sorry, I don't quite understand what the issue is. I generally see "No newline at end of file" in tooling, including GitHub which has a little red emoji iirc. I think it's quite common to be consistent with newlines at the end of a file. But perhaps I'm misunderstanding :)

Also, I'd like to see some test case(s) to ensure the behaviour you want is expected :grin:

[xphoniex]

@FintanH take a look here. Currently, the lineNum for "No newline at end of file" is the same as previous line so they get combined.

We decided to fix it here once, since there are multiple consumers for api.

Re tests, it's a pretty small change & already tested locally.

[FintanH]

take a look here. Currently, the lineNum for "No newline at end of file" is the same as previous line so they get combined.

Ah, that makes sense, thanks for the clarification.

Re tests, it's a pretty small change & already tested locally.

Appreciate that you tested locally, but testing is also to help understand current behaviour and also ward off accidental changing of that behaviour. I'd still appreciate some testing :)

[xphoniex]

Appreciate that you tested locally, but testing is also to help understand current behaviour and also ward off accidental changing of that behaviour. I'd still appreciate some testing :)

Hm... this would be an integration test, and not easy to implement unlike other tests. I can show the behavior if git2::Diff could be somehow stored in memory? Alternatively, we can clone a repo we know, such as radicle-client-tools which had this issue and do the diff on that commit, and then remove it afterwards.

[kim]

if git2::Diff could be somehow stored in memory?

You can create one from a textual patch: https://docs.rs/git2/0.13.25/git2/struct.Diff.html#method.from_buffer

[FintanH]

You can create one from a textual patch: https://docs.rs/git2/0.13.25/git2/struct.Diff.html#method.from_buffer

Either this or we add a case in git-platinum. Perhaps @rudolfs and @geigerzaehler are also interested in this and how it's presented in Upstream?

[xphoniex]

if git2::Diff could be somehow stored in memory?

You can create one from a textual patch: https://docs.rs/git2/0.13.25/git2/struct.Diff.html#method.from_buffer

Ah, thanks. I was looking at docs of an older version and missed this.

[xphoniex]

Hm... from_buffer method works, but lines are missing old_lineno and new_lineno which makes LineDiff fail too, any experience with this @FintanH @kim ?

[FintanH]

Hm... from_buffer method works, but lines are missing old_lineno and new_lineno which makes LineDiff fail too

Haven't used this before, personally. Do you have some code I can try running on my side and help investigate?

[xphoniex]

Haven't used this before, personally. Do you have some code I can try running on my side and help investigate?

// surf/src/diff/git.rs
#[cfg(test)]
mod tests {
    use super::*;
    #[test]
    fn test_both_missing_eof_newline() {
        let buf = r#"diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
\ No newline at end of file
+hello=1234
\ No newline at end of file"#;

        let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
        let diff = Diff::try_from(diff).unwrap();
        assert_eq!(diff.modified.len(), 1);
    }
}

I tried patch-mode, diff, and show. also removing headers, etc. None worked so far.

[FintanH]

@xphoniex it seems like it wants a whole patch file, which you can get from git format-patch. I set one up in a test repo and got an example here:

        #[test]
        fn test_both_missing_eof_newline() {
            let buf = r#"
From e1f4980b5ef3cf4ae777c8b888fbd8f7a0d05876 Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Fri, 18 Feb 2022 14:55:14 +0000
Subject: [PATCH] yOlO

---
 test | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test b/test
index 29ba004..7fa0056 100644
--- a/test
+++ b/test
@@ -1 +1 @@
-hello=123
+hello=1234
-- 
2.31.1"#;

            let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
            let diff = Diff::try_from(diff).unwrap();
            assert_eq!(diff.modified.len(), 1);
        }
    }

So if you could create one with/without newlines we'd be golden.

[xphoniex]

Your sample works but the second you add the culprit "\ No newline at end of file" to either side, you'll get the error. Also you don't need the whole thing, try:

diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
+hello=1234

and then:

diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
\ No newline at end of file
+hello=1234
\ No newline at end of file

we probably could fix this by checking if content is ^ here and do something with it (I didn't know what to return instead to test locally).

[FintanH]

Right there's actually an enum that includes this information in git2, it's called LineDiffType. Which makes sense, it's not necessarily on the file, it's on the particular line. So could we change the tact here and perhaps consider adding this in LineDiff. Would that work for the upstream application?

[cloudhead]

Oh nice find, I think we can work with that.

[xphoniex]

I added the tests but there's an underlying bug in libgit2 which makes tests fail when using git2::Diff::from_buffer and returns the opposite side. Here's the PR I opened that would fix this, if gets merged: https://github.com/libgit2/libgit2/pull/6240.

And here's a code snippet to demonstrate the bug with a local repo:

$ git diff 17ec0f3 394a717
diff --git a/.env b/.env
index f9fb22f..ed7b51e 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=12345
+hello=123456
\ No newline at end of file

Code:

use git2::Repository;
//use radicle_surf as surf;

fn print_diff(diff: &git2::Diff) {
    diff.print(
        git2::DiffFormat::Patch,
        |_diff_delta, _diff_hunk, diff_line| {
            println!(
                "{} {:?} {:?}",
                diff_line.origin(),
                diff_line.origin_value(),
                std::str::from_utf8(diff_line.content()).unwrap(),
            );
            true
        },
    )
    .unwrap();
}

fn main() {
    let repo = match Repository::open("./end-git-test") {
        Ok(repo) => repo,
        Err(e) => panic!("failed to open: {}", e),
    };

    let hash = git2::Oid::from_str("17ec0f30306d0c80e509e1b48892cbc7c0dcc1cd").unwrap();
    let tree1 = repo.find_commit(hash).unwrap().tree().unwrap();

    let hash = git2::Oid::from_str("394a71726c3e9a86131d7160733bf9270c34deb6").unwrap();
    let tree2 = repo.find_commit(hash).unwrap().tree().unwrap();

    let diff = repo
        .diff_tree_to_tree(Some(&tree1), Some(&tree2), None)
        .unwrap();
    print_diff(&diff);
    //let rad_diff = surf::git::Diff::try_from(diff).unwrap();
    //println!("diff is {:?}", rad_diff);

    println!();
    let buf = r#"
diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=12345
+hello=123456
\ No newline at end of file
"#;
    let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
    print_diff(&diff);
    //let diff = surf::git::Diff::try_from(diff).unwrap();
    //println!("diff = {:?}", diff);
}

Output:

F FileHeader "diff --git a/.env b/.env\nindex f9fb22f..ed7b51e 100644\n--- a/.env\n+++ b/.env\n"
H HunkHeader "@@ -1 +1 @@\n"
- Deletion "hello=12345\n"
+ Addition "hello=123456"
< DeleteEOFNL "\n\\ No newline at end of file\n"

F FileHeader "diff --git a/.env b/.env\nindex f89e4c0..7c56eb7 100644\n--- a/.env\n+++ b/.env\n"
H HunkHeader "@@ -1 +1 @@\n"
- Deletion "hello=12345\n"
+ Addition "hello=123456\n"
> AddEOFNL "\\ No newline at end of file\n"

So the two non-working test cases are commented out for now, and will be uncommented once the PR is merged.

[FintanH]

Happy to accept and merge these changes. Nice one on tracking down that libgit2 bug :smile:

I'm just wondering if you considered my comment here https://github.com/radicle-dev/radicle-surf/pull/201#issuecomment-1047876074?

Thanks for the time and patience!

[xphoniex]

Happy to accept and merge these changes. Nice one on tracking down that libgit2 bug smile

I'm just wondering if you considered my comment here #201 (comment)?

Thanks for the time and patience!

Yup, I started using line.origin_value() instead of .origin() as it's more clear in the code.

Re putting it on the line itself, we're already sending the type and currently eof nl missing line would be:

{
  type: "deletion | addition", // <- we can change this
  line: "\n\\ No newline at end of file\n",
  lineNum: 1
}

which sends two other redunant fields as well. I think a flag on the file itself would be more appropriate as suggested by Alexis here too https://github.com/radicle-dev/radicle-surf/pull/201#discussion_r807034639.