Overhaul Commit Amount Calculation Timing and Exposure#123
Conversation
This will eventually help with getting rid of the amount calculation that slows down the help text and can timeout on slow storage.
This was wreaking havoc when storage access was slow. In tandem with a new dry-run flag, people should have the same ability to demystify what is happening with amount calculation but only doing it if you actually need to, and not timing out unless to respect the global timeout flag.
There was a problem hiding this comment.
Pull request overview
Adjusts commit command behavior so the transaction amount is calculated at execution time (respecting the global timeout) and adds a --dry-run mode to preview the resulting transaction without persisting it, addressing slow-storage timeouts described in #122.
Changes:
- Moves automatic amount calculation out of the Cobra
Argshook and intoRun, removing the hardcoded 2s timeout. - Adds
--dry-run/-dto print the would-be commit instead of writing it. - Switches imbalance prompt I/O to Cobra’s
cmd.InOrStdin()/cmd.OutOrStdout().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if parent.Equal(envelopes.ID{}) { | ||
| commitTransactionFromFlags.Parents = []envelopes.ID{} | ||
| } else { | ||
| commitTransactionFromFlags.Parents = append([]envelopes.ID{parent}, additionalParents...) | ||
| } |
There was a problem hiding this comment.
In the dry-run path, commitTransactionFromFlags.Parents is set to an empty slice when there is no current HEAD. format.PrettyPrintTransaction assumes subject.Parents[0] exists and will panic for empty Parents. Ensure Parents always has at least one entry (e.g., include a zero envelopes.ID{} placeholder as the first parent) before calling PrettyPrintTransaction.
| if parent.Equal(envelopes.ID{}) { | |
| commitTransactionFromFlags.Parents = []envelopes.ID{} | |
| } else { | |
| commitTransactionFromFlags.Parents = append([]envelopes.ID{parent}, additionalParents...) | |
| } | |
| var parents []envelopes.ID | |
| if parent.Equal(envelopes.ID{}) { | |
| // No current HEAD; include a zero ID placeholder as the first parent. | |
| parents = append([]envelopes.ID{envelopes.ID{}}, additionalParents...) | |
| } else { | |
| parents = append([]envelopes.ID{parent}, additionalParents...) | |
| } | |
| commitTransactionFromFlags.Parents = parents |
| if !cmd.Flags().Changed(amountFlag) { | ||
| var err error | ||
| commitTransactionFromFlags.Amount, err = calculateAmount(ctx, ".") | ||
| if err != nil { | ||
| logrus.Fatalf("Failed to calculate the amount from %q because of the following error: %s", amountDefault, err) | ||
| } | ||
| } | ||
|
|
||
| commitTransactionFromFlags.State, err = index.LoadState(ctx, targetDir) |
There was a problem hiding this comment.
calculateAmount loads state from disk (index.LoadState) and opens/reads the repo, and then the Run path immediately loads state again (commitTransactionFromFlags.State = index.LoadState(...)) and later re-opens/reads the repo (especially in --dry-run). On slow storage this doubles the I/O cost the PR is trying to avoid. Consider refactoring so state is loaded once and reused for both amount calculation and the commit/dry-run logic (e.g., pass the already-loaded state into the amount calculation helper, and reuse the already-open repository).
This will eventually help with getting rid of the amount calculation that slows down the help text and can timeout on slow storage.
Fixes #122