Skip to content

fix(background-file) Manage path at git repository root directory Ref…#327

Open
Victor-D wants to merge 1 commit into
alibaba:mainfrom
Victor-D:fix/read_file_from_git_repo_root_dir
Open

fix(background-file) Manage path at git repository root directory Ref…#327
Victor-D wants to merge 1 commit into
alibaba:mainfrom
Victor-D:fix/read_file_from_git_repo_root_dir

Conversation

@Victor-D

@Victor-D Victor-D commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

fix(background-file) Manage path at git repository root directory

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Quick tests such as ./dist/opencodereview review --repo $PWD -B ./../../../../misc/ocr/mycontext.md were done

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

closes #324
#324

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: No comments generated. Looks good to me.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice fix! The implementation is clean and minimal — resolveBackgroundFilePath handles the three cases (empty, absolute, relative) correctly, and the test coverage is solid with both unit tests and an end-to-end scenario.

One suggestion: could you add a test case for when repoDir is an empty string? This would happen if --repo is not specified and repo detection fails. In that case filepath.Join("", relativePath) returns the relative path as-is, which is a reasonable fallback to the old behavior — but it would be good to have an explicit test documenting that expectation.

Other than that, LGTM. Thanks for the contribution!

@Victor-D Victor-D force-pushed the fix/read_file_from_git_repo_root_dir branch from 3622f36 to 37e1ecb Compare July 11, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent path resolution: --background-file resolves against CWD, not the repo root

2 participants