Add CLI completions #265

iesahin · 2025-01-15T10:47:28Z

Upgraded and set version to 0.6.14-alpha.1
Added completions to xvc-test-helper
Added completions for xvc
Bump version
Removed test_aliases and added test_completions
Add xvc completions page to ref
Add aliases to xvc file
add aliases to top commands
Bump version
Testing dynamic completions
bump version
add list references and list branches functions
add gix errors
make git module public
Add dynamic completions ✅ and cleaned up duplicate code
add cli handling errors
changes for dynamic completions
moved all completion related functions to completions
Bump version
Adding completers

✨ Description by Callstackai

This PR introduces CLI completions for various commands, enhances error handling, and updates dependencies to version 0.6.14-alpha.8.

Diagrams of code changes

sequenceDiagram
    participant User
    participant Shell
    participant XvcCLI
    participant Completer
    participant Store
    participant Git

    User->>Shell: Type partial command
    Shell->>XvcCLI: Request completions
    
    alt Command Completion
        XvcCLI->>Completer: Get command completions
        Completer-->>XvcCLI: Return available commands/options
    else Dynamic Path Completion
        XvcCLI->>Store: Load store for completion
        Store-->>XvcCLI: Return stored paths
        XvcCLI->>Completer: Filter paths by prefix
        Completer-->>XvcCLI: Return matching paths
    else Git Reference Completion
        XvcCLI->>Git: List references/branches
        Git-->>XvcCLI: Return git refs
        XvcCLI->>Completer: Filter by prefix
        Completer-->>XvcCLI: Return matching refs
    end

    XvcCLI-->>Shell: Return completion candidates
    Shell-->>User: Show completion suggestions

Files Changed

File	Summary
CHANGELOG.md	Updated changelog for version 0.6.14-alpha.8 with new features and improvements.
book/src/SUMMARY.md	Added a reference to the new completions page.
book/src/ref/xvc-completions.md	Created a new documentation page for xvc completions.
core/src/error.rs	Added new error types for Gix repository handling.
core/src/util/completer.rs	Implemented completion helpers for Git references and branches.
lib/src/main.rs	Added handling for shell completions in the main entry point.
lib/tests/test_completions.rs	Added tests for the new completion functionality.

This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions .md, .toml, .zsh. See list of supported languages.

netlify · 2025-01-15T10:47:44Z

✅ Deploy Preview for xvc canceled.

Name	Link
🔨 Latest commit	`b58cda1`
🔍 Latest deploy log	https://app.netlify.com/sites/xvc/deploys/6799f048337f5b0009338c15

callstackai

Key Issues

The PR review highlights the need for proper error handling to prevent silent failures, particularly with the use of unwrap() and unwrap_or_else, which can lead to panics or data corruption. Additionally, converting OsStr to string using to_string_lossy() may result in data loss with invalid Unicode sequences, and the code should implement functionality for setting a default pipeline instead of returning a Todo error.

callstackai · 2025-01-15T10:50:59Z

core/src/util/git.rs

+    ref_platform.all().map(|all| {
+        all.for_each(|reference| {
+            if let Ok(reference) = reference {
+                if let Some((_, name)) = reference.name().category_and_short_name() {
+                    refs.push(name.to_string());
+                }
+            }
+        });
+        Ok(refs)
+    })?


🐛 Possible Bug
The map closure silently ignores errors from reference and category_and_short_name(). This could hide important errors and lead to incomplete results. The closure should properly handle and propagate these errors using the ? operator.

Suggested change

ref_platform.all().map(|all| {

all.for_each(|reference| {

if let Ok(reference) = reference {

if let Some((_, name)) = reference.name().category_and_short_name() {

refs.push(name.to_string());

}

}

});

Ok(refs)

})?

ref_platform.all().map(|all| {

all.try_for_each(|reference| -> Result<()> {

let reference = reference?;

if let Some((_, name)) = reference.name().category_and_short_name() {

refs.push(name.to_string());

}

Ok(())

})?;

Ok(refs)

})?

callstackai · 2025-01-15T10:51:01Z

core/src/util/completer.rs

+/// Return completions for all Git branches starting with `current` in the current directory
+/// Used in `--to-branch` option
+pub fn git_branch_completer(current: &std::ffi::OsStr) -> Vec<CompletionCandidate> {
+    let current = current.to_string_lossy();


🐛 Possible Bug
Converting OsStr to string using to_string_lossy() could lead to data loss or unexpected behavior with invalid Unicode sequences. Consider handling non-Unicode paths explicitly.

Suggested change

let current = current.to_string_lossy();

let current = match current.to_str() {

Some(s) => s.to_string(),

None => return Vec::new(), // or handle invalid Unicode explicitly

};

callstackai · 2025-01-15T10:51:03Z

pipeline/src/lib.rs

@@ -194,17 +205,16 @@ pub fn cmd_pipeline<R: BufRead>(
    // This should already be filled from the conf if not given
    let pipeline_name = command.pipeline_name.unwrap();


🐛 Possible Bug
Using unwrap() on pipeline_name can cause a panic if the value is None. Even though there's a comment suggesting it should be filled from config, it's better to handle the None case explicitly with proper error handling.

Suggested change

let pipeline_name = command.pipeline_name.unwrap();

let pipeline_name = command.pipeline_name.ok_or_else(|| anyhow::anyhow!("Pipeline name is required but not provided"))?

callstackai

Key Issues

The PR review highlights critical issues including potential data loss from improper OsStr to str conversion, unhandled feature flags leading to compile-time or runtime errors, and insufficient error handling in file system operations that could cause program crashes or partial initialization.

callstackai · 2025-01-26T08:35:04Z

pipeline/src/pipeline/util.rs

+/// Return all step names starting with `prefix`
+pub fn step_name_completer(prefix: &OsStr) -> Vec<CompletionCandidate> {
+    // This must be safe as we don't allow Non-UTF-8 strings for storage identifiers
+    let prefix = prefix.to_str().unwrap_or("");


🐛 Possible Bug
Using unwrap_or("") on OsStr to str conversion could lead to data loss or incorrect behavior if the prefix contains non-UTF8 characters. Since this is used for name matching, it could cause valid matches to be missed.

Suggested change

let prefix = prefix.to_str().unwrap_or("");

let prefix = prefix.to_str().unwrap_or_else(|| {

log::warn!("Non-UTF8 prefix received: {:?}", prefix);

""

});

callstackai · 2025-01-26T08:35:05Z

storage/src/storage/common_ops.rs

+        match self {
+            XvcStorage::Local(lr) => lr.send(output, xvc_root, paths, force),
+            XvcStorage::Generic(gr) => gr.send(output, xvc_root, paths, force),
+            XvcStorage::Rsync(r) => r.send(output, xvc_root, paths, force),
+            #[cfg(feature = "s3")]
+            XvcStorage::S3(s3r) => s3r.send(output, xvc_root, paths, force),
+            #[cfg(feature = "minio")]
+            XvcStorage::Minio(mr) => mr.send(output, xvc_root, paths, force),
+            #[cfg(feature = "r2")]
+            XvcStorage::R2(r) => r.send(output, xvc_root, paths, force),
+            #[cfg(feature = "gcs")]
+            XvcStorage::Gcs(r) => r.send(output, xvc_root, paths, force),
+            #[cfg(feature = "wasabi")]
+            XvcStorage::Wasabi(r) => r.send(output, xvc_root, paths, force),
+            #[cfg(feature = "digital-ocean")]
+            XvcStorage::DigitalOcean(r) => r.send(output, xvc_root, paths, force),
+        }


🐛 Possible Bug
The code doesn't handle the case where a storage type is used but its feature flag is not enabled. This could lead to compile-time errors or runtime issues if the code tries to use a storage type that isn't available.

Suggested change

match self {

XvcStorage::Local(lr) => lr.send(output, xvc_root, paths, force),

XvcStorage::Generic(gr) => gr.send(output, xvc_root, paths, force),

XvcStorage::Rsync(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "s3")]

XvcStorage::S3(s3r) => s3r.send(output, xvc_root, paths, force),

#[cfg(feature = "minio")]

XvcStorage::Minio(mr) => mr.send(output, xvc_root, paths, force),

#[cfg(feature = "r2")]

XvcStorage::R2(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "gcs")]

XvcStorage::Gcs(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "wasabi")]

XvcStorage::Wasabi(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "digital-ocean")]

XvcStorage::DigitalOcean(r) => r.send(output, xvc_root, paths, force),

}

match self {

XvcStorage::Local(lr) => lr.send(output, xvc_root, paths, force),

XvcStorage::Generic(gr) => gr.send(output, xvc_root, paths, force),

XvcStorage::Rsync(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "s3")]

XvcStorage::S3(s3r) => s3r.send(output, xvc_root, paths, force),

#[cfg(feature = "minio")]

XvcStorage::Minio(mr) => mr.send(output, xvc_root, paths, force),

#[cfg(feature = "r2")]

XvcStorage::R2(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "gcs")]

XvcStorage::Gcs(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "wasabi")]

XvcStorage::Wasabi(r) => r.send(output, xvc_root, paths, force),

#[cfg(feature = "digital-ocean")]

XvcStorage::DigitalOcean(r) => r.send(output, xvc_root, paths, force),

_ => Err(anyhow::anyhow!("Storage type not available. Enable the corresponding feature flag."))

callstackai · 2025-01-26T08:35:06Z

core/src/types/xvcroot.rs

+    let abs_path = PathBuf::from(path)
+        .canonicalize()
+        .expect("Cannot canonicalize the path. Possible symlink loop.");


🐛 Possible Bug
Using expect() on canonicalize() will cause the program to panic if there's a symlink loop or permission issues. This should be handled gracefully with proper error propagation.

Suggested change

let abs_path = PathBuf::from(path)

.canonicalize()

.expect("Cannot canonicalize the path. Possible symlink loop.");

let abs_path = PathBuf::from(path)

.canonicalize()

.map_err(|e| Error::PathCanonicalizeError { path: path.into(), source: e })?

callstackai · 2025-01-26T08:35:07Z

core/src/util/completer.rs

+                // FIXME: This doesn't consider current dir to filter the elements
+                let filtered = xvc_path_store.filter(|_, xp| xp.starts_with_str(&prefix));


🐛 Possible Bug
The code collects all paths that start with the prefix without considering the current directory, which could lead to performance issues with large repositories and return irrelevant results. The FIXME comment acknowledges this, but the current implementation could cause confusion for users.

Suggested change

// FIXME: This doesn't consider current dir to filter the elements

let filtered = xvc_path_store.filter(|_, xp| xp.starts_with_str(&prefix));

let current_dir_str = current_dir.to_str().ok_or(Error::NonUtf8Path)?;

let filtered = xvc_path_store.filter(|_, xp| {

xp.starts_with_str(current_dir_str) && xp.starts_with_str(prefix)

});

codecov · 2025-01-29T09:05:08Z

Codecov Report

Attention: Patch coverage is 32.30769% with 264 lines in your changes missing coverage. Please review.

Project coverage is 71.21%. Comparing base (7022cc7) to head (b58cda1).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
core/src/util/completer.rs	0.00%	64 Missing ⚠️
storage/src/storage/common_ops.rs	41.23%	57 Missing ⚠️
storage/src/storage/mod.rs	0.00%	54 Missing ⚠️
core/src/util/git.rs	0.00%	36 Missing ⚠️
pipeline/src/pipeline/util.rs	0.00%	36 Missing ⚠️
test_helper/src/main.rs	0.00%	9 Missing ⚠️
lib/src/main.rs	80.00%	2 Missing ⚠️
pipeline/src/pipeline/deps/mod.rs	0.00%	2 Missing ⚠️
file/src/list/mod.rs	0.00%	1 Missing ⚠️
lib/src/init/mod.rs	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #265      +/-   ##
==========================================
- Coverage   71.80%   71.21%   -0.59%     
==========================================
  Files         123      125       +2     
  Lines       14172    14313     +141     
==========================================
+ Hits        10176    10193      +17     
- Misses       3996     4120     +124

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iesahin added 20 commits January 3, 2025 08:28

Upgraded and set version to 0.6.14-alpha.1

10fb6a7

Added completions to xvc-test-helper

02beb65

Added completions for xvc

cbf2d3c

Bump version

882f24b

Removed test_aliases and added test_completions

a3cbc09

Add xvc completions page to ref

eaff8c7

Add aliases to xvc file

7f7ccbb

add aliases to top commands

75a99cf

Bump version

30219d7

Testing dynamic completions

e92577f

bump version

509f076

add list references and list branches functions

e414045

add gix errors

2850bed

make git module public

68e0f78

Add dynamic completions ✅ and cleaned up duplicate code

429811b

add cli handling errors

7c009e9

changes for dynamic completions

6bf78cd

moved all completion related functions to completions

984c4da

Bump version

c6d339c

Adding completers

fc5db8a

callstackai bot reviewed Jan 15, 2025

View reviewed changes

iesahin added 8 commits January 17, 2025 13:05

Completed strum based completions

1c959a8

fixme

0196564

move find_root out of xvc_root_inner

8b0c28d

add xvc_path completer

3c6e0b8

add xvc_path_completers

6dc77a8

rename

aabaa1b

target completers

3526ffb

add strum_variants_completers

9b7ee07

iesahin added 4 commits January 26, 2025 11:30

fix issue with the help text shown

5826772

added pipeline_name and step_name completions

e711112

added storage_identifier completions

aaadfaa

update changelog

c1ef63b

callstackai bot reviewed Jan 26, 2025

View reviewed changes

iesahin added 20 commits January 26, 2025 11:35

Bump version

eaa6182

update changelog

68fe217

fix warnings

56a9ad2

update help text

cc10c6a

update help text

ea169c9

added completions doc

7d5f2b0

help text changes

5392f3d

Help text changes

808ae3e

updated messages

06adb85

Updated returned error type from anyhow

bd3159b

bump version

670c085

update messages from rsync

4f46846

update rsync ref tests

2ecc6a8

comment out run-tests.zsh

0f1adcb

cleanup

695f2a2

bump version

75fa397

version

1ffee3b

update dev tests

0f27c03

fix md differences

11aae08

Update changelog

e04baa7

iesahin added 2 commits January 29, 2025 12:05

turn off dev tests

8d062f4

bump version

b58cda1

iesahin merged commit 9c1c6bb into main Jan 29, 2025
7 checks passed

iesahin deleted the clap-complete-16608 branch January 29, 2025 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI completions #265

Add CLI completions #265

iesahin commented Jan 15, 2025 •

edited by callstackai bot

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

callstackai bot left a comment

callstackai bot Jan 15, 2025

callstackai bot Jan 15, 2025

callstackai bot Jan 15, 2025

callstackai bot left a comment

callstackai bot Jan 26, 2025

callstackai bot Jan 26, 2025

callstackai bot Jan 26, 2025

callstackai bot Jan 26, 2025

codecov bot commented Jan 29, 2025 •

edited

Loading

-    let current = current.to_string_lossy();
+    let current = match current.to_str() {
+        Some(s) => s.to_string(),
+        None => return Vec::new(), // or handle invalid Unicode explicitly
+    };

		@@ -194,17 +205,16 @@ pub fn cmd_pipeline<R: BufRead>(
		// This should already be filled from the conf if not given
		let pipeline_name = command.pipeline_name.unwrap();

-    let prefix = prefix.to_str().unwrap_or("");
+    let prefix = prefix.to_str().unwrap_or_else(|| {
+        log::warn!("Non-UTF8 prefix received: {:?}", prefix);
+        ""
+    });

		// FIXME: This doesn't consider current dir to filter the elements
		let filtered = xvc_path_store.filter(\|_, xp\| xp.starts_with_str(&prefix));

-                // FIXME: This doesn't consider current dir to filter the elements
-                let filtered = xvc_path_store.filter(|_, xp| xp.starts_with_str(&prefix));
+                let current_dir_str = current_dir.to_str().ok_or(Error::NonUtf8Path)?;
+                let filtered = xvc_path_store.filter(|_, xp| {
+                    xp.starts_with_str(current_dir_str) && xp.starts_with_str(prefix)
+                });

Add CLI completions #265

Add CLI completions #265

Conversation

iesahin commented Jan 15, 2025 • edited by callstackai bot Loading

Description by Callstackai

netlify bot commented Jan 15, 2025 • edited Loading

✅ Deploy Preview for xvc canceled.

callstackai bot left a comment

Choose a reason for hiding this comment

Key Issues

callstackai bot Jan 15, 2025

Choose a reason for hiding this comment

callstackai bot Jan 15, 2025

Choose a reason for hiding this comment

callstackai bot Jan 15, 2025

Choose a reason for hiding this comment

callstackai bot left a comment

Choose a reason for hiding this comment

Key Issues

callstackai bot Jan 26, 2025

Choose a reason for hiding this comment

callstackai bot Jan 26, 2025

Choose a reason for hiding this comment

callstackai bot Jan 26, 2025

Choose a reason for hiding this comment

callstackai bot Jan 26, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

iesahin commented Jan 15, 2025 •

edited by callstackai bot

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading