Repository navigation
Don't show path to script - #27
Conversation
|
Warning Rate limit exceeded@mernst has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes affect two shell scripts. In the first script, the usage output message now displays the script's basename instead of the literal Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
git-find-fork (1)
23-33:git-find-forkfails silently ifwgetis unavailable, incorrectly falling back to the upstream repository — add a dependency check or fallback.The script uses
wget -q --spiderto probe fork existence (line 26) without checking whetherwgetis installed. If the probe fails (missing binary or network error), the conditional silently returns false, causing the script to useUPSTREAM_ORGinstead ofORG. This matches the observed test failure: fork exists but is not selected because the probe command fails.To fix, either:
- Fail explicitly if neither
wgetnorcurlexists- Check for
wget, fall back tocurl -fsIif available, or fail with a clear errorOne possible fix (dependency check + curl fallback)
-# export GITEXISTS="git ls-remote" -# export GITEXISTS="wget -q --spider" -export GITEXISTS="wget -q --spider" +if command -v wget >/dev/null 2>&1; then + gitexists() { wget -q --spider "$1"; } +elif command -v curl >/dev/null 2>&1; then + gitexists() { curl -fsI "$1" >/dev/null; } +else + echo "Error: need either wget or curl to probe GitHub URLs" >&2 + exit 2 +fi -if (${GITEXISTS} "https://github.com/${ORG}/${REPONAME}.git" > /dev/null); then +if gitexists "https://github.com/${ORG}/${REPONAME}.git"; then OWNER="${ORG}" else OWNER="${UPSTREAM_ORG}" fi
🤖 Fix all issues with AI agents
In @git-find-fork:
- Around line 12-16: Replace the external call to basename with POSIX parameter
expansion: assign SCRIPT_NAME using ${0##*/} instead of calling basename -- "$0"
(the symbol to change is SCRIPT_NAME assignment where basename is used), leaving
the rest of the argument-checking and error/usage echo lines unchanged so they
now reference the POSIX-safe SCRIPT_NAME.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
git-clone-relatedgit-find-fork
🧰 Additional context used
🪛 GitHub Actions: Run Small test
git-clone-related
[error] 1-1: Command failed: make -C tests test. Test 'git-clone-related-test' failed: expected repo https://github.com/mernst/git-clone-related-test-repo2.git, got: https://github.com/plume-lib/git-clone-related-test-repo2.git
🔇 Additional comments (1)
git-clone-related (1)
59-62: Good UX change; consider${0##*/}instead ofbasename -- "$0"(POSIX/portability) and ensure no tests assert the exact usage string.Optional tweak
if [ "$#" -lt 2 ]; then - echo "Usage: $(basename -- "$0") UPSTREAM_ORG REPO [DESTINATION] [GIT_CLONE_ARGS...]" >&2 + echo "Usage: ${0##*/} UPSTREAM_ORG REPO [DESTINATION] [GIT_CLONE_ARGS...]" >&2 exit 1 fi
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
git-clone-related (1)
170-170: Inconsistent usage of$0— should also usebasename.This line still displays the full path to the script, which is inconsistent with the change at line 60. For a consistent user experience, apply the same treatment here.
Proposed fix
-echo "$0: ${DESTINATION} is at $(cd "${DESTINATION}" && git rev-parse HEAD)" +echo "$(basename -- "$0"): ${DESTINATION} is at $(cd "${DESTINATION}" && git rev-parse HEAD)"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
git-clone-related
🔇 Additional comments (1)
git-clone-related (1)
59-61: LGTM!The use of
basename -- "$0"correctly displays only the script name in the usage message. The--guard handles edge cases where the script path might start with a dash.
No description provided.