Skip to content

fix a bug introduced in 8a23597b#5295

Merged
pks-t merged 1 commit into
libgit2:masterfrom
romkatv:fix-diff-res
Nov 5, 2019
Merged

fix a bug introduced in 8a23597b#5295
pks-t merged 1 commit into
libgit2:masterfrom
romkatv:fix-diff-res

Conversation

@romkatv

@romkatv romkatv commented Nov 5, 2019

Copy link
Copy Markdown
Contributor

No description provided.

romkatv referenced this pull request Nov 5, 2019
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.

@pks-t pks-t left a comment

Copy link
Copy Markdown
Member

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)

Comment thread src/diff_generate.c
(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)

Copy link
Copy Markdown
Member

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!

@pks-t pks-t merged commit 45c8d3f into libgit2:master Nov 5, 2019
@ethomson

ethomson commented Nov 5, 2019

Copy link
Copy Markdown
Member

Thanks @romkatv!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants