Auto merge of #223 - emilio:empty-path-push, r=hoverbear+simonsapin

Truncate the serialization in Url::take_after_path

Fixes #222

I did this under the assumption that extending with an empty segment is a no-op. If we want it to cause something like `foo.com/bar//baz`, then the bit in `path_segments_mut` needs to be removed.

r? @SimonSapin

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/223)
<!-- Reviewable:end -->

src/lib.rs

@@ -326,7 +326,7 @@ impl Url {
             } else {
                 assert_eq!(self.byte_at(self.host_end), b':');
                 let port_str = self.slice(self.host_end + 1..self.path_start);
-                assert_eq!(self.port, Some(port_str.parse::<u16>().unwrap()));
+                assert_eq!(self.port, Some(port_str.parse::<u16>().expect("Couldn't parse port?")));
             }
             assert_eq!(self.byte_at(self.path_start), b'/');
         } else {
@@ -350,7 +350,7 @@ impl Url {
             assert!(fragment_start > query_start);
         }
 
-        let other = Url::parse(self.as_str()).unwrap();
+        let other = Url::parse(self.as_str()).expect("Failed to parse myself?");
         assert_eq!(&self.serialization, &other.serialization);
         assert_eq!(self.scheme_end, other.scheme_end);
         assert_eq!(self.username_end, other.username_end);
@@ -789,16 +789,21 @@ impl Url {
         form_urlencoded::Serializer::for_suffix(query, query_start + "?".len())
     }
 
-    fn take_after_path(&mut self) -> (u32, String) {
+    fn take_after_path(&mut self) -> String {
         match (self.query_start, self.fragment_start) {
-            (Some(i), _) | (None, Some(i)) => (i, self.slice(i..).to_owned()),
-            (None, None) => (to_u32(self.serialization.len()).unwrap(), String::new())
+            (Some(i), _) | (None, Some(i)) => {
+                let after_path = self.slice(i..).to_owned();
+                self.serialization.truncate(i as usize);
+                after_path
+            },
+            (None, None) => String::new(),
         }
     }
 
     /// Change this URL’s path.
     pub fn set_path(&mut self, mut path: &str) {
-        let (old_after_path_pos, after_path) = self.take_after_path();
+        let after_path = self.take_after_path();
+        let old_after_path_pos = to_u32(self.serialization.len()).unwrap();
         let cannot_be_a_base = self.cannot_be_a_base();
         let scheme_type = SchemeType::from(self.scheme());
         self.serialization.truncate(self.path_start as usize);

src/path_segments.rs

@@ -6,7 +6,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use parser::{self, SchemeType};
+use parser::{self, SchemeType, to_u32};
 use std::str;
 use Url;
 
@@ -35,7 +35,8 @@ pub struct PathSegmentsMut<'a> {
 
 // Not re-exported outside the crate
 pub fn new(url: &mut Url) -> PathSegmentsMut {
-    let (old_after_path_position, after_path) = url.take_after_path();
+    let after_path = url.take_after_path();
+    let old_after_path_position = to_u32(url.serialization.len()).unwrap();
     debug_assert!(url.byte_at(url.path_start) == b'/');
     PathSegmentsMut {
         after_first_slash: url.path_start as usize + "/".len(),

tests/unit.rs

@@ -261,10 +261,43 @@ fn issue_61() {
 }
 
 #[test]
+#[cfg(not(windows))]
 /// https://github.com/servo/rust-url/issues/197
 fn issue_197() {
-    let mut url = Url::from_file_path("/").unwrap();
+    let mut url = Url::from_file_path("/").expect("Failed to parse path");
     url.assert_invariants();
-    assert_eq!(url, Url::parse("file:///").unwrap());
-    url.path_segments_mut().unwrap().pop_if_empty();
+    assert_eq!(url, Url::parse("file:///").expect("Failed to parse path + protocol"));
+    url.path_segments_mut().expect("path_segments_mut").pop_if_empty();
+}
+
+#[test]
+/// https://github.com/servo/rust-url/issues/222
+fn append_trailing_slash() {
+    let mut url: Url = "http://localhost:6767/foo/bar?a=b".parse().unwrap();
+    url.assert_invariants();
+    url.path_segments_mut().unwrap().push("");
+    url.assert_invariants();
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar/?a=b");
+}
+
+#[test]
+/// https://github.com/servo/rust-url/issues/227
+fn extend_query_pairs_then_mutate() {
+    let mut url: Url = "http://localhost:6767/foo/bar".parse().unwrap();
+    url.query_pairs_mut().extend_pairs(vec![ ("auth", "my-token") ].into_iter());
+    url.assert_invariants();
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?auth=my-token");
+    url.path_segments_mut().unwrap().push("some_other_path");
+    url.assert_invariants();
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar/some_other_path?auth=my-token");
+}
+
+#[test]
+/// https://github.com/servo/rust-url/issues/222
+fn append_empty_segment_then_mutate() {
+    let mut url: Url = "http://localhost:6767/foo/bar?a=b".parse().unwrap();
+    url.assert_invariants();
+    url.path_segments_mut().unwrap().push("").pop();
+    url.assert_invariants();
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?a=b");
 }