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

Update log crate and create shim method(s) between git2's tracing and log #1066

Closed
wants to merge 8 commits into from

Conversation

vcfxb
Copy link
Contributor

@vcfxb vcfxb commented Jul 10, 2024

This change came out of some work I'm doing on https://github.com/mitre/hipcheck, and it seemed like it would be useful upstream as well.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

I'm a little uncertain about adding this, because it makes log become a public dependency of git2. Updating public dependencies can cause problems, and I generally would like to avoid them if at all possible. It's also not clear to me why we would choose "log" over say "tracing" or any other option. I'm not yet sure if I would like to add that.

Generally, all changes should have a test.

Also, I'm a little uneasy with moving forward with this since it looks like the existing code has undefined behavior. Is that something you can maybe fix in a separate PR?

src/tracing.rs Outdated
///
/// [TraceLevel::Fatal] becomes [log::LevelFilter::Error] but otherwise all the conversions
/// are trivial.
pub const fn as_log_level_filter(self) -> log::LevelFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is theoretically useful for users writing a trace callback that needs to compare the log level given by git2 with the LevelFilter they're using with the log crate, however you're right that it's not enough utility to make the log dependency public, so I'll go ahead and remove it.

@vcfxb
Copy link
Contributor Author

vcfxb commented Jul 22, 2024

I'm a little uncertain about adding this, because it makes log become a public dependency of git2. Updating public dependencies can cause problems, and I generally would like to avoid them if at all possible. It's also not clear to me why we would choose "log" over say "tracing" or any other option. I'm not yet sure if I would like to add that.

Generally, all changes should have a test.

Also, I'm a little uneasy with moving forward with this since it looks like the existing code has undefined behavior. Is that something you can maybe fix in a separate PR?

I think you're right in your assessment of public dependencies and their issues. The best solution to this in my mind is to just make any function that interacts with a type from log private, so that we can still consume the crate's facade (much in the same way that src/cred.rs did before this PR) without publicly using log types. I'll make those changes and update this PR.

In regards to the existing undefined behavior, I'll have to take a look, but I would be happy to open another PR if I can find what you're referring to.

@vcfxb
Copy link
Contributor Author

vcfxb commented Jul 22, 2024

#1071 fixes the undefined behavior you mentioned.

@@ -75,6 +98,27 @@ pub fn trace_set(level: TraceLevel, cb: TracingCb) -> bool {
return true;
}

/// Passes [trace_set] a shim function to pass tracing info to the [log] crate.
pub fn trace_shim_log_crate() {
Copy link
Member

Choose a reason for hiding this comment

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

While this shim is nice, I don't think every git2 user needs this. git2, from my observation 1, has tried to be a thin layer upon libgit2. This kind of glue code is pretty application facing, and IMO should leave for users to decide.

Footnotes

  1. I am not really a so-called long-term maintainer


match self {
Self::None => None,
Self::Fatal | Self::Error => Some(Level::Error),
Copy link
Member

Choose a reason for hiding this comment

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

This has the same symptom. Different users may have different thoughts on mapping Fatal and Error to different levels. We try not to decide this kind of application logic for users.

@weihanglo
Copy link
Member

Just saw ehuss has left comments and noticed the public dependency issue I wasn't aware of. I totally agree with that being a way bigger problem than other issues I've pointed out. Given that, I don't think this is a thing we're going to merge. Thanks for being interested in upstreaming your changes anyway!

@vcfxb
Copy link
Contributor Author

vcfxb commented Aug 7, 2024

Of course! I completely understand -- it may be worth upgrading log regardless -- I leave that to you.

@vcfxb vcfxb closed this Aug 7, 2024
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