Skip to content

fix(update): honor --ignore-cr-at-eol on git < 2.48 via --numstat - #75220

Closed
Zeraphim wants to merge 1 commit into
NousResearch:mainfrom
Zeraphim:fix/75175-crlf-churn
Closed

fix(update): honor --ignore-cr-at-eol on git < 2.48 via --numstat#75220
Zeraphim wants to merge 1 commit into
NousResearch:mainfrom
Zeraphim:fix/75175-crlf-churn

Conversation

@Zeraphim

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes _normalize_managed_eol (hermes_cli/update_cmd.py) so it actually
clears CRLF churn on managed checkouts when running on git < 2.48.

git diff --name-only --ignore-cr-at-eol silently ignores
--ignore-cr-at-eol on git < 2.48 (the flag is only honored on the
--numstat/--patch/--quiet output paths). As a result the
"real dirty" set equaled the full dirty set, _eol_only() was always
empty, and the core.autocrlf=false pin was written over a still-dirty
tree — the 5 regression tests from #74487 failed deterministically
(reproduced on git 2.43.0 / 2.45.2; CI passed only because it ran git 2.54).

This swaps the ignore-eol probe to --numstat, which honors the option on
every supported git version, and parses its path records. The non-ignore
path is unchanged.

Related Issue

Fixes #75175

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/update_cmd.py: in _normalize_managed_eol._dirty, use
    --numstat instead of --name-only when --ignore-cr-at-eol is
    requested, and parse the path fields from each numstat record.

How to Test

scripts/run_tests.sh tests/hermes_cli/test_update_eol_churn.py -q

Expected: 9 passed. On git < 2.48 (e.g. git 2.43/2.45) this was
5 failed / 4 passed before the fix.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation — or N/A (no user-facing behavior change)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact — or N/A (version-robust by construction; any platform/git)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Not needed — deterministic pytest fix.

`_normalize_managed_eol` derived the line-ending-only churn set from
`git diff --name-only --ignore-cr-at-eol`. On git < 2.48 `--name-only`
silently drops `--ignore-cr-at-eol`, so the "real dirty" set equaled the
full set, `_eol_only()` was always empty, and the
`core.autocrlf=false` pin was written over uncleared CRLF churn (5
regression tests from NousResearch#74487 failed).

Switch the ignore-eol probe to `--numstat`, which honors the option on
every supported git version, and parse its path records. The non-ignore
path is unchanged.

Closes NousResearch#75175
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #75213: both open PRs repair the same --name-only --ignore-cr-at-eol EOL-detection failure by using --numstat. Please consolidate the alternative implementations.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the --name-only --ignore-cr-at-eol failure; the premise is confirmed on current main at hermes_cli/update_cmd.py:3075-3090.

Problems

  • hermes_cli/update_cmd.py:3101 uses record.split("\t"). With -z, Git outputs literal pathname characters verbatim, including tabs, so this can split one pathname into fragments. The resulting incomplete real_dirty set feeds _eol_only() and can classify a real edit as checkout-safe, contrary to the preservation contract in tests/hermes_cli/test_update_eol_churn.py:118-127.
  • The comment at hermes_cli/update_cmd.py:3098-3100 describes renamed records as tab-delimited. Git's documented --numstat -z rename/copy form instead has an empty path field followed by NUL-delimited preimage and postimage paths; the loop ignores those records.

Suggested changes

  • Parse ordinary entries with only the first two tab splits, and parse the empty-path rename/copy form by consuming its following NUL path pair. Add tab-path and rename/copy preservation tests.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/update_cmd.py
# numstat -z records are "added\tdeleted\tpath" (renames add a
# fourth field); the path fields are everything after the two
# counts. Binary/unknown counts appear as "-".
fields = record.split("\t")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With -z, literal tabs in a pathname are verbatim, so splitting on every tab turns one path into fragments. Rename/copy output is also NUL-delimited after the counts, not a fourth tab field. Parse only the first two tabs for ordinary entries and consume the following NUL path pair for rename/copy entries; otherwise a real edit can enter eol_only and be checked out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_normalize_managed_eol does not clear CRLF churn — 5-test cluster fails (merged PR #74487 fails its own tests)

3 participants