diff --git a/README.md b/README.md index 2ec83ec..f5bdb54 100644 --- a/README.md +++ b/README.md @@ -283,29 +283,35 @@ Neither operation proceeds while the review worktree is held by a live git-revie It resolves ambiguous saved sessions and is not an arbitrary filesystem path. If an interrupted original disappeared and no lineage identifies its replacement, use `git-review commits --replace`. -### Review working-tree files +### Review branch changes or annotate files ```sh git-review files +git-review files --all git-review files git-review files --untracked git-review files --untracked --json ``` -Without a path, git-review runs `tuicr tui --all-files` to review every tracked file. +Without a path or scope flag, git-review reviews the committed branch diff from the merge-base with upstream to HEAD. +This mode requires a configured upstream and excludes uncommitted changes. +With `--all`, it runs `tuicr tui --all-files` to review every tracked file. With a path, it runs `tuicr tui --file ` for that file or directory. With `--untracked` and no path, it runs `tuicr tui --file .` to include untracked files in the current directory. `--untracked` cannot be combined with an explicit path. +`--all` cannot be combined with `--untracked` or an explicit path. Use `--session` to select an exact saved session when several file reviews match the same repository. -File reviews run in the feature checkout, not the detached review worktree, and do not require a configured upstream. +File reviews run in the feature checkout, not the detached review worktree. +`--all`, explicit paths, and `--untracked` do not require a configured upstream. They hold the same repository-wide worktree lock as `commits` reviews, so `status --is-reviewing` reports true during either review. They do not create git-review attempts and do not affect `status` counts or `feedback` records. `approve` applies only to commit stacks, not file reviews. Successful `files` output uses `"command":"files"` to distinguish it from `"command":"commits"`. It contains the reviewed repository, requested path, `allFiles` and `untracked` flags, and the saved file review with comments and session notes. -Saved sessions are matched to tuicr's canonical file or directory scope, not just the Git repository root. +Branch diff sessions are matched to the captured commit range at the Git repository root. +Other saved sessions are matched to tuicr's canonical file or directory scope, not just the Git repository root. File comment paths are relative to the Git repository root, including reviews started from a subdirectory. Statuses mirror stack reviews without recovery decisions: diff --git a/internal/cli/files/files.go b/internal/cli/files/files.go index 4d8aad8..36a1165 100644 --- a/internal/cli/files/files.go +++ b/internal/cli/files/files.go @@ -17,6 +17,7 @@ type Command struct { Output io.Writer Terminal io.Writer Path string + All bool Untracked bool Session string } @@ -25,7 +26,13 @@ type Command struct { // with Install. func New(svc review.Service, in io.Reader, out, term io.Writer) *cobra.Command { c := &Command{Service: svc, Input: in, Output: out, Terminal: term} - cmd := &cobra.Command{Use: "files []", Short: "Review working-tree files with tuicr", Long: "Review working-tree files with tuicr. Without a path, review every tracked\nfile with tuicr tui --all-files. With a path, review that file or directory\nwith tuicr tui --file. With --untracked and no path, review untracked files\nin the current directory with tuicr tui --file . Address file feedback as new commits,\nlogically organized, not fixup commits.", Args: cobra.MaximumNArgs(1), RunE: c.RunE} + cmd := &cobra.Command{Use: "files []", Short: "Review branch changes or annotate files with tuicr", Long: `Without a path or scope flag, review the committed branch diff from the +merge-base with upstream to HEAD. This requires a configured Git upstream. +With --all, annotate every tracked file. With a path, annotate that file or +directory. With --untracked, annotate files in the current directory, +including untracked files. These explicit scopes do not require an upstream. +Address file feedback as new commits, logically organized, not fixup commits.`, Args: cobra.MaximumNArgs(1), RunE: c.RunE} + cmd.Flags().BoolVar(&c.All, "all", false, "Review every tracked file in the repository") cmd.Flags().BoolVar(&c.Untracked, "untracked", false, "Review untracked files in the current directory (defaults path to .)") cmd.Flags().StringVar(&c.Session, "session", "", "Select an exact saved session ID, slug, or indexed session path") return cmd @@ -38,7 +45,7 @@ func Install(parent *cobra.Command, svc review.Service, in io.Reader, out, term // RunE runs the files review workflow. func (c *Command) RunE(cmd *cobra.Command, args []string) error { - options := review.FilesOptions{Session: c.Session, Untracked: c.Untracked} + options := review.FilesOptions{Session: c.Session, All: c.All, Untracked: c.Untracked} if len(args) > 0 { options.Path = args[0] c.Path = args[0] diff --git a/internal/cli/files_functional_test.go b/internal/cli/files_functional_test.go index 9b4153b..988997b 100644 --- a/internal/cli/files_functional_test.go +++ b/internal/cli/files_functional_test.go @@ -24,7 +24,7 @@ func TestRealTuicrFileScopes(t *testing.T) { {name: "nested file after root review", args: []string{"src/nested.txt"}, previous: true}, {name: "nested directory", args: []string{"src"}, previous: true}, {name: "untracked from subdirectory", args: []string{"--untracked"}, cwd: "src", previous: true}, - {name: "tracked from subdirectory", cwd: "src", previous: true}, + {name: "tracked from subdirectory", args: []string{"--all"}, cwd: "src", previous: true}, {name: "symlink to nested file", args: []string{"linked.txt"}, previous: true}, } { t.Run(tc.name, func(t *testing.T) { @@ -46,6 +46,52 @@ func TestRealTuicrFileScopes(t *testing.T) { } } +func TestRealTuicrBranchFiles(t *testing.T) { + env := realTuicrEnvironment(t) + binary := buildGitReview(t) + for _, tc := range []struct { + name string + cwd string + merge bool + }{ + {name: "root"}, + {name: "subdirectory", cwd: "src"}, + {name: "merged upstream", merge: true}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := realFeatureRepo(t) + require.NoError(t, os.Mkdir(filepath.Join(dir, "src"), 0o700)) + // The upstream advances independently. Its new file must not be + // included in the branch diff, nor should an untracked file. + git(t, dir, "checkout", "main") + require.NoError(t, os.WriteFile(filepath.Join(dir, "upstream.txt"), []byte("Upstream only\n"), 0o600)) + git(t, dir, "add", "upstream.txt") + git(t, dir, "commit", "-m", "Advance upstream") + git(t, dir, "checkout", "feature") + if tc.merge { + git(t, dir, "merge", "--no-edit", "main") + } + require.NoError(t, os.WriteFile(filepath.Join(dir, "untracked.txt"), []byte("Not committed\n"), 0o600)) + reviewRealFile(t, binary, dir, env, "feature.txt", "Prior annotation", "--all") + result := reviewRealFile(t, binary, filepath.Join(dir, tc.cwd), env, "feature.txt", "Branch feedback") + require.Equal(t, dir, result.Repo) + require.False(t, result.AllFiles) + raw, err := os.ReadFile(result.Session) + require.NoError(t, err) + var saved struct { + Files map[string]any `json:"files"` + } + require.NoError(t, json.Unmarshal(raw, &saved)) + require.Contains(t, saved.Files, "feature.txt") + require.NotContains(t, saved.Files, "upstream.txt") + require.NotContains(t, saved.Files, "untracked.txt") + require.Len(t, result.Review.Comments, 1) + require.Equal(t, "Branch feedback", result.Review.Comments[0]["content"]) + require.Equal(t, "feature.txt", result.Review.Comments[0]["path"]) + }) + } +} + func reviewRealFile(t *testing.T, binary, dir string, env []string, file, feedback string, args ...string) review.FilesResult { t.Helper() command := append([]string{"files", "--json"}, args...) diff --git a/internal/cli/files_test.go b/internal/cli/files_test.go index f819dba..08ce1e4 100644 --- a/internal/cli/files_test.go +++ b/internal/cli/files_test.go @@ -27,7 +27,8 @@ func TestFilesCommandArgMapping(t *testing.T) { args []string wantArgs string }{ - {"all files", []string{"files", "--json"}, "tui\n--no-update-check\n--stdout\n--all-files"}, + {"branch diff", []string{"files", "--json"}, "tui\n--no-update-check\n--stdout\n--revisions\n" + git(t, dir, "merge-base", "@{upstream}", "HEAD") + ".." + git(t, dir, "rev-parse", "HEAD")}, + {"explicit all files", []string{"files", "--all", "--json"}, "tui\n--no-update-check\n--stdout\n--all-files"}, {"single file", []string{"files", "file.txt", "--json"}, "tui\n--no-update-check\n--stdout\n--file\nfile.txt"}, {"untracked defaults to dot", []string{"files", "--untracked", "--json"}, "tui\n--no-update-check\n--stdout\n--file\n."}, } { @@ -48,6 +49,35 @@ func TestFilesCommandArgMapping(t *testing.T) { } } +func TestFilesUpstreamAndEmptyRange(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + log := filepath.Join(bin, "launched") + t.Setenv("TEST_LAUNCH_LOG", log) + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nprintf launched > \"$TEST_LAUNCH_LOG\"\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + git(t, dir, "branch", "--unset-upstream") + _, _, err := execute(t, dir, "files", "--json") + require.ErrorContains(t, err, "configured Git upstream") + _, err = os.Stat(log) + require.ErrorIs(t, err, os.ErrNotExist) + _, _, err = execute(t, dir, "files", "--all", "--json") + require.NoError(t, err) + require.FileExists(t, log) + require.NoError(t, os.Remove(log)) + git(t, dir, "branch", "--set-upstream-to=main") + git(t, dir, "reset", "--hard", "main") + out, _, err := execute(t, dir, "files", "--json") + require.NoError(t, err) + var result review.FilesResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "cancelled", result.Status) + _, err = os.Stat(log) + require.ErrorIs(t, err, os.ErrNotExist) +} + func TestFilesCommandDistinguishesCommits(t *testing.T) { dir := featureRepo(t) t.Setenv("XDG_CACHE_HOME", t.TempDir()) @@ -85,6 +115,8 @@ func TestFilesCommandTerminalAndErrors(t *testing.T) { {"files", "a", "b", "--json"}, {"files", "--untracked", "file.txt", "--json"}, {"files", "--unknown", "--json"}, + {"files", "--all", "--untracked", "--json"}, + {"files", "--all", "file.txt", "--json"}, } { out, terminal, err := execute(t, dir, args...) require.Error(t, err) diff --git a/internal/review/files.go b/internal/review/files.go index 7869bd9..5227a50 100644 --- a/internal/review/files.go +++ b/internal/review/files.go @@ -7,11 +7,13 @@ import ( "os" "os/exec" "path/filepath" + "strings" ) // FilesOptions selects a working-tree file review. type FilesOptions struct { Path string + All bool Untracked bool Session string } @@ -73,6 +75,9 @@ type FileReviewer struct { // Review launches tuicr for working-tree files and reads its saved session. func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesResult, error) { var result FilesResult + if options.All && (options.Untracked || options.Path != "") { + return result, fmt.Errorf("--all cannot be combined with --untracked or an explicit path") + } if conflictingFileScope(options) { return result, fmt.Errorf("--untracked cannot be combined with an explicit path") } @@ -90,7 +95,10 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe _ = lock.Close() }() - allFiles := reviewAllFiles(options) + if !options.All && !options.Untracked && options.Path == "" { + return r.reviewBranchDiff(ctx, repo, options.Session, lock) + } + allFiles := options.All filePath := options.Path if reviewUntrackedDirectory(options) { filePath = "." @@ -125,6 +133,50 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe return fileReviewResult(result, saved) } +// reviewBranchDiff uses Git's upstream merge-base without imposing the linear +// history and stack-placement rules required by commit reviews. +func (r FileReviewer) reviewBranchDiff(ctx context.Context, repo, selector string, lock *os.File) (FilesResult, error) { + result := FilesResult{Version: 1, Command: "files", Repo: repo} + repository := r.Service.Repository + view, _, err := repository.branchView(ctx) + if err != nil { + return result, err + } + head, err := repository.Resolve(ctx, "HEAD") + if err != nil { + return result, err + } + base, err := repository.git(ctx, "merge-base", "--all", view.Target, head) + if err != nil { + return result, err + } + if strings.Contains(base, "\n") { + return result, fmt.Errorf("review requires one merge-base with upstream") + } + history, err := repository.git(ctx, "rev-list", "--reverse", base+".."+head) + if err != nil { + return result, err + } + if history == "" { + return fileReviewResult(result, nil) + } + // Run at the root so invocation from a subdirectory keeps the same scope + // and saved-session identity. + r.Service.Repository.Dir = repo + if err := r.launch(ctx, []string{"tui", "--no-update-check", "--stdout", "--revisions", base + ".." + head}, lock); err != nil { + return result, err + } + scope := Stack{Repo: repo, Base: base, Tip: head, Commits: strings.Split(history, "\n")} + saved, err := r.Reader.Read(scope, selector) + if err != nil { + return result, err + } + if saved != nil { + result.Session = saved.Path + } + return fileReviewResult(result, saved) +} + func (r FileReviewer) launch(ctx context.Context, args []string, lock *os.File) error { cmd := exec.CommandContext(ctx, "tuicr", args...) cmd.Dir = r.Service.Repository.Dir @@ -244,13 +296,6 @@ func conflictingFileScope(options FilesOptions) bool { return options.Path != "" } -func reviewAllFiles(options FilesOptions) bool { - if options.Untracked { - return false - } - return options.Path == "" -} - func reviewUntrackedDirectory(options FilesOptions) bool { if !options.Untracked { return false diff --git a/internal/review/files_test.go b/internal/review/files_test.go index 6897c21..9a8f813 100644 --- a/internal/review/files_test.go +++ b/internal/review/files_test.go @@ -133,7 +133,7 @@ func TestFilesReviewArgMappingAndLock(t *testing.T) { diffSource string base string }{ - {"all files", FilesOptions{}, "--all-files", "pristine", "pristine:abc:def"}, + {"all files", FilesOptions{All: true}, "--all-files", "pristine", "pristine:abc:def"}, {"single file", FilesOptions{Path: "file.txt"}, "--file\nfile.txt", "working_tree", "file"}, {"untracked defaults to dot", FilesOptions{Untracked: true}, "--file\n.", "working_tree", "file"}, } { diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index aeb3e59..cdc9ede 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -49,8 +49,14 @@ Counts do not identify feedback targets or authorize approval. `git-review commits --json` reviews commit stacks and returns `"command":"commits"`. Address its feedback with fixup commits inserted immediately after their target stacks. -`git-review files [] --json` reviews working-tree files and returns `"command":"files"`. -It runs `tuicr tui --all-files` without a path, `tuicr tui --file ` with a path, and `tuicr tui --file .` with `--untracked` and no path. +`git-review files [] --json` returns `"command":"files"`. +Without a path or scope flag, it reviews the committed branch diff from the merge-base with upstream to HEAD. +The default requires a configured upstream and excludes uncommitted changes. +Use `--all` to annotate every tracked file with `tuicr tui --all-files`. +An explicit path uses `tuicr tui --file `. +`--untracked` uses `tuicr tui --file .` in the current directory. +These explicit scopes do not require an upstream. +Do not combine `--all` with a path or `--untracked`. Address its feedback as one or more new commits, logically organized and sized. Do not use `fixup!`, `amend!`, or `reword!` commits for file feedback.