Update commit timeout#121
Conversation
There was a problem hiding this comment.
Pull request overview
Updates how baronial commit calculates the default transaction amount so it can run under the CLI’s global timeout instead of a fixed short timeout, improving usability on slow filesystems (e.g., NAS mounts).
Changes:
- Switch amount auto-calculation to use the shared root context rather than a local 5s timeout context.
- Tidy module dependencies (removes an indirect
golang.org/x/cryptorequirement and adjustsgo.sumentries).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/commit.go | Uses the root context for amount calculation during flag parsing/validation. |
| go.mod | Removes an indirect dependency entry. |
| go.sum | Updates dependency hashes/entries after module graph change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| var err error | ||
| commitTransactionFromFlags.Amount, err = calculateAmount(ctx, ".") | ||
| commitTransactionFromFlags.Amount, err = calculateAmount(rootContext, ".") | ||
| if err != nil { | ||
| logrus.Fatalf("Failed to calculate the amount from %q because of the following error: %s", amountDefault, err) | ||
| } |
There was a problem hiding this comment.
rootContext is used during Args validation, but it is only initialized by calling RootContext(cmd) (currently done in Run). If Args runs first (typical for Cobra), rootContext will be nil here and calculateAmount will panic when it calls context.WithTimeout on a nil context. Initialize a context in Args via RootContext(cmd) (and if you need cancellation, cancel only a derived child context so you don’t cancel the shared root context).
| var err error | ||
| commitTransactionFromFlags.Amount, err = calculateAmount(ctx, ".") | ||
| commitTransactionFromFlags.Amount, err = calculateAmount(rootContext, ".") | ||
| if err != nil { |
There was a problem hiding this comment.
This change won’t actually make calculateAmount respect the global --timeout value, because calculateAmount wraps the provided context with its own hard-coded 2*time.Second timeout. As a result, the operation can still time out at 2s even if rootContext has a larger deadline (and the previous 5s wrapper didn’t matter either). Consider removing the internal fixed timeout or deriving it from the remaining time/deadline of the incoming context.
| @@ -23,7 +23,6 @@ require ( | |||
| github.com/spf13/afero v1.1.2 // indirect | |||
| github.com/spf13/jwalterweatherman v1.0.0 // indirect | |||
| github.com/spf13/pflag v1.0.6 // indirect | |||
There was a problem hiding this comment.
This PR’s stated purpose is a timeout behavior change, but it also removes the indirect golang.org/x/crypto requirement. If this is just go mod tidy churn, consider reverting/splitting it to keep the review focused; otherwise, please document why the dependency graph change is required for the timeout fix.
| github.com/spf13/pflag v1.0.6 // indirect | |
| github.com/spf13/pflag v1.0.6 // indirect | |
| golang.org/x/crypto v0.31.0 // indirect |
I was just doing my finances with a local copy of
baronialwith a gvfs mounted NAS repository. Performance was poor - but more frustratingly the calculateAmount function would fail everytime because of a hardcoded timeout. This switches it to use the rootContext, respecting the global timeout flag.