Fix patch crash#945
Conversation
Libgit2 documents that the content isn’t NUL-terminated, so we can’t call strlen on it.
| } | ||
|
|
||
| if (j == 0) { | ||
| // calculate strlen only once for the first line of the first hunk. |
There was a problem hiding this comment.
This comment doesn't really describe what we were doing, and I'm not sure I understand what we're trying to do. So I'm not sure if this comment was wrong, or the code is wrong 😅
There was a problem hiding this comment.
This comment means that we don't want to calculate the length of content every iteration of the loop because that is numLinesInContent * strlen iterations. This slows down performance substantially.
There was a problem hiding this comment.
I guess my confusion is that (1) it's not like we were re-using the length, and (2) we're doing this for the first line of every hunk, not just the first line of the first hunk as the comment says.
There was a problem hiding this comment.
So the way that libgit2 represents the content pointer is like this:
Content ptr points to the start of the first line in the hunk, but continues until the end of the file.
content_len is the length of the line that we are looking at in the hunk
\n\ No newline at end of file\n" is located at the end of content ptr (not at content_len).
content ptr continues to the last line of the file because content ptr does not terminate at the end of lines.
What we need to know at any given line is whether there is no new line at the end of the file as this is used in nodegit for staging. When I worked on this feature it was requested that we preserve the api as closely as possible. The best way to preserve the api was to append the no new line flag on the end of lines where we have discovered a file does not contain a new line at the end.
There was a problem hiding this comment.
I'd like to investigate what a diff line looks like before we finalize this.
There was a problem hiding this comment.
No, you're right to be a bit confused. lol.
There was a problem hiding this comment.
Yes, the comment is completely wrong.
|
This fixes a crash we saw in Atom. The crashlog would look like this: |
|
This is ready 🙏 |
|
@implausible could you also 👀 this? |
|
This looks good to me 👍. I don't even know how I got to this solution anymore. So huge thanks to Josh for fixing yet another one of my errors. ❤️ |
|
@joshaber took a quick walk. I think I understand where I got this wrong now. The no new line flag only matters for the last hunk in the diff. I assumed it was required for every line of every hunk. This is wrong. Thanks again! |
|
Ah, I see. Thanks for clarifying @implausible! |
Libgit2 documents that
contentisn’t NUL-terminated, so we can’tcall
strlenon it. If we do, we’ll probably crash.