blame_git: fix coalescing step never being executed - #4580
Merged
Conversation
Member
Author
|
Fixes #2658. If anybody could come up with a test I'd be happy to include that. Still trying to wrap my head around to our blaming implementation |
Since blame has been imported from git.git and had its first share of refactorings in b6f60a4 (Clean up ported code, 2013-09-21), the code is actually not doing the coalescing step of the generated blame. While the code to do the coalescing does exist, it is never being called as the function `git_blame__like_git` will directly return from its `while (true)` loop. The function that was being imported from git.git was the `assign_blame` function from "builtin/blame.c" from 717d1462b (git-blame --incremental, 2007-01-28), which hasn't really changed much. Upon taking an initial look, one can seet hat `coalesce` is actually never getting called in `assign_blame`, as well, so one may assume that not calling `coalesce` by accident is actually the right thing. But it is not, as `coalesce` is being called ever since cee7f245d (git-pickaxe: blame rewritten., 2006-10-19) after the blame has been done in the caller of `assign_blame`. Thus we can conclude the code of libgit2 is actually buggy since forever. To fix the issue, simply break out of the loop instead of doing a direct return. Note that this does not alter behaviour in any way visible to our tests, which is unfortunate. But in order to not diverge from what git.git does, I'd rather adapt to how it is being done upstream in order to avoid breaking certain edge cases than to just remove that code.
pks-t
force-pushed
the
pks/diff-like-git-coalesce
branch
from
March 28, 2018 12:16
78fd2a5 to
75203d0
Compare
Member
|
Thanks for digging in here; I agree that tests would be nice, but this is a strict improvement anyway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since blame has been imported from git.git and had its first share of
refactorings in b6f60a4 (Clean up ported code, 2013-09-21), the code
is actually not doing the coalescing step of the generated blame. While
the code to do the coalescing does exist, it is never being called as
the function
git_blame__like_gitwill directly return from itswhile (true)loop.The function that was being imported from git.git was the
assign_blamefunction from "builtin/blame.c" from 717d1462b (git-blame --incremental,
2007-01-28), which hasn't really changed much. Upon taking an initial
look, one can seet hat
coalesceis actually never getting called inassign_blame, as well, so one may assume that not callingcoalesceby accident is actually the right thing. But it is not, as
coalesceisbeing called ever since cee7f245d (git-pickaxe: blame rewritten.,
2006-10-19) after the blame has been done in the caller of
assign_blame. Thus we can conclude the code of libgit2 is actuallybuggy since forever.
To fix the issue, simply break out of the loop instead of doing a direct
return. Note that this does not alter behaviour in any way visible to
our tests, which is unfortunate. But in order to not diverge from what
git.git does, I'd rather adapt to how it is being done upstream in order
to avoid breaking certain edge cases than to just remove that code.