Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions 22 generate/templates/manual/src/convenient_patch.cc
View file
Open in desktop
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;
Copy link
Collaborator Author

@joshaber joshaber Mar 9, 2016

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

@implausible implausible Mar 9, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@joshaber joshaber Mar 9, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@implausible implausible Mar 9, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@implausible implausible Mar 9, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@implausible implausible Mar 9, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@implausible implausible Mar 9, 2016

Choose a reason for hiding this comment

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

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.