Skip to content

Conversation

arturcic
Copy link
Member

@arturcic arturcic commented Oct 8, 2025

Follow up for #4681

Simplifies branch/tag cache keys by including the remote name and SHA.
@HHobeck
Copy link
Contributor

HHobeck commented Oct 8, 2025

@pvanbuijtene: Thank you for your contribution and the work you have done.

I have following remarks/suggestions:

  • The classes Commit, Branch and so on are wrapper classes around the LibGit2Sharp objects. I would say the wrapper classes should not have a reference to GitRepostory. Is it an idea to extract the functionality of caching in a separate class and use this instead?
  • Because of the nature of a wrapper class I would expect that the LibGit2Sharp objects are reused as well and the hash code is the same (otherwise we would have still a memory problem). Probably you can use the hash code instead of using an arbitrary string (needs to be tested).

@arturcic
Copy link
Member Author

arturcic commented Oct 8, 2025

@pvanbuijtene: Thank you for your contribution and the work you have done.

I have following remarks/suggestions:

  • The classes Commit, Branch and so on are wrapper classes around the LibGit2Sharp objects. I would say the wrapper classes should not have a reference to GitRepostory. Is it an idea to extract the functionality of caching in a separate class and use this instead?
  • Because of the nature of a wrapper class I would expect that the LibGit2Sharp objects are reused as well and the hash code is the same (otherwise we would have still a memory problem). Probably you can use the hash code instead of using an arbitrary string (needs to be tested).

That was my idea to move the functionality from the GitCache class back into the GitRepository as I think the repository wrapper should handle the caching mechanism as it's the abstraction of the database which maintains the collections and the cache, but we can reconsider that

Renames `GetOrCreate` to `GetOrWrap` to better reflect its caching behavior.
@pvanbuijtene
Copy link

Happy to help :)

@arturcic as I understand you will implement the suggestions, feel free to ping me in case you want to have some changes tested against a repository causing issues.

@arturcic
Copy link
Member Author

arturcic commented Oct 8, 2025

@pvanbuijtene thank you, yeah, I implemented the suggestions so far in this PR, but it's a draft for now, I want to test caching the other collections as well before having this merged

refactors the project to improve code organization and maintainability.
@arturcic arturcic force-pushed the fix/cache-git-objects branch from 4decb3e to 0c068f7 Compare October 9, 2025 13:17
Copy link

sonarqubecloud bot commented Oct 9, 2025

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