fix: preserve branch names with "/" in switch_branch - #4486
Conversation
Parsing ls-remote with split("/")[-1] dropped path prefixes, so
branches like cursor/foo were not found. Use refs/heads/ instead.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Server docker: perconalab/pmm-server-fb:PR-4486-f0dfac2 |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/6801/ |
|
Server docker: perconalab/pmm-server-fb:PR-4486-86ad073 |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/7113/ |
|
Server docker: perconalab/pmm-server-fb:PR-4486-5f2f717 |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/7132/ |
| branches = check_output('git ls-remote --heads origin'.split(), cwd=path) | ||
| branches = [line.split("/")[-1] for line in branches.decode().strip().split("\n")] | ||
| branches = [ | ||
| line.split("refs/heads/", 1)[-1] |
There was a problem hiding this comment.
No assignment here? How can you do that in a programming language?
There was a problem hiding this comment.
It's Python Generator Expressions: https://www.geeksforgeeks.org/python/python-list-comprehensions-vs-generator-expressions
But I changed it to be more explicit now
There was a problem hiding this comment.
The question was not about whether this works or not, I was rather saying "the code should be readable and make sense for anybody". We don't have maintainers specializing in Python.
There was a problem hiding this comment.
yep, second that. Changed it to regular syntax
a5d6955 to
8abaa26
Compare
…ch_branch Addresses review feedback that the comprehension's line-before-for ordering was confusing to read.
8abaa26 to
1e7d532
Compare
|
Server docker: perconalab/pmm-server-fb:PR-4486-a5d6955 |
|
Server docker: perconalab/pmm-server-fb:PR-4486-1e7d532 |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/7138/ |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/7139/ |
|
Server docker: perconalab/pmm-server-fb:PR-4486-8abaa26 |
|
API tests have succeded: https://pmm.cd.percona.com/job/pmm3-api-tests/7141/ |
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
The fix itself is correct — line.split("refs/heads/", 1)[-1] keeps cursor/foo intact, where the old split("/")[-1] cut it down to foo. I tried the whole flow with a slash branch in a test repo and it works.
A few things below land outside the changed lines, so I couldn't attach them to the diff. Only the first one would hold up a merge, and the second one is fixed by the same change.
1. ci.py:282 — the checkout can quietly pick an old copy of the branch
git checkout cursor/foo prefers a local branch of that name if one already exists. On a Jenkins agent that reuses its workspace, one usually does exist, left behind by an earlier build. So the fetch on the line above updates our copy of the remote branch, and then the checkout jumps to the stale local branch and ignores it.
I reproduced this: sources/<dep> had a local cursor/foo at 7688c74 from a previous build, the fetch moved the remote copy to 6edef74, and after the checkout git itself said:
Your branch is behind 'origin/cursor/foo' by 1 commit
Nothing corrects it later. get_deps() runs git pull --ff-only before switch_branch, so it refreshes the branch we're leaving, not the one we're switching to. The build then compiles the old code, and the commit id written into .git-sources is the old one — which is the id we use to tell what a build actually contained.
Fix — point the local branch at what we just fetched:
check_call(f'git checkout -B {branch} origin/{branch}'.split(), cwd=path)2. ci.py:282 — the same line crashes when a branch name looks like a file path
If the branch name also matches a file in the checkout, git refuses to guess which one you meant. Branch names with slashes are exactly the ones that can look like paths, so this PR makes it more likely to come up.
With a branch called api/PMM-1.txt and a file of the same name in the repo, the fetch succeeds and then the checkout fails:
fatal: 'api/PMM-1.txt' could be both a local file and a tracking branch.
Please use -- (and optionally --no-guess) to disambiguate
check_call turns that into an exception nobody catches, so the job ends in a Python traceback instead of the clear error message and sys.exit(1) you have a few lines below. The checkout -B version above avoids the guessing entirely.
3. ci.py:284 — small typo in the error message
logging.error(f'Can\' find branch: {branch} in {path}')The apostrophe sits one character too far left, so the log reads Can' find branch: cursor/foo. That's the line people search for in Jenkins logs (and the one the PR description quotes as Can't find branch), so searching for it doesn't find it:
logging.error(f"Can't find branch: {branch} in {path}")4. ci.py:287 — the newline on the returned commit id is doing real work
This returns the output of git rev-parse HEAD with its newline still attached, and the caller quietly depends on that. ci.py:210 writes export ..._commit={commit_id} without adding a newline of its own, so that leftover newline is the only thing separating one line of .git-sources from the next.
Every other check_output in this file calls .strip(), so this looks like an oversight and someone will eventually "tidy" it. If they do, .git-sources becomes export pmm_commit=<sha>export pmm_branch=main. Jenkins loads that file with source, so both values end up wrong and nothing reports an error. Worth either stripping here and adding \n where it's written, or adding a short comment saying why it isn't stripped.
| if cur_branch != branch: | ||
| branches = check_output('git ls-remote --heads origin'.split(), cwd=path) | ||
| branches = [line.split("/")[-1] for line in branches.decode().strip().split("\n")] | ||
| raw_branches = check_output('git ls-remote --heads origin'.split(), cwd=path) |
There was a problem hiding this comment.
Stepping back: git can answer "does this branch exist?" on its own, so none of this listing and splitting is really needed.
if call(f'git ls-remote --heads --exit-code origin refs/heads/{branch}'.split(), cwd=path) == 0:--exit-code returns 0 if the branch exists and 2 if it doesn't (I checked). That replaces the list, the loop and the in branches test, and there's no text to parse, so the bug this PR fixes can't happen in the first place.
It's also a lot less work: right now we ask the server for the full list of branches once per dependency (13 in .gitmodules), and for a repo like percona/grafana that's thousands of branch names downloaded to check one.
Not blocking — just worth knowing the whole block could go away.
| branches = [line.split("/")[-1] for line in branches.decode().strip().split("\n")] | ||
| raw_branches = check_output('git ls-remote --heads origin'.split(), cwd=path) | ||
| branches = [] | ||
| for line in raw_branches.decode().strip().split("\n"): |
There was a problem hiding this comment.
The whitespace check on the next line and the value stored two lines down don't match: the check looks at line.strip(), but what gets appended is the original line. Anything after the branch name comes along for the ride.
>>> line = "def456\trefs/heads/main\r"
>>> bool(line.strip()) # passes the check below
True
>>> line.split("refs/heads/", 1)[-1]
'main\r' # stored like this, and 'main\r' != 'main'So the branch is reported as missing even though it's right there — the same wrong outcome this PR is fixing, except a stray \r is invisible in the log, so the name looks correct.
The .strip() on this line doesn't cover it either: it only trims the very start and end of the whole output, so only the last line would be cleaned up.
If stray whitespace is worth guarding against, the stored value needs cleaning too — line.strip().split(...), or use .splitlines() here, which handles Windows-style line endings for you.
| raw_branches = check_output('git ls-remote --heads origin'.split(), cwd=path) | ||
| branches = [] | ||
| for line in raw_branches.decode().strip().split("\n"): | ||
| if not line.strip(): |
There was a problem hiding this comment.
Flip side of the comment above: if stray whitespace can't happen, this check isn't needed at all.
.strip() on the previous line has already removed anything around the edges, and git ls-remote doesn't print blank lines in the middle. So this can only trigger for the single empty string you get from completely empty output — and an empty entry couldn't have caused a wrong match anyway, exactly as before this PR.
So it's one or the other: either clean the stored value (comment above), or drop this check plus the raw_branches variable and keep the original one-liner:
branches = [line.split("refs/heads/", 1)[-1] for line in check_output(...).decode().strip().split("\n")]Right now we pay for the check but don't get the protection.
| for line in raw_branches.decode().strip().split("\n"): | ||
| if not line.strip(): | ||
| continue | ||
| branches.append(line.split("refs/heads/", 1)[-1]) |
There was a problem hiding this comment.
[-1] means a line we don't recognise gets stored as if it were a branch name:
>>> "some unexpected output".split("refs/heads/", 1)[-1]
'some unexpected output'That goes into branches, never matches anything, and the script exits with the same confusing Can' find branch: <name> this PR is trying to get rid of — with nothing pointing at the real cause.
Skipping such lines outright makes it obvious:
if "refs/heads/" not in line:
continue
branches.append(line.split("refs/heads/", 1)[1])Worth doing because the output isn't fixed forever: git 2.46 renamed --heads to --branches and now treats --heads as the old spelling.
3 participants
Summary
switch_branch()inci.pyso feature-build branches whose names contain/(e.g.cursor/foo) are found correctly.git ls-remoterefs were parsed withsplit("/")[-1], which dropped the path prefix and made branch lookup fail.Tests
/(e.g.cursor/...) and confirmci.pyswitches to that branch withoutCan't find branch.: PMM-15071 #4483