FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix patch crash by joshaber · Pull Request #945 · nodegit/nodegit · GitHub

  1. nodegit
  2. nodegit

Repository navigation

Fix patch crash #945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
implausible merged 3 commits into from
Mar 9, 2016
Conversation 13 Commits 3 Checks 0 Files changed
Merged

Fix patch crash #945

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b5e9fb
Bail if there's an error.
joshaber Mar 9, 2016
049b501
Use the content length libgit2 gives us.
joshaber Mar 9, 2016
a98ae5c
*sigh* Windows.
joshaber Mar 9, 2016
File filter

Filter by extension

Filter by extension .cc  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
22 changes: 14 additions & 8 deletions generate/templates/manual/src/convenient_patch.cc
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,12 @@ PatchData *createFromRaw(git_patch *raw) {

for (unsigned int i = 0; i < patch->numHunks; ++i) {
HunkData *hunkData = new HunkData;
const git_diff_hunk *hunk;
git_patch_get_hunk(&hunk, &hunkData->numLines, raw, i);
const git_diff_hunk *hunk = NULL;
int result = git_patch_get_hunk(&hunk, &hunkData->numLines, raw, i);
if (result != 0) {
continue;
}

hunkData->hunk.old_start = hunk->old_start;
hunkData->hunk.old_lines = hunk->old_lines;
hunkData->hunk.new_start = hunk->new_start;
Expand All @@ -72,17 +76,19 @@ PatchData *createFromRaw(git_patch *raw) {
bool EOFFlag = false;
for (unsigned int j = 0; j < hunkData->numLines; ++j) {
git_diff_line *storeLine = (git_diff_line *)malloc(sizeof(git_diff_line));
const git_diff_line *line;
git_patch_get_line_in_hunk(&line, raw, i, j);
const git_diff_line *line = NULL;

joshaber Mar 9, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 😅

implausible Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

joshaber Mar 9, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

implausible Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

implausible Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd like to investigate what a diff line looks like before we finalize this.

implausible Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, you're right to be a bit confused. lol.

implausible Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, the comment is completely wrong.
int result = git_patch_get_line_in_hunk(&line, raw, i, j);
if (result != 0) {
continue;
}

if (j == 0) {
// calculate strlen only once for the first line of the first hunk.
int calculatedContentLength = strlen(line->content);
int calculatedContentLength = line->content_len;
if (
calculatedContentLength > noNewlineStringLength &&
!strcmp(
!strncmp(
&line->content[calculatedContentLength - noNewlineStringLength],
"\n\\ No newline at end of file\n"
"\n\\ No newline at end of file\n", (std::min)(calculatedContentLength, noNewlineStringLength)
)) {
EOFFlag = true;
}
Expand Down
Back | FazBrowse Home | New Git URL