Add support for parsing subdirectories out of git URLs (redux) #10

camjackson · 2017-11-12T08:40:59Z

This is a cleaned up version of #9. See that PR for more context.

emk

Wow, I really like this version! It feels cleaner and more elegant.

I can think of one or two minor Rust API tweaks that we should consider before merging this. These are actually pretty simple, but apparently I need a lot of words for me to justify and explain what I'm thinking, so please don't be concerned by the length of this feedback!

Once again, thank you for tackling this. It's a great feature, and I'm delighted to have it.

emk · 2017-11-12T11:17:28Z

src/v2/git_url.rs

+    }
+
+    /// Extract the repository part of the URL
+    pub fn repository(&self) -> String {


If possible, I'd love to change String to &str. Logically we can assume that this object owns the necessary string somehow, so we can just return a slice.

emk · 2017-11-12T11:18:06Z

src/v2/git_url.rs

+    }
+
+    /// Extract the optional branch part of the git URL
+    pub fn branch(&self) -> Option<String> {


...and Option<&str>, and so on.

emk · 2017-11-12T11:18:29Z

src/v2/git_url.rs

+        self.parse_parts().2
+    }
+
+    fn parse_parts(&self) -> (String, Option<String>, Option<String>) {


(&str, Option<&str>, Option<&str>)

emk · 2017-11-12T11:19:09Z

src/v2/git_url.rs

+        (
+            captures.at(1).unwrap().to_string(),
+            captures.at(3).map(|branch| branch.to_string()),
+            captures.at(5).map(|branch| branch.to_string()),


Since we're using &str everywhere, we can delete the .to_string() calls here and save a bunch of heap allocations.

Oh wow yeah that's way cleaner. Not sure why I was using Strings everywhere.

emk · 2017-11-12T11:21:16Z

src/v2/git_url.rs

+
+    fn parse_parts(&self) -> (String, Option<String>, Option<String>) {
+        lazy_static! {
+            static ref URL_PARSE: Regex = Regex::new(r#"^([^#]+)(#([^:]+)?(:(.+))?)?$"#).unwrap();


If we were being purely pendantic, it might be nice to change .unwrap() to .expect("could not parse URL_PARSE"), but that's being a bit pendantic, and much of compose_yml is old enough that I hadn't started doing that yet myself. :-)

Either expect or unwrap is fine here, because this is a true assertion violation.

Also, we might add a couple of :? "no-capture" markers as follows:

^([^#]+)(?:#([^:]+)?(?::(.+))?)?$

I think those are in the right place. :-) This would also require changing the arguments to at below.

Aha, I didn't even know about no-capture, that's awesome.

emk · 2017-11-12T11:36:48Z

src/v2/git_url.rs

+        lazy_static! {
+            static ref URL_PARSE: Regex = Regex::new(r#"^([^#]+)(#([^:]+)?(:(.+))?)?$"#).unwrap();
+        }
+        let captures = URL_PARSE.captures(&self.url).unwrap();


This unwrap, on the other hand, looks like a possible runtime panic, because you're matching a complex regex against a user-provided string. And unwrap/expect are really meant to be assertions, not error-checks—except maybe in very short "scripts".

So we need to do one of two things here:

Provide an iron-clad argument that captures can never fail on any possible input, or

Redesign this class / API so that we can only construct a GitUrl with a valid URL.

The later might look like:

pub struct GitUrl { url: String, // Must be private to maintain invariants. } impl GitUrl { fn new<S: Into<String>>(url: S) -> Result<GitUrl> { let url = GitUrl { url: url.into() }; // Make sure our URL can always be parsed. url.parse_parts()?; Ok(url) } }

Then later you can write:

fn parse_parts(&self) -> Result<(&str, Option<&str>, Option<&str>)> { ... } pub fn subdirectory(&self) -> Option<&str> { let (_, _, subdir) = self.parse_parts() .expect("parse_parts failed on data that we already parsed once successfully"); subdir }

The nice thing about this is that it allows us to add other validity checks to new in the future as needed. Alternatively, we could keep the current API, but provide a comment explaining why parse_parts can never fail on any possible input.

Yeah I was conscious of the unwrap here. I'm pretty certain that it can't blow up though. As far as I can tell, the only way captures can fail is if the url is empty, or if it starts with a hash. And either of those cases would fail the the existing should_treat_as_url check.

Now that said, I can totally see a situation where someone might change the should_treat_as_url regex, without realising that there's an unwrap 70 lines down that depends on it.

Conclusion: I'll go with your suggestion and make new call parse_parts, so it definitely can't panic.

Just looking at your proposed snippet above, you've removed the should_treat_as_url check from new. I feel like we should keep that check there, seeing as it ensures Docker will accept the URL. The parse_parts regex will accept strings like "a#b:c", or even just "a", which is obviously not a URL.

Oh, yeah, definitely include whatever is already there in new. I wrote that new example from scratch.

Ah, ok, great.

Now that said, I can totally see a situation where someone might change the should_treat_as_url regex, without realising that there's an unwrap 70 lines down that depends on it.

Yeah, a good rule of thumb is that you shouldn't call unwrap in a doubtful situation like this. At a minimum, there should be an expect and a explanation of why this really is an assertion, and not a run-time error condition.

Ok I'm really close with this, just struggling with the final piece. Right now I have this:

fn parse_parts(&self) -> Result<(&str, Option<&str>, Option<&str>)> { lazy_static! { static ref URL_PARSE: Regex = Regex::new(r#"^([^#]+)(?:#([^:]+)?(?::(.+))?)?$"#) .expect("Could not parse regex URL_PARSE"); } let captures = URL_PARSE.captures(&self.url).unwrap(); Ok(( captures.at(1).unwrap(), captures.at(2), captures.at(3), )) }

I've wrapped the return type in Result, and I'm handling the Result everywhere that calls this, but as you can see I still have the unwrap. I tried to replace unwrap() withok_or(())? to convert the Option into a Result but couldn't get the types to work. Is it because the Result here is an error-chain thing, not a regular Result?

Yeah, you need to construct our local Error type here, which you can actually do with a string and .into(). Use ok_or_else(|| -> Error { format!(...).into() }) and it will construct an error for you.

emk · 2017-11-12T11:37:47Z

src/v2/git_url.rs

+        assert_eq!(with_ref.branch(), Some("mybranch".to_string()));
+        assert_eq!(with_ref.subdirectory(), None);
+
+        let with_subdir = GitUrl::new(format!("{}{}", url, "#:myfolder")).unwrap();


This is great, but let's also add a test case for format!("{}{}", url, ":myfolder")), specifying how that should be parsed.

Looking at the Docker docs, I don't think that's a valid URL? Also, the parsing as it's currently written relies on the hash as a nice, easy-to-spot delimeter. A lone colon is harder to split on, because it could be for a port number. 🤔

Oh, sorry! I should have been more clear. I want to make sure format!("{}{}", url, ":myfolder")) isn't parsed as a subdir. It's a suspicious input, where the implementation might plausibly fail, and I want to make sure that we interpret it correctly.

Does this make sense?

emk · 2017-11-12T11:41:41Z

src/v2/context.in.rs

+
+    /// Returns a new Context which is the same as the
+    /// this one, but without any subdirectory part
+    pub fn without_subdirectory(&self) -> Context {


This is an amazingly good choice of API. Thank you for figuring this out. Maybe it might be clearer if we named it without_repository_subdirectory?

- Also clean up some String allocations and tweak some naming

camjackson · 2017-11-13T11:55:51Z

Ok I think I've addressed now except for your last point on testing, which I replied to above. Let me know what you think 🙂

emk

This code is looking really polished.

Two very minor nitpicks, and then I'll merge it. :-)

emk · 2017-11-13T15:12:32Z

src/v2/git_url.rs

@@ -42,7 +42,9 @@ impl GitUrl {
    pub fn new<S: Into<String>>(url: S) -> Result<GitUrl> {
        let url = url.into();
        if GitUrl::should_treat_as_url(&url) {
-            Ok(GitUrl { url: url })
+            let git_url = GitUrl { url };
+            git_url.parse_parts()?;


Let's add a comment explaining the invariant we're trying to enforce (otherwise this might be a bit mysterious):

// Invariant: `parse_parts` should succeed for all `GitUrl` values after construction.

emk · 2017-11-13T15:14:01Z

src/v2/git_url.rs

+            static ref URL_PARSE: Regex = Regex::new(r#"^([^#]+)(?:#([^:]+)?(?::(.+))?)?$"#)
+                .expect("Could not parse regex URL_PARSE");
+        }
+        let captures = URL_PARSE.captures(&self.url).ok_or_else(|| -> Error { "".into() })?;


Let's make that error: format!("could not parse URL {:?}", self.url).into().

camjackson · 2017-11-14T00:50:03Z

I'm on the edge of my seat over here! 😄

emk · 2017-11-14T14:34:50Z

It's great, and it's merged! This should be released as 0.0.50 in a couple of minutes.

camjackson added 2 commits November 12, 2017 16:37

Add methods for parsing branches & subdirectories from git URLs

8535cb5

Add a method for cloning a Context without its subdirectory

9ee722b

emk requested changes Nov 12, 2017

View reviewed changes

PR feedback: Add more safety around GitUrl parsing

b36e25f

- Also clean up some String allocations and tweak some naming

emk requested changes Nov 13, 2017

View reviewed changes

Add some more error messaging and testing around GitUrl parsing

2c8e9b2

emk approved these changes Nov 14, 2017

View reviewed changes

emk merged commit 5fb8af6 into emk:master Nov 14, 2017

camjackson deleted the subdir_clean branch November 14, 2017 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing subdirectories out of git URLs (redux) #10

Add support for parsing subdirectories out of git URLs (redux) #10

camjackson commented Nov 12, 2017

emk left a comment •

edited

Loading

emk Nov 12, 2017

emk Nov 12, 2017

emk Nov 12, 2017

emk Nov 12, 2017

camjackson Nov 13, 2017

emk Nov 12, 2017

camjackson Nov 12, 2017

emk Nov 12, 2017

camjackson Nov 12, 2017 •

edited

Loading

camjackson Nov 12, 2017 •

edited

Loading

emk Nov 12, 2017

camjackson Nov 12, 2017

emk Nov 12, 2017

camjackson Nov 12, 2017

emk Nov 13, 2017

emk Nov 12, 2017

camjackson Nov 13, 2017 •

edited

Loading

emk Nov 13, 2017

emk Nov 12, 2017

camjackson commented Nov 13, 2017 •

edited

Loading

emk left a comment

emk Nov 13, 2017

emk Nov 13, 2017

camjackson commented Nov 14, 2017

emk commented Nov 14, 2017

Add support for parsing subdirectories out of git URLs (redux) #10

Add support for parsing subdirectories out of git URLs (redux) #10

Conversation

camjackson commented Nov 12, 2017

emk left a comment • 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

camjackson Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

camjackson Nov 12, 2017 • 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

camjackson Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camjackson commented Nov 13, 2017 • edited Loading

emk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camjackson commented Nov 14, 2017

emk commented Nov 14, 2017

emk left a comment •

edited

Loading

camjackson Nov 12, 2017 •

edited

Loading

camjackson Nov 12, 2017 •

edited

Loading

camjackson Nov 13, 2017 •

edited

Loading

camjackson commented Nov 13, 2017 •

edited

Loading