Conversation
src/wrapper/index_wrapper.cpp
Outdated
|
|
||
| void index_wrapper::add_impl(std::vector<std::string> patterns) | ||
| { | ||
| git_strarray array{new char*[patterns.size()], patterns.size()}; |
There was a problem hiding this comment.
This leaks. I think it would be worth having a wrapper of git_strarray, that correctly manages the memory and use it here. It will be useful for other commands anyway.
src/wrapper/index_wrapper.cpp
Outdated
|
|
||
| void index_wrapper::add_impl(std::vector<std::string> patterns) | ||
| { | ||
| git_strarray_wrapper array=git_strarray_wrapper(patterns); |
There was a problem hiding this comment.
| git_strarray_wrapper array=git_strarray_wrapper(patterns); | |
| git_strarray_wrapper array{patterns}; |
| #define GIT2CPP_VERSION_MAJOR 0 | ||
| #define GIT2CPP_VERSION_MINOR 0 | ||
| #define GIT2CPP_VERSION_PATCH 1 | ||
| #define GIT2CPP_VERSION_PATCH 2 |
There was a problem hiding this comment.
The 0.0.2 has been released but we forgot to change it there, so it's to catch up.
src/subcommand/add_subcommand.hpp
Outdated
|
|
||
| private: | ||
| bool all_flag = false; | ||
| std::vector<std::string> add_files; |
There was a problem hiding this comment.
Convention: data member names should start with m_ (or p_) so it is easy to distinguish them from local variables.
There was a problem hiding this comment.
m_ stands for "member" and p_ stands for what ?
test/test_add.py
Outdated
| def test_add(git2cpp_path, all_flag): | ||
| with open("./test/mook_file.txt", "x") as f: | ||
| pass | ||
| f.close() |
There was a problem hiding this comment.
This line isn't needed as the f will be automatically closed when the with ... block is exited. The same applies to 4 lines below.
|
|
||
| os.remove("./test/mook_file.txt") | ||
| os.remove("./test/mook_file_2.txt") | ||
| subprocess.run(cmd_add, capture_output=True, text=True) |
There was a problem hiding this comment.
Is this line just to undo the add, to leave the test directory at the end the same as it was at the start? If so can you add a comment saying this, otherwise it will look suspicious in future to have this line without any following assert.
There was a problem hiding this comment.
Yes, that's the reason why ! I'll write a comment.
3 participants
Add the
git addsub-command, with the--allflag.