Add checkout file#165
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
- Coverage 88.55% 86.78% -1.78%
==========================================
Files 62 62
Lines 2893 2973 +80
Branches 361 372 +11
==========================================
+ Hits 2562 2580 +18
- Misses 331 393 +62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
If you do a git2cpp checkout v1.0 v2.0then it looks like the second argument is ignored. Should we explicitly check for this and error out rather than silently ignoring the second arg? |
That would be consistent with git behavior. |
JohanMabille
left a comment
JohanMabille
left a comment
There was a problem hiding this comment.
LGTM once the error handling is added.
for more information, see https://pre-commit.ci
ianthomas23
left a comment
ianthomas23
left a comment
There was a problem hiding this comment.
Just a couple of comments about the tests.
| second_file.write_text("second content") | ||
|
|
||
| add_cmd = [git2cpp_path, "add", "second.txt"] | ||
| subprocess.run(add_cmd, cwd=tmp_path, text=True) |
There was a problem hiding this comment.
We should either explicitly check the returncode is zero, or add a check=True to this subprocess call.
| add_cmd = [git2cpp_path, "add", "second.txt"] | ||
| subprocess.run(add_cmd, cwd=tmp_path, text=True) | ||
| commit_cmd = [git2cpp_path, "commit", "-m", "Add second file"] | ||
| subprocess.run(commit_cmd, cwd=tmp_path, text=True) |
There was a problem hiding this comment.
Check return code here too.
| second_file.write_text("second content") | ||
|
|
||
| add_cmd = [git2cpp_path, "add", "second.txt"] | ||
| subprocess.run(add_cmd, cwd=tmp_path, text=True) |
There was a problem hiding this comment.
Check return code, and 2 lines below.
| } | ||
| } | ||
|
|
||
| void checkout_subcommand::checkout_files( |
There was a problem hiding this comment.
Could be renamed checkout_head_files
| throw_if_error(git_checkout_head(repo, &options)); | ||
| } | ||
|
|
||
| void checkout_subcommand::checkout_paths( |
There was a problem hiding this comment.
Could be renamed checkout_tree_files or checkout_ref_files
| return; | ||
| } | ||
|
|
||
| if (!pathspecs.empty()) |
There was a problem hiding this comment.
Prefer else if instead of multi return statements. (Early return to handle error / specific case is usually ok, but it makes it harder to nuderstand the code when used to encode the logic).
| return; | ||
| } | ||
| catch (const git_exception& e) | ||
|
|
||
| // Else treat as files | ||
| for (const auto& p : pathspecs) |
There was a problem hiding this comment.
Here too prefer an else branch rather than a return statement.
for more information, see https://pre-commit.ci
3 participants
Add
checkout file