From 64c122b8f262bd2d3c5f7db7af05e3ad7cdb0f56 Mon Sep 17 00:00:00 2001 From: Travis DePrato <773453+travigd@users.noreply.github.com> Date: Tue, 6 Jun 2023 09:48:36 -0700 Subject: [PATCH] fix(stack diff): Compute stack diff correctly (#159) This was originally gonna just be running the diff in interactive mode to enable the pager but I figured out the way were computing the diff wasn't quite right. I describe the complexities in comments in `stack_diff.go`. # Test plan I have branches like ``` * 2023-06-05-one-b 2d1f535 (2023-06-05-one) | | * 2023-06-05-two-b 001f28f (HEAD -> 2023-06-05-two) |/ | * 2023-06-05-one-a aba0167 ``` Before this change, running `av stack diff` on the second branch includes removing the changes from the second commit of branch one. ```diff $ av stack diff diff --git a/2023-06-05 b/2023-06-05 index aee8822..6dc6612 100644 --- a/2023-06-05 +++ b/2023-06-05 @@ -1,2 +1,2 @@ Mon Jun 5 15:02:42 PDT 2023 -Mon Jun 5 15:05:50 PDT 2023 +Mon Jun 5 15:05:04 PDT 2023 ``` After this change, it shows just the diff from the second branch commits and also a warning that the branch is not up to date. ```diff $ av stack diff diff --git a/2023-06-05 b/2023-06-05 index 5982c55..6dc6612 100644 --- a/2023-06-05 +++ b/2023-06-05 @@ -1 +1,2 @@ Mon Jun 5 15:02:42 PDT 2023 +Mon Jun 5 15:05:04 PDT 2023 WARNING: Branch 2023-06-05-two is not up to date with parent branch 2023-06-05-one. Run av stack sync to synchronize the branch. ``` --- cmd/av/stack_diff.go | 88 +++++++++++++++++++++++++++++++++--- cmd/av/stack_tree.go | 2 +- internal/actions/reparent.go | 2 +- internal/git/diff.go | 42 ++++++++--------- 4 files changed, 103 insertions(+), 31 deletions(-) diff --git a/cmd/av/stack_diff.go b/cmd/av/stack_diff.go index 865c794a..891829d0 100644 --- a/cmd/av/stack_diff.go +++ b/cmd/av/stack_diff.go @@ -5,8 +5,11 @@ import ( "os" "strings" + "github.com/aviator-co/av/internal/actions" + "github.com/aviator-co/av/internal/meta" + "github.com/aviator-co/av/internal/utils/colors" + "github.com/aviator-co/av/internal/git" - "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -34,17 +37,90 @@ Generates the diff between the working tree and the parent branch } tx := db.ReadTx() - branch, _ := tx.Branch(currentBranchName) + branch, exists := tx.Branch(currentBranchName) + if !exists { + defaultBranch, err := repo.DefaultBranch() + if err != nil { + return err + } + branch.Parent = meta.BranchState{ + Name: defaultBranch, + Trunk: true, + } + } + + diffArgs := []string{"diff"} + notUpToDate := false + + if branch.Parent.Trunk { + // Compare against the merge-base so that we effectively only see the + // diff associated with this branch. Without this, if main has + // advanced since this branch was created, we'd also see the (inverse) + // diff between main and this branch. For example, if we have: + // main: X -> Y + // one: \-> 1a -> 1b + // without --merge-base, we compute the diff between Y and 1b, which + // shows that we're undoing the changes that were introduced in Y + // (since 1b doesn't have those changes). With --merge-base, we + // compute the diff relative to X, which is probably what the user + // expects. + // This roughly matches the diff that GitHub will show in the pull + // request files changed view. + diffArgs = append(diffArgs, "--merge-base", branch.Parent.Name) + } else { + // For a non-root branch, we compare against the original branch point. + // We don't want to compare exactly against the parent branch since + // the parent branch might have been modified but not yet synced. + // For example, if we have: + // main: X -> Y + // one: \-> 1a + // two: \-> 2a + // and then one is updated (either by adding a new commit or amending), + // we still only want to show the diff associated with two as {2a}. + // Concretely, if we have + // main: X -> Y + // one: \-> 1a -> 1b + // two: \-> 2a + // we don't want to compute the diff between 1b and 2a since that + // will show the diff between 1b and 2a, which shows that we're + // undoing the changes that were introduced in 1b (since 2a doesn't + // have those changes). Instead, we want to compute the diff between + // 1a and 2a. We can't just use merge-base here to account for the + // fact that one might be amended (not just advanced). + diffArgs = append(diffArgs, branch.Parent.Head) - diff, err := repo.Diff(&git.DiffOpts{ - Color: !color.NoColor, - Commit: branch.Parent.Name, + // Determine if the branch is up-to-date with the parent and warn if + // not. + currentParentHead, err := repo.RevParse(&git.RevParse{Rev: branch.Parent.Name}) + if err != nil { + return err + } + notUpToDate = currentParentHead != branch.Parent.Head + } + + // NOTE: + // We don't use repo.Diff here since that sets the --exit-error flag + // which in turn disables the output pager. We want this command to + // behave similarly to default `git diff` for the user. + _, err = repo.Run(&git.RunOpts{ + Args: diffArgs, + Interactive: true, }) if err != nil { return err } - _, _ = fmt.Fprint(os.Stderr, diff.Contents) + // We need to display this **after** the diff to ensure that the diff + // output pager doesn't eat this message. + if notUpToDate { + _, _ = fmt.Fprint(os.Stderr, + colors.Warning("\nWARNING: Branch "), colors.UserInput(currentBranchName), + colors.Warning(" is not up to date with parent branch "), + colors.UserInput(branch.Parent.Name), colors.Warning(". Run "), + colors.CliCmd("av stack sync"), colors.Warning(" to synchronize the branch.\n"), + ) + return actions.ErrExitSilently{ExitCode: 1} + } return nil }, diff --git a/cmd/av/stack_tree.go b/cmd/av/stack_tree.go index 1725aa0e..4527cfdf 100644 --- a/cmd/av/stack_tree.go +++ b/cmd/av/stack_tree.go @@ -150,7 +150,7 @@ func getUpstreamStatus(repo *git.Repo, branch meta.Branch) (string, error) { upstreamBranch := fmt.Sprintf("remotes/origin/%s", branch.Name) upstreamDiff, err := repo.Diff( - &git.DiffOpts{Quiet: true, Branch1: branch.Name, Branch2: upstreamBranch}, + &git.DiffOpts{Quiet: true, Specifiers: []string{branch.Name, upstreamBranch}}, ) if err != nil { return "", err diff --git a/internal/actions/reparent.go b/internal/actions/reparent.go index 339cd7f9..6c5f700b 100644 --- a/internal/actions/reparent.go +++ b/internal/actions/reparent.go @@ -61,7 +61,7 @@ func Reparent( "\n", ) - diff, err := repo.Diff(&git.DiffOpts{Commit: "HEAD", Quiet: true}) + diff, err := repo.Diff(&git.DiffOpts{Specifiers: []string{"HEAD"}, Quiet: true}) if err != nil { return nil, err } diff --git a/internal/git/diff.go b/internal/git/diff.go index f7bcdd90..934e9553 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -1,25 +1,23 @@ package git import ( - "os/exec" - "emperror.dev/errors" ) type DiffOpts struct { - // If specified, generate the diff between the working tree and this commit. - // If empty (default), generates the diff between the working tree and the - // current index (i.e., the diff containing all unstaged changes). - Commit string + // The revisions to compare. + // The behavior of the diff changes depending on how these are specified. + // - If empty, the generated diff is relative to the current staging area. + // - If one commit is given, the diff is calculated between the working tree + // and the given commit. + // - If two commits are given (or one string representing a commit range + // like `..`), the diff is calculated between the two commits. + Specifiers []string // If true, don't actually generate the diff, just return whether or not its // empty. If set, Diff.Contents will always be an empty string. Quiet bool // If true, shows the colored diff. Color bool - // Both branches need to be specified in order to find the diff between the two branches. - // If a Commit is specified, the branches will not be used. - Branch1 string - Branch2 string // If specified, compare only the specified paths. Paths []string } @@ -35,29 +33,27 @@ func (r *Repo) Diff(d *DiffOpts) (*Diff, error) { if d.Quiet { args = append(args, "--quiet") } - if d.Commit != "" { - args = append(args, d.Commit) - } else if d.Branch1 != "" && d.Branch2 != "" { - args = append(args, d.Branch1) - args = append(args, d.Branch2) - } - if d.Color { args = append(args, "--color=always") } + args = append(args, d.Specifiers...) // This needs to be last because everything after the `--` is interpreted // as a path, not a flag. if len(d.Paths) > 0 { args = append(args, "--") args = append(args, d.Paths...) } - contents, err := r.Git(args...) - var exitError *exec.ExitError - if errors.As(err, &exitError) && exitError.ExitCode() == 1 { - return &Diff{Empty: false, Contents: contents}, nil - } else if err != nil { + output, err := r.Run(&RunOpts{ + Args: args, + }) + if err != nil { return nil, err } - return &Diff{Empty: true, Contents: contents}, nil + if output.ExitCode == 1 { + return &Diff{Empty: false, Contents: string(output.Stdout)}, nil + } else if output.ExitCode != 0 { + return nil, errors.Errorf("git diff failed: %s", string(output.Stderr)) + } + return &Diff{Empty: true, Contents: string(output.Stdout)}, nil }