Skip to content

Overhaul Commit Amount Calculation Timing and Exposure #123

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

Merged
marstr merged 2 commits into from
Apr 2, 2026
Merged

Overhaul Commit Amount Calculation Timing and Exposure #123

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b05da4
Add commit --dry-run flag
marstr Apr 2, 2026
30c3115
Move commit amount calculation out of args
marstr Apr 2, 2026
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
73 changes: 55 additions & 18 deletions cmd/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"fmt"
"io"
"os"
"path/filepath"
"strings"
"time"
Expand All @@ -32,13 +31,14 @@ import (
"github.com/spf13/cast"
"github.com/spf13/cobra"

"github.com/marstr/baronial/internal/format"
"github.com/marstr/baronial/internal/index"
)

const (
amountFlag = "amount"
amountShorthand = "a"
amountDefault = "<calculated>"
amountDefault = "<gross change>"
amountUsage = "The magnitude of the transaction that should be displayed in logs."
)

Expand Down Expand Up @@ -77,6 +77,13 @@ const (
forceUsage = "Ignore warnings, commit the transaction anyway."
)

const (
dryrunFlag = "dry-run"
dryrunShorthand = "d"
dryrunDefault = false
dryrunUsage = "Generates and prints a commit without writing it or updating any references."
)

const (
bankRecordIDFlag = "bank-record-id"
bankRecordIDShorthand = "b"
Expand Down Expand Up @@ -121,16 +128,6 @@ var commitCmd = &cobra.Command{
if err != nil {
return err
}
} else {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

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.EnteredTime = time.Now()
Expand All @@ -146,6 +143,14 @@ var commitCmd = &cobra.Command{
logrus.Fatal(err)
}

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)
Comment on lines +146 to 154

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
if err != nil {
logrus.Fatal(err)
Expand Down Expand Up @@ -174,8 +179,8 @@ var commitCmd = &cobra.Command{
shouldContinue, err := promptToContinue(
ctx,
"proceed despite imbalance?",
os.Stdout,
os.Stdin)
cmd.OutOrStdout(),
cmd.InOrStdin())
if err != nil {
logrus.Fatal(err)
}
Expand Down Expand Up @@ -255,10 +260,44 @@ var commitCmd = &cobra.Command{
}
commitTransactionFromFlags.RecordID = envelopes.BankRecordID(rawRecordId)

err = persist.Commit(ctx, repo, commitTransactionFromFlags, additionalParents...)
var dryrun bool
dryrun, err = cmd.Flags().GetBool(dryrunFlag)
if err != nil {
logrus.Fatal(err)
}

if dryrun {
var head persist.RefSpec
head, err = repo.Current(ctx)
if err != nil {
logrus.Fatal(err)
}

var parent envelopes.ID
if head != "" {
parent, err = persist.Resolve(ctx, repo, head)
if err != nil {
logrus.Fatal(err)
}
}

if parent.Equal(envelopes.ID{}) {
commitTransactionFromFlags.Parents = []envelopes.ID{}
} else {
commitTransactionFromFlags.Parents = append([]envelopes.ID{parent}, additionalParents...)
}
Comment on lines +284 to +288

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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

Copilot uses AI. Check for mistakes.

err = format.PrettyPrintTransaction(ctx, cmd.OutOrStdout(), repo, commitTransactionFromFlags)
if err != nil {
logrus.Fatal(err)
}

} else {
err = persist.Commit(ctx, repo, commitTransactionFromFlags, additionalParents...)
if err != nil {
logrus.Fatal(err)
}
}
},
}

Expand All @@ -272,6 +311,7 @@ func init() {
commitCmd.Flags().StringP(amountFlag, amountShorthand, amountDefault, amountUsage)
commitCmd.Flags().StringP(bankRecordIDFlag, bankRecordIDShorthand, bankRecordIDDefault, bankRecordIDUsage)
commitCmd.Flags().BoolP(forceFlag, forceShorthand, forceDefault, forceUsage)
commitCmd.Flags().BoolP(dryrunFlag, dryrunShorthand, dryrunDefault, dryrunUsage)
}

func promptToContinue(ctx context.Context, message string, output io.Writer, input io.Reader) (bool, error) {
Expand Down Expand Up @@ -346,9 +386,6 @@ func promptToContinue(ctx context.Context, message string, output io.Writer, inp
}

func calculateAmount(ctx context.Context, targetDir string) (envelopes.Balance, error) {
ctx, cancel := context.WithTimeout(ctx, 2*time.Second)
defer cancel()

targetDir, err := index.RootDirectory(targetDir)
if err != nil {
return envelopes.Balance{}, err
Expand Down
Loading