Skip to content
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

Repository.Commits.QueryBy is missing changes #1752

Open
hybridherbst opened this issue Jan 18, 2020 · 3 comments · May be fixed by #1753
Open

Repository.Commits.QueryBy is missing changes #1752

hybridherbst opened this issue Jan 18, 2020 · 3 comments · May be fixed by #1753

Comments

@hybridherbst
Copy link

hybridherbst commented Jan 18, 2020

Reproduction steps

This seems to have been brought up a couple of times already (a quick search showed #1401, #1591, #963 (comment)).

This is pretty weird: QueryBy returns 0 LogEntries, while going through all commits and diffing to the previous one shows the same changes/additions/renames as e.g. the SourceTree or Fork UI for the exact same path as for the QueryBy call.

Note that it only does that for some files, most files work fine. I haven't found a common pattern so far (e.g. specific characters, or only merges, or ...) but it looks like QueryBy is omitting merges and that might cause problems.

image
(commits in reverse order)

Expected behavior

QueryBy should return all changes to the file (as outlined in #963 where it was originally added).

Actual behavior

QueryBy returns 0 LogEntries.

Version of LibGit2Sharp (release number or SHA1)

git2-106a5f2

Operating system(s) tested; .NET runtime tested

Win 10, .NET 4.6

EDIT: Maybe I'm understanding the code wrong, but taking a quick peek at https://github.com/libgit2/libgit2sharp/blob/master/LibGit2Sharp/Core/FileHistory.cs#L137, I think QueryBy in fact ignores all commits that have more than 1 parent (e.g. all merges).

@hybridherbst
Copy link
Author

hybridherbst commented Jan 19, 2020

After looking into the code and fixing it for my usecase (showing ALL commits touching the file, as I had expected), I think there are multiple issues and ideally the behaviour should be configurable to avoid confusion:

  • QueryBy ignores commits with multiple parents (e.g. merges) and never walks into both parents
  • QueryBy stops when the current tree does not contain the file

This leads to the following case failing:

  • add A on "master"
  • do some changes on A
  • branch off a branch "develop"
  • change A on "develop"
  • go back to "master"
  • remove A on "master"
  • merge "develop" into "master", choose "theirs" on conflict resolution

20200119-145626_Fork

Now "master" contains A again, which has been changed quite a lot.
But the current QueryBy implementation hides all those changes, returning 0 commits, since it does not see the merge and stops searching through master where the file was not in the tree (when it was deleted).

I think ignoring the merge is a bug. As far as I'm aware merges can contain anything - changes to the file, file additions, deletions ... it's pretty easy to hide a lot of changes this way:
image
(QueryBy still returns 0 changes on TestScript2)

In some cases QueryBy will return some commits but falsely ignore most of them:
image
(QueryBy returns only 1 commit - "reset TestScript2 to earlier state". All commits seen here do something with TestScript2.)

From a performance point of view, I understand that stopping when the file is not in the tree improves performance a lot on large repos. I think the code needs to become a bit smarter here and only stop if the file is not in the tree and there has been no commit seen yet which "added" the file.

@ErikSchierboom
Copy link

I've recently also hit this issue, and it would be great if there would be a fix.

@lofcz
Copy link

lofcz commented Aug 2, 2021

tested, with this PR merges were - at least in my cases - correctly reffed. For a quick test you can swap nuget to: https://www.nuget.org/packages/LibGit2SharpLofcz/0.27.0-preview-0113 (built against this PR and net6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants