Fix false-positive formatting in git format-patch headers.

Marking this a dup of #1839136: Update patch review CSS.

Re-opening, since that issue attempts to do too much at once.

Yes the patch still works, however I have refactored this patch to detect the entire git format-patch header before iterating through each line. This will remove it and place it above the patch. Goes well with the aforementioned issue and patch's styling changes.

Oops, fixed an issue with the regex where the header wasn't detected if all there was is insertions and no deletions.

Moving the headers out of the diff line processing as in #6 doesn't feel right to me.

The patch further tries to add + populate additional jump menus for git patch serials, but actually does not introduce a jump link facility. (for which serial header IDs would be required)

I think we should go back to the patch in #1, which retains the header within the diff line processing and adds an appropriate class to relevant lines. It has a couple of issues though:

+++ b/dreditor.user.js
@@ -1173,6 +1173,7 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {
+  on_git_header = false;

1) Needs to be declared with var.

2) "on_patch_header" would make more sense to me.

+++ b/dreditor.user.js
@@ -1191,13 +1192,26 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {
+    if (on_git_header) {
+      classes.push('header');
+    }

I wonder whether on_patch_header shouldn't initially be true?

Essentially: Everything up until the first file header must belong to the patch header, no? And as soon as we hit the file header, we can reset the classes to an empty array.

+++ b/dreditor.user.js
@@ -1191,13 +1192,26 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {
+      if (line.match(/^diff.*$/i)) {
+        on_git_header = false;
+      }

What was the reason to limit the patch header reset to only "diff*"?

AFAIK, interdiffs don't always show "diff" lines.

I can't see why it would be needed, so I'd suggest to remove the condition.

+++ b/dreditor.user.js
@@ -1191,13 +1192,26 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {
     else if (line.match(/^((?!\-\-\-)\-.*)$/)) {
-      classes.push('old');
+      if (!on_git_header) {

I wonder whether we shouldn't wrap the entire old/new code conditions into a !on_patch_header condition?

+++ b/dreditor.user.js
@@ -1191,13 +1192,26 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {
+      if (!on_git_header) {
+        classes.push('old');
+      }
       diffstat.deletions++;

The deletions definitely shouldn't be counted in the patch header ;)

What about #6 doesn't make sense? What he was trying to do was colorize the extra information at the top of a patch that comes with git format-patch. This doesn't haven't anything to do with the actual diff file and header information (which is why I separated it out).

+++ b/dreditor.user.js¶
@@ -1191,13 +1192,26 @@ Drupal.dreditor.patchReview.behaviors.setup = function (context, code) {¶
    // Colorize file diff lines.¶
     if (line.match(/^((Index|===|RCS|retrieving|diff|\-\-\- |\+\+\+ |@@ ).*)$/i)) {¶
       classes.push('file');¶

Diff specific lines are already iterated line by line and are given a class of file. There's no need to have a header class as well, it's redundant. We could change the class from file to header if it's desired.

Sorry, but the original intention of this issue was to fix a false-positive formatting only. ;)

I'm not comfortable with separating out the entire patch header block through a multiline regex, since patch headers are clearly defined for the diff file format and patch applications are commonly very strict about following the rules. Thus, since the file format already defines by itself how it has to be structured and formatted, I think we should follow that and process the file as a serial stream.

There is actually a difference between the patch header and file headers. That is, a patch can contain multiple file headers, which are distinct from the patch header.

git patch serials only advance on that and allow multiple patches (patch headers) per [patch] file. That is, file > patches > files > hunks.

Hmm... I'm two for all today.. I must have seriously misunderstood this issue or just plain ignorant on the purpose of patch reviews :-/ Would you be willing to link to an example on d.o (if you know of one off the top of your head). Just curious to view them in dreditor to see what these multiple patch (not file) headers look like as I've never really seen them. I've only ever encountered standard diff patches and git format-patches, which I thought was just a diff with extra meta information at the top about the overall commit.

In any case, I was really just trying to mimic (as close to possible) the author block on drupalcode.org commit messages by making things a little easier to read. Sorry.

Was hoping this might get fixed with #2008964: Github styles for the diff, but I can still see that it's not in #1546724-1: Repository families.

This will need to get rerolled/refactored.

Updated issue summary: minor clarification.