change!: fall back to the raw text when ansi-c unquoting fails - #2891
Open
Amey Pawar (ameyypawar) wants to merge 3 commits into
Open
change!: fall back to the raw text when ansi-c unquoting fails#2891Amey Pawar (ameyypawar) wants to merge 3 commits into
Amey Pawar (ameyypawar) wants to merge 3 commits into
Conversation
`undo()` returned the input unaltered once it ran off the end while looking for the closing quote, so `"abc` unquoted to `abc` and reported all 4 bytes as consumed. Git's `unquote_c_style()` has no such branch and fails instead. A lone `"` already errored, so the two cases disagreed with each other. The arm was written before `undo()` reported consumed bytes; a052d79 added that feature by extending it with `consumed += input.len()`, treating the unterminated remainder as consumed rather than asking whether reaching the end should have succeeded at all.
Both callers of `gix_quote::ansi_c::undo()` turned a failure into an error of their own, while Git keeps the raw, still-quoted token as the value: * `parse_attr_line()` in `attr.c` falls through to its unquoted branch * Git's alternates parsing in `odb.c` spells the unterminated case out in a comment of its own For attributes this also covers invalid escapes, which were previously a hard error for the whole line. Git keeps `"\!x"` as the pattern, where the backslash goes on to escape the `!` for the matcher rather than negating the pattern. Both fall back before checking for the `[attr]` macro prefix, as Git does, so a line like `"[attr]x` stays a pattern on either side. The `Unquote` variants are unreachable now and have been removed.
The previous test reached `content()` through `alternate::resolve()`, which meant creating a directory whose name begins with the quote that is never closed. That is not a legal filename on Windows, where it failed with `InvalidFilename`. Calling `content()` directly tests the same parsing without a filesystem, and adds a companion case showing that intact quoting still decodes its escapes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created by Claude Code on behalf of Amey, who reviewed it before submitting. Everything below this line is the agent's writing, not his.
Summary
ansi_c::undo()now fails when a quote is never closed, instead of returning the rest of the input as if it had been unquoted.Error::Unquotevariants become unreachable and are removed.Git baseline
unquote_c_style()inquote.chas no branch for running out of input:strcspn(quoted, "\"\\")stops at the terminating NUL, and theswitch (*quoted++)that follows reachesdefault: goto error, returning -1.undo()instead treated a missing closing quote as success, so"abcunquoted toabcand reported all 4 bytes as consumed. A lone"already errored, so the two cases disagreed with each other.Both callers in this workspace are places where Git recovers rather than aborts:
The alternates parser in
odb.csays so outright — "Broken quoting (e.g., an entry that doesn't end with a quote) falls back to the unquoted case below". (That function isparse_alt_odb_entry()in 2.52.0 andparse_alternates()onmaster, so it is referred to by file here rather than by name.)Measured with
git check-attr, where the pattern is inferred from the file each line matches:.gitattributesline"abc"abc"abc"\!hello"\!then escapes the!"!hello""ab\qcd""abqcd""ab\tcd"ab<TAB>cdab<TAB>cdThe second row is why the invalid-escape case is included:
gix-attributesturned it into an error for the whole line, while Git keeps the pattern and the leading!never negates, because after the fallback the token still begins with a quote. Both implementations fall back before testing for the[attr]prefix, so"[attr]xstays a pattern on either side.This does not extend to every unquoting site in Git —
git checkout-index --stdinaborts withfatal: line is badly quotedon the same input — so the doc onundo()says callers differ rather than claiming a universal rule.Validation
A differential sweep of 28 quoting shapes, taking Git's own answer via
git check-attrrather than an expectation, goes from 12 divergences to 1. The harness was first run against an unmodified build to confirm it could detect the divergences at all, and corrected once when it scored dropped lines as agreement.The remaining row is
"\\", a pattern consisting of a single backslash. Both implementations produce that same pattern — the probe output is byte-identical before and after — and neither matches any file with it, so the oracle has no filename that can demonstrate agreement. It is a limit of matching-based comparison, not a divergence.To show the refactor in
parse_line()changes nothing it should not, 26 lines whose quoting succeeds or never begins were run through the old and new parsers: pattern text, mode,first_wildcard_pos, line number and every attribute assignment are byte-for-byte identical. The only behavioural difference is the failure path.Reverting the
gix-quotearm alone makes all three new tests fail.cargo test— gix-quote 17, gix-attributes 39, gix-odb 73, gix-pathspec 65, gix-dir 79, gix-worktree 10, gix-glob 40, gix 417cargo fmt --check— gix-quote, gix-attributes, gix-odbgix-ignoreis deliberately untouched: Git does not ansi-c unquote exclude patterns, so"quoted.txt"there ignores a file literally named"quoted.txt", and this crate already agrees.Notes
The change is split as
DEVELOPMENT.mdasks, with the breaking change togix-quotefirst and the two adaptations second.It is confined to parsing.
alternate::resolve()still applies its own handling to whatever path comes out, which is unchanged here.One interaction worth flagging: #2847 hand-writes
Display,Error::sourceandFromimpls for theUnquotevariant in bothgix-attributesandgix-odb, and carries aTODO(review)on thegix-odbone about it wrapping anExnthat does not implementstd::error::Error. Removing the variants makes those impls, and that question, unnecessary. Whichever lands first, the other side is a deletion rather than a conflict of intent.As for how the arm came to look like this: it was written before
undo()reported consumed bytes, and a052d79 added that feature a couple of hours later in the same PR by extending this arm withconsumed += input.len()— treating the unterminated remainder as consumed, rather than asking whether reaching the end should have succeeded at all.