-
Notifications
You must be signed in to change notification settings - Fork 897
FileHistory Feature #963
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
FileHistory Feature #963
Conversation
@@ -0,0 +1,144 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
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.
This shouldn't have landed here. 🍊
I reverted the undesired changes to the solution and project files and also deleted the NuGet files that were added. The question is whether git settings should ignore these files. |
Hey @ThomasBarnekow thanks for this. It looks like the CI servers cringe a bit 😉 In order oavoid this, please specify some |
Hey @nulltoken, you are welcome. Can you be more specific on "specify[ing] some Signatures in your Commit calls)? |
@ThomasBarnekow I think leveraging this overload would do the trick. |
I also saw that the MetaFixture test complained about missing constructors to support abstraction in test contexts. It also wanted the public methods and properties to be virtual. In addition to adding the Signature to the test commits, I also made those required changes. When I ran the tests on my machine, my test cases and the MetaFixture test cases all passed. However, other test cases failed but I can't see any connection between my stuff and those test cases. |
The last change fixes an error I've seen on the CI system. One challenge is that my tests passed nicely on my machine but failed on the CI server. Thus, it would be helpful to have some guide saying how tests should be set up to make sure they also run on the CI server, i.e., they should also fail on my machine if they would fail on the CI server. BTW, a few of the tests, e.g.,
How do I deal with that? |
Hmmm. This is unexpected. Can you please share the whole stack trace? Would it possible that a local service (Antivirus, Search indexer, ...) may hold a temporary lock of those files? |
The only case I can think of relies on the methods/tests that would depend on configuration. The bulletproof approach to cope with this is to isolate the tests from ambient configuration by injecting some fake configuration stores. Another approach (as hinted here, is to pass explicit parameters to the methods and not let the library retrieve the missing required piece of data from config. |
Here's a full stack trace:
It is possible, though, that this is related to a local service. I've not found out which one that might be, because I still had 2-3 errors like the one above after deactivating my antivirus and malware services. One further thing I noted is that 42 "temporary" folders are left in the TestRepos subdirectory after one test run and a previously empty TestRepos folder. My folder contained a few hundred of these, though. Not sure whether that's expected. |
@ThomasBarnekow The stack trace shows that the failure happened upon the removal of the test repo. Running a full test suite would create a lot more than 42 temporary folders. So I'd say most of them have been successfully removed. Take a look at the |
@nulltoken Indeed, I saw that many test folders were created and successfully removed. The only "issue" I'd have is that, after a while, the leftovers might become a problem. Your guarding against IOException does not mean the test will be passed, though. They are reported as a failure. Not sure whether that should be considered a failed test case. Would it make sense to include a "housekeeping" test case that is run at the very end to remove whatever folders are left? I would not have any issue with some leftover folders. But during the limited time I've been using this, hundreds of folders have been created that way. |
I agree. But to my best knowledge, this should only happen when an external process (anti-virus, ...) races (and wins) against the test runner.
Actually, when an
IMHO I don't think that failing to clean the test repo once the test is done should be seen as a failure.
Why not? But I think this may be a little out of the scope of this PR. How about logging a new issue in order to keep track of it? As it may take some time to iron in a way that's sensible (For instance, NCrunch and XUnit v2 run tests in parallel. In that perspective, it may be tricky to determine the last test. Another option would be to add a finalizer to |
All agreed. And these failures are totally unrelated to my code. I'd be interested to have it pulled as soon as possible and don't see any issue standing in its way. Mit freundlichen Grüßen / Kind regards Dr. Thomas Barnekow Sent from my iPhone
|
if (lastSha != null) | ||
{ | ||
CommitFilter endOfPreviousRangeFilter = GetCommitFilter(filter, commitRange.Last()); | ||
Commit nextCommit = commitLog.QueryBy(endOfPreviousRangeFilter).Skip(1) |
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.
Nit: Can we have these like one linq chain each line?
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.
We probably could and I actually thought about that (because I've written a good number of pure functional transforms for XML and like that style). But I decided to go down this route because I felt it would be easier to read.
What benefits do you see in "having these like one line chain each line"?
Thanks a lot for this contribution! I'm actually not sure about the implementation (as the git native Could you please take a look at the very basic use cases that are described here and create some additional unit tests that would verify them? You can take a peek at |
return repo.Commit("Changed " + relativePath, repo.Config.BuildSignature(DateTimeOffset.Now)); | ||
} | ||
|
||
protected string CreateFile(string repoPath, string relativePath, string text) |
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.
We've already got a very similar helper method named Touch()
. Could you please rather leverage it?
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.
Done.
@nulltoken I've just cloned your follow-test repo and quickly run the three test cases using my interactive test program (so no unit tests yet). With the exception of the first one, the results are the same. The first one is currently not handled by my feature because it includes a rename AND a change in one commit. Thus, it only shows the top-most commit but not the second one. The question is how I can safely establish that these two commits or objects are part of the latest object's (so-renamed.txt) history? Currently, I'm only using the file name and the SHA, both of which are changed when renaming AND updating. |
@nulltoken Another question would be whether this feature should do exactly what My application, for example, would be to identify all commits in which a Word document's contents were changed and then show potentially all document revisions side by side (with track changes highlighting changes between commits). If a merge commit included manual merge activities, that would be relevant as well. |
The |
Thanks, that's currently the most important part for me. Does that also work for binaries? I'd be specifically interested in Word documents, which are essentially zipped collections of XML files. |
@nulltoken Squashed all commits into one and pushed. Quick question. I have ReSharper installed and this always updates the existing LibGit2Sharp.sln.DotSettings. I've never committed those changes. Do you have an idea why that happens? Would it be safe to add the changes? It adds the following line:
|
It's in. Thanks for this amazing job! ✨ ✨ ✨ ✨ ✨ ✨ ✨ |
Thanks! And you are most welcome. |
I'm also regularly bothered by this. It's happening when a new version of Resharper is installed. Let's merge the change it. Could you please send a PR to take care of this? |
Sure, will do.
|
Published as NuGet pre-release package |
Has the usage for this feature changed from the first comment? |
@linkerro
(Please correct me if I'm wrong) |
That is correct. I started reading the tests and got it working, but this should stay here for reference, if not in the wiki :) |
@ThomasBarnekow edited |
Hi @ThomasBarnekow thanks this gr8 library. Unfortunately i didn't find FileHistory class. i looked into code and found this call as "internal" access. Have this class been removed from public interface? i am creating a VS addin which requires to have history of files. i would like to know if this class is still there. |
His last comment literally points out that @quisse last comment is the current way of achieving that. |
yahh.. thanks @txdv . i got it. i am using below code which always return zero length LogEntry enumerator. Am i using it correct way @quisse public static void GetFileHistory(string filepath,string repopath) /// here is what i get |
Are you passing the entire path? If you have a file in a subdirectory, you need to pass Path.Combine("diectory", "file") to the QueryBy |
As @deepak-arya said Repository.Commits.QueryBy always returns an empty collection. What is the resolution to that? I pass the absolute path to the method. |
@mirsaeedi Please ask a question on StackOverflow for support. |
Based on my research, this feature was discussed in the past but I could not find the functionality in libgit2sharp. As I needed it as well, I implemented it and would like to share it.
The
FileHistory
class provides a history of commits in which a given file was created, changed, or renamed. Based on that commit history, it also provides the collection of Blobs representing a change of the file's contents, i.e., ignoring renames.I've also implemented a
FileHistoryExtensions
class that provides extension methods for theRepository
class. These could potentially be moved to theRepositoryExtensions
class.Usage would be as follows: