picatz / picatz/flowstate

tools/artifacts: the removal advice is shell-safe but not pathspec-safe — a tracked `:(glob)**` unstages more than it names

Open
#2,020 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

Problem

TestNoCompiledExecutableIsTracked renders one git rm --cached -- <path> line per tracked file that begins with executable magic, and the maintainer reading the failure is told to run it. picatz/flowstate#2017 fixed the shell half of that: the path is now a single-quoted shell word, so $(...), a backtick and $ are inert.

What remains is git's own layer. A word after -- is still a pathspec, and git reads pathspec magic inside it. Measured in a checkout at f1bada4:

$ git ls-files -- ':(glob)tools/**'   | wc -l
76
$ git ls-files -- ':(literal)tools/**' | wc -l
0

So a tracked file literally named :(glob)**, whose first four bytes are executable magic, renders advice that unstages far more than the one path it names. Shell quoting does not help, because the expansion is git's, not the shell's.

Scope

  • Consequence is bounded: --cached is index-only, git prints one line per file it removes, and git reset undoes it.
  • Not a regression from picatz/flowstate#2017 — the same hazard existed while the advice used strconv.Quote, and that change neither introduced nor widened it. It was found while reviewing that PR (flowstate-reviewer, optional finding) and is filed here rather than folded into it.
  • Reachability is the same shape as the shell bug: git ls-files reports whatever the checked-out revision holds, so the name can come from a proposed revision rather than from this repository.

Acceptance criteria

  • The advice a maintainer is told to paste removes exactly the paths the Found: list names, for every name git permits — including one beginning with : — or the check declines to print a command for a name it cannot render safely and says so.
  • A regression test drives a path whose name is pathspec magic and asserts the rendered line, in the mold of TestTheRemovalAdviceCannotRunWhatItNames.

Two candidate shapes

  1. ':(literal)' + path inside the quoted word. Precise per path, and visibly odd in the common case.
  2. GIT_LITERAL_PATHSPECS=1 once, in front of the printed command. One token for the whole block, and it makes every line in the block literal at the cost of a line that no longer looks like a bare git rm.

Neither is obviously right, which is why this is an issue rather than a follow-up commit.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with TestNoCompiledExecutableIsTracked and the related TestTheRemovalAdviceCannotRunWhatItNames regression mold, then inspect the tools/artifacts removal-advice entry point. Reproduce the pathspec-magic case and choose a rendering that makes each printed command affect exactly the paths in the Found list; the regression test should assert the rendered line.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, go
Domain
testing, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.