fix/prdescribe-patch #8

Merged

gmgauthier merged 2 commits from fix/prdescribe-patch into master 2026-03-31 10:02:35 +00:00

Conversation 0 Commits 2 Files Changed 3 +47 -11

gmgauthier commented 2026-03-31 09:59:56 +00:00

Owner

Copy Link

Fix git diff handling in pr-describe command and add mockable entry point for tests

This PR addresses an issue in the pr-describe command where git diff calls were not properly handling non-zero exit codes (specifically exit code 1, which is normal when differences exist). It introduces a new git.Diff function to correctly capture diff output, makes it mockable for unit tests, and includes minor improvements for usability and robustness.

Changes

• internal/git/git.go:

  • Added a new Diff function that runs git diff using exec.Command.CombinedOutput() and explicitly tolerates exit code 1 (indicating changes were found). This prevents treating legitimate diffs as errors.
  • Logs the command and errors using the existing logger for better debugging.
  • Returns the diff output as a string or an error if the command fails for other reasons.

• cmd/prdescribe.go:

  • Introduced a mockable gitDiff variable assigned to git.Diff, similar to the existing gitRun pattern, to facilitate unit testing.
  • Updated the --base flag description to clarify the default value ("master").
  • Replaced direct gitRun calls with gitDiff for fetching diffs, preferring the local base branch first and falling back to origin/<base>.
  • Used strings.TrimSpace to more accurately check for empty diffs (ignoring whitespace).
  • Enhanced the status message to include the base branch (e.g., "Writing PR description (base=master)...").
  • Added a missing newline for better code readability.

• cmd/run_test.go:

  • Added a new withMockGitDiff helper function to inject mocks for gitDiff, mirroring the existing withMockGit pattern.
  • Updated all relevant unit tests for TestRunPRDescribe to use withMockGitDiff instead of withMockGit, ensuring they test the new diff logic.
  • No changes to test logic or assertions—only adaptations for the new mockable entry point.

Motivation

The original implementation used gitRun for git diff, which relies on cmd.Output() and treats any non-zero exit code as a failure. However, git diff intentionally exits with code 1 when changes are present, leading to incorrect error handling and potential failures in generating PR descriptions. This PR introduces a dedicated Diff function to handle this Git-specific behavior correctly, improving reliability.

Additionally, making gitDiff mockable aligns with the project's testing patterns (e.g., gitRun), allowing for cleaner and more isolated unit tests without affecting other Git operations. Minor tweaks like trimming whitespace and clarifying output messages enhance user experience and prevent edge cases (e.g., diffs with only whitespace being treated as non-empty).

No breaking changes are introduced—this is fully backward-compatible.

Testing Notes

• Unit Tests: All changes are covered by updates to TestRunPRDescribe in cmd/run_test.go. The tests now mock gitDiff to simulate various scenarios:
  • Empty diffs (local and remote fallbacks).
  • Successful diffs with changes.
  • Fallback from local to remote base branches.
  • Custom base branches via flags.
  • These tests ensure the command skips AI calls for no-op branches, handles errors gracefully, and captures the correct diff args.
• Manual Testing:
  • Tested locally on a sample repository by running grokkit pr-describe on branches with/without changes against "master" and custom bases.
  • Verified fallback to origin/<base> works when the local base is absent.
  • Confirmed empty diffs (including those with only whitespace) trigger the "No changes" message.
  • Ensured the AI streaming (via client.Stream) is only invoked when a valid diff is present.
• Coverage: Existing test coverage remains high; no new untested paths were introduced. Ran go test ./... with no failures.
• Edge Cases: Handled cases like Git not being installed (error logging) and non-1 exit codes (proper error propagation).

If approved, this should be merged to the main branch. Let me know if additional tests or adjustments are needed!

### Fix `git diff` handling in `pr-describe` command and add mockable entry point for tests This PR addresses an issue in the `pr-describe` command where `git diff` calls were not properly handling non-zero exit codes (specifically exit code 1, which is normal when differences exist). It introduces a new `git.Diff` function to correctly capture diff output, makes it mockable for unit tests, and includes minor improvements for usability and robustness. #### Changes - **internal/git/git.go**: - Added a new `Diff` function that runs `git diff` using `exec.Command.CombinedOutput()` and explicitly tolerates exit code 1 (indicating changes were found). This prevents treating legitimate diffs as errors. - Logs the command and errors using the existing logger for better debugging. - Returns the diff output as a string or an error if the command fails for other reasons. - **cmd/prdescribe.go**: - Introduced a mockable `gitDiff` variable assigned to `git.Diff`, similar to the existing `gitRun` pattern, to facilitate unit testing. - Updated the `--base` flag description to clarify the default value ("master"). - Replaced direct `gitRun` calls with `gitDiff` for fetching diffs, preferring the local base branch first and falling back to `origin/<base>`. - Used `strings.TrimSpace` to more accurately check for empty diffs (ignoring whitespace). - Enhanced the status message to include the base branch (e.g., "Writing PR description (base=master)..."). - Added a missing newline for better code readability. - **cmd/run_test.go**: - Added a new `withMockGitDiff` helper function to inject mocks for `gitDiff`, mirroring the existing `withMockGit` pattern. - Updated all relevant unit tests for `TestRunPRDescribe` to use `withMockGitDiff` instead of `withMockGit`, ensuring they test the new diff logic. - No changes to test logic or assertions—only adaptations for the new mockable entry point. #### Motivation The original implementation used `gitRun` for `git diff`, which relies on `cmd.Output()` and treats any non-zero exit code as a failure. However, `git diff` intentionally exits with code 1 when changes are present, leading to incorrect error handling and potential failures in generating PR descriptions. This PR introduces a dedicated `Diff` function to handle this Git-specific behavior correctly, improving reliability. Additionally, making `gitDiff` mockable aligns with the project's testing patterns (e.g., `gitRun`), allowing for cleaner and more isolated unit tests without affecting other Git operations. Minor tweaks like trimming whitespace and clarifying output messages enhance user experience and prevent edge cases (e.g., diffs with only whitespace being treated as non-empty). No breaking changes are introduced—this is fully backward-compatible. #### Testing Notes - **Unit Tests**: All changes are covered by updates to `TestRunPRDescribe` in `cmd/run_test.go`. The tests now mock `gitDiff` to simulate various scenarios: - Empty diffs (local and remote fallbacks). - Successful diffs with changes. - Fallback from local to remote base branches. - Custom base branches via flags. - These tests ensure the command skips AI calls for no-op branches, handles errors gracefully, and captures the correct diff args. - **Manual Testing**: - Tested locally on a sample repository by running `grokkit pr-describe` on branches with/without changes against "master" and custom bases. - Verified fallback to `origin/<base>` works when the local base is absent. - Confirmed empty diffs (including those with only whitespace) trigger the "No changes" message. - Ensured the AI streaming (via `client.Stream`) is only invoked when a valid diff is present. - **Coverage**: Existing test coverage remains high; no new untested paths were introduced. Ran `go test ./...` with no failures. - **Edge Cases**: Handled cases like Git not being installed (error logging) and non-1 exit codes (proper error propagation). If approved, this should be merged to the main branch. Let me know if additional tests or adjustments are needed!

gmgauthier added 2 commits 2026-03-31 09:59:56 +00:00

refactor(git): improve diff handling to tolerate exit code 1 ... 452051f0c1

- Introduce git.Diff function that uses CombinedOutput and treats exit code 1 as success (indicating changes exist).
- Update prdescribe command to use git.Diff and trim whitespace when checking for empty diffs.
- Enhance prdescribe output to include base branch in the status message.

refactor(cmd): make git diff mockable in pr-describe command and tests ... 403a408f8d

- Introduce `gitDiff` variable as a mockable wrapper for `git.Diff`.
- Update tests to use `withMockGitDiff` for injecting diff mocks.
- Minor flag description tweak for clarity.

gmgauthier merged commit debcf94f2e into master 2026-03-31 10:02:35 +00:00

gmgauthier deleted branch fix/prdescribe-patch 2026-03-31 10:02:35 +00:00

gmgauthier referenced this issue from a commit 2026-03-31 10:02:36 +00:00

Merge pull request 'fix/prdescribe-patch' (#8) from fix/prdescribe-patch into master

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No Label

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

1 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gmgauthier/grokkit#8

No description provided.