Conversation
spacewander
left a comment
There was a problem hiding this comment.
Here's the AI review result:
The PR replaces gitignore.io with GitHub's github/gitignore repo in git-ignore-io. The reviewer verified against live GitHub endpoints and found the core idea works, but two issues block merge:
High — Global/ templates are broken (silent 404s):
- ~70 of the most-used templates (Vim, Emacs, macOS, Windows, JetBrains, Xcode, VirtualEnv, etc.) live in the
Global/subdirectory, not the repo root - The PR only lists/fetches root-level
*.gitignorefiles, sogit ignore-io vim— the exact example in the updated man page — silently returns nothing curl -fsSLswallows the 404, so users get zero output, no error- This is a regression: gitignore.io previously served all of these
Medium — rate limits & silent failures:
- Unauthenticated GitHub Contents API = 60 req/hour, and
gi()now hits it on every invocation even with a valid cache update_gi_listwrites an empty cache file on failure, breaking-l/-L/-suntil next successful-ugit ignore-io -r typotruncates.gitignoreto empty with no warning (pre-existing risk, now more likely due to silent 404s)
Minor:
man/git-ignore-io.mdexample still shows the old toptal URL (would revert on regeneration); the updated.1/.htmlexample URL is itself a 404- "Initial gitignore.io list" banner at
bin/git-ignore-io:86not updated
Proposed Fix Plan
- Include
Global/templates: switch to the git trees API (git/trees/main?recursive=1) in one call, filter for*.gitignore, and store each file's fullpath(or build URLs from path) so both root andGlobal/templates resolve - Respect the cache: only call the API when the cache is missing/expired or
-uis passed, avoiding the 60 req/hour ceiling - Error handling: on curl failure, print an error to stderr and skip cache truncation / refuse
-rwrite instead of silently emptying files - Docs: update the
.mdexample, fix the 404 example URL (use e.g.Global/Vim.gitignore), and update the init banner string
One tradeoff to confirm: trees API responses are large (~1MB for the whole repo) — acceptable for a one-off -u refresh, which is why I'd cache and only fetch on demand.
Signed-off-by: vjymisal0 <misalvijay153@gmail.com>
|
Thanks for the detailed review @spacewander. I switched list refreshes to the recursive git tree API so Global/* templates such as Global/Vim are cached and fetched correctly, and template generation now uses the cache instead of hitting GitHub on every invocation. Refreshes are atomic and report failures without overwriting a valid cache; --replace/--append also avoid modifying files when a requested template cannot be fetched. Updated the generated man pages and init banner. Verified with live -u, vim, and missing-template scenarios, plus bash syntax and diff checks. Pushed as bb9fb05. |
spacewander
left a comment
There was a problem hiding this comment.
Main issue (must fix before merge)
Legacy caches resolve but then 404 — the PR's stated goal of keeping existing cached lists working isn't met:
raw.githubusercontent.comis case-sensitive (Python.gitignore→ 200,python.gitignore→ 404)- Legacy gitignore.io caches store all-lowercase names (
python,vim, ...), so lookup succeeds but fetch 404s - Stale/invalid caches are never auto-repaired —
check_list_existonly refreshes missing files; a user whose lastupdate_gi_listfailed under old code has a cache full of Cloudflare HTML that's never refreshed
Proposed fix plan (for bin/git-ignore-io)
- Legacy/invalid cache detection + refresh: in
check_list_exist, treat the cache as invalid if it doesn't look like a GitHub template list (e.g. single comma-joined line, or no valid path entries) and force re-download. This fixes both lowercase legacy caches and HTML-poisoned caches in one shot. - Fallback retry on 404 (defense in depth): after resolving a name, if the exact-case fetch 404s, retry with the correct-cased name from a refreshed list.
…es, and return cat exit status
spacewander
left a comment
There was a problem hiding this comment.
LGTM. Before I could merge it, could you update https://github.com/tj/git-extras/blob/main/Commands.md#git-ignore-io too?
Signed-off-by: vjymisal0 <misalvijay153@gmail.com>
|
Thanks @spacewander! I updated Commands.md to reference the GitHub gitignore templates and corrected the |
|
Thanks @spacewander for the detailed review. The branch now uses the recursive GitHub tree endpoint, includes Global templates, refreshes invalid/legacy caches safely, preserves the cache between invocations, handles case-insensitive template lookup and 404 failures, and updates the generated documentation examples. I verified the branch is clean and pushed. |
Summary
git-ignore-iocurrently fetches templates through the Toptal-hosted gitignore.io endpoint, which can reject requests with Cloudflare. Use GitHub's publicgitignorerepository as the source instead. Template names are resolved case-insensitively so existing cached lists continue to work.Test plan
bash -n bin/git-ignore-io./check_integrity.sh ignore-ioFixes #1272