Add tag support #62

clearydude · 2018-07-10T19:27:58Z

This PR adds support for secure-push/fetch of tags to git-rsl.

Closes #48

Prior to this commit, we were wrapping strings representing branch names into BranchName types to reduce potential for misuse. This commit renames that to ReferenceName to reflect upcoming tag support.

Prior to this commit, the git methods for push were bound to using branches. This commit removes that and generalizes it to enable pushing a branch or tag ref. It adds a new test for tag pushing as well as a helper method to tag the latest commit in a repo. It also updates the integration tests to use the ReferenceName type.

Prior to this commit, the help prompts for the binaries still told the user that they need to supply BRANCH references. This commit generalizes that to include branches OR tags by using REFERENCE

Prior to this commit, push entries would try to locate a branch using the shorthand name provided from the user. This was problematic because if the user was attempting to push a tag, the operation would fail. This commit changes it to instead use the git helper method to find a git2::Reference using the shorthand name, so that we can stash the target git2::Oid of either a branch OR a tag in the RSL push entry.

Prior to this commit, the `find_remote_push_entry_for_branch` method tried to use the shorthand name to locate the full reference name. This commit changes it to instead find the reference, either a branch or a tag, and get that name instead. This allows this method to locate RSL push entries for tags as well as branches.

Prior to this commit, methods to locate refererences that had been previously changed from locating branches had not been renamed to reflect their new functionality. This commit does all that stuff.

mullr · 2018-07-10T20:49:33Z

src/utils/git.rs

+fn get_ref_from_name<'repo>(repo: &'repo Repository, name: &str) -> Option<Reference<'repo>> {
+    match repo.find_branch(name, BranchType::Local) {
+        Ok(branch) => Some(branch.into_reference()),
+        Err(e) => { // toDo may be other errors here...


leftover todo

mullr · 2018-07-10T20:50:41Z

src/utils/git.rs

+    match repo.find_branch(name, BranchType::Local) {
+        Ok(branch) => Some(branch.into_reference()),
+        Err(e) => { // toDo may be other errors here...
+            let tag_ref = repo.find_reference(&format!("refs/tags/{}", name)).expect("reference not found");


panicing when the reference isn't found definitely isn't what we should do, especially considering this can be used as a library.

mullr · 2018-07-10T20:52:29Z

src/utils/git.rs

+        let reference = get_ref_from_name(&repo, name).expect("failed to find reference from name");
+        let full_name = String::from(reference.name().expect("failed to get full name of ref"));
+        full_names.push(full_name)
+    }


this is more idiomatically done with ref_names.map(|n| ...)

mullr · 2018-07-10T20:53:24Z

src/utils/git.rs

+    let mut full_names: Vec<String> = vec![];
+    for name in ref_names {
+        let reference = get_ref_from_name(&repo, name).expect("failed to find reference from name");
+        let full_name = String::from(reference.name().expect("failed to get full name of ref"));


We shouldn't panic here either; basically, down in the internal modules, nothing should ever panic. It's more forgivable at the top level of a CLI application.

mullr · 2018-07-10T20:54:17Z

src/utils/test_helper.rs

@@ -87,6 +87,13 @@ pub fn create_file_with_text<P: AsRef<Path>>(path: P, text: &str) -> () {
    file.write_all(text.as_bytes()).unwrap();
 }

+
+pub fn tag_lightweight<'repo>(repo: &'repo Repository, tag_name: &str) -> Reference<'repo> {


what does lightweight mean?

It's basically just a pointer vs an annotated tag with all kinds of other information. I feel like for the test I probably don't need to create a full on annotated tag, but am open to doing that instead if it makes more sense.

If we already have signing set up for the integration tests we may as well?

Ok, I accept this

mullr · 2018-07-10T20:59:02Z

tests/integration_test.rs

+            do_work_on_branch(&context.local, "refs/heads/master");
+            // let local_tag = tag_lightweight(&mut context.local, "v6.66");
+
+            // git_rsl::utils::git::push(&context.local, &mut context.local.find_remote("origin").expect("failed to find remote"), &["v6.66"]);


should delete the commented lines

They're out as of 2fc4cdc

mullr · 2018-07-10T21:06:58Z

src/rsl.rs

@@ -232,7 +237,8 @@ impl<'remote, 'repo> RSL<'remote, 'repo> {
        let mut revwalk: Revwalk = self.repo.revwalk()?;
        revwalk.push(self.remote_head)?;
        let mut current = Some(self.remote_head);
-        let ref_name = format!("refs/heads/{}", branch);
+        let reference = git::get_ref_from_name(self.repo, branch).expect("failed to get ref");
+        let  ref_name = reference.name().expect("failed to get ref name");


extra space after let

windwardrail

Most of my comments are on error handling. I didn't flag all the instances, but using expect is essentially a panic! which we should avoid in favor of error handling such as Result or error chain.

Also there are long lines that should be wrapped. Didn't flag all of those either.

windwardrail · 2018-07-10T20:45:39Z

src/bin/git-secure-fetch.rs

-    let branch = match matches.value_of("BRANCH") {
-        None => panic!("Must supply a BRANCH argument"),
+    let reference = match matches.value_of("REFERENCE") {
+        None => panic!("Must supply a REFERENCE argument"),
        Some(v) => v.to_owned(),
    };
    // TODO - reduce code duplication across the top level of the binaries


Move todo into an issue

9acbc9f and issue incoming

windwardrail · 2018-07-10T21:07:50Z

src/bin/git-secure-fetch.rs

-    let branch = match matches.value_of("BRANCH") {
-        None => panic!("Must supply a BRANCH argument"),
+    let reference = match matches.value_of("REFERENCE") {
+        None => panic!("Must supply a REFERENCE argument"),


It would be nice to avoid panics. You could use bail! from error chain if you moved the logic in main to another function and then called handle_error from main?

I think that this specific case should probably go into the issue tracker and be addressed as part of a different PR, since this one is really just supposed to be adding tag support.

windwardrail · 2018-07-10T21:12:10Z

src/push_entry.rs

-            .get()
-            .target()
-            .unwrap();
+        let full_ref = utils::git::get_ref_from_name(repo, branch_str).expect("failed to get reference");


Use error handling here instead of unwrapping with expect (cause it's basically just a panic)

windwardrail · 2018-07-10T21:12:27Z

src/push_entry.rs

-            .target()
-            .unwrap();
+        let full_ref = utils::git::get_ref_from_name(repo, branch_str).expect("failed to get reference");
+        let target_oid = full_ref.target().expect("failed to get target oid");


Error handling

windwardrail · 2018-07-10T21:12:37Z

src/push_entry.rs


        PushEntry {
-            ref_name: format!("refs/heads/{}", branch_str),
-            oid: branch_head,
+            ref_name: String::from(full_ref.name().expect("failed to get ref's full name")),


Error handling

windwardrail · 2018-07-10T21:17:04Z

src/rsl.rs

    ) -> Result<Option<PushEntry>> {
        let mut revwalk: Revwalk = self.repo.revwalk()?;
        revwalk.push(self.remote_head)?;
        let mut current = Some(self.remote_head);
-        let ref_name = format!("refs/heads/{}", branch);
+        let reference = git::get_ref_from_name(self.repo, reference).expect("failed to get ref");
+        let  ref_name = reference.name().expect("failed to get ref name");


Error handling

windwardrail · 2018-07-10T21:19:34Z

tests/integration_test.rs

            assert_eq!(res3, ());

            do_work_on_branch(&context.local, "refs/heads/master");
-            let res4 = git_rsl::secure_push_with_cleanup(&mut context.local, &RemoteName::new("origin"), &BranchName::new("master")).expect("Could not run third push");
+            let res4 = git_rsl::secure_push_with_cleanup(&mut context.local, &RemoteName::new("origin"), &ReferenceName::new("master")).expect("Could not run third push");


These lines need to be wrapped somehow

2fc4cdc dig the fancy macro

windwardrail · 2018-07-10T21:19:54Z

tests/integration_test.rs

+            let remote_tag = &context.remote.find_reference("refs/tags/v6.66").expect("reference not found");
+            assert!(remote_tag.is_tag());
+
+            assert_eq!((), git_rsl::secure_fetch_with_cleanup(&mut context.local, &RemoteName::new("origin"), &ReferenceName::new("v6.66")).expect("could not fetch tag"));


windwardrail · 2018-07-10T21:20:18Z

tests/integration_test.rs

            assert_eq!(res2, ());

            attack::rollback(&context.remote, "master");

            do_work_on_branch(&context.local, "refs/heads/master");
-            let res3 = git_rsl::secure_push_with_cleanup(&mut context.local, &RemoteName::new("origin"), &BranchName::new("master")).expect_err("Checking for invalid RSL detection");
+            let res3 = git_rsl::secure_push_with_cleanup(&mut context.local, &RemoteName::new("origin"), &ReferenceName::new("master")).expect_err("Checking for invalid RSL detection");


Wrap long lines

windwardrail · 2018-07-10T21:24:53Z

Compiler warnings...

warning: unused import: `BranchType`
 --> src/push_entry.rs:5:18
  |
5 | use git2::{self, BranchType, Oid, Reference, Repository};
  |                  ^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

warning: unused import: `Object`
  --> src/utils/test_helper.rs:13:24
   |
13 | use git2::{Repository, Object, ObjectType, Reference};
   |                        ^^^^^^

warning: unused variable: `e`
   --> src/utils/git.rs:273:13
    |
273 |         Err(e) => { // toDo may be other errors here...
    |             ^ help: consider using `_e` instead
    |
    = note: #[warn(unused_variables)] on by default

warning: unused variable: `tag_oid`
  --> src/utils/test_helper.rs:93:9
   |
93 |     let tag_oid = repo.tag_lightweight(tag_name, tag_target, false);
   |         ^^^^^^^ help: consider using `_tag_oid` instead

This commit cleans up some formatting issues and implements a macro for checking that method calls are ok in the integration tests file.

clearydude · 2018-07-10T23:15:04Z

@windwardrail compiler warnings fixed as of 2fc4cdc

Prior to this commit, the tag test created a lightweight tag for tagging a specific release. This commit changes the helper method and test to instead create a more informative annotated tag.

Prior to this commit, I was doing this awkwardly. This commit uses map instead.

Prior to this commit, the src files for the binaries still had toDo's in them. Those toDos are still things that we need toDo, but would be better served being represented as issues in the issue tracker, so this commit removes them from the code.

Prior to this commit, there were lots of panics in the new lib functions. This commit adds in error handling for those cases.

clearydude · 2018-07-13T21:04:07Z

Addressed all the comments. I'd appreciate another quick look through when you have the chance @windwardrail @mullr

Katie Cleary added 6 commits July 6, 2018 15:23

Rename BranchName type

33bc92a

Prior to this commit, we were wrapping strings representing branch names into BranchName types to reduce potential for misuse. This commit renames that to ReferenceName to reflect upcoming tag support.

Change bin docs to REFERENCE

5315bc7

Prior to this commit, the help prompts for the binaries still told the user that they need to supply BRANCH references. This commit generalizes that to include branches OR tags by using REFERENCE

Change some naming

9febba0

Prior to this commit, methods to locate refererences that had been previously changed from locating branches had not been renamed to reflect their new functionality. This commit does all that stuff.

clearydude requested review from mullr and windwardrail July 10, 2018 19:27

mullr reviewed Jul 10, 2018

View reviewed changes

windwardrail suggested changes Jul 10, 2018

View reviewed changes

Code cleanup

2fc4cdc

This commit cleans up some formatting issues and implements a macro for checking that method calls are ok in the integration tests file.

Katie Cleary added 4 commits July 10, 2018 16:19

Change to annotated tag

f687fb8

Prior to this commit, the tag test created a lightweight tag for tagging a specific release. This commit changes the helper method and test to instead create a more informative annotated tag.

Use map for converting refnames

b776d8f

Prior to this commit, I was doing this awkwardly. This commit uses map instead.

Remove toDo's

9acbc9f

Prior to this commit, the src files for the binaries still had toDo's in them. Those toDos are still things that we need toDo, but would be better served being represented as issues in the issue tracker, so this commit removes them from the code.

Add error handling to lib functions

638721c

Prior to this commit, there were lots of panics in the new lib functions. This commit adds in error handling for those cases.

windwardrail approved these changes Jul 17, 2018

View reviewed changes

Add tag support #62

Are you sure you want to change the base?

Add tag support #62

Conversation

clearydude commented Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullr Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windwardrail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windwardrail commented Jul 10, 2018

clearydude commented Jul 10, 2018

clearydude commented Jul 13, 2018

clearydude commented Jul 10, 2018 •

edited

Loading

mullr Jul 10, 2018 •

edited

Loading