9

I just reviewed this crap suggested edit with a deceptive diff. Looking at the rendered output on the left hand side, it looks like there were a couple of stray spaces - in front of the lines opt[0].value = 'a'; and var select1 = document.createElement("select"); - which the edit removed:

Rendered Output View:

enter image description here

But looking at the markdown view, and the question itself, we can see that these stray spaces don't really exist:

Markdown view:

enter image description here

Actual question page:

enter image description here

It seems likely that this deceptive rendered output diff is what led three reviewers to accept the edit despite it being, in fact, pointless and destructive.

2
  • (After staring at the diff view for too long) Are you sure it is "pointless and destructive"? Do you infer that from the amount of deleted and inserted code? To me it really looks like the diff view is only over-emphasizing the inserted blank lines by considering everything after "deleted+new text".
    – Jongware
    May 30, 2015 at 13:59
  • @Jongware I'm sure, and the amount of deleted and inserted code has nothing to do with it. The edit wrongly indents all but the first line by four spaces, and adds an erroneous space at the beginning of the first line. "Destructive" might be too strong a word since the edit only changes whitespace, but the change is definitely bad.
    – Mark Amery
    May 30, 2015 at 14:17

1 Answer 1

2

The problem was that when creating the diffs we grouped subsequent newline characters together, so instead of "insert one newline", the diff engine output "remove one newline, then insert two newlines". And "remove something invisible" is shown as removing a space in the diff so you see that something was removed here.

From the next build on the newlines won't be grouped together anymore, so it will look like this instead:

screenshot of the diff after the change

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .