Skip to content

Log a message if the background worker resets HEAD #3591

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 18, 2021
Merged

Log a message if the background worker resets HEAD #3591

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09c3bbb
Refactor some of the index repository logic
jtgeibel May 6, 2021
d012525
Log a message if the background worker resets HEAD
jtgeibel May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ impl Repository {
}
}

fn head_oid(&self) -> Result<git2::Oid, PerformError> {
Ok(self.repository.head()?.target().unwrap())
}

fn perform_commit_and_push(&self, msg: &str, modified_file: &Path) -> Result<(), PerformError> {
// git add $file
let mut index = self.repository.index()?;
Expand All @@ -195,13 +199,18 @@ impl Repository {
let tree = self.repository.find_tree(tree_id)?;

// git commit -m "..."
let head = self.repository.head()?;
let parent = self.repository.find_commit(head.target().unwrap())?;
let head = self.head_oid()?;
let parent = self.repository.find_commit(head)?;
let sig = self.repository.signature()?;
self.repository
.commit(Some("HEAD"), &sig, &sig, &msg, &tree, &[&parent])?;

// git push
self.push()
}

/// Push the current branch to "refs/heads/master"
fn push(&self) -> Result<(), PerformError> {
let refname = "refs/heads/master";
let mut ref_status = Ok(());
let mut callback_called = false;
{
Expand All @@ -210,8 +219,8 @@ impl Repository {
callbacks.credentials(|_, user_from_url, cred_type| {
self.credentials.git2_callback(user_from_url, cred_type)
});
callbacks.push_update_reference(|refname, status| {
assert_eq!(refname, "refs/heads/master");
callbacks.push_update_reference(|cb_refname, status| {
assert_eq!(refname, cb_refname);
if let Some(s) = status {
ref_status = Err(format!("failed to push a ref: {}", s).into())
}
Expand All @@ -220,7 +229,7 @@ impl Repository {
});
let mut opts = git2::PushOptions::new();
opts.remote_callbacks(callbacks);
origin.push(&["refs/heads/master"], Some(&mut opts))?;
origin.push(&[refname], Some(&mut opts))?;
}

if !callback_called {
Expand All @@ -230,7 +239,7 @@ impl Repository {
ref_status
}

pub fn commit_and_push(&self, message: &str, modified_file: &Path) -> Result<(), PerformError> {
fn commit_and_push(&self, message: &str, modified_file: &Path) -> Result<(), PerformError> {
println!("Committing and pushing \"{}\"", message);

self.perform_commit_and_push(message, modified_file)
Expand All @@ -243,12 +252,18 @@ impl Repository {

pub fn reset_head(&self) -> Result<(), PerformError> {
let mut origin = self.repository.find_remote("origin")?;
let original_head = self.head_oid()?;
origin.fetch(
&["refs/heads/*:refs/heads/*"],
Some(&mut Self::fetch_options(&self.credentials)),
None,
)?;
let head = self.repository.head()?.target().unwrap();
let head = self.head_oid()?;

if head != original_head {
println!("Resetting index from {} to {}", original_head, head);
}
Copy link
Member

Choose a reason for hiding this comment

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

maybe I'm misunderstanding the logic here, but do we even need to call self.repository.reset() if head == original_head?

Copy link
Member Author

Choose a reason for hiding this comment

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

I avoided doing an early return here mainly to avoid any unexpected behavior changes to edge cases. It is possible that a previously failed job has staged changes in the index, but failed before the staged changes were committed. In that case, the local and remote HEAD commits would match, but the staged changes would become mingled with new changes from the next job.

I now actually wonder if a reset is always sufficient. I believe a reset will leave any untracked files alone. So it might be possible that the last job created the local file but failed before the new file was staged. In both cases I think these scenarios are extremely unlikely - we're much more likely to fail when accessing the network than for local file I/O - but maybe we should consider the equivalent of a git clean -fdx as well.

Copy link
Member

Choose a reason for hiding this comment

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

if we're not sure about these edge cases, would it make sense to unconditionally log the operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could probably do the equivalent of git status and log a message if either the index or working directory isn't clean. Then we could log that as well if there is stale state hanging around. But for this message, I think it makes sense to only log it if the hashes are different.

Copy link
Member

Choose a reason for hiding this comment

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

okay, sounds good to me


let obj = self.repository.find_object(head, None)?;
self.repository.reset(&obj, git2::ResetType::Hard, None)?;
Ok(())
Expand Down