Skip to content

blame_git: fix coalescing step never being executed - #4580

Merged
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/diff-like-git-coalesce
Apr 22, 2018
Merged

blame_git: fix coalescing step never being executed#4580
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/diff-like-git-coalesce

Conversation

@pks-t

@pks-t pks-t commented Mar 16, 2018

Copy link
Copy Markdown
Member

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

pks-t commented Mar 16, 2018

Copy link
Copy Markdown
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
pks-t force-pushed the pks/diff-like-git-coalesce branch from 78fd2a5 to 75203d0 Compare March 28, 2018 12:16
@ethomson

Copy link
Copy Markdown
Member

Thanks for digging in here; I agree that tests would be nice, but this is a strict improvement anyway.

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.

2 participants