Take two on remote Git targets #44
Conversation
Previously this worked with the example because it actually resolved to crates.io even for "git" packages. The example was included before it was ready, and only because it relied exclusively on things already on crates.io did it compile in the first place. Tests back to compiling and running true -> True Fix a pathing issue Fix some unused imports
impl/src/context.rs
Outdated
| @@ -37,6 +37,12 @@ pub struct LicenseData { | |||
| pub rating: String, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)] | |||
| pub struct GitData { | |||
There was a problem hiding this comment.
Nit: I'd prefer "GitRepo" both here, and as the name of the field in crate_context, unless there is an existing convention for GitData (e.g. Cargo calls it that).
| let crate_path_prefix = manifest_pathbuf.parent().unwrap().display().to_string(); | ||
| let is_git = source_id.as_ref().map_or(false, SourceId::is_git); | ||
| let local_root = match (settings.genmode.clone(), is_git) { | ||
| // UNWRAP: We know from source_id that this is a git package, so it must have a repo root |
There was a problem hiding this comment.
This unwrap justification seems weak to me. package_git_root can fail if the git directory itself is malformed.
Consider
(GenMode::Remote, true) => {
try!(package_git_root(&manifest)).to_str().unwrap().to_owned()
},... instead, so that the error gets bubbled up correctly (and not unwrapped right here).
There was a problem hiding this comment.
Checking in on this again - the package_git_root() has been updated for try!(), is there anything else needed?
impl/src/planning.rs
Outdated
| } | ||
| } | ||
|
|
||
| Err(CargoError::from(format!("Unable to locate git repository root for manifest {:?}", manifest))) |
There was a problem hiding this comment.
Add comment above line
// Reached filesystem root and did not find git repo
It took me a bit to understand what the exit condition of the above loop was.
|
CLA bot is still here, everything is marked as signed. The remote non-cratesio example is already set to take advantage of this. Theoretically, it shouldn't have been passing in the current regime; it magically worked because all the referenced crates were also published on crates.io. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
|
I've resolved the last merge conflict (related to supporting re-declared deps) and I'll merge on green (or fix on red) |
Previously the remote non-cratesio example worked because it accidentally resolved to crates.io even for "git" packages. The example was included before it was ready, and only because it relied exclusively on things already on
crates.iodid it compile in the first place.Also takes a much lighter touch with metadata.rs - we know that we're checking out a git repository, so can guarantee that there's a
.gitfolder somewhere when we walk back up the path, rather than relying on Cargo to tell us where the repo root is actually at.Continues on from #31