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.
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
fix a bug introduced in 8a23597b #5295
Conversation
While the `DIFF_FROM_ITERATORS` does make it shorter to implement the various `git_diff_foo_to_bar` functions, it is a complex and unreadable beast that implicitly assumes certain local variable names. This is not something desirable to have at all and obstructs understanding and more importantly debugging the code by quite a bit. The `DIFF_FROM_ITERATORS` macro basically removed the burden of having to derive the options for both iterators from a pair of iterator flags and the diff options. This patch introduces a new function that does the that exact and refactors all callers to manage the iterators by themselves. As we potentially need to allocate a shared prefix for the iterator, we need to tell the caller to allocate that prefix as soon as the options aren't required anymore. Thus, the function has a `char **prefix` out pointer that will get set to the allocated string and subsequently be free'd by the caller. While this patch increases the line count, I personally deem this to an acceptable tradeoff for increased readbiblity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. I'm merging as the CI error is unrelated (badssl once again)
@@ -1371,7 +1371,7 @@ int git_diff_tree_to_index( | |||
if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, iflag, &b_opts, iflag, opts)) < 0 || | |||
(error = git_iterator_for_tree(&a, old_tree, &a_opts)) < 0 || | |||
(error = git_iterator_for_index(&b, repo, index, &b_opts)) < 0 || | |||
(error = git_diff__from_iterators(&diff, repo, a, b, opts) < 0)) | |||
(error = git_diff__from_iterators(&diff, repo, a, b, opts)) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yeah, that's an embarassing typo. The fix looks obviously correct to me, thanks a lot!
Thanks @romkatv! |
No description provided.