Repository navigation
CAMEL-26192: camel-git - checkout of an existing branch, and CamelGitCommitId is the sha of the commit - #28629
vignesh-manel wants to merge 6 commits into
Conversation
…CommitId is the sha of the commit The checkout operation always created the branch, so checking out a branch that exists failed with RefAlreadyExistsException. Without a tagName, it now creates the branch only when it does not exist. With a tagName, it still creates the branch from the tag. The commit consumer set CamelGitCommitId to the RevCommit, whose String form is not the sha, so the header could not be given to the cherryPick operation. It is now the sha as a String, as the header is documented. The consumer kept the header value to skip the commits it already consumed, so it now reads the sha back as an ObjectId. The commit and commitAll operations now set CamelGitCommitId to the sha of the new commit; the body is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. No issues found.
The three fixes are correct:
doCheckoutnow idempotently creates the branch only when it doesn't exist, fixingRefAlreadyExistsExceptionon existing branchesCamelGitCommitIdin the consumer is nowcommit.getName()(the 40-char SHA string), andObjectId.fromString()correctly reconstructs the sameObjectIdfor the deduplication set — the round-trip is consistentdoCommitanddoCommitAllnow capture and expose the returnedRevCommitSHA as a header
Breaking change is documented in the 4.24 upgrade guide. Tests properly validate the fixed behavior.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
|
🧪 CI tested the following changed modules:
🔁 3 tests passed only after a retry on JDK 21 (3 retried attempts) Recovered flaky tests on JDK 21 (3)
🔬 Scalpel shadow comparison — Scalpel: 8 of 708 tested, 24 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (4 direct + 7 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (24)
All tested modules (35 modules, 5m 9s total)Total reactor time: 5m 9s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Comment. The three fixes are correct, each has a test that would fail without it, and CI ran all four new tests; no code blocker, but the branch conflicts with main in the 4.24 upgrade guide and needs a rebase before it can merge.
Blockers: None. (The conflict in camel-4x-upgrade-guide-4_24.adoc needs a rebase, see 1; it is not a defect in the change.)
Other findings: 4 (1 rebase needed, 3 non-blocking suggestions).
Thank you for the careful work here, and for a description that shows the failures before the fix and the mutation the consumer test catches.
What works well
- Good catch that the consumer's de-duplication depends on the header type (
GitCommitConsumer.java:75-84): with only the header changed, every poll would deliver every commit again.commitIdIsTheShaTest(GitConsumerCommitTest.java:69-86) pins that withsetAssertPeriod(2000)instead of a sleep. - The existence check at
GitProducer.java:250is the same one JGit'sCreateBranchCommandruns (CreateBranchCommand.java:91in JGit 7.8.0, the version camel-git uses), so our check and JGit's never disagree on whether the branch exists. Leaving thetagNamepath as it was, so that a tag is never ignored, is a good call. - The commit consumer now matches the branch and tag consumers, which already put
ref.getObjectId().getName()in their header (GitBranchConsumer.java:49,GitTagConsumer.java:48). The newdirect:commit-all-operationroute (GitProducerTest.java:1003) givescommitAllits first real test, sincedirect:commit-all(:1002) usesoperation=commit.
Findings
1. [Important] Rebase needed: the upgrade guide conflicts with main at its last line.
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc is the only file that both this PR and main changed since the merge base 931bee0a80d1. The PR appends its section at the end of the file (lines 3872-3885 at head), and 012274993a9b (CAMEL-26135, #28576, merged today) appended === camel-spring - a CamelEvent only goes to the consumers of the same name at the same place, so GitHub reports a conflict. Merge commit 297f9d23 already resolved one conflict at this spot. The 4.24 guide has no fixed order, and several of today's PRs inserted their section mid-file (camel-stax, camel-csv, camel-sjms, camel-ftp). When we rebase, moving the camel-git section away from the end should stop it from colliding with every PR that appends to the guide, for example after === camel-github2 - the commit consumer routes the commits beyond the newest 100.
2. [Suggestion, non-blocking] Checkout of an existing branch picks a tag with the same name.
On the existing-branch path, GitProducer.java:251 passes the short name to git.checkout().setName(...). JGit's CheckoutCommand resolves that name through RefDatabase.SEARCH_PATH, where refs/tags/ comes before refs/heads/ (CheckoutCommand.java:220 and :256-258, RefDatabase.java:49-54, JGit 7.8.0). So when a tag has the same name as the branch, the checkout ends on a detached HEAD at the tag's commit. I ran these two calls in a small standalone JGit 7.8.0 program. With branch x at commit A and tag x at commit B, HEAD ended detached at B, and a following commit landed on no branch (neither x nor master moved). Before this PR, that case failed with RefAlreadyExistsException; now it succeeds without any error, on another commit. Passing the full ref name on this path fixes it (same program: HEAD on refs/heads/x at A):
boolean exists = repo.findRef(Constants.R_HEADS + endpoint.getBranchName()) != null;
if (exists) {
// the full name, so that a tag with the same name is not checked out instead
git.checkout().setName(Constants.R_HEADS + endpoint.getBranchName()).call();
} else {
git.checkout().setCreateBranch(true).setName(endpoint.getBranchName()).call();
}The creation path has to keep the short name, because CreateBranchCommand adds refs/heads/ itself (CreateBranchCommand.java:141). That path has the same ambiguity on main today: with a tag y and no branch y, it creates y at HEAD and then detaches at the tag. The other operations that switch branch with setCreateBranch(false).setName(branchName) (add, remove, commit, status, log, cherryPick, clean) share it too. A follow-up Jira looks like a better place for those than this PR.
3. [Suggestion] checkoutExistingBranchTest cannot tell the existing branch from HEAD.
In GitProducerTest.java:97-111 the branch is created on the commit master is on. So the final getBranch() check would also pass if checkout re-created the branch at HEAD, for example after a later change to a forced create. Moving master one commit ahead and comparing HEAD with the branch's commit closes that gap, and needs no new import:
template.sendBody("direct:create-branch", "");
String branchCommit = git.getRepository().resolve(branchTest).getName();
// move master one commit past the branch
File fileToAdd1 = new File(getGitDir(), filenameBranchToAdd);
fileToAdd1.createNewFile();
git.add().addFilepattern(filenameBranchToAdd).call();
git.commit().setMessage(commitMessageBranch).call();
template.sendBody("direct:checkout", "");
assertEquals(branchTest, git.getRepository().getBranch());
assertEquals(branchCommit, git.getRepository().resolve("HEAD").getName());Adding git.tag().setName(branchTest).call(); after the second commit would make this the regression test for point 2: it fails on the current head and passes with the full ref name.
4. [Suggestion] Upgrade guide: say that commit and commitAll replace the header.
GitProducer.java:348 and :385 overwrite any CamelGitCommitId already on the message. A route that consumes commits (type=commit) and then runs commit now sees the sha of the new commit instead of the consumed one. A route that sets the header for a later cherryPick (read at GitProducer.java:588-589) and runs a commit in between now cherry-picks the new commit. One sentence in the second paragraph of the section would cover both, for example: "The header replaces a CamelGitCommitId already on the message, such as the one set by the commit consumer."
Checked
Each fix has a test that would fail without it. The new tests use JUnit assertions like the rest of camel-git. There is no Thread.sleep and no fully qualified class name. Both copies of git.json and GitEndpointBuilderFactory are regenerated, and the uncommitted-changes check is green. The upgrade-guide entry is present, and the commit carries the Co-Authored-By trailer. CI run 40694050135 on b1da32b8 ran GitProducerTest (38 tests, 35 on main) and GitConsumerCommitTest (2 tests, 1 on main) with no failures. The earlier gnodet-bot review (on 297f9d23) reported no issues, and the only commit since then is the Regen commit; the points above add to it.
This is a rules-and-conventions review and not a substitute for static analysis tools.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ed by a tag of the same name JGit's checkout resolves a short name through the search path, where refs/tags comes before refs/heads, so a tag with the name of the branch was checked out instead (detached HEAD at the tag's commit). The existing branch is now checked out by its full name, refs/heads/<branchName>. checkoutExistingBranchTest now moves master one commit past the branch and tags it with the branch name, and checks that HEAD is the branch at its own commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc
…ommitId already on the message Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@oscerd thanks for the review, and for running the JGit case. All four points are done:
Locally, Claude Code on behalf of vignesh-manel |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Comment. Thanks @vignesh-manel, all four points of my earlier review are addressed on a24980b8, and CI is green.
Blockers: None
Other findings: none.
- Conflict: main is merged in, and the camel-git note now sits mid-file in the 4.24 guide, away from the appends at the end.
- Tag with the same name as the branch: the existing-branch path now checks out
Constants.R_HEADS + branchName(GitProducer.java), and the creation path keeps the short name, asCreateBranchCommandneeds. - Test:
checkoutExistingBranchTestnow movesmasterone commit past the branch and tags that commit with the branch's name. It then asserts both the branch name and that HEAD is the branch's commit. So it pins the existing-branch behaviour and the tag case together. - Upgrade guide: the note says that
commit/commitAllreplace aCamelGitCommitIdalready on the message.
The same short-name ambiguity in the other operations that switch branch (add, remove, commit, status, log, cherryPick, clean) is still a candidate for a follow-up Jira, outside this PR.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review: the points I raised are addressed, and CI is green on the current head.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Description
https://issues.apache.org/jira/browse/CAMEL-26192
Problem
GitProducer.doCheckoutalways calledsetCreateBranch(true), so thecheckoutoperation could only create a branch: checking out a branch that already exists failed withRefAlreadyExistsException.GitCommitConsumersetCamelGitCommitIdtocommit.getId(), which is theRevCommititself. ItsStringform iscommit <sha> <time> -----sp, not the sha, althoughGitConstantsdocuments the header withjavaType = "String". Giving the header to thecherryPickoperation, which reads it as aString, failed.commitandcommitAlldiscarded theRevCommitreturned by JGit, so a route could not know the id of the commit it had just made.Change
checkoutwithout atagNamecreates the branch only whenrefs/heads/<branchName>does not exist, and otherwise checks out the existing branch by its full name,refs/heads/<branchName>. Following the review, the full name is needed because JGit resolves a short name withrefs/tags/beforerefs/heads/, so a tag with the name of the branch would be checked out instead (detached HEAD at the tag's commit). With atagNameit still always creates the branch from the tag, as before (checking out an existing branch at a tag would silently ignore the tag). The create path keeps the short name, as JGit'sCreateBranchCommandaddsrefs/heads/itself. The same short-name ambiguity exists onmainfor the create path and for the other operations that switch branch withsetName(branchName); that is left for a follow-up, as the review suggests.CamelGitCommitIdtocommit.getName(), the sha as aString.onPreProcessed/onProcessedkept it only when it was anObjectId). It now reads the sha back withObjectId.fromString. Without that, each poll would deliver every commit again.commitandcommitAllsetCamelGitCommitIdto the sha of the new commit. The body is unchanged, as the endpoint's body-type metadata says for these operations.CamelGitCommitIdinGitConstantssays where the header is set and read;git.jsonis regenerated and copied to the catalog. The Javadoc of the header in the generatedGitEndpointBuilderFactory(endpoint DSL) changes with it: I missed that file, and the maintainer bot added it in the "Regen" commit b1da32b.commit/commitAll(which replaces aCamelGitCommitIdalready on the message) and thecheckoutbehaviour. Following the review, the entry is placed after the camel-github2 commit-consumer entry instead of at the end of the file, so later appends do not conflict with it.New tests:
GitProducerTest.checkoutExistingBranchTest: creates the branch withcreateBranch, movesmasterone commit past it and tags that commit with the branch name, then checks out the branch and checks that HEAD is the branch at its own commit.GitProducerTest.commitSetsCommitIdTestandcommitAllOperationSetsCommitIdTest: the header is the sha of the new commit. The second uses a new route withoperation=commitAll(the existingdirect:commit-allroute usesoperation=commit, socommitAllhad no test).GitConsumerCommitTest.commitIdIsTheShaTest: the header is the sha, and with a 2 s assert period (several polls) the commit is delivered only once.Verification (local, JDK 21, Maven 3.9.12)
RefAlreadyExistsException: Ref testBranch already exists,expected: <sha> but was: <null>(twice), andexpected: <sha> but was: <commit sha 1793186035 -----sp>.GitProducerTest(38) andGitConsumerCommitTest(2) pass.instanceof ObjectIdcheck inonProcessedmakescommitIdIsTheShaTestfail withExpected: <1> but was: <4>(the commit delivered again on each poll). The existingcommitConsumerTestdoes not catch that.GitConsumerTest(4) errors locally in its setup, on this branch and on unchanged main alike: its route loads a git config file from gist.githubusercontent.com, and the JVM getsjava.net.ConnectException: Operation timed outon this machine. I left it to CI.GitRemoteProducerTestis skipped.GitConsumerTesttests, which passed. The one retried test,MonitorStartupTestin camel-jbang-plugin-tui, is unrelated.checkoutExistingBranchTestfailed before the full-name change withexpected: <testBranch> but was: <31c83b3a…>(detached HEAD at the tag), and passes after.GitProducerTest(38) andGitConsumerCommitTest(2) pass, and sourcecheck is clean. main did not touch camel-git since the previous merge. CI on a24980b is pending.mvn install -Psourcecheck -DskipTestspasses incomponents/camel-git, with no uncommitted changes afterwards. The cataloggit.jsonis a byte-identical copy of the component one, as on main.Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.Not from the root: I built
components/camel-gitwith-Psourcecheck, and copied the regeneratedgit.jsonto the catalog (the catalog module could not resolve its dependencies locally). CI's uncommitted-changes check covers the rest.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code on behalf of vignesh-manel
🤖 Generated with Claude Code