more detailed tests of what's allowed and where (#450)

Byron · Aug 7, 2022 · 57a6e69 · 57a6e69
1 parent 9c280b2
commit 57a6e69
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 43 deletions.
diff --git a/git-refspec/src/parse.rs b/git-refspec/src/parse.rs
@@ -10,6 +10,8 @@ pub enum Error {
     NegativeUnsupported,
     #[error("Negative specs must be object hashes")]
     NegativeObjectHash,
+    #[error("Fetch destinations must be ref-names, like 'HEAD:refs/heads/branch'")]
+    InvalidFetchDestination,
     #[error("Cannot push into an empty destination")]
     PushToEmpty,
     #[error("glob patterns may only involved a single '*' character, found {pattern:?}")]
@@ -98,9 +100,7 @@ pub(crate) mod function {
         if mode == Mode::Negative {
             match src {
                 Some(spec) => {
-                    if spec.len() >= git_hash::Kind::shortest().len_in_hex()
-                        && spec.iter().all(|b| b.is_ascii_hexdigit())
-                    {
+                    if looks_like_object_hash(spec) {
                         return Err(Error::NegativeObjectHash);
                     }
                 }
@@ -115,6 +115,9 @@ pub(crate) mod function {
         }
         let (src, src_had_pattern) = validated(src, operation == Operation::Push)?;
         let (dst, dst_had_pattern) = validated(dst, false)?;
+        if !dst_had_pattern && looks_like_object_hash(dst.unwrap_or_default()) {
+            return Err(Error::InvalidFetchDestination);
+        }
         if mode != Mode::Negative && src_had_pattern != dst_had_pattern {
             return Err(Error::PatternUnbalanced);
         }
@@ -126,6 +129,10 @@ pub(crate) mod function {
         })
     }
 
+    fn looks_like_object_hash(spec: &BStr) -> bool {
+        spec.len() >= git_hash::Kind::shortest().len_in_hex() && spec.iter().all(|b| b.is_ascii_hexdigit())
+    }
+
     fn validated(spec: Option<&BStr>, allow_revspecs: bool) -> Result<(Option<&BStr>, bool), Error> {
         match spec {
             Some(spec) => {

diff --git a/git-refspec/tests/parse/fetch.rs b/git-refspec/tests/parse/fetch.rs
@@ -1,5 +1,59 @@
-use crate::parse::{assert_parse, b};
-use git_refspec::{Fetch, Instruction, Mode};
+use crate::parse::{assert_parse, b, try_parse};
+use git_refspec::{parse::Error, Fetch, Instruction, Mode, Operation};
+
+#[test]
+fn revspecs_are_disallowed() {
+    for spec in ["main~1", "^@^{}", "HEAD:main~1"] {
+        assert!(matches!(
+            try_parse(spec, Operation::Fetch).unwrap_err(),
+            Error::ReferenceName(_)
+        ));
+    }
+}
+
+#[test]
+fn object_hash_as_source() {
+    assert_parse(
+        "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391:",
+        Instruction::Fetch(Fetch::Only {
+            src: b("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"),
+        }),
+    );
+}
+
+#[test]
+fn object_hash_destination_is_invalid() {
+    assert!(matches!(
+        try_parse("a:e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", Operation::Fetch).unwrap_err(),
+        Error::InvalidFetchDestination
+    ));
+}
+
+#[test]
+fn negative_must_not_be_empty() {
+    assert!(matches!(
+        try_parse("^", Operation::Fetch).unwrap_err(),
+        Error::NegativeEmpty
+    ));
+}
+
+#[test]
+fn negative_must_not_be_object_hash() {
+    assert!(matches!(
+        try_parse("^e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", Operation::Fetch).unwrap_err(),
+        Error::NegativeObjectHash
+    ));
+}
+
+#[test]
+fn negative_with_destination() {
+    for spec in ["^a:b", "^a:", "^:", "^:b"] {
+        assert!(matches!(
+            try_parse(spec, Operation::Fetch).unwrap_err(),
+            Error::NegativeWithDestination
+        ));
+    }
+}
 
 #[test]
 fn exclude() {

diff --git a/git-refspec/tests/parse/invalid.rs b/git-refspec/tests/parse/invalid.rs
@@ -6,42 +6,6 @@ fn empty() {
     assert!(matches!(try_parse("", Operation::Push).unwrap_err(), Error::Empty));
 }
 
-#[test]
-fn negative_must_not_be_empty() {
-    assert!(matches!(
-        try_parse("^", Operation::Fetch).unwrap_err(),
-        Error::NegativeEmpty
-    ));
-}
-
-#[test]
-fn negative_must_not_be_object_hash() {
-    assert!(matches!(
-        try_parse("^e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", Operation::Fetch).unwrap_err(),
-        Error::NegativeObjectHash
-    ));
-}
-
-#[test]
-fn negative_with_destination() {
-    for spec in ["^a:b", "^a:", "^:", "^:b"] {
-        assert!(matches!(
-            try_parse(spec, Operation::Fetch).unwrap_err(),
-            Error::NegativeWithDestination
-        ));
-    }
-}
-
-#[test]
-fn negative_unsupported_when_pushing() {
-    for spec in ["^a:b", "^a:", "^:", "^:b", "^"] {
-        assert!(matches!(
-            try_parse(spec, Operation::Push).unwrap_err(),
-            Error::NegativeUnsupported
-        ));
-    }
-}
-
 #[test]
 fn complex_patterns_with_more_than_one_asterisk() {
     for op in [Operation::Fetch, Operation::Push] {

diff --git a/git-refspec/tests/parse/push.rs b/git-refspec/tests/parse/push.rs
@@ -1,5 +1,15 @@
-use crate::parse::{assert_parse, b};
-use git_refspec::{Instruction, Mode, Push};
+use crate::parse::{assert_parse, b, try_parse};
+use git_refspec::{parse::Error, Instruction, Mode, Operation, Push};
+
+#[test]
+fn negative_unsupported() {
+    for spec in ["^a:b", "^a:", "^:", "^:b", "^"] {
+        assert!(matches!(
+            try_parse(spec, Operation::Push).unwrap_err(),
+            Error::NegativeUnsupported
+        ));
+    }
+}
 
 #[test]
 fn ampersand_is_resolved_to_head() {