feat: add merge_base to check the branch is rebased
#192
Conversation
WalkthroughThe changes introduced in this pull request enhance the commit-check process by adding a new validation check for verifying if the current branch is rebased on the latest version of the target branch. This is implemented through a regex pattern and integrated into the existing commit-check framework. Additionally, updates to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
- Coverage 99.20% 97.26% -1.94%
==========================================
Files 7 7
Lines 252 293 +41
==========================================
+ Hits 250 285 +35
- Misses 2 8 +6 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
.gitignore (1)
7-7: LGTM! Good practice to include both virtual environment naming conventions.Having both
venvand.venventries is beneficial as it covers the two most common virtual environment directory naming conventions:
venv: Default name when usingpython -m venv venv.venv: Used by tools like Poetry and follows the hidden directory conventioncommit_check/main.py (1)
79-86: Enhance the help message for better clarity.The argument implementation looks good and follows the established pattern. However, consider making the help message more descriptive to better explain what "common ancestors" means in this context.
- help='check common ancestors', + help='check if the current branch is rebased on the target branch by verifying common ancestors',commit_check/util.py (2)
54-60: Improve function documentation.The docstring could be more descriptive and include:
- Return value explanation (0 means ancestor exists, 1 means no ancestor)
- Example usage
- Preconditions (e.g., git repository must exist)
Consider updating the docstring:
def git_merge_base(target_branch: str, sha: str) -> int: """Check ancestors for a given commit. :param target_branch: target branch :param sha: commit hash. default is HEAD - :returns: Get 0 if there is ancestor else 1. + + This function checks if the specified commit is an ancestor of the target branch. + It helps verify if a branch is up-to-date with its target branch. + + Example: + >>> git_merge_base("main", "HEAD") + 0 # Current HEAD is up-to-date with main + + Preconditions: + - Must be run within a git repository + - Target branch must exist in the remote """
61-66: Consider usingcmd_outputfor consistency.The module already has a
cmd_outputhelper function for running Git commands, butgit_merge_baseusessubprocess.rundirectly.Consider refactoring to use the existing helper:
- result = subprocess.run( - commands, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding='utf-8' - ) - return result.returncode + output = cmd_output(commands) + return 0 if not output else 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.commit-check.yml(1 hunks).gitignore(1 hunks)commit_check/__init__.py(1 hunks)commit_check/branch.py(2 hunks)commit_check/main.py(2 hunks)commit_check/util.py(1 hunks)
🔇 Additional comments (4)
commit_check/branch.py (1)
4-4: LGTM: Import statement is correctly organized.
The addition of git_merge_base to the existing utility imports is appropriate and follows the module's import organization pattern.
.commit-check.yml (1)
31-34: Verify the regex pattern implementation.
The regex pattern simply matching "main" seems unusual for a merge-base check. This might not correctly validate the actual merge-base relationship between branches.
Let's verify how this regex is used in the implementation:
✅ Verification successful
Let me gather more information about the git_merge_base function implementation:
The regex pattern "main" is correctly implemented for the merge-base check
The implementation in commit_check/util.py shows that the regex pattern is simply used as the target branch name in the git command git merge-base --is-ancestor origin/{target_branch} {sha}. This is the correct usage as:
- The regex pattern "main" is used as a literal branch name
- The command checks if the target branch (origin/main) is an ancestor of the current HEAD
- The implementation properly prefixes the branch with "origin/"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the merge_base regex is used in the implementation
# Test 1: Look for the merge_base check implementation
echo "Looking for merge_base check implementation..."
rg -A 10 "def check_merge_base"
# Test 2: Look for git merge-base command usage
echo "Looking for git merge-base command usage..."
rg -A 5 "git merge-base"
Length of output: 1192
Script:
#!/bin/bash
# Look for git_merge_base function implementation
echo "Looking for git_merge_base function implementation..."
rg -A 10 "def git_merge_base"
# Look for any git command execution related to merge-base
echo "Looking for git command execution..."
rg "subprocess.*git.*merge-base"
Length of output: 964
commit_check/__init__.py (1)
51-56: Verify the integration with command-line interface.
The new check needs to be properly integrated with the CLI to support the toggleable feature mentioned in the requirements.
Let's verify the CLI integration:
✅ Verification successful
Let me check the CLI implementation in the files found to verify the integration.
The merge-base check is properly integrated with CLI
The verification shows that the merge-base check is correctly integrated with the CLI:
- It has a dedicated CLI flag
-mb/--merge-basewith proper help text - The argument is properly handled in the main flow with
if args.merge_base: - It follows the same pattern as other toggleable checks in the codebase
- The implementation is consistent with the requirements for a toggleable feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the merge-base check can be toggled via CLI
# Expected: Find a CLI option to enable/disable merge-base check
# Look for merge-base related CLI options
rg -l "merge.?base" --type py
# Look for argument parser configuration
ast-grep --pattern 'parser.add_argument("--merge-base"'
Length of output: 179
Script:
#!/bin/bash
# Check CLI implementation in main.py for merge-base related arguments
rg "merge.?base" commit_check/main.py -B 2 -A 2
# Check if there's any argument parser setup
ast-grep --pattern 'parser = $_($$$)
$$$
parser.add_argument($$$)'
# Look for how checks are configured/toggled
rg "add_argument|ArgumentParser" commit_check/main.py -B 2 -A 4
Length of output: 2139
commit_check/util.py (1)
54-69: Verify integration with branch checking workflow.
The function is part of a larger feature to ensure branches are rebased. Let's verify its integration points.
✅ Verification successful
Let me gather more information about the integration by checking the actual implementation in branch.py and the configuration details.
Integration with branch checking workflow is properly implemented
The git_merge_base function is correctly integrated into the branch checking workflow:
- Used in
commit_check/branch.pyfor merge base verification against target branch - Configured in
.commit-check.ymlunder themerge_basecheck with appropriate error messages and suggestions - Implementation follows a consistent pattern with other checks in the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the git_merge_base function is used in the codebase
echo "Checking for imports of git_merge_base..."
rg "from commit_check.util import git_merge_base" -l
echo "\nChecking for direct usage of git_merge_base..."
rg "git_merge_base\(" -l
echo "\nChecking for related configuration in yaml files..."
rg "merge.?base" --type yaml
Length of output: 663
Script:
#!/bin/bash
# Check the implementation in branch.py and configuration details
echo "Checking branch.py implementation..."
rg -A 10 "git_merge_base" commit_check/branch.py
echo "\nChecking complete configuration context..."
cat .commit-check.yml
Length of output: 2242
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.commit-check.yml (1)
34-39: Consider enhancing the suggestion message with additional safety checks.The current suggestion provides good basic guidance but could be more robust with additional safety measures.
Consider updating the suggestion to include:
suggest: | Please ensure your branch is up to date with the target branch by running: + # First, save your current work if needed + git stash git fetch origin ${TARGET_BRANCH:-main} + # Verify the target branch exists + git show-ref --verify --quiet refs/remotes/origin/${TARGET_BRANCH:-main} || echo "Error: Target branch not found" git rebase origin/${TARGET_BRANCH:-main} # If you encounter conflicts, resolve them and continue with: git rebase --continue + # Or if you need to abort: + git rebase --abort + # Restore your work if you stashed it + git stash popcommit_check/branch.py (1)
28-55: Implementation looks good with minor suggestions for improvement.The function is well-structured and consistent with the codebase. Consider these improvements:
- Use
dict.get()consistently for optional keys:- if check['suggest']: + if check.get('suggest'):
- Add type hints in the docstring:
"""Check if the current branch is based on the latest target branch. Args: checks: List of check configurations containing merge_base rules Returns: - PASS if merge base check succeeds, FAIL otherwise + int: PASS (0) if merge base check succeeds, FAIL (1) otherwise """
- Consider making the error message more descriptive by including the current commit hash:
- f"Branch is not up to date with {check['regex']}", + f"Branch (HEAD) is not up to date with target branch {check['regex']}. Local branch needs to be rebased.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.commit-check.yml(1 hunks)commit_check/branch.py(2 hunks)
🔇 Additional comments (3)
.commit-check.yml (1)
31-39: LGTM! The implementation addresses previous feedback.
The new merge_base check successfully implements the requested feature with good configurability via ${TARGET_BRANCH:-main}. Nice work on making the target branch configurable as suggested in the previous review.
commit_check/branch.py (2)
4-4: LGTM: Import statement is correctly updated.
The import statement is properly organized and includes the necessary git_merge_base function for the new merge base check.
28-55: Add unit tests to improve coverage.
The function needs comprehensive test coverage to ensure reliability.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
noxfile.py (4)
4-5: Consider adding "commit-check" to default sessions.Since branch rebasing verification is a key feature being added, consider including "commit-check" in the default sessions to ensure it runs by default.
-nox.options.sessions = ["lint"] +nox.options.sessions = ["lint", "commit-check"]
26-30: Add error handling for installation failures.The session should handle potential failures during package installation or pre-commit execution.
@nox.session(name="test-hook") def test_hook(session): - session.install("-e", ".") - session.install("pre-commit") - session.run("pre-commit", "try-repo", ".") + try: + session.install("-e", ".") + session.install("pre-commit") + session.run("pre-commit", "try-repo", ".") + except Exception as e: + session.error(f"Failed to run test-hook: {e}")
40-41: Remove commented-out decorator.The duplicate decorator line should be removed to maintain clean code.
-# @nox.session(name="commit-check", requires=["install-wheel"]) @nox.session(name="commit-check", requires=["install-wheel"])
52-56: Consider making docs output path configurable.The output path for documentation could be made configurable for better flexibility.
+DOCS_OUTPUT = Path("_build/html") + @nox.session() def docs(session): session.install("-e", ".") session.install("-r", REQUIREMENTS["docs"]) - session.run("sphinx-build", "-E", "-W", "-b", "html", "docs", "_build/html") + DOCS_OUTPUT.parent.mkdir(parents=True, exist_ok=True) + session.run("sphinx-build", "-E", "-W", "-b", "html", "docs", str(DOCS_OUTPUT)).github/workflows/main.yml (3)
29-30: Consider adding error handling between nox sessions.While splitting into separate sessions is good practice, a failure in the lint session would still proceed to the test-hook session.
Consider using shell conditional execution:
- nox -s lint - nox -s test-hook + nox -s lint && nox -s test-hook
75-75: Consider Python version compatibility.Including Python 3.13 in the test matrix might be premature as it's still in development. Consider waiting until it reaches beta or stable status.
- py: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13'] + py: ['3.8', '3.9', '3.10', '3.11', '3.12']
86-86: Consider using latest stable Python for docs build.Instead of pinning to Python 3.10, consider using the latest stable version for documentation building.
- python-version: "3.10" + python-version: "3.12" # Latest stable version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/main.yml(3 hunks).gitignore(1 hunks)noxfile.py(1 hunks)requirements-dev.txt(1 hunks)tests/util_test.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements-dev.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Ruff
tests/util_test.py
217-217: Local variable m_cmd_output is assigned to but never used
Remove assignment to unused variable m_cmd_output
(F841)
🔇 Additional comments (5)
noxfile.py (1)
16-24: LGTM! Well-structured lint session.
The implementation provides good flexibility with argument handling and useful defaults.
.github/workflows/main.yml (3)
33-40: LGTM! Clean implementation of build and artifact upload.
The build step is properly integrated with nox and correctly uploads the wheel as an artifact.
45-46: LGTM! Well-integrated coverage collection.
The coverage step is properly integrated with nox and correctly configured for codecov upload.
42-42: Verify commit-check nox session configuration for rebase checking.
Given that this PR aims to implement branch rebase checking, ensure that the nox session is properly configured to include this new functionality.
✅ Verification successful
The nox session is properly configured for rebase checking functionality
The commit-check implementation already includes rebase verification through the check_merge_base function in branch.py, which is accessible via the --merge-base flag. The nox session in noxfile.py runs commit-check with the necessary flags including --branch, which enables branch-related checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the nox session includes rebase verification configuration
rg -A 5 "commit-check" noxfile.py
Length of output: 489
Script:
#!/bin/bash
# Let's check the commit-check command implementation to verify rebase checking functionality
ast-grep --pattern 'def main($$$)' commit_check/cli.py
Length of output: 123
Script:
#!/bin/bash
# Let's first find the correct location of the commit-check implementation
fd "commit.?check" --type f
Length of output: 29
Script:
#!/bin/bash
# Let's check the repository structure to understand where the code might be located
ls -R
Length of output: 419
Script:
#!/bin/bash
# Let's check the main implementation and branch checking functionality
cat commit_check/main.py commit_check/branch.py
Length of output: 5223
tests/util_test.py (1)
4-4: LGTM!
The import statement is correctly placed with other utility imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/main.yml (1)
29-30: Add fail-fast behavior to pre-commit steps.Consider adding
set -eto ensure the workflow fails immediately if the lint step fails:- name: Run pre-commit run: | + set -e nox -s lint nox -s test-hook
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/main.yml(3 hunks)noxfile.py(1 hunks)tests/util_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- noxfile.py
🔇 Additional comments (4)
.github/workflows/main.yml (2)
75-75: LGTM!
The change to use nox for installation testing is consistent with the overall migration strategy and maintains proper job dependencies.
86-86: LGTM!
The documentation build step has been successfully migrated to nox while maintaining all necessary deployment functionality.
tests/util_test.py (2)
4-4: LGTM!
The import statement follows the existing style and is correctly placed with other imports from the same module.
215-222:
Critical issues in test implementation
The test implementation has several critical issues:
-
Circular Reference: The test mocks
git_merge_basebut then calls the same function, creating a circular reference. Instead, you should mockcmd_outputas it's the underlying function. -
Incorrect Assertion: The test tries to access
returncodeon the mock object, which won't work as expected. -
Insufficient Coverage: The test only covers one basic case, missing important scenarios like:
- Error cases (CalledProcessError)
- Different branch names
- Different return values
Please refer to the previous review comment that provided a comprehensive solution addressing these issues. The suggested implementation there aligns with the existing test patterns in this file and provides better coverage.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
.commit-check.yml (1)
31-39: Make the target branch configurable via environment variables.The merge base check implementation looks good overall, but consider making it more flexible by supporting different target branches.
Apply this diff to make the target branch configurable:
- check: merge_base - regex: main # configurable target branch, defaults to main + regex: ${TARGET_BRANCH:-main} # configurable target branch, defaults to main error: Current branch is not up to date with main suggest: | Please ensure your branch is up to date with the target branch by running: - git fetch origin main - git rebase origin/main + git fetch origin ${TARGET_BRANCH:-main} + git rebase origin/${TARGET_BRANCH:-main} # If you encounter conflicts, resolve them and continue with: git rebase --continueThis change allows users to:
- Use a different target branch by setting the
TARGET_BRANCHenvironment variable- Fall back to "main" if no target branch is specified
- Support repositories with different default branch names (e.g., master, develop)
commit_check/branch.py (2)
31-35: Fix docstring format.The docstring should follow Google style format:
- params checks: List of check configurations containing merge_base rules + Args: + checks: List of check configurations containing merge_base rules - :returns PASS(0) if merge base check succeeds, FAIL(1) otherwise + Returns: + int: PASS(0) if merge base check succeeds, FAIL(1) otherwise
47-50: Improve error message clarity.The current error message uses the branch name but doesn't clearly indicate the merge base issue. Consider making it more specific about the rebase requirement.
print_error_message( check['check'], check['regex'], - check['error'], branch_name, + f"Branch '{branch_name}' is not up to date with target branch '{check['regex']}'", branch_name, )commit_check/commit.py (1)
40-41: Consider a more robust error handling pattern.While the current implementation effectively prevents duplicate error headers, using function attributes for state management could lead to issues in concurrent scenarios or between validation runs.
Consider these alternative approaches:
- Pass error state as a parameter
- Use a context manager for error handling
- Implement an error collector pattern
Example of an error collector pattern:
class ErrorCollector: def __init__(self): self._header_printed = False self.errors = [] def add_error(self, error): if not self._header_printed: print_error_head() self._header_printed = True self.errors.append(error)commit_check/util.py (2)
103-108: Add typing hints and documentation to the decorator.The decorator implementation is correct but could benefit from better documentation and type hints.
Apply this diff to improve the implementation:
+from typing import Callable, TypeVar, Any + +T = TypeVar('T', bound=Callable[..., Any]) + -def track_print_call(func): +def track_print_call(func: T) -> T: + """Decorator that tracks whether a function has been called. + + Args: + func: The function to be wrapped + + Returns: + A wrapped function that tracks its calls + """ def wrapper(*args, **kwargs): wrapper.has_been_called = True return func(*args, **kwargs) wrapper.has_been_called = False # Initialize as False return wrapper
Line range hint
111-129: Optimize print statements and improve documentation.The function could benefit from better performance and documentation.
Apply this diff to improve the implementation:
@track_print_call def print_error_head(): - """Print error message. - :returns: Print error head to user + """Print a formatted error header with ASCII art. + + This function prints a visually appealing error message using ASCII art + that spells out "CHECK ERROR". It's used to make error messages more + noticeable to users. """ - print("Commit rejected by Commit-Check. ") - print(" ") - print(r" (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) ") - print(r" / ._. \ / ._. \ / ._. \ / ._. \ / ._. \ ") - print(r" __\( C )/__ __\( H )/__ __\( E )/__ __\( C )/__ __\( K )/__ ") - print(r"(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)") - print(r" || E || || R || || R || || O || || R || ") - print(r" _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ ") - print(r"(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)") - print(r" `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ ") - print(" ") - print("Commit rejected. ") - print(" ") + error_message = """ +Commit rejected by Commit-Check. + + (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) + / ._. \ / ._. \ / ._. \ / ._. \ / ._. \ + __\( C )/__ __\( H )/__ __\( E )/__ __\( C )/__ __\( K )/__ +(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._) + || E || || R || || R || || O || || R || + _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ +(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.) + `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ + +Commit rejected. +""" + print(error_message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.commit-check.yml(1 hunks)commit_check/__init__.py(1 hunks)commit_check/author.py(2 hunks)commit_check/branch.py(3 hunks)commit_check/commit.py(3 hunks)commit_check/util.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- commit_check/init.py
🔇 Additional comments (7)
commit_check/author.py (2)
4-4: LGTM!
The import statement is properly organized and maintains consistency with the existing imports.
22-23: 🛠️ Refactor suggestion
Consider a more robust error header management approach.
The current implementation uses function attributes for state management (print_error_head.has_been_called), which could lead to issues:
- Potential race conditions in concurrent scenarios
- Risk of AttributeError if the attribute is not properly initialized
- Unclear state management (implicit rather than explicit)
Consider these alternatives:
- Pass the state explicitly through function parameters
- Use a dedicated error formatter class
- Use a context manager for error handling
Example using a context manager:
class ErrorContext:
def __init__(self):
self.header_printed = False
def print_error(self, message):
if not self.header_printed:
self._print_header()
self.header_printed = True
print(message)
def _print_header(self):
# Header printing logic here
pass
# Usage
error_ctx = ErrorContext()
error_ctx.print_error("Your error message")Let's check if this state management pattern is used elsewhere:
.commit-check.yml (1)
31-39:
Address the low test coverage for the merge base check.
The implementation of this feature has very low test coverage according to the Codecov report. Given that this is a critical feature for validating branch relationships, comprehensive testing is essential.
Consider adding tests for:
- Different branch scenarios (ahead, behind, diverged)
- Edge cases (no common ancestor, detached HEAD)
- Configuration variations (different target branches)
Would you like me to help generate comprehensive test cases for these scenarios?
commit_check/branch.py (4)
4-4: LGTM: Import statement is properly organized.
The new imports are correctly placed and support the added functionality.
18-19: LGTM: Error reporting enhancement.
Good addition of error header check to maintain consistent error message formatting.
30-54: Add unit tests for the new functionality.
The previous review comment about low test coverage (18.18%) is still valid. Please add comprehensive tests covering:
- Empty regex case
- Successful merge base check
- Failed merge base check
- Invalid configuration scenarios
Would you like me to help generate the test cases for this function?
42-43: Verify git command integration.
Let's verify the git command integration and error handling:
✅ Verification successful
Git command integration is properly implemented and verified
The implementation of git_merge_base is well-structured with:
- Proper error handling using try-catch blocks
- Returns appropriate status codes (0 for success, non-zero for failure)
- Correctly uses subprocess.run with stdout/stderr pipes
- Has unit tests covering the functionality
- The function is used correctly in branch.py with proper result checking
The integration follows Git's standard behavior where:
- Return code 0 indicates the target branch is an ancestor
- Return code non-zero indicates it's not an ancestor or there was an error
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify git_merge_base implementation and usage
# Test 1: Find git_merge_base implementation
echo "=== Checking git_merge_base implementation ==="
rg -A 10 "def git_merge_base"
# Test 2: Check error handling patterns
echo -e "\n=== Checking error handling patterns ==="
rg "try.*git_merge_base" -A 5
Length of output: 954
Script:
#!/bin/bash
# Let's check how git_merge_base is used in the codebase and its error handling
# Check all usages of git_merge_base
echo "=== Checking git_merge_base usages ==="
rg "git_merge_base" -B 2 -A 2
# Check the complete implementation context
echo -e "\n=== Complete implementation context ==="
rg "def git_merge_base" -B 5 -A 15 commit_check/util.py
# Check if there's any exception handling around the usage
echo -e "\n=== Exception handling patterns ==="
rg "except.*subprocess" -B 5 -A 2 commit_check/util.py
Length of output: 2731
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/main.yml(4 hunks)
🔇 Additional comments (4)
.github/workflows/main.yml (4)
29-30: LGTM! Good separation of concerns.
The split into separate lint and test-hook sessions improves clarity and maintainability.
45-46: Address test coverage in the nox coverage session.
While moving to a nox session is good, ensure it includes coverage for the new branch rebase checking functionality, as the coverage report shows significant gaps.
76-76: LGTM! Consistent use of nox sessions.
The migration to nox sessions for install-wheel and docs builds maintains consistency across the workflow.
Also applies to: 87-87
24-25:
Consider a more stable installation approach.
While waiting for PR wntrblm/nox#631, consider using a stable version or pinning to a specific commit instead of the main branch. This would prevent potential breakage from upstream changes.
-# TODO: waiting for https://github.com/wntrblm/nox/pull/631 to deliver
-pip install git+https://github.com/wntrblm/nox.git@main --force-reinstall
+# TODO: Update to newer version once https://github.com/wntrblm/nox/pull/631 is merged
+pip install 'nox @ git+https://github.com/wntrblm/nox.git@{specific-commit-hash}'
shenxianpeng
force-pushed
the
feature/check-rebase
branch
from
8cfe4d8 to
4b53208
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
commit_check/branch.py (1)
31-35: Standardize docstring format.The docstring mixes different format styles (
:returnsandparams). Consider using consistent Google-style docstrings:- params checks: List of check configurations containing merge_base rules - - :returns PASS(0) if merge base check succeeds, FAIL(1) otherwise + Args: + checks: List of check configurations containing merge_base rules + + Returns: + int: PASS(0) if merge base check succeeds, FAIL(1) otherwisecommit_check/util.py (4)
103-108: Add type hints and docstring to the decorator.The decorator implementation is correct but lacks proper documentation and type hints.
Apply this diff to improve the documentation:
+from typing import Callable, TypeVar, Any + +T = TypeVar('T', bound=Callable[..., Any]) + -def track_print_call(func): +def track_print_call(func: T) -> T: + """Decorator that tracks if a function has been called. + + Args: + func: The function to be decorated + + Returns: + A wrapped function that tracks its calls + """ def wrapper(*args, **kwargs): wrapper.has_been_called = True return func(*args, **kwargs) wrapper.has_been_called = False # Initialize as False return wrapper
Line range hint
111-129: Improve documentation and code organization.Consider extracting the ASCII art to a constant and improving the docstring.
Apply this diff to improve the code:
+ERROR_ASCII_ART = ''' +Commit rejected by Commit-Check. + + (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) + / ._. \ / ._. \ / ._. \ / ._. \ / ._. \ + __\( C )/__ __\( H )/__ __\( E )/__ __\( C )/__ __\( K )/__ +(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._) + || E || || R || || R || || O || || R || + _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ +(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.) + `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ + +Commit rejected. + +''' + @track_print_call -def print_error_header(): +def print_error_header() -> None: """Print error message. - :returns: Print error head to user + + Prints a formatted ASCII art error message to indicate that a commit + has been rejected by the commit-check system. This function is tracked + to ensure it's only printed once per execution. + + Returns: + None """ - print("Commit rejected by Commit-Check. ") - print(" ") - print(r" (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) (c).-.(c) ") - print(r" / ._. \ / ._. \ / ._. \ / ._. \ / ._. \ ") - print(r" __\( C )/__ __\( H )/__ __\( E )/__ __\( C )/__ __\( K )/__ ") - print(r"(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)") - print(r" || E || || R || || R || || O || || R || ") - print(r" _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ _.' '-' '._ ") - print(r"(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)") - print(r" `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ `-´ ") - print(" ") - print("Commit rejected. ") - print(" ") + print(ERROR_ASCII_ART)
131-139: Complete the function documentation.The function's docstring should include parameter descriptions and return type.
Apply this diff to improve the documentation:
-def print_error_message(check_type: str, regex: str, error: str, reason: str): +def print_error_message(check_type: str, regex: str, error: str, reason: str) -> None: """Print error message. - :param check_type: - :param regex: - :param error: - :param reason: + Args: + check_type: The type of check that failed (e.g., 'commit', 'branch') + regex: The regular expression pattern that wasn't matched + error: The error message describing what went wrong + reason: The reason for the check failure - :returns: Give error messages to user + Returns: + None """
Line range hint
54-139: Add unit tests for new functionality.The Codecov report indicates low coverage (14.28%) for this file. Please add unit tests for:
- The
track_print_calldecorator- The
print_error_headerfunction- The updated
print_error_messagefunctionWould you like me to help generate comprehensive unit tests for these functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.commit-check.yml(1 hunks)commit_check/__init__.py(1 hunks)commit_check/author.py(2 hunks)commit_check/branch.py(3 hunks)commit_check/commit.py(3 hunks)commit_check/util.py(3 hunks)noxfile.py(1 hunks)tests/util_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- commit_check/init.py
- commit_check/author.py
- commit_check/commit.py
- noxfile.py
- tests/util_test.py
🔇 Additional comments (5)
.commit-check.yml (2)
30-30: LGTM! The new check is well-integrated.
The merge_base check follows the established pattern and structure of existing checks, maintaining consistency in the configuration file.
31-39: 🛠️ Refactor suggestion
Enhance the merge_base check configuration for better flexibility.
The current configuration has several limitations that could be improved:
- The regex pattern is restrictive and doesn't support custom branch names
- The suggestion hardcodes "main" and "origin" values
- The error message could provide more context
Consider applying these improvements:
- check: merge_base
- regex: (main|master|develop|devel)
+ regex: ${TARGET_BRANCH:-.*} # Support any branch name, with optional default
error: |
- Current branch is not up to date with target branch
+ Current branch is not up to date with target branch '${TARGET_BRANCH}'.
+ This check ensures a clean semi-linear history by requiring branches to be rebased.
suggest: |
Please ensure your branch is up to date with the target branch by running:
- git fetch origin main
- git rebase origin/main
+ git fetch ${REMOTE_NAME:-origin} ${TARGET_BRANCH}
+ git rebase ${REMOTE_NAME:-origin}/${TARGET_BRANCH}
# If you encounter conflicts, resolve them and continue with:
git rebase --continue
+ # To disable this check, set SKIP_REBASE_CHECK=trueLet's verify if there are any other configuration files that might need similar updates:
commit_check/branch.py (3)
4-4: LGTM: Import changes are appropriate.
The new imports align with the added functionality and are all utilized in the code.
18-19: LGTM: Error handling improvement.
Good addition of the error header check to ensure it's only printed once, improving the error output formatting.
43-54: Verify git merge-base behavior and improve error handling.
A few suggestions for improvement:
- The condition
result != 0is more permissive than checking forresult == 1. Consider being more specific about the failure condition. - The error message could be more descriptive about why the rebase is needed.
- Using 'HEAD' as a fixed reference might not always be correct in all git workflows.
Let's verify the git merge-base behavior:
Consider these improvements:
- result = git_merge_base(check['regex'], 'HEAD')
- if result != 0:
+ result = git_merge_base(check['regex'], 'HEAD')
+ if result == 1: # Specifically check for "not an ancestor" condition
branch_name = get_branch_name()
if not print_error_header.has_been_called:
print_error_header()
print_error_message(
check['check'], check['regex'],
- check['error'], branch_name,
+ f"Branch '{branch_name}' is behind {check['regex']} and needs to be rebased",
+ branch_name,
)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.commit-check.yml(1 hunks)commit_check/__init__.py(1 hunks)commit_check/branch.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- commit_check/init.py
- commit_check/branch.py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/util_test.py(3 hunks)
🔇 Additional comments (2)
tests/util_test.py (2)
2-4: LGTM: Import statements are correctly placed and necessary.
The new imports support the TestGitMergeBase test class implementation.
201-207: LGTM: Good separation of test concerns.
The new test method improves organization by isolating header testing from message testing, reducing duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/util_test.py (2)
2-12: Consider organizing imports by groupsConsider organizing imports into standard library, third-party, and local application imports, separated by blank lines:
-import pytest -import subprocess -from commit_check.util import get_branch_name -from commit_check.util import git_merge_base -from commit_check.util import get_commit_info -from commit_check.util import cmd_output -from commit_check.util import validate_config -from commit_check.util import print_error_header -from commit_check.util import print_error_message -from commit_check.util import print_suggestion -from subprocess import CalledProcessError, PIPE -from unittest.mock import MagicMock +import subprocess +from subprocess import CalledProcessError, PIPE +from unittest.mock import MagicMock + +import pytest + +from commit_check.util import ( + cmd_output, + get_branch_name, + get_commit_info, + git_merge_base, + print_error_header, + print_error_message, + print_suggestion, + validate_config, )
201-207: Enhance test assertions for print_error_headerWhile the test covers basic functionality, consider enhancing it to:
- Test the exact format and order of messages
- Use more descriptive assertions
def test_print_error_header(self, capfd): - # Must print on stdout with given argument. + # Must print error header messages in correct format and order print_error_header() stdout, _ = capfd.readouterr() - assert "Commit rejected by Commit-Check" in stdout - assert "Commit rejected." in stdout + expected_output = ( + "Commit rejected by Commit-Check\n" + "Commit rejected.\n" + ) + assert stdout == expected_output
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
commit_check/branch.py (3)
31-35: Improve docstring format to follow Python conventions.The docstring should follow standard Python conventions (Google, NumPy, or reStructuredText style).
- """Check if the current branch is based on the latest target branch. - params checks: List of check configurations containing merge_base rules - - :returns PASS(0) if merge base check succeeds, FAIL(1) otherwise - """ + """Check if the current branch is based on the latest target branch. + + Args: + checks (list): List of check configurations containing merge_base rules. + + Returns: + int: PASS(0) if merge base check succeeds, FAIL(1) otherwise. + """
43-44: Improve error handling specificity.The current error handling doesn't distinguish between different types of failures (e.g., invalid branch name vs. not being up-to-date).
- result = git_merge_base(check['regex'], 'HEAD') - if result != 0: + try: + result = git_merge_base(check['regex'], 'HEAD') + if result != 0: + error_msg = f"Branch is not up to date with {check['regex']}" + else: + return PASS + except Exception as e: + error_msg = f"Failed to check merge base: {str(e)}" + branch_name = get_branch_name()
43-43: Consider making the reference configurable.The function hardcodes 'HEAD' as the reference to check against. Consider making this configurable through the check configuration.
- result = git_merge_base(check['regex'], 'HEAD') + reference = check.get('reference', 'HEAD') + result = git_merge_base(check['regex'], reference)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
commit_check/branch.py(2 hunks)tests/branch_test.py(2 hunks)
🔇 Additional comments (3)
commit_check/branch.py (1)
18-26: Verify error handling flow across different scenarios.
The error handling looks good, but let's verify the flow across different scenarios to ensure consistent behavior.
✅ Verification successful
Error handling flow is consistent and well-implemented
The error handling flow in branch.py follows the same pattern used consistently across the codebase:
- Proper check for
print_error_header.has_been_calledbefore printing headers - Consistent use of
print_error_messagewith required parameters - Conditional
print_suggestionbased on the presence of suggestion text - Consistent return of
FAILafter error handling
The implementation aligns with the patterns found in other files (author.py, commit.py) and is well-tested as evidenced by the test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling flow and message printing
# Test: Check for consistent error handling patterns across the codebase
# Look for other uses of print_error_header.has_been_called
echo "=== Checking print_error_header.has_been_called usage ==="
rg "print_error_header\.has_been_called"
# Look for error message printing patterns
echo -e "\n=== Checking error message printing patterns ==="
rg "print_error_message.*check\['check'\].*check\['regex'\]"
Length of output: 765
Script:
#!/bin/bash
# Let's check the error handling patterns more comprehensively
# Check all print_error_message usages
echo "=== Checking print_error_message patterns ==="
rg "print_error_message" -A 2
# Check print_suggestion usages
echo -e "\n=== Checking print_suggestion patterns ==="
rg "print_suggestion" -A 1
# Check the error handling flow in branch.py
echo -e "\n=== Checking branch.py error handling ==="
cat commit_check/branch.py
Length of output: 11384
tests/branch_test.py (2)
1-7: LGTM: Imports and constants are well-structured.
The imports and test constants are appropriately defined for the test requirements.
9-9: LGTM: Class rename improves clarity.
The rename from TestBranch to TestCheckBranch better reflects the class's purpose.
shenxianpeng
changed the title
merge_base to check the branch is rebased
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
commit_check/branch.py (1)
31-35: Standardize the docstring format.The docstring mixes different format styles. Consider using consistent Google-style docstring format:
- """Check if the current branch is based on the latest target branch. - params checks: List of check configurations containing merge_base rules - - :returns PASS(0) if merge base check succeeds, FAIL(1) otherwise - """ + """Check if the current branch is based on the latest target branch. + + Args: + checks: List of check configurations containing merge_base rules + + Returns: + int: PASS(0) if merge base check succeeds, FAIL(1) otherwise + """tests/branch_test.py (1)
148-149: Remove debug print and improve assertion robustness.Two issues to address:
- The debug print statement on line 149 should be removed
- The assertion on line 148 uses array indexing which could be fragile
Apply this change:
- assert "Current branch is not" in m_print_error.call_args[0][2] - print(m_print_error) + m_print_error.assert_called_once_with("Current branch is not")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
commit_check/branch.py(3 hunks)tests/branch_test.py(2 hunks)
🔇 Additional comments (4)
commit_check/branch.py (3)
4-4: LGTM!
The new imports are well-organized and necessary for the added functionality.
18-19: LGTM!
The addition of error header handling improves the consistency of error reporting across the codebase.
30-55: Add unit tests to improve coverage.
The previous review comment about test coverage is still valid. Please add comprehensive tests covering:
- Empty regex case
- Successful merge base check
- Failed merge base check
- Invalid configuration scenarios
Let's verify the current test coverage:
#!/bin/bash
# Description: Check test coverage for check_merge_base function
# Test 1: Look for existing test cases
rg -l "TestCheckMergeBase" tests/
rg "def test.*merge.*base" tests/ -A 5tests/branch_test.py (1)
9-9: LGTM! Clear and descriptive class renaming.
The rename from TestBranch to TestCheckBranch better reflects the class's purpose and improves code clarity.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/main_test.py (1)
113-113: Consider adding explicit assertions for merge base config.While the mock is properly added, consider enhancing test coverage by adding explicit assertions for the merge base configuration, similar to how
check_commitis verified againstDEFAULT_CONFIG.Example addition:
assert m_check_merge_base.call_count == 0 # or expected value based on configtests/branch_test.py (1)
148-149: Improve assertion precision.The string assertions using
inoperator could pass with partial matches. Consider using exact equality for more precise testing:- assert "Current branch is not" in m_print_error.call_args[0][2] - assert "Please rebase" in m_print_suggest.call_args[0][0] + assert m_print_error.call_args[0][2] == "Current branch is not" + assert m_print_suggest.call_args[0][0] == "Please rebase"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/branch_test.py(2 hunks)tests/main_test.py(6 hunks)
🔇 Additional comments (9)
tests/main_test.py (6)
10-23: Well-structured test matrix!
The parameterized test cases provide comprehensive coverage of all command-line argument combinations, including the new --merge-base flag. The test matrix effectively validates both individual flags and their combinations.
33-33: LGTM! Clean integration of new test components.
The new parameter and mock are properly integrated into the existing test structure, maintaining consistency with the established patterns.
Also applies to: 47-47
54-54: LGTM! Consistent assertion pattern.
The assertion for the merge base check call count follows the established pattern and properly verifies the expected behavior.
69-69: LGTM! Proper help flag handling.
The help test correctly verifies that the merge base check, like other checks, is not executed when the help flag is used.
Also applies to: 77-77
94-94: LGTM! Proper version flag handling.
The version test correctly verifies that the merge base check, like other checks, is not executed when the version flag is used.
Also applies to: 102-102
Line range hint 1-116: Verify test coverage aligns with Codecov report.
The test implementation is thorough, but let's verify it provides adequate coverage for the new merge base functionality, as Codecov reports a coverage decrease to 96.92%.
✅ Verification successful
Let me analyze the test coverage more thoroughly by checking the implementation details and their corresponding test cases.
Test coverage for merge base functionality is comprehensive
The test coverage analysis shows that all code paths are well-tested:
- The
check_merge_basefunction inbranch.pyis covered by multiple test cases:- Empty checks scenario
- Non-merge-base check scenario
- Failure scenario with error messages
- The underlying
git_merge_baseutility is tested for different return codes including error cases
The coverage decrease to 96.92% is likely due to other changes in the codebase, as the merge base functionality appears to have thorough test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for merge base functionality
# Find all files that might contain merge base related code
echo "Files potentially containing merge base functionality:"
fd -e py | xargs rg -l "merge_base"
# Look for untested code paths in these files
echo -e "\nPotential untested code paths:"
rg "def.*merge_base" -A 5
Length of output: 2758
Script:
#!/bin/bash
# Check implementation details of merge base functionality
echo "Implementation of check_merge_base in branch.py:"
rg -A 20 "def check_merge_base" commit_check/branch.py
echo -e "\nTest cases for merge base functionality:"
rg -A 20 "test_check_merge_base" tests/branch_test.py
echo -e "\nTest coverage for git_merge_base utility:"
rg -A 10 "test_git_merge_base" tests/util_test.py
Length of output: 3156
tests/branch_test.py (3)
1-2: LGTM: Import statements are correctly updated.
The addition of check_merge_base import aligns with the new test class implementation.
Line range hint 9-115: LGTM: Class rename improves clarity.
The rename from TestBranch to TestCheckBranch better reflects the class's purpose of testing the check_branch function.
117-149: Add missing test scenarios for comprehensive coverage.
While the current implementation covers basic scenarios, several critical test cases are missing:
#!/bin/bash
# Description: Check current test coverage for merge base functionality
# Check current test coverage
echo "Current test coverage for merge base functionality:"
rg -A 5 "def test.*merge_base" tests/
# Check for CLI argument handling tests
echo "CLI argument handling tests:"
rg -A 5 "test.*merge.*base.*arg" tests/
# Check for git command failure tests
echo "Git command failure tests:"
rg -A 5 "test.*merge.*base.*fail" tests/
closes #191
closes #29
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
.gitignoreto exclude additional directories.noxpackage to development requirements for improved session management.Tests
git_merge_basefunction and improved coverage for error header printing.check_merge_basefunctionality.