Skip to content

Fix make git-update: time zones, memcheck, removed directories - #100

Closed
mernst wants to merge 3 commits into
codespecs:masterfrom
mernst:fix-git-update
Closed

mernst wants to merge 3 commits into
codespecs:masterfrom
mernst:fix-git-update

Conversation

@mernst

@mernst mernst commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

make git-update prints the git add/git rm commands for files that a Valgrind merge added or removed. It had three bugs:

  1. Time zone. It found files added upstream by the date 1969-12-31, which diff --unidirectional-new-file prints for the epoch only west of UTC. In UTC it prints 1970-01-01, so the target silently found no added files at all.
  2. Memcheck. Fjalar's copy of memcheck is valgrind/fjalar/, but the memcheck part printed add .../remove ... (not commands) with valgrind/memcheck/... paths. Its remove branch matched +++ ... 1969-12-31, which never occurs with --unidirectional-new-file.
  3. Removed directories. It printed plain git rm for directories that upstream removed, which fails without -r.

This PR is based on master and doesn't depend on any other open PR; it can merge in any order with #96–#101.

Changes

  • Match either epoch date.
  • The memcheck part uses the same patterns as the coregrind part, maps valgrind-old/memcheck/ to valgrind/fjalar/, and finds removals from Only in valgrind-old/memcheck... lines.
  • Use git rm -r.
  • Print one command per line, under comment headers (# Memcheck files added upstream; Fjalar may not need all of them, etc.), so the output is easier to review and still runs as a script.
  • Doc: "Tell Git about files that were added or removed" describes what the target prints and what to review.

Testing

Synthetic old/new upstream trees with files added, removed, and modified in both coregrind/ and memcheck/, a removed directory in each, and a new file under memcheck/tests/ (which memcheck.patch excludes). Patches were made with the same diff commands as generate-patch-files.sh, in both TZ=UTC and TZ=America/Los_Angeles.

  • Old target: in UTC, no git add at all; in Pacific time, add valgrind/memcheck/mc_added.c for memcheck and plain git rm for directories.
  • New target: identical, correct output in both time zones. Running it with bash -e in a toy Fjalar repository staged exactly the expected adds and removals, including both removed directories.
  • make doc builds.

Real upstream examples are rare: the last memcheck source file added was mc_main_asm.c (3.14.0), and the last removal was in 3.8.0. But the time-zone bug affects every added file.

- Files added upstream were found by the epoch date 1969-12-31, which is
  how diff prints it only west of UTC.  In UTC it prints 1970-01-01, so
  no added files were found.  Match either.
- The memcheck part printed "add ..."/"remove ...", which are not
  commands, with valgrind/memcheck/ paths instead of Fjalar's copy in
  valgrind/fjalar/; and its remove branch could never match.  Now it
  prints git add / git rm -r for valgrind/fjalar/ paths, using the same
  patterns as the coregrind part.
- Use git rm -r, because upstream sometimes removes a whole directory.
- Print one command per line, under comment headers, for easier review.
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

Or wait 53 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: codespecs/fjalar/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 26807e35-5ec3-41f1-b062-93151c68b997
📥 Commits

Reviewing files that changed from the base of the PR and between 4263218 and cc052c7.

📒 Files selected for processing: 5
  • Makefile
  • doc/valgrind-merge.texinfo
  • git-update.sh
  • plan-upgrade-valgrind.md
  • texinfo.dict
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Compare valgrind-old with valgrind-new directly instead of parsing patch
timestamps.  Copy binary files, which patch skips; comment out removals of
Fjalar's memcheck files; quote paths; fail if run in the wrong directory.
Document running the output with bash -ev.
@mernst

mernst commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Superseded by #103.

@mernst mernst closed this Oct 17, 2026
@mernst
mernst deleted the fix-git-update branch October 17, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant