-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add local repo cleanup hooks #21
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.
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
base: main
Are you sure you want to change the base?
Conversation
🔒 Scanned for secrets using gitleaks 8.28.0
ExitSuccess = 0 | ||
ExitPartialSuccess = 1 | ||
ExitFailure = 2 | ||
ExitUserCancelled = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you planning to use these? They seem unused atm.
verbose := c.Bool("verbose") | ||
force := c.Bool("force") | ||
|
||
ctx := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No point in passing this context around, right? You could use signal.NotifyContext
at least we can gracefully terminate the scanning with a single CTRL+C.
if err := removeHooksPath(&repos[i]); err != nil { | ||
res.Error = err | ||
} else { | ||
res.Removed = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're here, isn't res.Removed
already true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this make more sense? 🤔
if err := removeHooksPath(&repos[i]); err != nil {
res.Error = err
} else {
res.Removed = true
sum.ConfigsRemoved++
}
entries, err := os.ReadDir(path) | ||
if err != nil { | ||
if os.IsPermission(err) { | ||
return nil // Skip permission errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could print a warning here.
` | ||
err = os.WriteFile(configPath, []byte(configContent), 0o644) | ||
if err != nil { | ||
t.Fatalf("Failed to write config: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] You can consider using the usual require
for all assertions to increase readability and for consistency across Go projects.
return nil, err | ||
} | ||
|
||
err = walkDir(ctx, absRoot, 0, maxDepth, func(r Repository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] could have used github.com/karrick/godirwalk
it's faster (and imo also easier to use)
Introduce a
clean-local-hooks [hooks]
command, that will recursively search for git repositories that contain ahookPath
configuration in.git/config
Local hookPath overrides prevent the global git-hooks from running.
To test locally, download the repo and do: