Add checkout file - #165
ianthomas23 merged 8 commits into
Conversation
Sorry, something went wrong.
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is 84.95575% with 17 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #165 +/- ##
==========================================
- Coverage 88.55% 86.78% -1.78%
==========================================
Files 62 62
Lines 2893 2974 +81
Branches 361 372 +11
==========================================
+ Hits 2562 2581 +19
- Misses 331 393 +62 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
If you do a checkout with 2 positional arguments and the first is a branch or tag: 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? |
Sorry, something went wrong.
That would be consistent with git behavior. |
Sorry, something went wrong.
JohanMabille
left a comment
There was a problem hiding this comment.
LGTM once the error handling is added.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
ianthomas23
left a comment
There was a problem hiding this comment.
Just a couple of comments about the tests.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| void checkout_subcommand::checkout_files( |
There was a problem hiding this comment.
Could be renamed checkout_head_files
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| 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).
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
ianthomas23
left a comment
There was a problem hiding this comment.
LGTM. The 'detached HEAD' information is as I'd expect for my usual workflow.
Sorry, something went wrong.
Add checkout file