From 66e979390465363ccf1f27b8771a4a701bc4adeb Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 30 Dec 2025 09:28:57 +0000 Subject: [PATCH] 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. --- findings.md | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++ git-recent | 73 ++++++++++++++++++++++++++------------- git-recent-cl | 32 ++++++++++++++++++ 3 files changed, 176 insertions(+), 23 deletions(-) create mode 100644 findings.md create mode 100755 git-recent-cl diff --git a/findings.md b/findings.md new file mode 100644 index 0000000..ff5d8b4 --- /dev/null +++ b/findings.md @@ -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 +``` diff --git a/git-recent b/git-recent index e6084e7..2f6c04e 100755 --- a/git-recent +++ b/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 diff --git a/git-recent-cl b/git-recent-cl new file mode 100755 index 0000000..b4c8fba --- /dev/null +++ b/git-recent-cl @@ -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