-
Notifications
You must be signed in to change notification settings - Fork 951
{{ message }}
chore(git): find the closer .git #4588
Open
+70
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous method preferred finding `.git` files over `.git/` directories. That doesn't work if, for some reason, the user a `.git/` directory inside the repo with commitlint and that repo path is nested inside another git repo. Ignoring the fact that this isn't a standard use of git, it still works, and can be trivially supported by comparing the path lengths. Priority: Low Tests: None, but there were none in @commitlint before this, either. Risk: Low
Add a unit test showing the previous change works as expected.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the .git discovery logic to prefer the closer .git path when both a file and directory are found. This addresses an edge case where a git repository with commitlint is nested inside another git repository.
Key Changes:
- Modified the
searchDotGitfunction to compare path lengths when both.gitfile and directory are found - Added comprehensive test coverage for the new logic including edge cases for nested repositories and submodules
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
@commitlint/top-level/src/index.ts |
Added logic to compare path lengths and return the closer .git when both file and directory exist |
@commitlint/top-level/src/index.test.ts |
Added comprehensive test suite with 4 tests covering basic functionality, nested repos, submodules, and missing .git scenarios |
@commitlint/top-level/package.json |
Added @commitlint/test as a dev dependency for testing utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+28
| // Return whichever is deeper (closer to cwd) by comparing path lengths | ||
| return foundFile.length > foundDir.length ? foundFile : foundDir; |
There was a problem hiding this comment.
While path length comparison works for determining proximity in typical cases, it's fragile and relies on filesystem conventions. Consider using path segment counting or explicit path comparison methods for more robust and maintainable code. For example, comparing foundFile.split(path.sep).length vs foundDir.split(path.sep).length would make the intent clearer and be more resilient to edge cases.
Suggested change
| // Return whichever is deeper (closer to cwd) by comparing path lengths | |
| return foundFile.length > foundDir.length ? foundFile : foundDir; | |
| // Return whichever is deeper (closer to cwd) by comparing path segment counts | |
| const fileDepth = path.resolve(foundFile).split(path.sep).length; | |
| const dirDepth = path.resolve(foundDir).split(path.sep).length; | |
| return fileDepth > dirDepth ? foundFile : foundDir; |
| const foundDir = await findUp(".git", { cwd, type: "directory" }); | ||
|
|
||
| if (foundFile && foundDir) { | ||
| // Return whichever is deeper (closer to cwd) by comparing path lengths |
There was a problem hiding this comment.
The comment says "whichever is deeper (closer to cwd)" but the code returns the longer path. In the context of file paths, "deeper" typically means more nested (more path segments), not a longer string. This mismatch between comment and implementation could be confusing.
Suggested change
| // Return whichever is deeper (closer to cwd) by comparing path lengths | |
| // Return whichever has the longer path string |
Member
|
Hey, thanks! Have a look at copilots feedback please. And adjust your commit message ;)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
return the shorter (closer to cwd) path rather than preferring .git files to .git folders.
Motivation and Context
The previous method preferred finding
.gitfiles over.git/directories. That doesn't work if, for some reason, the user a.git/directory inside the repo with commitlint and that repo path is nested inside another git repo. Ignoring the fact that this isn't a standard use of git, it still works, and can be trivially supported by comparing the path lengths.Fixed bug when running commitlint as part of pre-commit from git repo nested inside another git repo.
Usage examples
n/a
How Has This Been Tested?
Created unit test. Ran
yarn test @commitlint/top-levelTypes of changes
Checklist: