Skip to content

Add option to exclude parent directory when using * wildcard for scp-action #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add option to exclude parent directory when using * wildcard for scp-action #198

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0b93345
fix: Check common base directory and remove that directory structure …
ForestHouse2316 Jul 25, 2025
4761dd2
chore: Fix if-else syntax
ForestHouse2316 Jul 25, 2025
d28e143
fix: Check emptiness of Source path list
ForestHouse2316 Jul 25, 2025
9802e5b
fix: Fix variable scope
ForestHouse2316 Jul 25, 2025
dc5ba6c
fix: Try to fix TestIgnoreFolder failure
ForestHouse2316 Jul 25, 2025
332d52e
fix: Try to fix TestIgnoreFolder failure 2
ForestHouse2316 Jul 25, 2025
9a069a2
chore: Test with original code
ForestHouse2316 Jul 25, 2025
554f19f
chore: Rollback file
ForestHouse2316 Jul 25, 2025
64a7f0c
fix: Fix path prefix check logic
ForestHouse2316 Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,50 @@ func (p *Plugin) buildTarArgs(src string) []string {

args = append(args, "-zcf")
args = append(args, getRealPath(src))
args = append(args, files.Source...)

// For precise operation, adding an additional on/off option needed.
// e.g. SCP_ACTION_WILDCARD_COMPATIBLE
hasCommonFolder := true
Comment on lines +188 to +190
Copy link

@coderabbitai coderabbitai bot Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Make the wildcard compatibility feature optional to avoid breaking changes.

The comments indicate this feature should have an on/off option, but the current implementation makes it mandatory. This could break existing workflows that depend on preserving the directory structure.

Add a configuration field to make this feature optional:

  1. Add a field to the Config struct:
WildcardCompatible bool
  1. Wrap the new logic in a conditional:
-	// For precise operation, adding an additional on/off option needed.
-	// e.g. SCP_ACTION_WILDCARD_COMPATIBLE
-	hasCommonFolder := true
+	if !p.Config.WildcardCompatible {
+		args = append(args, files.Source...)
+		return args
+	}
+	
+	hasCommonFolder := true

This ensures backward compatibility while providing the new functionality when explicitly enabled.

🤖 Prompt for AI Agents 
In plugin.go around lines 188 to 190, the wildcard compatibility feature is
currently always enabled, which risks breaking existing workflows. To fix this,
add a boolean field named WildcardCompatible to the Config struct to control
this feature. Then, wrap the logic that sets hasCommonFolder or related wildcard
compatibility code inside a conditional that checks this new field, enabling the
feature only when explicitly set to true. This preserves backward compatibility
while allowing the new behavior when desired.

var basePrefix string
if len(files.Source) > 0 {
basePrefix = strings.TrimPrefix(filepath.Dir(files.Source[0]), "!")
for i := 1; i < len(files.Source) && hasCommonFolder; i++ {
comparePath := files.Source[i]
if strings.HasPrefix(files.Source[i], "!") {
comparePath = comparePath[1:]
}
for !strings.HasPrefix(comparePath, basePrefix) {
lastSlashIdx := strings.LastIndex(basePrefix, string(os.PathSeparator))
if lastSlashIdx == -1 {
hasCommonFolder = false // if Source[i] doesn't have same prefix
break
}
basePrefix = basePrefix[:lastSlashIdx] // shrink prefix range
Copy link

@gemini-code-assist gemini-code-assist bot Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using filepath.Dir() is more idiomatic and robust for getting the parent directory of a path than manually slicing the string with strings.LastIndex. It correctly handles various edge cases and improves cross-platform compatibility.

Suggested change
basePrefix = basePrefix[:lastSlashIdx] // shrink prefix range
basePrefix = filepath.Dir(basePrefix) // shrink prefix range

}
}
} else {
hasCommonFolder = false
}
if hasCommonFolder { // if all files are in basePrefix folder, change execution position
args = append(args, "-C", basePrefix)
var relativePaths []string
for _, path := range files.Source {
ignorePrefix := ""
if strings.HasPrefix(path, "!") {
path = path[1:]
ignorePrefix = "!"
}
rel, err := filepath.Rel(basePrefix, path)
if err != nil {
fmt.Printf("Error while processing relative paths")
Copy link

@gemini-code-assist gemini-code-assist bot Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Errors from filepath.Rel are printed to standard output with a generic message. This can be easily missed. It's better to log errors to standard error (os.Stderr) and include the paths that caused the error for easier debugging.

Suggested change
fmt.Printf("Error while processing relative paths")
fmt.Fprintf(os.Stderr, "Error while processing relative path for '%s' from base '%s': %v\n", path, basePrefix, err)

continue
}
relativePaths = append(relativePaths, ignorePrefix + rel)
}
args = append(args, relativePaths...) // modified argument appending
} else {
args = append(args, files.Source...) // original argument appending
}
Copy link

@gemini-code-assist gemini-code-assist bot Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This is a great feature, but as you've noted in the PR description, there are a couple of critical points to address before this can be merged:

  1. Feature Flag: The new logic is always active. This should be controlled by a configuration option (e.g., SCP_ACTION_WILDCARD_COMPATIBLE) to avoid breaking existing workflows for users who expect the original behavior.
  2. Unit Tests: The change lacks unit tests for the new path stripping logic. New tests are needed to cover various scenarios (e.g., common parent, no common parent, mixed ignored/non-ignored paths) and ensure the feature works as expected. These tests should also be conditional on the new feature flag.

Comment on lines +211 to +230
Copy link

@coderabbitai coderabbitai bot Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and add validation for relative path conversion.

The current error handling is insufficient and could lead to inconsistent tar archives.

	if hasCommonFolder { // if all files are in basePrefix folder, change execution position
+		// Validate that basePrefix is a valid directory
+		if info, err := os.Stat(basePrefix); err != nil || !info.IsDir() {
+			// Fallback to original behavior if basePrefix is not a valid directory
+			args = append(args, files.Source...)
+			return args
+		}
+		
		args = append(args, "-C", basePrefix)
		var relativePaths []string
		for _, path := range files.Source {
			ignorePrefix := ""
			if strings.HasPrefix(path, "!") {
				path = path[1:]
				ignorePrefix = "!"
			}
			rel, err := filepath.Rel(basePrefix, path)
			if err != nil {
-				fmt.Printf("Error while processing relative paths")
+				fmt.Printf("Error converting path %q to relative path from %q: %v\n", path, basePrefix, err)
+				// Fallback to original behavior on any error
+				args = append(args, files.Source...)
+				return args
			}
			relativePaths = append(relativePaths, ignorePrefix + rel)
		}
		args = append(args, relativePaths...) // modified argument appending
	} else {
		args = append(args, files.Source...) // original argument appending
	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if hasCommonFolder { // if all files are in basePrefix folder, change execution position
args = append(args, "-C", basePrefix)
var relativePaths []string
for _, path := range files.Source {
ignorePrefix := ""
if strings.HasPrefix(path, "!") {
path = path[1:]
ignorePrefix = "!"
}
rel, err := filepath.Rel(basePrefix, path)
if err != nil {
fmt.Printf("Error while processing relative paths")
continue
}
relativePaths = append(relativePaths, ignorePrefix + rel)
}
args = append(args, relativePaths...) // modified argument appending
} else {
args = append(args, files.Source...) // original argument appending
}
if hasCommonFolder { // if all files are in basePrefix folder, change execution position
// Validate that basePrefix is a valid directory
if info, err := os.Stat(basePrefix); err != nil || !info.IsDir() {
// Fallback to original behavior if basePrefix is not a valid directory
args = append(args, files.Source...)
return args
}
args = append(args, "-C", basePrefix)
var relativePaths []string
for _, path := range files.Source {
ignorePrefix := ""
if strings.HasPrefix(path, "!") {
path = path[1:]
ignorePrefix = "!"
}
rel, err := filepath.Rel(basePrefix, path)
if err != nil {
fmt.Printf("Error converting path %q to relative path from %q: %v\n", path, basePrefix, err)
// Fallback to original behavior on any error
args = append(args, files.Source...)
return args
}
relativePaths = append(relativePaths, ignorePrefix+rel)
}
args = append(args, relativePaths...) // modified argument appending
} else {
args = append(args, files.Source...) // original argument appending
}
🤖 Prompt for AI Agents 
In plugin.go around lines 211 to 230, the error handling when filepath.Rel fails
is insufficient and may cause inconsistent tar archives. Improve this by logging
the error details clearly instead of just printing a generic message, and
consider returning or handling the error to prevent appending invalid relative
paths. Add validation to ensure relative paths are correctly computed before
appending them to args, and handle any errors gracefully to maintain consistent
archive creation.

return args
}

Expand Down