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

feat: Discover current repository #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spencerwilson
Copy link
Contributor

@spencerwilson spencerwilson commented Nov 12, 2019

git2 is a crate of Rust bindings to libgit2. The git book suggests it's the dominant way to "embed" git-like functionality in apps w/o shelling out to the git binary.

Cargo.lock changes are the result of running cargo build on this branch, w/ cargo version 1.39.0.

Manual tests:

# State of this branch:
~/code/git-codeowners/src
$ ../target/debug/git-codeowners src/main.rs
@softprops

# When temporarily deleting CODEOWNERS
~/code/git-codeowners/src
$ ../target/debug/git-codeowners src/main.rs
No CODEOWNERS file found in this repo.
Ensure one exists at any of the locations documented here:
https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-file-location

# When outside of a git repo
~/code
$ ./git-codeowners/target/debug/git-codeowners src/main.rs
Repo discovery failed. Is /Users/ssw/code within a git repository? Error: could not find repository from '/Users/spencer/code'; class=Repository (6); code=NotFound (-3)

Resolves #3.

@@ -119,3 +132,17 @@ fn main() {
}

}

fn discover_codeowners() -> Option<PathBuf> {
let curr_dir = current_dir().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

If possible could we avoid unwrapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Docs say that current_dir() could be Err if (perhaps not comprehensive?)

  • Current directory does not exist.
  • There are insufficient permissions to access the current directory.

Perhaps desired behavior could be printing something like "Could not confirm working directory. Does it still exist? Is it readable?", then exit 2/some not-yet-taken number. What do you think?


fn discover_codeowners() -> Option<PathBuf> {
let curr_dir = current_dir().unwrap();
let repo = match Repository::discover(&curr_dir) {
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should just use std::process::Command with "git rev-parse --show-toplevel" since there's not much heavy git lifting needed here.

How much bigger did adding a crate to perform this a action add?

Copy link
Contributor Author

@spencerwilson spencerwilson Nov 17, 2019

Choose a reason for hiding this comment

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

Here are some numbers I get:

master branch

# Compilation time
$ rm -rf target/ && time cargo build --release
...
    Finished release [optimized] target(s) in 1m 12s

real	1m12.546s
user	3m13.753s
sys	0m3.983s

# Binary size
$ du -h target/release/git-codeowners
1.7M	target/release/git-codeowners

PR branch

$ rm -rf target/ && time cargo build --release
...
    Finished release [optimized] target(s) in 1m 58s

real	1m49.198s
user	6m23.143s
sys	0m19.478s

$ du -h target/release/git-codeowners
2.0M	target/release/git-codeowners

Thanks for encouraging me to measure this. I expect that whatever performance benefit to be had from leveraging git2 is likely not worth doubling the CPU time required to compile. But I'll try using std::process::Command as you described and see what perf difference I can measure.

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.

Discover current git repo
2 participants