Conversation
SandrineP
force-pushed
the
diff_cmd
branch
3 times, most recently
from
2acbb53 to
e8e7b97
Compare
|
When building locally on macos I see /Users/iant/github/git2cpp/src/utils/common.cpp:114:23: error: implicit instantiation of undefined template 'std::basic_stringstream<char>'
114 | std::stringstream buffer;
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__fwd/sstream.h:30:28: note: template is declared here
30 | class _LIBCPP_TEMPLATE_VIS basic_stringstream;
| ^
1 error generated.To fix this I need to add |
| if (repo.state() != GIT_REPOSITORY_STATE_NONE) | ||
| { | ||
| throw std::runtime_error("Cannot checkout, repository is in unexpected state"); | ||
| std::runtime_error("Cannot checkout, repository is in unexpected state"); |
There was a problem hiding this comment.
It looks like was deleted by accident?
src/subcommand/diff_subcommand.cpp
Outdated
|
|
||
| static int colour_printer([[maybe_unused]] const git_diff_delta* delta, [[maybe_unused]] const git_diff_hunk* hunk, const git_diff_line* line, void* payload) | ||
| { | ||
| bool* use_colour = reinterpret_cast<bool*>(payload); |
There was a problem hiding this comment.
I think it would be better to store bool use_colour rather than bool*. As it stands, later in this function both *use_colour and use_colour (without the dereference) are used, and they can't both work correctly!
| if (m_summary_flag) | ||
| { | ||
| format = GIT_DIFF_STATS_INCLUDE_SUMMARY; | ||
| } |
There was a problem hiding this comment.
This implicitly sets a "priority" order on the flags; is it legit to pass all those flags together? If so, I would rewrite it in the reverse order with if/else so that we directly init format with the correct value. Otherwise I would first check that we have at most one flag set.
There was a problem hiding this comment.
git accepts them all together and prints all the outputs, but I'm not sure that's what we want to reproduce. Should we just return a warning when more than one of those flags is provided ?
There was a problem hiding this comment.
Let's keep git's behavior AND warn the user which flag was used.
I read too fast and thought git was using only one flag based on some priority. However, I prefer the implementation here where we produce only one output.
| if (m_no_colour_flag) | ||
| { | ||
| use_colour = false; | ||
| } |
There was a problem hiding this comment.
Same remark here, is it legit to pass both flags?
There was a problem hiding this comment.
git accepts both and apply the last one, but same, not sure that's what we want...
3 participants
Add diff subcommand