Skip to content

Exposed filter_list; Fixed discard lines to use filters #1414

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 5 commits into from
Dec 20, 2017
Merged

Exposed filter_list; Fixed discard lines to use filters #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12919bf
Exposed filter_list; Fixed discard lines to use filters
Dec 15, 2017
6f5c033
Fixed linter errors
Dec 15, 2017
9af9e4c
Fixed tests on windows
Dec 15, 2017
e2a740f
Fixed test errors
Dec 18, 2017
044465f
Addressed PR
Dec 19, 2017
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
79 changes: 73 additions & 6 deletions generate/input/descriptor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@
"args": {
"out": {
"isReturn": true,
"cppClassName": "GitBuf",
"cppClassName": "GitBuf",
"jsClassName": "Buffer",
"cType": "git_buf *",
"shouldAlloc": true
Expand Down Expand Up @@ -1008,19 +1008,76 @@
"hasConstructor": true,
"functions": {
"git_filter_list_apply_to_blob": {
"ignore": true
"async": true,
"return": {
"isErrorCode": true
},
"args": {
"out": {
"isReturn": true,
"shouldAlloc": true
},
"filters": {
"isSelf": true
}
}
},
"git_filter_list_apply_to_data": {
"ignore": true
"async": true,
"return": {
"isErrorCode": true
},
"args": {
"out": {
"isReturn": true,
"shouldAlloc": true
},
"filters": {
"isSelf": true
}
}
},
"git_filter_list_apply_to_file": {
"ignore": true
"async": true,
"return": {
"isErrorCode": true
},
"args": {
"out": {
"isReturn": true,
"shouldAlloc": true
},
"filters": {
"isSelf": true
}
}
},
"git_filter_list_free": {
"ignore": true
"async": true,
"args": {
"filters": {
"isSelf": true
}
}
},
"git_filter_list_load": {
"ignore": true
"async": true,
"return": {
"isErrorCode": true
},
"args": {
"filters": {
"isReturn": true,
"cType": "git_filter_list **"
},
"repo": {
"cType": "git_repository *"
},
"blob": {
"isOptional": true,
"cType": "git_blob *"
}
}
},
"git_filter_list_push": {
"ignore": true
Expand Down Expand Up @@ -1769,6 +1826,9 @@
}
},
"pathspec": {
"dependencies": [
"../include/str_array_converter.h"
],
"functions": {
"git_pathspec_match_list_free": {
"ignore": true
Expand Down Expand Up @@ -2001,6 +2061,7 @@
},
"remote": {
"dependencies": [
"../include/str_array_converter.h",
"../include/remote_head.h"
],
"cType": "git_remote",
Expand Down Expand Up @@ -2282,6 +2343,9 @@
}
},
"reset": {
"dependencies": [
"../include/str_array_converter.h"
],
"functions": {
"git_reset": {
"args": {
Expand Down Expand Up @@ -2615,6 +2679,9 @@
}
},
"tag": {
"dependencies": [
"../include/str_array_converter.h"
],
"selfFreeing": true,
"functions": {
"git_tag_foreach": {
Expand Down
17 changes: 10 additions & 7 deletions generate/input/libgit2-supplement.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,16 @@
"git_annotated_commit_lookup"
]
],
[
"filter_list",
[
"git_filter_list_apply_to_blob",
"git_filter_list_apply_to_data",
"git_filter_list_apply_to_file",
"git_filter_list_free",
"git_filter_list_load"
]
],
[
"filter_source",
[
Expand Down Expand Up @@ -772,13 +782,6 @@
},
"filter": {
"functions": [
"git_filter_list_apply_to_blob",
"git_filter_list_apply_to_data",
"git_filter_list_apply_to_file",
"git_filter_list_contains",
"git_filter_list_free",
"git_filter_list_length",
"git_filter_list_load",
"git_filter_list_new",
"git_filter_list_push",
"git_filter_list_stream_blob",
Expand Down
6 changes: 3 additions & 3 deletions generate/templates/manual/include/git_buf_converter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
#include <v8.h>

#include "nan.h"
#include "git2/strarray.h"
#include "git2/buffer.h"

using namespace v8;

class StrArrayConverter {
class GitBufConverter {
public:
static git_strarray *Convert (v8::Local<v8::Value> val);
static git_buf *Convert(v8::Local<v8::Value> val);
};

#endif
10 changes: 6 additions & 4 deletions generate/templates/manual/src/git_buf_converter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
#include <nan.h>
#include <node.h>
#include <string>
#include <cstring>

#include "../include/git_buf_converter.h"
#include "git2/buffer.h"

using namespace v8;
using namespace node;

git_buf *StrArrayConverter::Convert(Local<v8::Value> val) {
git_buf *GitBufConverter::Convert(Local<v8::Value> val) {
if (val->IsString() || val->IsStringObject()) {
string v8String = ConvertString(val->ToString());
v8::String::Utf8Value param1(val->ToString());
std::string v8String = std::string(*param1);

const size_t size = sizeof(git_buf);
uint8_t* memory = reinterpret_cast<uint8_t *>(malloc(size));
git_buf *result = reinterpret_cast<git_buf *>(memory);
size_t stringLength = v8String.length() - 1;
size_t stringLength = v8String.length();

memory = reinterpret_cast<uint8_t *>(malloc(stringLength));

memcpy(memory, v8String.c_str(), stringLength);

result->asize = stringLength;
result->size = stringLength;
result->ptr = reinterpret_cast<char *>(memory);
return result;
Expand Down
1 change: 1 addition & 0 deletions generate/templates/templates/binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"src/convenient_patch.cc",
"src/convenient_hunk.cc",
"src/filter_registry.cc",
"src/git_buf_converter.cc",
"src/str_array_converter.cc",
"src/thread_pool.cc",
{% each %}
Expand Down
53 changes: 50 additions & 3 deletions lib/repository.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ var Commit = NodeGit.Commit;
var normalizeOptions = NodeGit.Utils.normalizeOptions;
var shallowClone = NodeGit.Utils.shallowClone;
var path = require("path");
var Filter = NodeGit.Filter;
var FilterList = NodeGit.FilterList;
var Reference = NodeGit.Reference;
var Remote = NodeGit.Remote;
var Repository = NodeGit.Repository;
Expand Down Expand Up @@ -37,7 +39,6 @@ function applySelectedLinesToTarget
var oldIndex = 0;
var linesPromises = [];

// split the original file into lines
var oldLines = originalContent.toString().split("\n");

// if no selected lines were sent, return the original content
Expand Down Expand Up @@ -121,6 +122,7 @@ function applySelectedLinesToTarget
}
}
}

return newContent;
});
}
Expand Down Expand Up @@ -155,7 +157,7 @@ function getPathHunks(repo, index, filePath, isStaged, additionalDiffOptions) {
if (!(status & NodeGit.Status.STATUS.WT_MODIFIED) &&
!(status & NodeGit.Status.STATUS.INDEX_MODIFIED)) {
return Promise.reject
("Selected staging is only available on modified files.");
("Selected staging is only available on modified files.");
}
return diff.patches();
});
Expand Down Expand Up @@ -729,15 +731,34 @@ Repository.prototype.discardLines =
var fullFilePath = path.join(repo.workdir(), filePath);
var index;
var originalContent;
var filterList;

return repo.refreshIndex()
.then(function(indexResult) {
index = indexResult;
return FilterList.load(
repo,
null,
filePath,
Filter.MODE.CLEAN,
Filter.FLAG.DEFAULT
);
})
.then(function(_filterList) {
filterList = _filterList;

if (filterList) {
return filterList.applyToFile(repo, filePath);
}

return fse.readFile(fullFilePath, "utf8");
})
.then(function(content) {
originalContent = content;
if (filterList) {
filterList.free();
Copy link
Copy Markdown
Member

@implausible implausible Dec 19, 2017

Choose a reason for hiding this comment

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

My first inclination was to express how we shouldn't do this, and that we should strive to improve nodegit's automatic memory management... My second inclination was that we should really throw away the automatic memory management in nodegit and just expose the free methods on anything that needs freeing and leave memory management up to the consumer of nodegit. It's a little more cognitive on the programmer, but it is way more straightforward and potentially complete, compared to what we have now.

Copy link
Copy Markdown
Contributor Author

@cjhoward92 cjhoward92 Dec 19, 2017

Choose a reason for hiding this comment

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

You think I should change it now just to be consistent?

Copy link
Copy Markdown
Member

@implausible implausible Dec 20, 2017

Choose a reason for hiding this comment

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

No. I am pretty certain we're going to move away from our current garbage collection system.

filterList = null;
}

return getPathHunks(repo, index, filePath, false, additionalDiffOptions);
})
Expand All @@ -747,7 +768,33 @@ Repository.prototype.discardLines =
);
})
.then(function(newContent) {
return fse.writeFile(fullFilePath, newContent);
return FilterList.load(
repo,
null,
filePath,
Filter.MODE.SMUDGE,
Filter.FLAG.DEFAULT
)
.then(function(_filterList) {
filterList = _filterList;
if (filterList) {
/* jshint ignore:start */
// We need the constructor for the check in NodeGit's C++ layer
// to accept an object, and this seems to be a nice way to do it
return filterList.applyToData(new String(newContent));
/* jshint ignore:end */
}

return newContent;
});
})
.then(function(filteredContent) {
if (filterList) {
filterList.free();
filterList = null;
}

return fse.writeFile(fullFilePath, filteredContent);
});
};

Expand Down
Loading