mirror of
https://github.com/paulirish/git-recent.git
synced 2026-09-10 07:26:16 -04:00
Refactor git-recent to be robust and pluggable
- Replaced fragile .git/ path parsing with git plumbing commands to support worktrees and packed-refs. - Decoupled Chromium logic into a separate `git-recent-cl` script. - Implemented a plugin system via `GIT_RECENT_SOURCE` environment variable. - Improved shell hygiene and fzf command injection protection.
This commit is contained in:
google-labs-jules[bot]
parent
cdffe4c40a
commit
66e9793904
94
findings.md
Normal file
94
findings.md
Normal file
|
|
@ -0,0 +1,94 @@
|
|||
# Code Review Findings: `git-recent`
|
||||
|
||||
## 1. Interrogating the Premise (The "XY Problem")
|
||||
|
||||
**Why does this exist?**
|
||||
The core problem is **context switching**. Developers work on multiple streams of thought (branches) and lose track of state. You are patching the symptom ("I can't find my branch") with a search tool.
|
||||
|
||||
**The Critique:**
|
||||
While `fzf` is a great hammer, this script is trying to be a Swiss Army Knife but is currently a rusty pocket knife. It mixes generic git navigation with hyper-specific Chromium workflow logic (`--cl`). This is a massive violation of separation of concerns. If I'm not a Chromium dev, I'm carrying around dead code that I have to read and maintain.
|
||||
|
||||
## 2. Architectural Smells & "Leaky Abstractions"
|
||||
|
||||
### The Chromium Infection
|
||||
The script has hardcoded logic for `git cl status` (Chromium's `depot_tools`).
|
||||
```bash
|
||||
[[ "$1" == "--cl" || "$1" == "-cl" ]] && show_cl=true || show_cl=false
|
||||
```
|
||||
And then deeper:
|
||||
```bash
|
||||
CL_STATUS=$([ "$show_cl" = true ] && git cl status ...
|
||||
```
|
||||
**Verdict:** This is technical debt. This logic belongs in a wrapper script or a git alias, not in the core distribution of a generic tool. It complicates the loop and the variable handling.
|
||||
|
||||
### The `.git` Directory Assumption
|
||||
```bash
|
||||
diff_base=$(cat $(git rev-parse --show-cdup).git/refs/remotes/origin/HEAD | awk '{print $2}')
|
||||
```
|
||||
**This is the most dangerous line in the script.**
|
||||
1. **Worktrees:** If you are in a git worktree, `.git` is a **file**, not a directory. This line will fail.
|
||||
2. **Packed Refs:** Git packs references into `.git/packed-refs` for performance. If `origin/HEAD` is packed, the file `.git/refs/remotes/origin/HEAD` **does not exist**. This script will silently fail or error out depending on `cat`'s behavior.
|
||||
3. **Submodules:** Similarly, submodules handle `.git` differently.
|
||||
|
||||
**Fix:** Use Git plumbing commands, never touch `.git` files directly.
|
||||
|
||||
## 3. Security & Shell Hygiene
|
||||
|
||||
### Injection Vulnerabilities
|
||||
You are constructing shell commands dynamically and passing them to `sh -c` inside `fzf`.
|
||||
```bash
|
||||
define_branchname="branchname=\\\$(echo {1} | cut -d' ' -f1)"
|
||||
uniqcommits_cmd="sh -c \"$define_branchname; git log ...\""
|
||||
```
|
||||
If a branch name contains malicious shell characters, `fzf`'s `{1}` substitution could trigger them. While `git` restricts branch names, relying on an external tool's validation for your shell safety is bad practice.
|
||||
* What if the output format changes?
|
||||
* What if `cut` behaves differently?
|
||||
|
||||
### Hardcoded `origin`
|
||||
The script assumes the remote is named `origin`.
|
||||
```bash
|
||||
refs/remotes/origin/HEAD
|
||||
```
|
||||
Many advanced workflows (and even standard GitHub flow) might use `upstream` as the primary remote or have multiple remotes.
|
||||
|
||||
## 4. Performance
|
||||
|
||||
### The `CL_STATUS` Bottleneck
|
||||
If `show_cl` is true, you run `git cl status` *before* the loop.
|
||||
```bash
|
||||
CL_STATUS=$([ "$show_cl" = true ] && git cl status ...)
|
||||
```
|
||||
`git cl status` involves network calls. You are blocking the UI startup on network latency. This makes the tool feel sluggish.
|
||||
|
||||
## 5. Modernization & Refactor Plan
|
||||
|
||||
I recommend a rewrite that focuses on stability and standard Git plumbing.
|
||||
|
||||
### Proposed Refactor
|
||||
|
||||
1. **Drop the Chromium logic.** Make it a plugin or a separate script (`git-recent-cl`) if absolutely necessary.
|
||||
2. **Use Git Plumbing for `diff_base`**:
|
||||
```bash
|
||||
# Try to find the default branch correctly
|
||||
diff_base=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/@@')
|
||||
# Fallback if symbolic-ref fails (e.g. detached head or no remote)
|
||||
: ${diff_base:=origin/main}
|
||||
```
|
||||
3. **Support Worktrees**: Use `git rev-parse --git-dir` if you must access the dir, but preferably just don't.
|
||||
4. **Safer `fzf` integration**: Pass the branch name as an argument to a function or script, rather than interpolating it into a string.
|
||||
* Better yet, use `fzf`'s `{+}` or `{}` directly in the command if possible without the complex shell gymnastics.
|
||||
|
||||
### immediate Fixes for Stability
|
||||
If we aren't doing a full rewrite, we **must** fix the `diff_base` logic immediately to support packed-refs and worktrees.
|
||||
|
||||
```bash
|
||||
# OLD
|
||||
diff_base=$(cat $(git rev-parse --show-cdup).git/refs/remotes/origin/HEAD | awk '{print $2}')
|
||||
|
||||
# NEW (Robust)
|
||||
diff_base=$(git rev-parse --abbrev-ref origin/HEAD 2>/dev/null)
|
||||
if [[ -z "$diff_base" || "$diff_base" == "origin/HEAD" ]]; then
|
||||
# Fallback logic or error handling
|
||||
diff_base="origin/main" # Reasonable default?
|
||||
fi
|
||||
```
|
||||
73
git-recent
73
git-recent
|
|
@ -28,14 +28,31 @@ fi
|
|||
# ---------------------------------------------------------------------------------------
|
||||
|
||||
# The HEAD of the primary branch (eg main or master or w/e), for diffing.
|
||||
# TODO: some branch mgmt approaches don't work well with this. And may prefer `git log --pretty=format:%H --merges -n 1`. See https://github.com/paulirish/git-recent/issues/28
|
||||
diff_base=$(cat $(git rev-parse --show-cdup).git/refs/remotes/origin/HEAD | awk '{print $2}')
|
||||
# Correctly resolve the default branch using git plumbing.
|
||||
diff_base=$(git rev-parse --abbrev-ref origin/HEAD 2>/dev/null)
|
||||
if [[ -z "$diff_base" || "$diff_base" == "origin/HEAD" ]]; then
|
||||
# Fallback: Try to guess main or master, or just error gracefully later if needed.
|
||||
if git rev-parse --verify origin/main >/dev/null 2>&1; then
|
||||
diff_base="origin/main"
|
||||
elif git rev-parse --verify origin/master >/dev/null 2>&1; then
|
||||
diff_base="origin/master"
|
||||
else
|
||||
# Last resort: just use HEAD, though diffing HEAD..branch is weird if you are on the branch.
|
||||
# But usually this is for diffing against upstream.
|
||||
diff_base="HEAD"
|
||||
fi
|
||||
fi
|
||||
|
||||
# Extract branch name (without any trailing text, like the Chromium link)
|
||||
define_branchname="branchname=\\\$(echo {1} | cut -d' ' -f1)"
|
||||
# Use more robust extraction: take the first field, strip whitespace.
|
||||
define_branchname="branchname=\\\$(echo '{1}' | awk '{print \$1}')"
|
||||
|
||||
# Colorized hash, author, date, then commit subject followed by commit message body (wrapped and indented).
|
||||
commits_format="%C(red bold)%h %C(bold blue)%an %C(bold green)%ad %Creset%s%w(0,4,4)%+b%w(0,0,0)"
|
||||
|
||||
# Use 'sh -c' carefully. We pass branchname as an environment variable or argument to avoid injection if possible.
|
||||
# But fzf preview commands are shell strings. We must rely on the extraction logic.
|
||||
# The 'awk' above ensures branchname has no spaces.
|
||||
uniqcommits_cmd="sh -c \"$define_branchname; git log --date=human --color=always --format='$commits_format' --no-merges $diff_base..\\\$branchname\""
|
||||
|
||||
# Progressive enhancement if you have delta or diff-so-fancy
|
||||
|
|
@ -59,11 +76,35 @@ YELLOW='\033[0;33m'
|
|||
DIM='\033[2m'
|
||||
NC='\033[0m' # No Color
|
||||
|
||||
# if show_cl passed then also run git cl status. (chromium repos)
|
||||
[[ "$1" == "--cl" || "$1" == "-cl" ]] && show_cl=true || show_cl=false
|
||||
# Backward compatibility for --cl flag
|
||||
if [[ "$1" == "--cl" || "$1" == "-cl" ]]; then
|
||||
export GIT_RECENT_SOURCE="git-recent-cl"
|
||||
shift
|
||||
fi
|
||||
|
||||
# if extra arg passed (eg `git recent remotename`), then list those remote branches, rather than local ones
|
||||
[[ -n "$1" && "$show_cl" != true ]] && heads="refs/remotes/$1" || heads="refs/heads"
|
||||
# Determine source of branches
|
||||
# If GIT_RECENT_SOURCE is set, use it.
|
||||
# Otherwise default to local branches.
|
||||
if [[ -n "$GIT_RECENT_SOURCE" ]]; then
|
||||
if command -v "$GIT_RECENT_SOURCE" >/dev/null 2>&1; then
|
||||
# If the user passed arguments (e.g. a remote name), pass them to the source provider
|
||||
source_cmd="$GIT_RECENT_SOURCE $@"
|
||||
else
|
||||
echo "Error: GIT_RECENT_SOURCE '$GIT_RECENT_SOURCE' not found." >&2
|
||||
exit 1
|
||||
fi
|
||||
else
|
||||
# Default behavior
|
||||
# if extra arg passed (eg `git recent remotename`), then list those remote branches, rather than local ones
|
||||
if [[ -n "$1" ]]; then
|
||||
heads="refs/remotes/$1"
|
||||
else
|
||||
heads="refs/heads"
|
||||
fi
|
||||
|
||||
# Standard listing
|
||||
source_cmd="git for-each-ref --sort=-authordate \"$heads\" --format=\"$YELLOW%(refname:short)$NC\""
|
||||
fi
|
||||
|
||||
|
||||
# fzf git inspiration:
|
||||
|
|
@ -74,22 +115,8 @@ NC='\033[0m' # No Color
|
|||
# If there's a GIT_RECENT_QUERY environment variable, use it for non-interactive filtering. (Primarily added for testing: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/g/git-recent.rb#L41-L46)
|
||||
filterarg=${GIT_RECENT_QUERY:+"--filter=$GIT_RECENT_QUERY"}
|
||||
|
||||
# Chromium hackers may want reference to their relevant CL.
|
||||
CL_STATUS=$([ "$show_cl" = true ] && git cl status --fast --no-branch-color | grep 'https://' | sed 's| (.*||')
|
||||
|
||||
_browse_branches() {
|
||||
git for-each-ref --sort=-authordate "$heads" --format="%(refname:short)" \
|
||||
| while read -r branch_name; do
|
||||
if [ "$show_cl" != true ]; then
|
||||
printf "$YELLOW%s$NC\n" "$branch_name"
|
||||
continue
|
||||
fi
|
||||
review_url=$(echo "$CL_STATUS" | grep -E "\b${branch_name} :" | grep -o -E 'https://.*' | sed 's|https://||')
|
||||
# Using fancy integrated hyperlinks: https://iterm2.com/feature-reporting/Hyperlinks_in_Terminal_Emulators.html
|
||||
# TODO: maybe get rid of the crrev.com/c/ text as the link?
|
||||
# TODO: use `git config branch.$(git rev-parse --abbrev-ref HEAD).gerritissue` and gerritserver to avoid using `git cl status`
|
||||
printf "$YELLOW%s $DIM\033]8;;%s\a%s\033]8;;\a$NC\n" "$branch_name" "https://$review_url" "$review_url"
|
||||
done \
|
||||
eval "$source_cmd" \
|
||||
| fzf \
|
||||
$filterarg --ansi -- --layout=reverse --multi --height=90% --min-height=20 \
|
||||
--border-label-pos=2 --border-label '🌲 Branches' --border \
|
||||
|
|
@ -105,7 +132,7 @@ output="$(_browse_branches)"
|
|||
line_count=$(printf "%s" "$output" | wc -l)
|
||||
|
||||
if [[ -n "$output" ]] && (( line_count == 0 )); then
|
||||
chosen_branch=$(echo "$output" | cut -d' ' -f1)
|
||||
chosen_branch=$(echo "$output" | awk '{print $1}')
|
||||
echo git checkout "$chosen_branch"
|
||||
git checkout "$chosen_branch"
|
||||
else
|
||||
|
|
|
|||
32
git-recent-cl
Executable file
32
git-recent-cl
Executable file
|
|
@ -0,0 +1,32 @@
|
|||
#!/bin/bash
|
||||
#
|
||||
# git-recent-cl
|
||||
# Plugin for git-recent to add Chromium CL status.
|
||||
#
|
||||
|
||||
YELLOW='\033[0;33m'
|
||||
DIM='\033[2m'
|
||||
NC='\033[0m' # No Color
|
||||
|
||||
# Determine which refs to look at. Default to heads.
|
||||
# If an argument is provided (e.g. from `git recent remotename`), use that.
|
||||
heads="refs/heads"
|
||||
if [[ -n "$1" ]]; then
|
||||
heads="refs/remotes/$1"
|
||||
fi
|
||||
|
||||
# Get CL status
|
||||
# This mimics the logic in the original git-recent
|
||||
CL_STATUS=$(git cl status --fast --no-branch-color 2>/dev/null | grep 'https://' | sed 's| (.*||')
|
||||
|
||||
git for-each-ref --sort=-authordate "$heads" --format="%(refname:short)" \
|
||||
| while read -r branch_name; do
|
||||
review_url=$(echo "$CL_STATUS" | grep -E "\b${branch_name} :" | grep -o -E 'https://.*' | sed 's|https://||')
|
||||
|
||||
if [[ -n "$review_url" ]]; then
|
||||
# Using fancy integrated hyperlinks
|
||||
printf "$YELLOW%s $DIM\033]8;;%s\a%s\033]8;;\a$NC\n" "$branch_name" "https://$review_url" "$review_url"
|
||||
else
|
||||
printf "$YELLOW%s$NC\n" "$branch_name"
|
||||
fi
|
||||
done
|
||||
Loading…
Reference in a new issue