Make strict dropCR behavior optional via ParseOptions#78
Make strict dropCR behavior optional via ParseOptions#78EmmanuelAC1 wants to merge 4 commits intosourcegraph:masterfrom
Conversation
|
Hi @keegancsmith! I created this PR to make the dropCR behavior optional but keeping current behavior by default for backward compatibility. I'm not able to assign reviewers myself. Could you help guide me on how to get this reviewed? |
keegancsmith
left a comment
keegancsmith
left a comment
There was a problem hiding this comment.
Sorry for the slow feedback, finally took a look. I think the issue here is we have a bunch of code that will fail since it won't expect a trailing \r in it. This is mostly around the bits of code which are not the patch file itself.
I think this PR is missing some decent end to end parsing tests.
I do think this is a worthwhile addition. Almost universally tools strip / ignore \r for diffs (mostly due to the unix heritage of this stuff). But we should provide a way to preserve it.
| &r.hunk.OrigStartLine, &r.hunk.OrigLines, | ||
| &r.hunk.NewStartLine, &r.hunk.NewLines, | ||
| } | ||
| header, section, err := normalizeHeader(string(line)) |
There was a problem hiding this comment.
with dropCR off this will fail since normalizeHeader's scanf won't expect to see \r at the end.
| // handle that case. | ||
| return r.hunk, &ParseError{r.line, r.offset, &ErrBadHunkLine{Line: line}} | ||
| } | ||
| if bytes.Equal(line, []byte(noNewlineMessage)) { |
There was a problem hiding this comment.
this would also fail with noCR false?
| } | ||
|
|
||
| var success bool | ||
| fd.OrigName, fd.NewName, success = parseDiffGitArgs(fd.Extended[0][len("diff --git "):]) |
There was a problem hiding this comment.
NewName will have a trailing CR now?
2 participants
Currently, the library strictly drops all trailing carriage returns (\r) from lines. This change introduces a mechanism to optionally preserve them while maintaining backward compatibility.
Detail:
ParseOptionsstruct with aKeepCRfield.*Optionsvariants for [ParseMultiFileDiff],ParseHunks, and their correspondingNew*Readerconstructors.lineReaderandreadLineto respect theKeepCRoption.KeepCR: false, ensuring no breaking changes for current users.KeepCRbehavior.