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

Fix to allow any remote name to be used #413

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

meltinglava
Copy link
Contributor

Fixes #411

TODO: Is it ok if we take the first one we find. Are there any examples
of having more than one file in refs/remotes/origin/?.

Fixes killercup#411

TODO: Is it ok if we take the first one we find. Are there any examples
of having more than one file in 'refs/remotes/origin/'?.
@meltinglava
Copy link
Contributor Author

AppVeyor build fails on linking. Looks like this is the build not these changes, as I found the same in other PR-s.

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

We would also need to fix the code that checkouts the registry

cargo-edit/src/fetch.rs

Lines 126 to 159 in 034f6ef

let refspec = "refs/heads/master:refs/remotes/origin/master";
fetch_with_cli(&repo, registry.as_str(), refspec)?;
Ok(())
}
// https://github.com/rust-lang/cargo/blob/57986eac7157261c33f0123bade7ccd20f15200f/src/cargo/sources/git/utils.rs#L758
fn fetch_with_cli(repo: &git2::Repository, url: &str, refspec: &str) -> Result<()> {
let cmd = subprocess::Exec::shell("git")
.arg("fetch")
.arg("--tags") // fetch all tags
.arg("--force") // handle force pushes
.arg("--update-head-ok") // see discussion in rust-lang/cargo#2078
.arg(url)
.arg(refspec)
// If cargo is run by git (for example, the `exec` command in `git
// rebase`), the GIT_DIR is set by git and will point to the wrong
// location (this takes precedence over the cwd). Make sure this is
// unset so git will look at cwd for the repo.
.env_remove("GIT_DIR")
// The reset of these may not be necessary, but I'm including them
// just to be extra paranoid and avoid any issues.
.env_remove("GIT_WORK_TREE")
.env_remove("GIT_INDEX_FILE")
.env_remove("GIT_OBJECT_DIRECTORY")
.env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES")
.cwd(repo.path());
let _ = cmd.capture().map_err(|e| match e {
subprocess::PopenError::IoError(io) => ErrorKind::Io(io),
_ => unreachable!("expected only io error"),
})?;
Ok(())
}

src/fetch.rs Outdated Show resolved Hide resolved
src/fetch.rs Outdated
let checkout_dir = registry_path.as_ref().join(".git").join(&remotes);
let checkout = checkout_dir
.read_dir()?
.next() // Is there always only one branch? (expecting either master og HEAD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be safer to try only known names master and if it fails, then HEAD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is mostly ok. I wanted to know if there are any reference on how cargo deals according to this. As I think most users will never toutch this location I think it should be fine. If we want to check for names i would rather collect them into a vec and then first check master then HEAD then first element. Not sure about that order either.

still todo: What to do with when we get unkown or no checkouts
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks! (sorry for the delay)

@ordian
Copy link
Collaborator

ordian commented Aug 5, 2020

@killercup do you know what happened to travis CI? Can't merge the PR, tests seem to pass locally.

@killercup
Copy link
Owner

No idea, the build doesn't appear in the travis dashboard for some reason. I'll force merge it.

@killercup killercup merged commit 13206ee into killercup:master Aug 6, 2020
@killercup
Copy link
Owner

Github Action's CI now tells me some checks fail: https://github.com/killercup/cargo-edit/actions/runs/197551357

I dont have time to look into it right now, but it'd be cool if we can get a fix fo this merged soon.

@ordian
Copy link
Collaborator

ordian commented Aug 6, 2020

ah, that's clippy and fmt, windows was failing before due to #377

@meltinglava meltinglava deleted the refs_remotes_take_any branch August 6, 2020 18:59
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.

Clean updating the crates.io index sets 'refs/remotes/origin/' to HEAD insted of master
3 participants