Avoid case-insensitive patch name collisions

On platforms using case-insensitive filesystems, having two patch names in
the same stack that only differ by case will cause patch reference
collisions. E.g. `p0` and `P0` would use the same reference file:
`.git/refs/patches/<branch>/p0`.

To avoid this hazard, StGit is modified to disallow case-insensitive
collisions of patch names within a stack. When new patch names are
generated with PatchName::uniquify(), a case-insensitive comparision is
performed to ensure the uniqified name differs by more than just case from
all other patch names in the stack.

And the new Stack::collides() method is used instead of Stack::has_patch()
to test whether user-provided patch names collide with any existing patch
names.

Author: jpgrayson

src/cmd/new.rs

@@ -131,14 +131,14 @@ fn run(matches: &ArgMatches) -> Result<()> {
 
     let patchname = if let Some(name) = matches.value_of("patchname") {
         let patchname = name.parse::<PatchName>()?;
-        if stack.has_patch(&patchname) {
-            return Err(anyhow!("Patch `{patchname}` already exists"));
+        if let Some(colliding_patchname) = stack.collides(&patchname) {
+            Err(anyhow!("Patch `{colliding_patchname}` already exists"))
         } else {
-            Some(patchname)
+            Ok(Some(patchname))
         }
     } else {
-        None
-    };
+        Ok(None)
+    }?;
 
     let config = repo.config()?;
 

src/cmd/pick.rs

@@ -270,7 +270,7 @@ fn pick_picks(
     let mut new_patches: Vec<(PatchName, git2::Oid)> = Vec::with_capacity(picks.len());
 
     for (patchname, commit) in picks {
-        let disallow: Vec<&PatchName> = stack.all_patches().collect();
+        let mut disallow: Vec<&PatchName> = stack.all_patches().collect();
 
         let patchname = if let Some(name) = matches.value_of("name") {
             PatchName::from_str(name)?
@@ -334,6 +334,7 @@ fn pick_picks(
                 .repo
                 .commit_ex(&author, &committer, message, top.tree_id(), [bottom.id()])?;
         new_patches.push((patchname, new_commit_id));
+        disallow.push(&new_patches[new_patches.len() - 1].0);
     }
 
     stack

src/cmd/rename.rs

@@ -60,8 +60,8 @@ fn run(matches: &ArgMatches) -> Result<()> {
         return Err(Error::NoAppliedPatches.into());
     };
 
-    if old_patchname == new_patchname {
-        return Err(anyhow!("Patch `{new_patchname}` already exists"));
+    if old_patchname.collides(&new_patchname) {
+        return Err(anyhow!("Patch `{old_patchname}` already exists"));
     }
 
     stack

src/cmd/squash.rs

@@ -91,8 +91,10 @@ fn run(matches: &ArgMatches) -> Result<()> {
         .map(|s| PatchName::from_str(s).expect("clap already validated name"));
 
     if let Some(patchname) = patchname.as_ref() {
-        if !squash_patchnames.contains(patchname) && stack.has_patch(patchname) {
-            return Err(anyhow!("Patch name `{patchname}` already taken"));
+        if !squash_patchnames.contains(patchname) {
+            if let Some(colliding_patchname) = stack.collides(patchname) {
+                return Err(anyhow!("Patch name `{colliding_patchname}` already taken"));
+            }
         }
     }
 

src/cmd/uncommit.rs

@@ -1,6 +1,6 @@
 //! `stg uncommit` implementation.
 
-use std::{collections::HashSet, str::FromStr};
+use std::str::FromStr;
 
 use anyhow::{anyhow, Result};
 use clap::{Arg, ArgMatches};
@@ -239,13 +239,20 @@ fn make_patchnames(
 }
 
 fn check_patchnames(stack: &Stack, patchnames: &[PatchName]) -> Result<()> {
-    let mut taken_names: HashSet<_> = stack.all_patches().cloned().collect();
+    let mut taken_names: Vec<&PatchName> = Vec::new();
     for patchname in patchnames {
-        if taken_names.contains(patchname) {
-            return Err(anyhow!("Patch `{patchname}` already exists"));
+        if let Some(colliding_patchname) = stack.collides(patchname) {
+            return Err(anyhow!("Patch `{colliding_patchname}` already exists"));
+        } else if let Some(colliding_patchname) =
+            taken_names.iter().find(|pn| patchname.collides(pn))
+        {
+            return Err(anyhow!(
+                "Patch name `{patchname}` collides with `{colliding_patchname}`"
+            ));
         } else {
-            taken_names.insert(patchname.clone());
+            taken_names.push(patchname);
         }
     }
+
     Ok(())
 }

src/patchname.rs

@@ -173,7 +173,7 @@ impl PatchName {
         let mut candidate = self;
         loop {
             if allow.iter().any(|pn| pn.as_ref() == &candidate)
-                || disallow.iter().all(|pn| pn.as_ref() != &candidate)
+                || disallow.iter().all(|pn| !candidate.collides(pn.as_ref()))
             {
                 break candidate;
             } else {
@@ -191,6 +191,15 @@ impl PatchName {
         }
     }
 
+    /// Test if another patch name is the same as self, ignoring case.
+    ///
+    /// Having two patch names in the same stack that only differ by case will lead to
+    /// ref collisions in case-insensitive filesystems. E.g. `refs/patches/<branch>/p0`
+    /// would collide with `refs/patches/<branch>/P0`.
+    pub(crate) fn collides(&self, other: &PatchName) -> bool {
+        self.0.eq_ignore_ascii_case(&other.0)
+    }
+
     /// Validate a patch name `str`.
     fn validate(name: &str) -> Result<(), Error> {
         if name.is_empty() {

src/stack/state.rs

@@ -74,8 +74,19 @@ pub(crate) trait StackStateAccess<'repo> {
     fn get_patch(&self, patchname: &PatchName) -> &PatchState<'repo>;
 
     /// Test whether given patch name exists in the stack.
+    ///
+    /// N.B. use [`StackStateAccess::collides()`] to test for potential patch name
+    /// collisions.
     fn has_patch(&self, patchname: &PatchName) -> bool;
 
+    /// Test whether given patch name collides with an existing patch name.
+    ///
+    /// A patch name collides if it is different from only by case from a patch in the
+    /// stack.
+    fn collides(&self, patchname: &PatchName) -> Option<&PatchName> {
+        self.all_patches().find(|pn| patchname.collides(pn))
+    }
+
     /// Get stack's top commit, or base if no applied patches.
     fn top(&self) -> &Commit<'repo>;
 

src/stack/transaction.rs

@@ -877,13 +877,14 @@ impl<'repo> StackTransaction<'repo> {
     ) -> Result<()> {
         if new_patchname == old_patchname {
             return Ok(());
-        } else if self.stack.has_patch(new_patchname)
-            && self
+        } else if let Some(colliding_patchname) = self.stack.collides(new_patchname) {
+            if self
                 .patch_updates
-                .get(new_patchname)
+                .get(colliding_patchname)
                 .map_or(true, |maybe_patch| maybe_patch.is_some())
-        {
-            return Err(anyhow!("Patch `{new_patchname}` already exists"));
+            {
+                return Err(anyhow!("Patch `{colliding_patchname}` already exists"));
+            }
         } else if !self.stack.has_patch(old_patchname) {
             return Err(anyhow!("Patch `{old_patchname}` does not exist"));
         }

t/t3400-pick.sh

@@ -276,9 +276,9 @@ test_expect_success \
 	'Pick too many commits' \
 	'
 	stg pick --ref-branch foo $(cat C-id) D-foo &&
-	test "$(echo $(stg series --applied --noprefix))" = "C2 c d" &&
+	test "$(echo $(stg series --applied --noprefix))" = "C2 c-1 d-1" &&
 	test "$(echo $(stg series --unapplied --noprefix))" = "A B C D" &&
-    stg delete c d
+	stg delete c-1 d-1
 	'
 fi