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

Made new-user commit its changes #14

Merged
merged 5 commits into from
May 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 16 additions & 4 deletions src/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use self::openssl::crypto::rsa::RSA as openssl_RSA;
use self::openssl::crypto::hash::Type as openssl_HashType;
use self::openssl::ssl::error::SslError as openssl_Error;

use git2::Signature;

use Error;
use User;
use utils;
Expand All @@ -21,12 +23,21 @@ use utils;
///
/// Impure.
pub fn new_user<P: AsRef<Path>>(public_key_path: P, name: &str) -> Result<(), Error> {
// Add user
let pub_key = try!(get_public_key(public_key_path));
let project = try!(utils::read_protonfile(None::<P>));
let new_project = try!(project.add_user(name, &pub_key));
utils::write_protonfile(&new_project, None::<P>)
}
let new_project = try!(project.add_user(&name, &pub_key));
try!(utils::write_protonfile(&new_project, None::<P>));

// Commit changes
let signature = Signature::now("Proton Lights", "[email protected]").unwrap();
let msg = format!("Adding {} as new user", name);
let pf_path = Path::new("Protonfile.json");
let repo_path: Option<P> = None;

utils::commit_file(&pf_path, repo_path, &signature, &msg)
.map(|_| ())
}
/// Identifies a user by their private SSH key by finding the
/// corresponding public key in the project. This private key
/// acts like the user's password, and should be protected.
Expand Down Expand Up @@ -77,4 +88,5 @@ fn invalid_pub_key_error(key: &str) -> Error {

fn ssl_error(e: openssl_Error) -> Error {
Error::Ssl(e)
}
}

52 changes: 50 additions & 2 deletions src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,50 @@
use std::fs::File;
use std::io::{Read, Write};
use std::path::{Path, PathBuf};
use std::env;

use rustc_serialize::json;
use git2::{Repository, Signature};

use project_types::Project;
use error::Error;


/// Stages a file and commits it
/// If a path to the repository is not given, assume it is in the current directory
/// Impure.
pub fn commit_file<P: AsRef<Path>>(
file_path: &Path, repo_path: Option<P>, signature: &Signature, msg: &str
) -> Result<(), Error> {

let repo = try!(get_repo_from_path(repo_path));
let tree_oid = try!(repo.index()
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so I don't really like these changes. It seems like it got more complex. I think how you had it before was better. The only change I was looking for was that line 22 changed from a discarding assignment to a return.

I really dislike the code pattern where we have a bunch of stuff tryed! and then ended with an Ok(()). I think you should just get rid of the last try! and change it to a return with a .map(|_| OK(())). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, it's ugly. Didn't know how to make the map work before, but figured it out. Lmk if still could be improved. Some/all of the try!() statements have to stay, in order to keep repo in scope.

.and_then(|mut index| index.add_path(&file_path).map(|_| index))
.and_then(|mut index| index.write().map(|_| index))
.and_then(|mut index| index.write_tree())
.map_err(Error::Git));
let tree = try!(repo.find_tree(tree_oid).map_err(Error::Git));
let parent = try!(repo.refname_to_id("refs/heads/master")
.and_then(|oid| repo.find_commit(oid))
.map_err(Error::Git));

return repo.commit(
Some("HEAD"),
signature,
signature,
msg,
&tree,
&[&parent]
)
.map_err(Error::Git)
.map(|_| ())

}

/// Reads a Project from a Protonfile.
/// Wraps any errors in proton_cli::Error
/// Assumes Protonfile.json resides in the current directory
/// unless a path to the Protonfile is given
/// unless a path to the Protonfile is given.
pub fn read_protonfile<P: AsRef<Path>>(pf_path: Option<P>) -> Result<Project, Error> {
let protonfile_path = build_protonfile_path(pf_path);
let protonfile = try!(file_as_string(&protonfile_path));
Expand All @@ -20,7 +53,8 @@ pub fn read_protonfile<P: AsRef<Path>>(pf_path: Option<P>) -> Result<Project, Er

/// Saves a Project to a Protonfile.
/// Assumes the Protonfile is in the current directory
/// unless a path to the Protonfile is given
/// unless a path to the Protonfile is given.
/// Impure.
pub fn write_protonfile<P: AsRef<Path>>(project: &Project, pf_path: Option<P>) -> Result<(), Error> {
let pretty_json = json::as_pretty_json(&project);
let protonfile_path = build_protonfile_path(pf_path);
Expand Down Expand Up @@ -49,4 +83,18 @@ fn build_protonfile_path<P: AsRef<Path>>(path_opt: Option<P>) -> PathBuf {
};
protonfile_path.push("Protonfile.json");
protonfile_path
}

fn get_repo_from_path<P: AsRef<Path>>(path_opt: Option<P>) -> Result<Repository, Error> {
let mut repo_path = PathBuf::new();
let _ = match path_opt {
Some(path) => repo_path.push(path),
None => repo_path.push(env::current_dir().expect("Current directory invalid")),
};

println!("{}", repo_path.as_path().display());

Repository::open(repo_path.as_path())
.map_err(Error::Git)

}
36 changes: 36 additions & 0 deletions tests/new_user.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
extern crate proton_cli;
extern crate tempdir;
extern crate git2;

mod common;

use std::path::Path;

use git2::Repository;

use common::rsa_keys::TestKey;
use proton_cli::Error;


/// Warning: This test changes env::current_directory
Expand All @@ -28,13 +34,19 @@ fn works_with_new_and_existing_protonfile() {
// Assert that user was added
common::assert_user_added(key_path_a.as_path(), "Test User");

// Make sure the change was committed
assert_commits_added(root.path());

// Now try adding another user
let _ = proton_cli::new_user(&key_path_b.as_path(), &user_name2)
.expect("Error adding user 2");

// Assert that both users exist
common::assert_user_added(key_path_a.as_path(), &user_name);
common::assert_user_added(key_path_b.as_path(), &user_name2);

// Make sure the change was committed
assert_commits_added(root.path());
}

#[test]
Expand Down Expand Up @@ -127,3 +139,27 @@ fn fails_with_duplicate_user_name() {
let _ = proton_cli::new_user(&key_path_b.as_path(), "Test User")
.expect("Error adding second user");
}

/// Check that the new user changes were actually committed to the repository
fn assert_commits_added<P: AsRef<Path>>(repo_path: P) {
// Open the git repo and master branch
let repo = Repository::open(repo_path).unwrap();
let commit = repo.refname_to_id("refs/heads/master")
.and_then(|oid| repo.find_commit(oid))
.expect("Finding master failed");
let tree = commit.tree().expect("Opening master tree failed");

// Check that there aren't any non-commited changes
let _ = repo.diff_tree_to_workdir_with_index(Some(&tree), None)
.and_then(|diff| diff.stats())
.map(|stats| {
assert!(0 == stats.files_changed(), "No changes should be staged");
})
.map_err(Error::Git);

// Assert master is correct
assert!(1 == commit.parents().count(), "master must have 1 parent");

assert!(tree.get_name("Protonfile.json").is_some(), "master must have protonfile");
}