Use clap 3.2 and use only non-deprecated interfaces

clap 3.2 introduces several new interfaces that are backported from the
upcoming clap 4.0 branch. Many of these new interfaces deprecate
similar/equivalent intefaces.

StGit is updated to require clap 3.2 and to use only non-deprecated
interfaces.

clap::Arg::value_parser() and the corresponding clap::ArgMatches::get_one()
interface, which are new in 3.2, have the most impact to StGit.
Collectively they provide typed argument values. Within StGit, patch names
and patch ranges are the most common argument values to deal with. The
patchrange module is updated to take advantage of typed command line
arguments by introducing the patchrange::Specification type and associated
FromStr impl.

These changes should significantly ease the path to clap 4.0.

Author: jpgrayson

Cargo.toml

@@ -20,7 +20,7 @@ anyhow = "1.0"
 atty = "0.2"
 bstr = { version = "0.2", default-features = false, features = ["std"] }
 chrono = { version = "0.4", default-features = false, features = ["clock"] }
-clap = { version = "3.1", default-features = false, features = ["color", "std", "suggestions", "wrap_help"] }
+clap = { version = "3.2", default-features = false, features = ["color", "std", "suggestions", "wrap_help"] }
 encoding_rs = "0.8"
 git2 = { version = "0.13", default-features = false }
 indexmap = "1.8"

src/argset.rs

@@ -2,7 +2,7 @@
 
 //! [`clap::Arg`] definitions common to several StGit commands.
 
-use clap::{Arg, ValueHint};
+use clap::Arg;
 
 lazy_static! {
     /// The `--branch`/`-b` option for selecting an alternative branch.
@@ -12,7 +12,8 @@ lazy_static! {
         .help("Use BRANCH instead of current branch")
         .takes_value(true)
         .value_name("branch")
-        .value_hint(ValueHint::Other);
+        .value_hint(clap::ValueHint::Other)
+        .value_parser(parse_branch_name);
 
     /// The `--keep/-k` option.
     pub(crate) static ref KEEP_ARG: Arg<'static> =
@@ -31,9 +32,36 @@ lazy_static! {
         .help("Extra options to pass to \"git diff\"")
         .takes_value(true)
         .allow_hyphen_values(true)
-        .multiple_occurrences(true)
+        .action(clap::ArgAction::Append)
         .value_name("options")
-        .value_hint(ValueHint::Other);
+        .value_hint(clap::ValueHint::Other);
+}
+
+/// For use with `clap::Arg::value_parser()` to ensure a branch name is valid.
+pub(crate) fn parse_branch_name(name: &str) -> anyhow::Result<String> {
+    if git2::Branch::name_is_valid(name)? {
+        Ok(name.to_string())
+    } else {
+        Err(anyhow::anyhow!("invalid branch name `{name}`"))
+    }
+}
+
+/// Get a `&str` from a `clap::ArgMatches` instance for the given `id`.
+///
+/// This function may be cleaner than calling `ArgMatches::get_one::<String>()` directly
+/// since that function returns `Option<&String>` which often needs to be mapped to
+/// `Option<&str>`.
+pub(crate) fn get_one_str<'a>(matches: &'a clap::ArgMatches, id: &str) -> Option<&'a str> {
+    matches.get_one::<String>(id).map(|s| s.as_str())
+}
+
+/// For use with `clap::Arg::value_parser()` to parse a usize argument.
+///
+/// This function has a custom error message that is preferable to the messages reported
+/// by `str::parse::<usize>()`.
+pub(crate) fn parse_usize(s: &str) -> anyhow::Result<usize> {
+    s.parse::<usize>()
+        .map_err(|_| anyhow::anyhow!("'{s}' is not a positive integer"))
 }
 
 /// Compose aggregate set of git diff options from various sources.
@@ -62,7 +90,7 @@ pub(crate) fn get_diff_opts(
         }
     }
 
-    if let Some(values) = matches.values_of("diff-opts") {
+    if let Some(values) = matches.get_many::<String>("diff-opts") {
         for value in values {
             for arg in value.split_ascii_whitespace() {
                 opts.push(String::from(arg))

src/cmd/branch.rs

@@ -10,6 +10,7 @@ use clap::{Arg, ArgMatches};
 use termcolor::WriteColor;
 
 use crate::{
+    argset::{self, get_one_str},
     print_info_message, print_warning_message,
     repo::RepositoryExtended,
     stack::{get_branch_name, state_refname_from_branch_name, Stack, StackStateAccess},
@@ -82,12 +83,12 @@ fn make() -> clap::Command<'static> {
                     Arg::new("new-branch")
                         .help("New branch name")
                         .required(true)
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 )
                 .arg(
                     Arg::new("committish")
                         .help("Base commit for new branch")
-                        .forbid_empty_values(true),
+                        .value_parser(clap::builder::NonEmptyStringValueParser::new()),
                 ),
         )
         .subcommand(
@@ -105,7 +106,7 @@ fn make() -> clap::Command<'static> {
                 .arg(
                     Arg::new("new-branch")
                         .help("New branch name")
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 ),
         )
         .subcommand(
@@ -121,7 +122,7 @@ fn make() -> clap::Command<'static> {
                         .multiple_values(true)
                         .min_values(1)
                         .max_values(2)
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 ),
         )
         .subcommand(
@@ -133,7 +134,7 @@ fn make() -> clap::Command<'static> {
                     Arg::new("branch")
                         .help("Branch to protect")
                         .value_name("branch")
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 ),
         )
         .subcommand(
@@ -145,7 +146,7 @@ fn make() -> clap::Command<'static> {
                     Arg::new("branch")
                         .help("Branch to unprotect")
                         .value_name("branch")
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 ),
         )
         .subcommand(
@@ -165,7 +166,7 @@ fn make() -> clap::Command<'static> {
                         .help("Branch to delete")
                         .value_name("branch")
                         .required(true)
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 )
                 .arg(
                     Arg::new("force")
@@ -189,7 +190,7 @@ fn make() -> clap::Command<'static> {
                     Arg::new("branch")
                         .help("Branch to clean up")
                         .value_name("branch")
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 )
                 .arg(
                     Arg::new("force")
@@ -208,7 +209,7 @@ fn make() -> clap::Command<'static> {
                     Arg::new("branch-any")
                         .help("Branch to describe")
                         .value_name("branch")
-                        .forbid_empty_values(true),
+                        .value_parser(argset::parse_branch_name),
                 ),
         )
         .arg(
@@ -221,7 +222,7 @@ fn make() -> clap::Command<'static> {
             Arg::new("branch-any")
                 .help("Branch to switch to")
                 .value_name("branch")
-                .forbid_empty_values(true),
+                .value_parser(argset::parse_branch_name),
         )
 }
 
@@ -243,11 +244,11 @@ fn run(matches: &ArgMatches) -> Result<()> {
     } else {
         let current_branch = repo.get_branch(None)?;
         let current_branchname = get_branch_name(&current_branch)?;
-        if let Some(target_branchname) = matches.value_of("branch-any") {
+        if let Some(target_branchname) = get_one_str(matches, "branch-any") {
             if target_branchname == current_branchname {
                 Err(anyhow!("{target_branchname} is already the current branch"))
             } else {
-                if !matches.is_present("merge") {
+                if !matches.contains_id("merge") {
                     repo.check_worktree_clean()?;
                 }
                 let target_branch = repo.get_branch(Some(target_branchname))?;
@@ -422,12 +423,12 @@ fn list(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 }
 
 fn create(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let new_branchname = matches.value_of("new-branch").expect("required argument");
+    let new_branchname = get_one_str(matches, "new-branch").expect("required argument");
 
     repo.check_repository_state()?;
     repo.check_conflicts()?;
 
-    let parent_branch = if let Some(committish) = matches.value_of("committish") {
+    let parent_branch = if let Some(committish) = get_one_str(matches, "committish") {
         repo.check_worktree_clean()?;
         let mut parent_branch = None;
         if let Ok(local_parent_branch) = repo.find_branch(committish, git2::BranchType::Local) {
@@ -458,7 +459,7 @@ fn create(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 
     let target_commit = if let Some(parent_branch) = parent_branch.as_ref() {
         parent_branch.get().peel_to_commit()?
-    } else if let Some(committish) = matches.value_of("committish") {
+    } else if let Some(committish) = get_one_str(matches, "committish") {
         crate::revspec::parse_stgit_revision(&repo, Some(committish), None)?.peel_to_commit()?
     } else {
         repo.head()?.peel_to_commit()?
@@ -515,7 +516,7 @@ fn clone(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
     let current_branch = repo.get_branch(None)?;
     let current_branchname = get_branch_name(&current_branch)?;
 
-    let new_branchname = if let Some(new_branchname) = matches.value_of("new-branch") {
+    let new_branchname = if let Some(new_branchname) = get_one_str(matches, "new-branch") {
         new_branchname.to_string()
     } else {
         let suffix = chrono::Local::now().format("%Y%m%d-%H%M%S");
@@ -560,7 +561,11 @@ fn clone(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 }
 
 fn rename(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let names: Vec<_> = matches.values_of("branch-any").unwrap().collect();
+    let names: Vec<_> = matches
+        .get_many::<String>("branch-any")
+        .unwrap()
+        .map(|s| s.as_str())
+        .collect();
     let current_branchname;
     let (old_branchname, new_branchname) = if names.len() == 2 {
         repo.get_branch(Some(names[0]))?;
@@ -596,19 +601,19 @@ fn rename(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 }
 
 fn protect(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let stack = Stack::from_branch(&repo, matches.value_of("branch"))?;
+    let stack = Stack::from_branch(&repo, get_one_str(matches, "branch"))?;
     let mut config = repo.config()?;
     stack.set_protected(&mut config, true)
 }
 
 fn unprotect(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let stack = Stack::from_branch(&repo, matches.value_of("branch"))?;
+    let stack = Stack::from_branch(&repo, get_one_str(matches, "branch"))?;
     let mut config = repo.config()?;
     stack.set_protected(&mut config, false)
 }
 
 fn delete(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let target_branchname = matches.value_of("branch-any").expect("required argument");
+    let target_branchname = get_one_str(matches, "branch-any").expect("required argument");
     let mut target_branch = repo.get_branch(Some(target_branchname))?;
     let current_branch = repo.get_branch(None).ok();
     let current_branchname = current_branch.and_then(|branch| get_branch_name(&branch).ok());
@@ -621,7 +626,7 @@ fn delete(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
     if let Ok(stack) = Stack::from_branch(&repo, Some(target_branchname)) {
         if stack.is_protected(&config) {
             return Err(anyhow!("Delete not permitted: this branch is protected"));
-        } else if !matches.is_present("force") && stack.all_patches().count() > 0 {
+        } else if !matches.contains_id("force") && stack.all_patches().count() > 0 {
             return Err(anyhow!(
                 "Delete not permitted: the series still contains patches (override with --force)"
             ));
@@ -634,11 +639,11 @@ fn delete(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 }
 
 fn cleanup(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let stack = Stack::from_branch(&repo, matches.value_of("branch"))?;
+    let stack = Stack::from_branch(&repo, get_one_str(matches, "branch"))?;
     let config = repo.config()?;
     if stack.is_protected(&config) {
         return Err(anyhow!("Clean up not permitted: this branch is protected"));
-    } else if !matches.is_present("force") && stack.all_patches().count() > 0 {
+    } else if !matches.contains_id("force") && stack.all_patches().count() > 0 {
         return Err(anyhow!(
             "Clean up not permitted: the series still contains patches (override with --force)"
         ));
@@ -648,8 +653,8 @@ fn cleanup(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
 }
 
 fn describe(repo: git2::Repository, matches: &ArgMatches) -> Result<()> {
-    let branch = repo.get_branch(matches.value_of("branch-any"))?;
-    let description = matches.value_of("description").expect("required argument");
+    let branch = repo.get_branch(get_one_str(matches, "branch-any"))?;
+    let description = get_one_str(matches, "description").expect("required argument");
     let branchname = get_branch_name(&branch)?;
     let mut config = repo.config()?;
     set_description(&mut config, &branchname, description)

src/cmd/clean.rs

@@ -55,8 +55,8 @@ fn run(matches: &ArgMatches) -> Result<()> {
     repo.check_repository_state()?;
 
     let (clean_applied, clean_unapplied) = match (
-        matches.is_present("applied"),
-        matches.is_present("unapplied"),
+        matches.contains_id("applied"),
+        matches.contains_id("unapplied"),
     ) {
         (false, false) => (true, true),
         opts => opts,

src/cmd/commit.rs

@@ -52,7 +52,7 @@ fn make() -> clap::Command<'static> {
                 .help("Patches to commit")
                 .value_name("patch")
                 .multiple_values(true)
-                .forbid_empty_values(true)
+                .value_parser(clap::value_parser!(patchrange::Specification))
                 .conflicts_with_all(&["all", "number"]),
         )
         .arg(
@@ -61,10 +61,7 @@ fn make() -> clap::Command<'static> {
                 .short('n')
                 .help("Commit the specified number of applied patches")
                 .value_name("number")
-                .validator(|s| {
-                    s.parse::<usize>()
-                        .map_err(|_| format!("'{s}' is not an integer"))
-                })
+                .value_parser(crate::argset::parse_usize)
                 .conflicts_with("all"),
         )
         .arg(
@@ -84,10 +81,11 @@ fn run(matches: &ArgMatches) -> Result<()> {
     let repo = git2::Repository::open_from_env()?;
     let stack = Stack::from_branch(&repo, None)?;
 
-    let patches: Vec<PatchName> = if let Some(patchranges) = matches.values_of("patchranges") {
+    let range_specs = matches.get_many::<patchrange::Specification>("patchranges");
+    let patches: Vec<PatchName> = if let Some(range_specs) = range_specs {
         let applied_and_unapplied = stack.applied_and_unapplied().collect::<Vec<&PatchName>>();
-        let mut requested_patches = patchrange::parse(
-            patchranges,
+        let mut requested_patches = patchrange::patches_from_specs(
+            range_specs,
             &stack,
             patchrange::Allow::VisibleWithAppliedBoundary,
         )?;
@@ -98,11 +96,7 @@ fn run(matches: &ArgMatches) -> Result<()> {
                 .unwrap()
         });
         requested_patches
-    } else if let Some(number) = matches.value_of("number").map(|num_str| {
-        num_str
-            .parse::<usize>()
-            .expect("validator previously parsed this")
-    }) {
+    } else if let Some(number) = matches.get_one::<usize>("number").copied() {
         if number == 0 {
             return Ok(());
         } else if number > stack.applied().len() {
@@ -114,13 +108,13 @@ fn run(matches: &ArgMatches) -> Result<()> {
         }
     } else if stack.applied().is_empty() {
         return Err(Error::NoAppliedPatches.into());
-    } else if matches.is_present("all") {
+    } else if matches.contains_id("all") {
         stack.applied().to_vec()
     } else {
         vec![stack.applied()[0].clone()]
     };
 
-    if !matches.is_present("allow-empty") {
+    if !matches.contains_id("allow-empty") {
         let mut empty_patches: Vec<&PatchName> = Vec::new();
         for pn in &patches {
             let patch_commit = stack.get_patch_commit(pn);

src/cmd/completion/bash.rs

@@ -4,7 +4,7 @@
 
 use anyhow::Result;
 
-use std::format as f;
+use std::{format as f, path::PathBuf};
 
 use super::shstream::ShStream;
 
@@ -18,7 +18,7 @@ pub(super) fn command() -> clap::Command<'static> {
                 .help("Output completion script to <path>")
                 .value_name("path")
                 .value_hint(clap::ValueHint::FilePath)
-                .allow_invalid_utf8(true),
+                .value_parser(clap::value_parser!(PathBuf)),
         )
 }
 
@@ -142,7 +142,7 @@ fn write_command_func(script: &mut ShStream, fname: &str, command: &clap::Comman
     let mut pos_index = 0;
     for arg in command.get_positionals() {
         script.ensure_blank_line();
-        if arg.is_multiple_occurrences_set()
+        if matches!(arg.get_action(), clap::ArgAction::Append)
             || (arg.is_multiple_values_set() && arg.get_value_delimiter() == Some(' '))
         {
             if let Some(num_vals) = arg.get_num_vals() {
@@ -195,7 +195,7 @@ fn write_command_func(script: &mut ShStream, fname: &str, command: &clap::Comman
 fn insert_compreply(script: &mut ShStream, arg: &clap::Arg) {
     assert!(arg.is_takes_value_set());
 
-    if let Some(possible_values) = arg.get_possible_values() {
+    if let Some(possible_values) = arg.get_value_parser().possible_values() {
         let mut possible = ShStream::new();
         for pv in possible_values {
             possible.word(pv.get_name());