Diff is returning extra line when viewing first commit of a repository

Steps

pro = Project.find_with_namespace('gitlab-org/gitlab-ce')
ci2 = pro.commit('93efff945215a4407afcaf0cba15ac601b56df0d')
d2 = ci2.diffs.first
f2 = Gitlab::Diff::File.new(d2)

Expected

f2.diff_lines.map(&:text) should return:

=> ["+.bundle",
 "+.rbx/",
 "+db/*.sqlite3",
 "+log/*.log",
 "+tmp/",
 "+.sass-cache/",
 "+coverage/*",
 "+*.swp",
 "+public/uploads/"]

Actual

=> ["+++ a/.gitignore",
 "+.bundle",
 "+.rbx/",
 "+db/*.sqlite3",
 "+log/*.log",
 "+tmp/",
 "+.sass-cache/",
 "+coverage/*",
 "+*.swp",
 "+public/uploads/"]

The curious thing that I've noted when inspecting the diff is that the output is different than when using the native git diff command:

"--- /dev/null\n+++ a/.gitignore\n@@ -0,0 +1,9 @@\n+.bundle\n+.rbx/\n+db/*.sqlite3\n+log/*.log\n+tmp/\n+.sass-cache/\n+coverage/*\n+*.swp\n+public/uploads/\n”

It's showing a/.gitignore instead of b/.gitignore

Here is the output from the native git diff command:

cc/ @dzaporozhets @DouweM @rspeicher

Interesting. The cause seems to be the +++ a/.gitignore vs +++ b/.gitignore discrepancy you mention, since our Gitlab::Git::Parser only deems a line a filename if it starts with --- a or +++ b:

      def filename?(line)
        line.start_with?('--- /dev/null', '+++ /dev/null', '--- a', '+++ b',
                         '--- /tmp/diffy', '+++ /tmp/diffy')
      end

(Source)

The cause is in the way we're getting the diff in Gitlab::Git::Commit, which has reverse: true:

[23] pry(main)> puts ci2.raw.raw_commit.diff.patch
diff --git a/.gitignore b/.gitignore
deleted file mode 100644
index c83fcb6..0000000
--- a/.gitignore
+++ /dev/null
@@ -1,9 +0,0 @@
-.bundle
-.rbx/
-db/*.sqlite3
-log/*.log
-tmp/
-.sass-cache/
-coverage/*
-*.swp
-public/uploads/
=> nil
[24] pry(main)> puts ci2.raw.raw_commit.diff(reverse: true).patch
diff --git b/.gitignore a/.gitignore
new file mode 100644
index 0000000..c83fcb6
--- /dev/null
+++ a/.gitignore
@@ -0,0 +1,9 @@
+.bundle
+.rbx/
+db/*.sqlite3
+log/*.log
+tmp/
+.sass-cache/
+coverage/*
+*.swp
+public/uploads/
=> nil

As you can see, without the reverse: true, the diff is actually the reverse of what we want (deletion instead of addition), so the reverse: true has its place. I think we need to adapt Gitlab::Git::Parser to understand +++ a as well.

I've created an MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2427

Reassigned to @DouweM

Status changed to closed by commit gitlab-org/gitlab-ce@17d4b39806d147153d0bd864d2c23d4e6e6e08ef

mentioned in commit tnir/gitlab-ce@c179b48c

Mentioned in commit pfjason/gitlab-ce@12ab1141

Mentioned in commit pfjason/gitlab-ce@c179b48c

Mentioned in commit pfjason/gitlab-ce@17d4b398

mentioned in commit test-nick/gitlab-ce@e3801396