main, git: Rework creation of patch branch
diff mbox series

Message ID 20190710010529.7049-1-ajd@linux.ibm.com
State New
Headers show
Series
  • main, git: Rework creation of patch branch
Related show

Commit Message

Andrew Donnellan July 10, 2019, 1:05 a.m. UTC
When we update our remote tracking branch after the remote branch has been
rebased, using git pull will result in a merge commit being created, which
is not the behaviour we want.

Instead, fetch the remote, and create our new patch application branch
directly, without checking out a remote tracking branch and pulling. This
is a lot cleaner. Add a few helper functions to facilitate this.

This requires a new project config option, base_remote_name, to specify the
name of the remote from which we are fetching base branches. If not
specified, it will default to the same remote to which we are pushing. This
is a breaking change and will require existing users to update their
configuration.

Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
---
 docs/configuration.md   |  7 ++++--
 examples/openpower.toml |  2 ++
 src/git.rs              | 50 ++++++++++++++++++++++++++---------------
 src/main.rs             | 39 ++++++++++++++++++--------------
 src/settings.rs         |  1 +
 5 files changed, 62 insertions(+), 37 deletions(-)

Comments

Russell Currey July 10, 2019, 1:15 a.m. UTC | #1
On Wed, 2019-07-10 at 11:05 +1000, Andrew Donnellan wrote:
> When we update our remote tracking branch after the remote branch has
> been
> rebased, using git pull will result in a merge commit being created,
> which
> is not the behaviour we want.
> 
> Instead, fetch the remote, and create our new patch application
> branch
> directly, without checking out a remote tracking branch and pulling.
> This
> is a lot cleaner. Add a few helper functions to facilitate this.
> 
> This requires a new project config option, base_remote_name, to
> specify the
> name of the remote from which we are fetching base branches. If not
> specified, it will default to the same remote to which we are
> pushing. This
> is a breaking change and will require existing users to update their
> configuration.
> 
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>

Looks good - maybe we should give a warning if it's not specified
though, users can avoid the warning by specifying the config option,
and avoids people potentially not getting the behaviour they expect.
Andrew Donnellan July 10, 2019, 8:25 a.m. UTC | #2
On 10/7/19 11:15 am, Russell Currey wrote:
> Looks good - maybe we should give a warning if it's not specified
> though, users can avoid the warning by specifying the config option,
> and avoids people potentially not getting the behaviour they expect.
> 

Or we could just make the option mandatory...

Patch
diff mbox series

diff --git a/docs/configuration.md b/docs/configuration.md
index 4402693cab99..d4c383fefe9e 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -111,6 +111,7 @@  Example:
     # test_all_branches defaults to true
     remote_name = "github"
     remote_uri = "git@github.com:ruscur/linux.git"
+    base_remote_name = "origin"
     push_results = false
 
         [[projects.linuxppc-dev.jobs]]
@@ -132,8 +133,7 @@  Example:
 
 - `repository`: path to local clone of git repository
 
-- `branches`: a list of base branches (as defined by the local git repository)
-  that patches should be tested against
+- `branches`: a list of base branches that patches should be tested against
 
 - `test_all_branches`: if true, each patch will be tested against all base
   branches. If false, a patch will only be tested against the first base branch
@@ -144,6 +144,9 @@  Example:
 
 - `remote_uri`: the URI of the remote
 
+- `base_remote_name`: the name of the remote where base branches are retrieved
+  from (Optional, defaults to value of remote_name)
+
 - `push_results`: whether test results should be pushed to Patchwork for this project
 
 Individual jobs contain the following:
diff --git a/examples/openpower.toml b/examples/openpower.toml
index 59331455fc68..d72d47ad7c69 100644
--- a/examples/openpower.toml
+++ b/examples/openpower.toml
@@ -41,6 +41,7 @@  token = "33333333333333333333333333333333"
     test_all_branches = false
     remote_name = "github"
     remote_uri = "git@github.com:ruscur/skiboot.git"
+    base_remote_name = "origin"
     push_results = false
 
         [[projects.skiboot.jobs]]
@@ -61,6 +62,7 @@  token = "33333333333333333333333333333333"
     # test_all_branches defaults to true
     remote_name = "github"
     remote_uri = "git@github.com:ruscur/linux.git"
+    base_remote_name = "origin"
     push_results = false
 
         [[projects.linuxppc-dev.jobs]]
diff --git a/src/git.rs b/src/git.rs
index ad4bb781d23b..3ac75f2991e5 100644
--- a/src/git.rs
+++ b/src/git.rs
@@ -15,7 +15,9 @@ 
 //
 
 use git2::build::CheckoutBuilder;
-use git2::{Branch, Commit, Cred, Error, PushOptions, Remote, Repository};
+use git2::{
+    Branch, BranchType, Commit, Cred, Error, FetchOptions, PushOptions, Remote, Repository,
+};
 
 use std::path::Path;
 use std::process::{Command, Output};
@@ -47,25 +49,37 @@  pub fn push_to_remote(
     remote.push(refspecs, Some(&mut opts))
 }
 
-// TODO: rewrite this to use git2-rs, I think it's impossible currently
-pub fn pull(repo: &Repository) -> Result<Output, &'static str> {
-    let workdir = repo.workdir().unwrap(); // TODO: support bare repositories
+pub fn fetch_branch(
+    remote: &mut Remote,
+    branch: &str,
+    opts: Option<&mut FetchOptions>,
+) -> Result<(), Error> {
+    remote.fetch(&[branch], opts, None)
+}
 
-    let output = Command::new("git")
-        .arg("pull") // pull the cool kid's way
-        .current_dir(&workdir) // in the repo's working directory
-        .output() // run synchronously
-        .unwrap(); // TODO
+pub fn create_branch<'a>(
+    repo: &'a Repository,
+    branch_name: &str,
+    base_remote: &Remote,
+    target_ref: &str,
+) -> Result<Branch<'a>, Error> {
+    let base_branch = repo.find_branch(
+        &format!("{}/{}", base_remote.name().unwrap(), target_ref),
+        BranchType::Remote,
+    )?;
+    let commit = base_branch.get().peel_to_commit()?;
+
+    let _ = delete_branch(repo, branch_name);
+    repo.branch(branch_name, &commit, true)
+}
 
-    if output.status.success() {
-        debug!(
-            "Pull: {}",
-            String::from_utf8(output.clone().stdout).unwrap()
-        );
-        Ok(output)
-    } else {
-        Err("Error: couldn't pull changes")
+pub fn delete_branch(repo: &Repository, branch_name: &str) -> Result<(), Error> {
+    let mut branch = repo.find_branch(branch_name, BranchType::Local)?;
+    if branch.is_head() {
+        debug!("Detaching HEAD");
+        repo.set_head_detached(repo.head()?.target().unwrap())?;
     }
+    branch.delete()
 }
 
 pub fn checkout_branch(repo: &Repository, branch: &str) {
@@ -87,7 +101,7 @@  pub fn checkout_branch(repo: &Repository, branch: &str) {
         .output()
         .unwrap();
 
-    repo.set_head(&format!("{}/{}", GIT_REF_BASE, &branch))
+    repo.set_head(&branch)
         .unwrap_or_else(|err| panic!("Couldn't set HEAD: {}", err));
     repo.checkout_head(Some(&mut CheckoutBuilder::new().force()))
         .unwrap_or_else(|err| panic!("Couldn't checkout HEAD: {}", err));
diff --git a/src/main.rs b/src/main.rs
index c3c128faeb6f..ea09b07969ba 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -34,7 +34,7 @@  extern crate base64;
 extern crate serde_json;
 extern crate toml;
 
-use git2::{BranchType, PushOptions, RemoteCallbacks};
+use git2::{FetchOptions, PushOptions, RemoteCallbacks};
 
 use reqwest::{Client, Proxy};
 
@@ -200,31 +200,38 @@  fn test_patch(
     }
     let tag = utils::sanitise_path(path.file_name().unwrap().to_str().unwrap());
     let mut remote = repo.find_remote(&project.remote_name).unwrap();
+    let mut base_remote = repo
+        .find_remote(match &project.base_remote_name {
+            Some(name) => &name,
+            _ => &project.remote_name,
+        })
+        .unwrap();
 
     let mut push_callbacks = RemoteCallbacks::new();
     push_callbacks.credentials(|_, _, _| git::cred_from_settings(&settings.git));
-
     let mut push_opts = PushOptions::new();
     push_opts.remote_callbacks(push_callbacks);
+    let mut fetch_callbacks = RemoteCallbacks::new();
+    fetch_callbacks.credentials(|_, _, _| git::cred_from_settings(&settings.git));
+    let mut fetch_opts = FetchOptions::new();
+    fetch_opts.remote_callbacks(fetch_callbacks);
 
     let mut successfully_applied = false;
     for branch_name in project.branches.clone() {
         let tag = format!("{}_{}", tag, branch_name);
         info!("Configuring local branch for {}.", tag);
-        debug!("Switching to base branch {}...", branch_name);
-        git::checkout_branch(&repo, &branch_name);
 
-        // Make sure we're up to date
-        git::pull(&repo).unwrap();
+        debug!("Updating base branch {}...", branch_name);
+        git::fetch_branch(&mut base_remote, &branch_name, Some(&mut fetch_opts))
+            .unwrap_or_else(|err| panic!("Couldn't fetch branch: {}", err));
 
         debug!("Creating a new branch...");
-        let commit = git::get_latest_commit(&repo);
-        let mut branch = repo.branch(&tag, &commit, true).unwrap();
+        let branch = git::create_branch(&repo, &tag, &base_remote, &branch_name)
+            .unwrap_or_else(|err| panic!("Couldn't create branch: {}", err));
+
         debug!("Switching to branch...");
-        repo.set_head(branch.get().name().unwrap())
-            .unwrap_or_else(|err| panic!("Couldn't set HEAD: {}", err));
-        repo.checkout_head(None)
-            .unwrap_or_else(|err| panic!("Couldn't checkout HEAD: {}", err));
+        git::checkout_branch(&repo, branch.get().name().unwrap());
+        let commit = git::get_latest_commit(&repo);
         debug!(
             "Repo is now at head {}",
             repo.head().unwrap().name().unwrap()
@@ -236,11 +243,9 @@  fn test_patch(
             git::push_to_remote(&mut remote, &branch, false, &mut push_opts).unwrap();
         }
 
-        git::checkout_branch(&repo, &branch_name);
-        // we need to find the branch again since its head has moved
-        branch = repo.find_branch(&tag, BranchType::Local).unwrap();
-        branch.delete().unwrap();
-        debug!("Repo is back to {}", repo.head().unwrap().name().unwrap());
+        git::delete_branch(&repo, &tag).unwrap_or_else(|err| {
+            panic!("Couldn't delete branch: {}", err);
+        });
 
         match output {
             Ok(_) => {
diff --git a/src/settings.rs b/src/settings.rs
index 128f2030e467..40ac67cccf58 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -65,6 +65,7 @@  pub struct Project {
     pub test_all_branches: Option<bool>,
     pub remote_name: String,
     pub remote_uri: String,
+    pub base_remote_name: Option<String>,
     pub jobs: Vec<Job>,
     pub push_results: bool,
     pub category: Option<String>,