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

3148 chRemote #3149

Merged
merged 5 commits into from Apr 27, 2024
Merged

3148 chRemote #3149

merged 5 commits into from Apr 27, 2024

Conversation

levBagryansky
Copy link
Member

@levBagryansky levBagryansky commented Apr 26, 2024

Closes #3148


PR-Codex overview

The focus of this PR is to refactor the CommitHash interface to extend Scalar<String> and make improvements to the ChRemote class for better performance and thread safety.

Detailed summary

  • CommitHash interface now extends Scalar<String>
  • Removed unnecessary import in ChRemote
  • Improved thread safety in ChRemoteTest with new test method

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@levBagryansky
Copy link
Member Author

@maxonfjvipon please review. This test does not cover current vulnerability, however it protect us from thread-unsafety in future.
Current vulnerability is hard to test. We have similar case in #3114 (comment) and in #3123

/**
* Hash of tag.
*
* @since 0.28.11
*/
public interface CommitHash {
@FunctionalInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@levBagryansky why do we need it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxonfjvipon I added it because this actually is a functional interface.

/**
* Hash of tag.
*
* @since 0.28.11
*/
public interface CommitHash {
@FunctionalInterface
public interface CommitHash extends Scalar<String> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxonfjvipon I inhereted it firstly because it has the same method so it really can be inherited. Also it allowed to write

                    Stream.generate(
                        () -> new ChRemote("0.23.19")
                    ).limit(threads).collect(Collectors.toList())

and this is cool.

@levBagryansky
Copy link
Member Author

@yegor256 please take a look

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Contributor

rultor commented Apr 27, 2024

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 0c3f311 into objectionary:master Apr 27, 2024
18 checks passed
@rultor
Copy link
Contributor

rultor commented Apr 27, 2024

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 17min)

@levBagryansky levBagryansky deleted the 3148_chremore branch April 27, 2024 16:40
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.

ChRemote has threadsafety vulnerability
4 participants