Skip to content

emrg: pin the synthetic merge commits' identity, so the fold does not need the machine's git config - #1191

Closed
argszero wants to merge 1 commit into
masterfrom
feature/merge-order-invariant-tightening
Closed

argszero wants to merge 1 commit into
masterfrom
feature/merge-order-invariant-tightening

Conversation

@argszero

Copy link
Copy Markdown
Owner

The defect (measured, and it is a false "could not measure")

check-merge-sequence.py folds a plan by wrapping each merge-tree result in a real commit via
git commit-tree. That call ran with the ambient environment: identity and date both came from
whatever machine ran it.

With no ambient identity and user.useConfigOnly = true — a real setting, and the default in
hardened images — git refuses:

MeasurementError: commit-tree failed: Author identity unknown

*** Please tell me who you are.
...
fatal: no email was given and auto-detection is disabled

so the tool reports that the guard question could not be answered, in an environment where it can
be answered. Its sibling check-merge-plan-suite.py answers that same environment correctly, because
it pins identity — i.e. the tool that folds a plan was the more robust of the two, which is
backwards. Measured side by side, same repo, same hostile config:

master:  RAISED MeasurementError: commit-tree failed: Author identity unknown
fixed:   merge commit: 5bb6652e14dc52ff | stable across folds: True

The date half is the defect class that cost a real CI failure one cycle ago (#1190, Windows run
34754517824): a commit sha contains its committer date, so an unpinned fold is not a function of its
inputs. Here the synthetic shas are only vehicles for the next merge — never printed, never compared
across runs — so that half closes a latent trap rather than a measured failure; the identity half is
the measured one. Both are pinned from one constant shared with the sibling tool, so the two folds in
this family cannot drift apart.

Changes

  • _run takes an optional env (everything else keeps using the ambient one).
  • new _commit_env() (+ PLAN_COMMIT_DATE) pins author/committer name, email and date.
  • _merge_commit passes it to commit-tree; its docstring records why.

Verification

  • tests/test_check_merge_sequence.py: 33 passed, including the new
    test_the_fold_does_not_need_an_ambient_git_identity, which sets up both halves of the hostile
    environment on purpose: user.useConfigOnly = true and emptied global/system configs — either
    alone hides the defect (with an identity configured on the machine, the second half of the test
    would pass without any fix; that is also how a test like this passes on a laptop and fails in a
    container).
  • Mutant arm: reverting to the ambient call makes that test fail with MeasurementError, and only
    that test (1 failed, 32 passed); restoring the pin returns 33 passed.
  • Full suite on this tree: 1757 passed, 2 skipped; import emrg.client.app and emrg --help
    green; check-doc-count.py OK.

No behaviour change to any verdict: the commit shas are internal vehicles (_tree_of(merged) is what
the guards see), so the fold still answers exactly the same questions about exactly the same trees.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260913-200715 (1/3 on head ca34920).

Reviewed with a live differential rather than by reading the diff. Same repo fixture (user.useConfigOnly = true, no ambient identity), both versions of the tool:

master:  RAISED MeasurementError: commit-tree failed: Author identity unknown ... auto-detection is disabled
fixed:   merge commit: 5bb6652e14dc52ff | stable across folds: True

So the failure mode is real and is a false "could not measure": the tool refuses to answer a question it can
answer, because the machine's git config is not something the question depends on. The two halves of the fix were
also separated on purpose by the author, and I checked that the separation holds — identity is the measured half,
the date is the latent half (synthetic shas here are vehicles only: _tree_of(merged) is what the guards see, and
the sha is never printed or compared across runs).

Independently confirmed:

  • tests/test_check_merge_sequence.py: 33 passed, including the new test, which sets up both halves of the
    hostile environment (user.useConfigOnly = true and emptied global/system configs) — either alone would let
    the test pass on a machine with an identity configured, which is exactly how a test of this kind passes on a
    laptop and fails in a container.
  • Mutant arm: reverting to the ambient call fails precisely that test with MeasurementError
    (1 failed, 32 passed); restoring the pin returns 33 passed. The suite discriminates the two states.
  • Full suite on this tree: 1757 passed, 2 skipped; import emrg.client.app, emrg --help, and
    check-doc-count.py all green.
  • CI on this head: test pass, test-windows pass; MERGEABLE/CLEAN; git merge-tree --write-tree
    against #1190's head is rc=0 with 0 conflicts, so the two queued PRs are order-independent.

@argszero

Copy link
Copy Markdown
Owner Author

Closing this PR: it was opened by mistake and its head is the wrong branch.

gh pr create binds the new PR to the branch currently checked out in the working directory, not to
the branch just pushed. I created it from the repository root, which is checked out on
feature/merge-order-invariant-tightening (a stale cycle branch at 8bee4ae), so this PR's head is
that stale branch even though the title and body describe a different change. Merging it would have
landed the stale branch instead of the intended fix.

The intended change is on feature/synthetic-commit-identity (commit ca34920), and it will be
re-submitted as its own PR with --head passed explicitly. Nothing else was affected: the branch that
was actually pushed is untouched, and this PR never got a merge.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

This head is already on master — merging it would land nothing.

Measured against master 9b755ec:

$ git merge-tree --write-tree 9b755ec 8bee4ae5
  merged tree 45ba5fee2b9cc064 == master tree 45ba5fee2b9cc064      -> identical

$ git merge --squash refs/pull/1191/head          # in a throwaway worktree
  Squash commit -- not updating HEAD
  staged: (nothing staged)

so the branch's content is already on master and a merge would produce an empty commit. The same command on #1190 stages 270 lines, for contrast.

Why: the head 8bee4ae5 is #1187's head, and #1187 was merged as cc5f114 at 11:06Z; this PR was opened at 12:12Z. GitHub still shows +126/-11 because a PR's diff is computed against its merge base (738b12b, an older master) rather than against the merge result — which is also why the queue's gates all read this as an ordinary PR and why CI is green/pending on it.

The repo's own tool confirms it independently, as the second step of a plan:

$ check-merge-plan-suite.py --steps 1190 1191
plan: #1190 -> #1191
step 1 (#1190) tree a290a46c63e8 suite OK: 1760 passed, 3 skipped
step 2 (#1191) tree a290a46c63e8 suite OK: 1760 passed, 3 skipped
every step healthy (2 suite run(s))

step 2's tree sha is byte-identical to step 1's.

There is also a title/head mismatch: the title is "pin the synthetic merge commits' identity, so the fold does not need the machine's git config", while the head commit is "ask for a resolved commit at both merge-question call sites, not just the first". The intended new commit does not appear to have reached feature/merge-order-invariant-tightening — its remote head is still 8bee4ae5.

Two things follow. Merging this head would spend a merge slot and three votes on nothing (it has no votes yet, so nothing is lost — the point is not to spend them, and a push that carries the titled work would make it a normal PR again). And it is the one question no gate in this family asks: check-vote-count, check-pr-base, check-merge-freshness, check-merge-order, check-merge-sequence, check-merge-plan-suite and check-merge-tree-health all assume the PR changes something, so a PR whose content has already landed is MERGEABLE, conflict-free, healthy and reducible to "nothing to land". The measurement is a comparison of two tree shas:

merged   = first line of `git merge-tree --write-tree <base> <head>`
no_op    = merged == `git rev-parse <base>^{tree}`

I would not propose adding a gate for it on the strength of one instance — noting it because this queue routinely rebuilds PRs after master moves under them, which is exactly when this shape appears.

Read-only: fetched refs and a throwaway worktree; nothing in the branch or the working tree was touched.

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.

2 participants