Re: [Monotone-devel] BUG: "mtn diff" vs. "diff" with respect to not existing trailing newline on last line

[Ralf S. Engelschall]

On Mon, Mar 03, 2008, Nathaniel Smith wrote:

> On Mon, Mar 03, 2008 at 10:53:25PM +0100, Ralf S. Engelschall wrote:
> > The appended patch successfully resolves the bug for me.
>
> Looks fine in principle, but *really* needs some tests added...?

Ok, here it comes. I've first polished the code changes a little bit
further (variable name, reduced the use of constants::default_encoding,
etc), added a test suite which checks for all four combinations
(existing file with or without old trailing newline multiplied with
new file with our without new trailing newline), and I've adjusted the
existing test suite where two diff outputs still had added lines before
deleted lines.

So, what does the patch do:

1. It fixes the "mtn diff" output to be compatible with diff(1) for the
   special cases of a missing trailing newline (both in the old or new
   content).

2. It ensures that in the "mtn diff" output hunks always deleted lines
   come first and then added lines. This special case just occurred
   twice in the whole test suite, but especially is triggered for the
   missing trailing newline case and hence had to be fixed in parallel.

3. It corrects two existing diff outputs in the test suite and adds
   a new test for the four cases of trailing newlines.

A "make check" is passed (again) successfully, although I'm still at
15db9bc261c01c4ca5cdb052aec69d29f3bec58a because latest HEAD is broken
because of the recent "keydir" change...

Any opinions, objections, whatever?

                                       Ralf S. Engelschall
                                       address@hidden
                                       www.engelschall.com

#
# old_revision [15db9bc261c01c4ca5cdb052aec69d29f3bec58a]
#
# add_dir "tests/diff_on_missing_trailing_newline_at_end_of_file"
#
# add_file 
"tests/diff_on_missing_trailing_newline_at_end_of_file/__driver__.lua"
#  content [692118ef6cc7d9765b13d563b90614c1c791e3da]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file1"
#  content [a43dc27b1c92cccc533ceb3a27035128e26e5b07]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file13.diff"
#  content [d6e7ac58324be3537427f865e5925f187331f5de]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file14.diff"
#  content [dcdd2294fa92ad013ddc177c12f3a3bed0c2dd38]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file2"
#  content [4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file23.diff"
#  content [86af946a84931611d6625fe4b555ab42b4e8fc0d]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file24.diff"
#  content [e6c588f9eac350fac803802708a65f2807b68673]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file3"
#  content [8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11]
#
# add_file "tests/diff_on_missing_trailing_newline_at_end_of_file/file4"
#  content [400b49625ecf4bb7d2d7096ba86801bddb05bb9f]
#
# patch "diff_patch.cc"
#  from [e359a4c03c6ce548d6414dce11160fbcb6fcb831]
#    to [daeb9be7d8c10d33781b54debd541027618dbfce]
#
# patch "simplestring_xform.cc"
#  from [5f2e4fcd288a2ec1fd59feb97ebc777b56597608]
#    to [dcaa75eb0ed67ddc119ef62f06c21bd7d13bf5cb]
#
# patch "simplestring_xform.hh"
#  from [0280d49b889bc0b7b2900d5d123cc8ec95a115eb]
#    to [564a305c99ca931c93956ac3ef903683dcb15db7]
#
# patch "tests/merge_update_part_of_file/after.diff"
#  from [48e13022c089817683c14884566546af358f7ce9]
#    to [a86dd00b0ad3a0822139372cc83d8ddd746572dd]
#
# patch "tests/merge_update_part_of_file/before.diff"
#  from [6dffe627ffe07df8df1f15670b065f16264cba6a]
#    to [1f6b25391440c416b57b517d8ce552d7b7a52f80]
#
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/__driver__.lua        
692118ef6cc7d9765b13d563b90614c1c791e3da
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/__driver__.lua        
692118ef6cc7d9765b13d563b90614c1c791e3da
@@ -0,0 +1,58 @@
+--
+-- Checks that "mtn diff" output produces the correct results
+-- in case of a missing trailing newline at end of file.
+--
+-- This test goes like this:
+-- 1. Commits two files:
+--    "file1": containing "foo\nbar\nquux\n"
+--    "file2": containing "foo\nbar\nquux"
+-- 2. Override both "file1" and "file2" with:
+--    "file3": containing "foo\nbar\quux\nbaz\n"
+--    "file4": containing "foo\nbar\quux\nbaz"
+-- 3. Produce a diff output and compare against:
+--    "file13.diff"
+--    "file14.diff"
+--    "file23.diff"
+--    "file24.diff"
+--
+
+mtn_setup()
+
+check(get("file1"))
+check(get("file2"))
+check(get("file3"))
+check(get("file4"))
+check(get("file13.diff"))
+check(get("file14.diff"))
+check(get("file23.diff"))
+check(get("file24.diff"))
+
+addfile("file1")
+addfile("file2")
+commit()
+anc = base_revision()
+
+copy("file3", "file1")
+check(mtn("diff", "file1"), 0, true)
+canonicalize("stdout")
+rename("stdout", "file13.mtn-diff")
+check(samefile("file13.mtn-diff", "file13.diff"))
+
+copy("file4", "file1")
+check(mtn("diff", "file1"), 0, true)
+canonicalize("stdout")
+rename("stdout", "file14.mtn-diff")
+check(samefile("file14.mtn-diff", "file14.diff"))
+
+copy("file3", "file2")
+check(mtn("diff", "file2"), 0, true)
+canonicalize("stdout")
+rename("stdout", "file23.mtn-diff")
+check(samefile("file23.mtn-diff", "file23.diff"))
+
+copy("file4", "file2")
+check(mtn("diff", "file2"), 0, true)
+canonicalize("stdout")
+rename("stdout", "file24.mtn-diff")
+check(samefile("file24.mtn-diff", "file24.diff"))
+
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file1 
a43dc27b1c92cccc533ceb3a27035128e26e5b07
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file1 
a43dc27b1c92cccc533ceb3a27035128e26e5b07
@@ -0,0 +1,3 @@
+foo
+bar
+quux
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file13.diff   
d6e7ac58324be3537427f865e5925f187331f5de
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file13.diff   
d6e7ac58324be3537427f865e5925f187331f5de
@@ -0,0 +1,15 @@
+#
+# old_revision [40ea110392539e19c2d251569a3610c0051e0c64]
+#
+# patch "file1"
+#  from [a43dc27b1c92cccc533ceb3a27035128e26e5b07]
+#    to [8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11]
+#
+============================================================
+--- file1      a43dc27b1c92cccc533ceb3a27035128e26e5b07
++++ file1      8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11
+@@ -1,3 +1,4 @@ quux
+ foo
+ bar
+ quux
++baz
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file14.diff   
dcdd2294fa92ad013ddc177c12f3a3bed0c2dd38
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file14.diff   
dcdd2294fa92ad013ddc177c12f3a3bed0c2dd38
@@ -0,0 +1,16 @@
+#
+# old_revision [40ea110392539e19c2d251569a3610c0051e0c64]
+#
+# patch "file1"
+#  from [a43dc27b1c92cccc533ceb3a27035128e26e5b07]
+#    to [400b49625ecf4bb7d2d7096ba86801bddb05bb9f]
+#
+============================================================
+--- file1      a43dc27b1c92cccc533ceb3a27035128e26e5b07
++++ file1      400b49625ecf4bb7d2d7096ba86801bddb05bb9f
+@@ -1,3 +1,4 @@ quux
+ foo
+ bar
+ quux
++baz
+\ No newline at end of file
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file2 
4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file2 
4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1
@@ -0,0 +1,3 @@
+foo
+bar
+quux
\ No newline at end of file
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file23.diff   
86af946a84931611d6625fe4b555ab42b4e8fc0d
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file23.diff   
86af946a84931611d6625fe4b555ab42b4e8fc0d
@@ -0,0 +1,17 @@
+#
+# old_revision [40ea110392539e19c2d251569a3610c0051e0c64]
+#
+# patch "file2"
+#  from [4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1]
+#    to [8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11]
+#
+============================================================
+--- file2      4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1
++++ file2      8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11
+@@ -1,3 +1,4 @@ bar
+ foo
+ bar
+-quux
+\ No newline at end of file
++quux
++baz
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file24.diff   
e6c588f9eac350fac803802708a65f2807b68673
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file24.diff   
e6c588f9eac350fac803802708a65f2807b68673
@@ -0,0 +1,18 @@
+#
+# old_revision [40ea110392539e19c2d251569a3610c0051e0c64]
+#
+# patch "file2"
+#  from [4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1]
+#    to [400b49625ecf4bb7d2d7096ba86801bddb05bb9f]
+#
+============================================================
+--- file2      4f8091d2fa31b0598dd0ff6dc936f8d0b47b59d1
++++ file2      400b49625ecf4bb7d2d7096ba86801bddb05bb9f
+@@ -1,3 +1,4 @@ bar
+ foo
+ bar
+-quux
+\ No newline at end of file
++quux
++baz
+\ No newline at end of file
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file3 
8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file3 
8aa9b19b304a24ebcd71cfc4a27efc95f8ad8e11
@@ -0,0 +1,4 @@
+foo
+bar
+quux
+baz
============================================================
--- tests/diff_on_missing_trailing_newline_at_end_of_file/file4 
400b49625ecf4bb7d2d7096ba86801bddb05bb9f
+++ tests/diff_on_missing_trailing_newline_at_end_of_file/file4 
400b49625ecf4bb7d2d7096ba86801bddb05bb9f
@@ -0,0 +1,4 @@
+foo
+bar
+quux
+baz
\ No newline at end of file
============================================================
--- diff_patch.cc       e359a4c03c6ce548d6414dce11160fbcb6fcb831
+++ diff_patch.cc       daeb9be7d8c10d33781b54debd541027618dbfce
@@ -1010,17 +1010,17 @@ void walk_hunk_consumer(vector<long, QA(
           while (idx(lines2,b) != *i)
             cons.insert_at(b++);
         }
-      if (b < lines2.size())
+      if (a < lines1.size())
         {
           cons.advance_to(a);
-          while(b < lines2.size())
-            cons.insert_at(b++);
+          while(a < lines1.size())
+            cons.delete_at(a++);
         }
-      if (a < lines1.size())
+      if (b < lines2.size())
         {
           cons.advance_to(a);
-          while(a < lines1.size())
-            cons.delete_at(a++);
+          while(b < lines2.size())
+            cons.insert_at(b++);
         }
       cons.flush_hunk(a);
     }
@@ -1349,8 +1349,8 @@ make_diff(string const & filename1,
     }

   vector<string> lines1, lines2;
-  split_into_lines(data1(), lines1);
-  split_into_lines(data2(), lines2);
+  split_into_lines(data1(), lines1, true);
+  split_into_lines(data2(), lines2, true);

   vector<long, QA(long)> left_interned;
   vector<long, QA(long)> right_interned;
============================================================
--- simplestring_xform.cc       5f2e4fcd288a2ec1fd59feb97ebc777b56597608
+++ simplestring_xform.cc       dcaa75eb0ed67ddc119ef62f06c21bd7d13bf5cb
@@ -50,9 +50,24 @@ void split_into_lines(string const & in,
 }

 void split_into_lines(string const & in,
+                      vector<string> & out,
+                      bool diff_compat)
+{
+  return split_into_lines(in, constants::default_encoding, out, diff_compat);
+}
+
+void split_into_lines(string const & in,
                       string const & encoding,
                       vector<string> & out)
 {
+  return split_into_lines(in, encoding, out, false);
+}
+
+void split_into_lines(string const & in,
+                      string const & encoding,
+                      vector<string> & out,
+                      bool diff_compat)
+{
   string lc_encoding = lowercase(encoding);
   out.clear();

@@ -92,8 +107,16 @@ void split_into_lines(string const & in,
             break;
           end = in.find_first_of("\r\n", begin);
         }
-      if (begin < in.size())
-        out.push_back(in.substr(begin, in.size() - begin));
+      if (begin < in.size()) {
+        // special case: last line without trailing newline
+        string s = in.substr(begin, in.size() - begin);
+        if (diff_compat) {
+          // special handling: produce diff(1) compatible output
+          s += (in.find_first_of("\r") != string::npos ? "\r\n" : "\n");
+          s += "\\ No newline at end of file";
+        }
+        out.push_back(s);
+      }
     }
   else
     {
============================================================
--- simplestring_xform.hh       0280d49b889bc0b7b2900d5d123cc8ec95a115eb
+++ simplestring_xform.hh       564a305c99ca931c93956ac3ef903683dcb15db7
@@ -13,6 +13,15 @@ void split_into_lines(std::string const
                       std::string const & encoding,
                       std::vector<std::string> & out);

+void split_into_lines(std::string const & in,
+                      std::vector<std::string> & out,
+                      bool diff_compat);
+
+void split_into_lines(std::string const & in,
+                      std::string const & encoding,
+                      std::vector<std::string> & out,
+                      bool diff_compat);
+
 void join_lines(std::vector<std::string> const & in,
                 std::string & out,
                 std::string const & linesep);
============================================================
--- tests/merge_update_part_of_file/after.diff  
48e13022c089817683c14884566546af358f7ce9
+++ tests/merge_update_part_of_file/after.diff  
a86dd00b0ad3a0822139372cc83d8ddd746572dd
@@ -12,5 +12,5 @@
  two
  chunks

+-line to be changed
 +line changed
--line to be changed
============================================================
--- tests/merge_update_part_of_file/before.diff 
6dffe627ffe07df8df1f15670b065f16264cba6a
+++ tests/merge_update_part_of_file/before.diff 
1f6b25391440c416b57b517d8ce552d7b7a52f80
@@ -23,5 +23,5 @@
  two
  chunks

+-line to be changed
 +line changed
--line to be changed