More: git init
#487
More: git init
#487
Conversation
Reviewer's Guide by SourceryThis PR enhances the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #487 +/- ##
==========================================
+ Coverage 54.09% 56.34% +2.25%
==========================================
Files 40 40
Lines 3627 3789 +162
Branches 793 807 +14
==========================================
+ Hits 1962 2135 +173
+ Misses 1314 1301 -13
- Partials 351 353 +2 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](/https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to reduce the duplication in the validation logic for the
sharedparameter. - The
initial_branchparameter is an alias forbranch, but it's not clear from the code that they are interchangeable - consider explicitly settingbranch = initial_branch.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if shared is not None: | ||
| valid_shared_values = { | ||
| "false", | ||
| "true", | ||
| "umask", | ||
| "group", | ||
| "all", | ||
| "world", | ||
| "everybody", | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Boolean shared value check appends flag even for false.
The updated code checks if shared is an instance of bool and then unconditionally appends "--shared". This means that if shared is False (explicitly turning off sharing), the flag will still be added. To maintain behavior consistent with earlier logic—where the flag was only added for True—it would be better to explicitly check that shared is True (e.g. using if shared is True instead).
| "or a valid octal number between 0000 and 0777" | ||
| ) | ||
| raise ValueError(msg) | ||
| local_flags.append(f"--shared={shared}") |
There was a problem hiding this comment.
suggestion (bug_risk): Consistency in handling non-boolean shared values.
After converting the non-boolean shared value to lower case (stored in shared_str) for validation, the flag is appended using the original shared variable rather than the normalized one. Consider using the lowercased value in constructing the flag to ensure consistent representation of the flag value.
| local_flags.append(f"--shared={shared}") | |
| local_flags.append(f"--shared={shared_str}") |
| result = repo.init(bare=True) | ||
| assert "Initialized empty Git repository" in result | ||
| # Bare repos have files directly in the directory | ||
| assert (tmp_path / "HEAD").exists() |
There was a problem hiding this comment.
suggestion (testing): Missing assertion for the --bare flag's effect on config.
While the existence of HEAD is a good indicator, it's not definitive proof of a bare repository. A stronger assertion would be to check the core.bare config setting within the .git directory. This ensures the repository is truly configured as bare.
| assert (tmp_path / "HEAD").exists() | |
| assert (tmp_path / "HEAD").exists() | |
| config_path = tmp_path / "config" | |
| assert config_path.exists(), "Config file does not exist in bare repository" | |
| config_text = config_path.read_text() | |
| assert "bare = true" in config_text, "Repository core.bare flag not set to true" |
| # Note: sha256 test is commented out as it might not be supported in all | ||
| # git versions | ||
| # repo_dir = tmp_path / "sha256" | ||
| # repo_dir.mkdir() | ||
| # repo = git.Git(path=repo_dir) | ||
| # result = repo.init(object_format="sha256") | ||
| # assert "Initialized empty Git repository" in result |
There was a problem hiding this comment.
suggestion (testing): Handle potential unsupported sha256.
Instead of commenting out the sha256 test, use pytest.mark.skipif to conditionally skip the test if the current Git version doesn't support sha256. This provides a cleaner way to handle unsupported features and keeps the test suite more comprehensive.
Suggested implementation:
import pathlib
import git
import pytest
import subprocess
def supports_sha256() -> bool:
try:
output = subprocess.check_output(["git", "--version"], universal_newlines=True)
version_str = output.strip().split()[2]
major, minor, *_ = map(int, version_str.split('.'))
# SHA256 support is assumed for Git versions 2.29 and above.
return (major, minor) >= (2, 29)
except Exception:
return Falsedef test_git_init_object_format(tmp_path: pathlib.Path) -> None:
"""Test git init with different object formats."""
repo = git.Git(path=tmp_path)
# Test with sha1 (default)
result = repo.init(object_format="sha1")
assert "Initialized empty Git repository" in result
assert repo.path == tmp_path@pytest.mark.skipif(not supports_sha256(), reason="sha256 not supported by current git version")
def test_git_init_object_format_sha256(tmp_path: pathlib.Path) -> None:
"""Test git init with sha256 object format."""
repo_dir = tmp_path / "sha256"
repo_dir.mkdir()
repo = git.Git(path=repo_dir)
result = repo.init(object_format="sha256")
assert "Initialized empty Git repository" in resultMake sure that the new helper function supports_sha256() and the import for pytest and subprocess do not duplicate existing imports in your project. Adjust the version threshold if your requirements are different.
| @@ -1026,80 +1027,243 @@ def pull( | |||
| def init( | |||
| self, | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting parameter validation logic into dedicated helper functions to reduce nesting and improve readability.
Consider extracting parameter validation logic into dedicated helper functions to simplify the nesting. For example, you can extract the template and shared validations. This will make the init method shorter and easier to read without changing its behavior.
Example for extracting template validation:
def _validate_template(template: str | pathlib.Path) -> str:
if not isinstance(template, (str, pathlib.Path)):
raise TypeError("template must be a string or Path")
template_path = pathlib.Path(template)
if not template_path.is_dir():
raise ValueError(f"template directory does not exist: {template}")
return f"--template={template}"Example for extracting shared validation:
def _validate_shared(shared: bool | t.Literal["false", "true", "umask", "group", "all", "world", "everybody"] | str | None) -> str | None:
if shared is None:
return None
valid_shared_values = {"false", "true", "umask", "group", "all", "world", "everybody"}
if isinstance(shared, bool):
return "--shared" if shared else None
shared_str = str(shared).lower()
if not (
shared_str in valid_shared_values or (
shared_str.isdigit() and len(shared_str) <= 4 and
all(c in string.octdigits for c in shared_str) and int(shared_str, 8) <= 0o777
)
):
raise ValueError(
f"Invalid shared value. Must be one of {valid_shared_values} or a valid octal number between 0000 and 0777"
)
return f"--shared={shared}"Then, in the init method, you can simplify the code:
if template is not None:
local_flags.append(_validate_template(template))
# ... other flags ...
if shared is not None:
shared_flag = _validate_shared(shared)
if shared_flag:
local_flags.append(shared_flag)This refactoring reduces nesting and centralizes validation logic, making it easier to maintain and test.
| valid_shared_values = { | ||
| "false", | ||
| "true", | ||
| "umask", | ||
| "group", | ||
| "all", | ||
| "world", | ||
| "everybody", | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Move assignments closer to their usage (
move-assign) - Low code quality found in Git.init - 11% (
low-code-quality)
| valid_shared_values = { | |
| "false", | |
| "true", | |
| "umask", | |
| "group", | |
| "all", | |
| "world", | |
| "everybody", | |
| } | |
| valid_shared_values = { | |
| "false", | |
| "true", | |
| "umask", | |
| "group", | |
| "all", | |
| "world", | |
| "everybody", | |
| } |
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
- Add support for all git-init options (template, separate_git_dir, object_format, etc.) - Add comprehensive tests for each option - Fix path handling for separate_git_dir - Fix string formatting for bytes paths - Update docstrings with examples for all options
- Enhance Git.init docstrings with detailed parameter descriptions - Add comprehensive examples including SHA-256 object format - Add return value and exception documentation - Improve type hints for shared parameter with Literal types - Add extensive validation tests for all parameters
- Add validation for template parameter type and existence - Add validation for object_format parameter values - Improve type formatting for shared parameter - Complete docstring example output
- Add ref-format parameter support for git init - Add make_parents parameter to control directory creation - Improve type hints and validation for template and shared parameters - Add comprehensive tests for all shared values and octal permissions - Add validation for octal number range in shared parameter
Changes
Cmd:
Git.initimprovementsThis PR enhances the
Git.init()method with improved parameter validation, comprehensive documentation, and extensive test coverage. Key improvements include:Enhanced Parameter Support:
ref_formatparameter (files/reftable)defaultparameter for default permissionsmake_parentsparameter to control directory creationsharedparameter to support all git-supported valuesImproved Type Handling:
templateparameter to accept bothstrandpathlib.PathDocumentation Improvements:
Validation & Error Handling:
Examples
Testing
The PR includes comprehensive test coverage. To run the tests:
Validation Tests
The PR includes tests for various validation scenarios:
Breaking Changes
None. All changes are backward compatible while adding new functionality.
Dependencies
ref_format="reftable") require git >= 2.37.0Summary by Sourcery
Enhances the
Git.init()method with improved parameter validation, comprehensive documentation, and extensive test coverage. It introduces new features such as support forref_format,default, andmake_parentsparameters, and enhances existing parameters liketemplateandsharedwith improved type handling and validation.New Features:
ref_formatparameter to specify the reference storage format.defaultparameter to use default permissions for directories and files.make_parentsparameter to create parent directories if they don't exist.Enhancements:
templateparameter to accept bothstrandpathlib.Pathtypes.sharedparameter to support all git-supported values, including boolean, string literals, and octal number strings.template,object_format,branch, andsharedparameters.Tests:
git initcommand, including tests for various parameters and validation scenarios.