Fix requirements -r after other options (#1028)

crates/socket-patch-cli/tests/scan_requirements_lock_only.rs

@@ -8,7 +8,9 @@
 //! * #412: pins reached through in-root `-r` includes;
 //! * #994: include targets pip unquotes (`-r "dev reqs.txt"`,
 //!   `--requirement="dev.txt"`, `-r dev\ reqs.txt`) or expands
-//!   (`-r ${REQDIR}/dev.txt`).
+//!   (`-r ${REQDIR}/dev.txt`);
+//! * #1028: a `-r` that follows other options on the line
+//!   (`--pre -r dev.txt`, `-i URL -r dev.txt`).
 //!
 //! Driven through the built binary against a mock patch API; the
 //! assertion is what discovery sends to the batch endpoint and the
@@ -207,3 +209,30 @@ async fn lock_only_scan_discovers_env_var_include_target() {
     )
     .await;
 }
+
+/// #1028: pip runs optparse over every option word of a line, so a `-r`
+/// that follows another option (`--pre`, `-i URL`, `-c FILE`) is still
+/// an include pip follows.
+#[tokio::test]
+async fn lock_only_scan_discovers_include_after_other_options() {
+    let cases: &[&str] = &[
+        "--pre -r dev.txt\n",
+        "-i https://pypi.org/simple -r dev.txt\n",
+        "--index-url=https://pypi.org/simple -r dev.txt\n",
+        "--prefer-binary -r dev.txt\n",
+        "-c c.txt -r dev.txt\n",
+        "--prefer-binary --requirement=dev.txt\n",
+    ];
+    for root in cases {
+        eprintln!("case {root:?}");
+        assert_lock_only_discovers(
+            &[
+                ("requirements.txt", root),
+                ("dev.txt", "sp-fixture-optfirst==1.0.0\n"),
+                ("c.txt", "\n"),
+            ],
+            &["pkg:pypi/sp-fixture-optfirst@1.0.0"],
+        )
+        .await;
+    }
+}

crates/socket-patch-core/src/vendor/pypi_requirements.rs

@@ -1000,31 +1000,121 @@ fn include_target(text: &str) -> Option<String> {
     include_target_with(text, |name| std::env::var(name).ok())
 }
 
+/// The long options pip's requirements-file parser knows that take a
+/// value (`SUPPORTED_OPTIONS` plus the per-requirement
+/// `SUPPORTED_OPTIONS_REQ`, and `--install-option` from older pips).
+const REQ_FILE_VALUE_OPTIONS: &[&str] = &[
+    "--index-url",
+    "--extra-index-url",
+    "--constraint",
+    "--requirement",
+    "--editable",
+    "--find-links",
+    "--no-binary",
+    "--only-binary",
+    "--trusted-host",
+    "--use-feature",
+    "--global-option",
+    "--install-option",
+    "--hash",
+    "--config-settings",
+];
+
+/// The flag-only long options of pip's requirements-file parser. Only
+/// used to resolve optparse's unique-prefix abbreviations
+/// (`--requirem` is `--requirement`, `--require` is ambiguous).
+const REQ_FILE_FLAG_OPTIONS: &[&str] =
+    &["--no-index", "--prefer-binary", "--require-hashes", "--pre"];
+
+/// Resolve a long option name the way optparse's `_match_abbrev` does: an
+/// exact name, else the one known option it is a unique prefix of.
+/// `None` for an unknown or ambiguous name.
+fn resolve_long_option(name: &str) -> Option<&'static str> {
+    let all = REQ_FILE_VALUE_OPTIONS
+        .iter()
+        .chain(REQ_FILE_FLAG_OPTIONS)
+        .copied();
+    if let Some(exact) = all.clone().find(|o| *o == name) {
+        return Some(exact);
+    }
+    let mut matches = all.filter(|o| o.starts_with(name));
+    let first = matches.next()?;
+    matches.next().is_none().then_some(first)
+}
+
 /// [`include_target`] with the environment lookup injected (tests).
+///
+/// pip splits a line into its requirement part (the leading words that
+/// don't start with `-`) and its options, and runs optparse over every
+/// option word (#1028). A line with a requirement or an `-e` is a
+/// requirement, never an include; otherwise the first `-r` value is the
+/// file pip follows (`opts.requirements[0]`). An option's value is
+/// consumed even when it looks like `-r`, as optparse does.
 fn include_target_with(text: &str, env: impl Fn(&str) -> Option<String>) -> Option<String> {
     let code = expand_env_vars(strip_comment(text), env);
+    // pip's break_args_options: a leading word without `-` is a
+    // requirement, whose per-requirement options never recurse.
+    if !code.trim_start().starts_with('-') {
+        return None;
+    }
     // An unbalanced quote is pip's "Could not split options" error: there
     // is no file to follow.
     let mut words = shlex_split(&code)?.into_iter();
-    let first = words.next()?;
-    let target = if let Some(rest) = first.strip_prefix("--requirement=") {
-        // `--requirement= dev.txt` (a space after the `=`) is read as
-        // the next word, as before the shlex split.
-        if rest.is_empty() {
-            words.next()
-        } else {
-            Some(rest.to_string())
+    let mut target: Option<String> = None;
+    while let Some(word) = words.next() {
+        if word == "--" {
+            break;
         }
-    } else {
-        match first.as_str() {
-            "-r" | "--requirement" => words.next(),
+        if let Some(long) = word.strip_prefix("--") {
+            let (name, attached) = match long.split_once('=') {
+                Some((name, value)) => (name, Some(value.to_string())),
+                None => (long, None),
+            };
+            // An unknown option is pip's parse error; read past it as a
+            // flag rather than drop the rest of the line.
+            let Some(option) = resolve_long_option(&format!("--{name}")) else {
+                continue;
+            };
+            if !REQ_FILE_VALUE_OPTIONS.contains(&option) {
+                continue;
+            }
+            let value = match attached {
+                // `--requirement= dev.txt` (a space after the `=`) is
+                // read as the next word, as before the shlex split.
+                Some(v) if v.is_empty() && option == "--requirement" => words.next(),
+                Some(v) => Some(v),
+                None => words.next(),
+            };
+            match option {
+                "--editable" => return None,
+                "--requirement" if target.is_none() => target = value,
+                _ => {}
+            }
+        } else if let Some(short) = word.strip_prefix('-') {
+            let mut chars = short.chars();
+            let Some(flag) = chars.next() else {
+                continue;
+            };
+            if !matches!(flag, 'i' | 'c' | 'r' | 'e' | 'f') {
+                continue;
+            }
             // pip's optparse also accepts the attached short form
-            // (`-rdev.txt`, `-r"dev reqs.txt"`). No other
-            // requirements-file option starts with `-r`.
-            t if t.starts_with("-r") && !t.starts_with("--") => Some(t[2..].to_string()),
-            _ => None,
+            // (`-rdev.txt`, `-r"dev reqs.txt"`).
+            let rest = chars.as_str();
+            let value = if rest.is_empty() {
+                words.next()
+            } else {
+                Some(rest.to_string())
+            };
+            match flag {
+                'e' => return None,
+                'r' if target.is_none() => target = value,
+                _ => {}
+            }
         }
-    };
+        // A bare word among the options is an optparse positional that
+        // pip ignores.
+    }
     target.filter(|t| !t.is_empty())
 }
 
@@ -2542,6 +2632,76 @@ mod tests {
         }
     }
 
+    /// #1028: pip parses every option word of a line, so a `-r` after
+    /// other options is an include, an option's value is never read as
+    /// `-r`, and the first of several `-r`s wins (`opts.requirements[0]`).
+    /// A requirement line (`six==1.16.0 ...`) or an editable is never an
+    /// include.
+    #[test]
+    fn include_target_scans_every_option_word() {
+        let env = |_: &str| None;
+        let cases: &[(&str, Option<&str>)] = &[
+            ("--pre -r dev.txt", Some("dev.txt")),
+            ("-i https://pypi.org/simple -r dev.txt", Some("dev.txt")),
+            ("-ihttps://pypi.org/simple -r dev.txt", Some("dev.txt")),
+            ("--index-url https://x/simple -r dev.txt", Some("dev.txt")),
+            ("--index-url=https://x/simple -r dev.txt", Some("dev.txt")),
+            (
+                "--extra-index-url https://x/simple -r dev.txt",
+                Some("dev.txt"),
+            ),
+            ("--prefer-binary -r dev.txt", Some("dev.txt")),
+            ("-c c.txt -r dev.txt", Some("dev.txt")),
+            ("--constraint c.txt --requirement dev.txt", Some("dev.txt")),
+            ("--prefer-binary --requirement=dev.txt", Some("dev.txt")),
+            ("-f ./wheels -rdev.txt", Some("dev.txt")),
+            ("--no-binary :all: -r dev.txt", Some("dev.txt")),
+            ("--trusted-host h -r \"dev reqs.txt\"", Some("dev reqs.txt")),
+            ("-r dev.txt -r other.txt", Some("dev.txt")),
+            ("-r dev.txt --pre", Some("dev.txt")),
+            // optparse's unique-prefix long options.
+            ("--requirem dev.txt", Some("dev.txt")),
+            ("--pre --requirem=dev.txt", Some("dev.txt")),
+            // An option value that looks like `-r` is the value.
+            ("-i -r dev.txt", None),
+            ("--index-url -r", None),
+            ("-c -r", None),
+            // Only constraints: not a requirements include.
+            ("-c c.txt", None),
+            // A requirement line's per-requirement options never recurse.
+            ("six==1.16.0 -r dev.txt", None),
+            // An editable makes the line a requirement.
+            ("-e ./pkg -r dev.txt", None),
+            ("-r dev.txt -e ./pkg", None),
+            // `--` ends the options.
+            ("--pre -- -r dev.txt", None),
+            ("--pre", None),
+        ];
+        for (line, want) in cases {
+            assert_eq!(include_target_with(line, env).as_deref(), *want, "{line:?}");
+        }
+    }
+
+    /// #1028: the planner follows an include that comes after another
+    /// option and wires the pin there instead of appending at the root.
+    #[tokio::test]
+    async fn include_after_other_option_is_followed() {
+        let tmp = write_root("--pre -r dev.txt\n").await;
+        tokio::fs::write(tmp.path().join("dev.txt"), "six==1.16.0\n")
+            .await
+            .unwrap();
+        let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA)
+            .await
+            .unwrap();
+        assert_eq!(wiring.len(), 1);
+        assert_eq!(wiring[0].file, "dev.txt");
+        assert_eq!(
+            read_root(tmp.path()).await,
+            "--pre -r dev.txt\n",
+            "root untouched — no duplicate appended"
+        );
+    }
+
     /// #994: the planner follows a quoted include with a space in its name
     /// and wires the pin there instead of appending a duplicate at the root.
     #[tokio::test]