fix(dist-git): Walk parent branches to handle merge commits#187
Closed
Tonisal-byte wants to merge 1 commit into
Closed
fix(dist-git): Walk parent branches to handle merge commits#187Tonisal-byte wants to merge 1 commit into
Tonisal-byte wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes synthetic history generation for dist-git branches that contain cross-branch merge commits (discovered with the systemtap package on f43, where import-commit is a merge and upstream-commit lives on the merged-in branch). The previous first-parent walk from detached HEAD could fail with "import-commit not found" or follow the wrong lineage; this PR replaces it with a committer-time-ordered all-parent traversal starting from origin/<distGitBranch>, matching git log ordering.
Changes:
- Threads a new
distGitBranchparameter throughWithGitRepo→sourcePreparerImpl→CommitInterleavedHistory→collectUpstreamCommits, and resolves the walk start viaorigin/<branch>(falling back to HEAD). - Replaces the manual first-parent walk with
object.NewCommitIterCTime(all parents, committer-time order) and stops viastorer.ErrStoponceimportCommitis reached. - Updates and reshapes the merge-commit test (now
TestCommitInterleavedHistory_MergeCommitCrossBranch), adds a regression test (TestCommitInterleavedHistory_UpstreamOnMergedBranch) that mirrors the systemtap graph, and removes the priorTestCommitInterleavedHistory_SingleCommit.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/app/azldev/core/sources/synthistory.go | Switches walk algorithm; adds distGitBranch parameter and resolveWalkStart helper. |
| internal/app/azldev/core/sources/synthistory_test.go | Updates existing tests for new signature; adds cross-branch and systemtap regression tests; removes single-commit test. |
| internal/app/azldev/core/sources/sourceprep.go | Adds distGitBranch to WithGitRepo option and propagates to CommitInterleavedHistory. |
| internal/app/azldev/cmds/component/render.go | Passes distro.Version.DistGitBranch to WithGitRepo. |
| internal/app/azldev/cmds/component/preparesources.go | Same — propagates resolved per-component dist-git branch. |
| internal/app/azldev/cmds/component/build.go | Same — propagates resolved per-component dist-git branch. |
Comment on lines
+680
to
+713
| // Use committer-time-ordered traversal (all parents). This matches | ||
| // 'git log' ordering and visits commits from merged-in branches. | ||
| iter := object.NewCommitIterCTime(startCommit, nil, nil) | ||
| defer iter.Close() | ||
|
|
||
| var ( | ||
| commits []*object.Commit | ||
| foundUpstream bool | ||
| foundImport bool | ||
| collecting = upstreamCommit == "" // if no upper bound, collect from start. | ||
| currentHash = head.Hash() | ||
| ) | ||
|
|
||
| for { | ||
| commit, err := repo.CommitObject(currentHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read commit %#q:\n%w", currentHash.String(), err) | ||
| } | ||
|
|
||
| err = iter.ForEach(func(commit *object.Commit) error { | ||
| hash := commit.Hash.String() | ||
|
|
||
| // Start collecting once we see the upstream-commit (newest boundary). | ||
| if !collecting && hash == upstreamCommit { | ||
| if hash == upstreamCommit { | ||
| collecting = true | ||
| foundUpstream = true | ||
| } | ||
|
|
||
| if collecting { | ||
| commits = append(commits, commit) | ||
| } | ||
|
|
||
| if hash == upstreamCommit { | ||
| foundUpstream = true | ||
| } | ||
|
|
||
| // Stop once we reach the import-commit (oldest boundary). | ||
| if importCommit != "" && hash == importCommit { | ||
| foundImport = true | ||
|
|
||
| break | ||
| } | ||
|
|
||
| // Follow only the first parent to stay on the mainline. | ||
| if len(commit.ParentHashes) == 0 { | ||
| break | ||
| return storer.ErrStop | ||
| } | ||
|
|
||
| currentHash = commit.ParentHashes[0] | ||
| return nil | ||
| }) |
Comment on lines
+746
to
+772
| func resolveWalkStart(repo *gogit.Repository, distGitBranch string) (plumbing.Hash, error) { | ||
| if distGitBranch != "" { | ||
| refName := plumbing.NewRemoteReferenceName("origin", distGitBranch) | ||
|
|
||
| ref, err := repo.Reference(refName, true) | ||
| if err == nil { | ||
| slog.Debug("Resolved dist-git branch tip for history walk", | ||
| "branch", distGitBranch, | ||
| "ref", refName, | ||
| "hash", ref.Hash()) | ||
|
|
||
| return ref.Hash(), nil | ||
| } | ||
|
|
||
| slog.Debug("Could not resolve remote branch ref; falling back to HEAD", | ||
| "branch", distGitBranch, | ||
| "ref", refName, | ||
| "reason", err) | ||
| } | ||
|
|
||
| head, err := repo.Head() | ||
| if err != nil { | ||
| return plumbing.ZeroHash, fmt.Errorf("failed to get HEAD reference:\n%w", err) | ||
| } | ||
|
|
||
| return head.Hash(), nil | ||
| } |
| @@ -231,81 +231,6 @@ func TestCommitInterleavedHistory_Interleaved(t *testing.T) { | |||
| assert.Contains(t, logCommits[3].Message, "upstream: v1.0") // import-commit (kept) | |||
| } | |||
|
|
|||
| } | ||
|
|
||
| err = sources.CommitInterleavedHistory(repo, changes, "") | ||
| err = sources.CommitInterleavedHistory(repo, changes, "", "") |
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.
Problem
When the upstream dist-git branch (e.g. f43) contains merge commits from other
branches (e.g. f44), the synthetic history generation could fail or produce
incorrect results:
If import-commit was a merge commit and upstream-commit was resolved to a
commit on the merged-in branch, the old first-parent walk from the detached
HEAD could not reach import-commit at all, erroring with "import-commit not
found in dist-git history".
Even when both commits were on the same first-parent lineage, merge commits
whose first parent led to a different branch caused the walk to follow the
wrong lineage.
This was discovered with the systemtap package on f43, where import-commit
86f8849 ("Merge branch 'rawhide' into f43") and upstream-commit a5c5bd
("Rebuilt for Fedora 44") are on diverged lineages that only converge at the
branch tip.
Fix
Replace the manual first-parent walk with a committer-time-ordered all-parent
traversal (NewCommitIterCTime) starting from the dist-git branch tip
(origin/) instead of the detached HEAD. This matches
git logorderingand is consistent with how rpmautospec counts commits for %autorelease.