fix(build-cli): Fix up-to-date branch check #23968
Conversation
There was a problem hiding this comment.
PR Overview
This PR fixes issues related to fetching and parsing Git branch references to determine branch up-to-dateness.
- In git.ts, the SHA is now parsed correctly from the raw output of git show-ref.
- The fetchBranch method call has been updated to pass the branch name directly instead of an array.
- The releasePrepChecks file has been updated to include the error stack in the error message for improved debugging.
Reviewed Changes
| File | Description |
|---|---|
| build-tools/packages/build-cli/src/library/git.ts | Corrects the SHA parsing logic and updates fetchBranch argument. |
| build-tools/packages/build-cli/src/library/releasePrepChecks.ts | Appends the error stack to the error message for remote branch check failures. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
build-tools/packages/build-cli/src/library/releasePrepChecks.ts:138
- [nitpick] Including the error stack in user-facing error messages can expose internal details. It is recommended to log the stack trace separately and show a concise error message to the user.
}
${(error as Error).stack}`
| @@ -465,7 +467,7 @@ export class Repository implements GitContext { | |||
| * Fetch branch | |||
| */ | |||
| public async fetchBranch(remote: string, branchName: string): Promise<void> { | |||
| await this.gitClient.fetch(remote, [branchName]); | |||
| await this.gitClient.fetch(remote, branchName); | |||
There was a problem hiding this comment.
Seems like we didn't catch this because there's a signature that can actually take a string array of options? Can we use the returned FetchResult to validate that what we want to happen actually happened? We know the branch name we want to fetch so I imagine that checking FetchResult.branches or FetchResult.updated could give us some confidence that this actually did what we want.
| @@ -165,9 +165,11 @@ export class Repository implements GitContext { | |||
| public async getShaForBranch(branch: string, remote?: string): Promise<string> { | |||
| const refspec = | |||
| remote === undefined ? `refs/heads/${branch}` : `refs/remotes/${remote}/${branch}`; | |||
| // result is a string of the form '64adcdba56deb16e0641c91ca825401a9f7a01f9 refs/heads/release/client/2.23' | |||
There was a problem hiding this comment.
I noticed this code has not changed in 2 years, and git show-ref also doesn't seem to have changed its output in the 2.x train of git... have we really been that lucky with this somehow still working?
If we only care about the sha, I'd suggest we use git show-ref --hash, and that we also validate that the result contains a single line to be completely sure the sha is actually for the thing we intend.
Fixes two related issues:
git show-refwere not parsed correctly, which caused failures when comparing remote refs to local refs, such as when determining if the local branch is up-to-date with the remote.Includes an opportunistic logging change to add the stack to an error message.