-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[rush] Change getChangedFiles to pass ref only in git diff #4678
[rush] Change getChangedFiles to pass ref only in git diff #4678
Conversation
This is to avoid git determining merge base of given ref and current head
The most common scenario for In a typical CI environment, there will be a merge commit at Most likely the |
Is there a reason you can't clone with a depth of two? |
@dmichon-msft I see... that makes sense
@dmichon-msft @iclanton Yes I'm doing a shallow clone of depth 1 in CI build. It is not that I couldn't do a clone with depth of two, it is just that 2 is an arbitrary number... In a long lived branch, it can be 5, 10, or even 100 until it finds the "merge base". So I'm trying hard to avoid that, since the repo that I worked on has a huge commit history (like 100,000+ commits) In the case of my CI build, since I already have the commit hash of the merge base (passed by the CI executor), it would be a waste to clone the repo with more history just for the tooling to determine the merge base again. Is there a possibility of something like: if the given "target-branch" value is a commit hash, use a different getChangedFiles that calls |
The value of 2 is based on the CI mechanism used by both Azure DevOps and GitHub for PR builds, that they create a merge commit parented on the tip of the target branch and run the pipeline against that, therefore the first parent of the current commit is always the tip of the target branch as of the time the build was queued. Creating that merge commit is a prerequisite for queueing PR builds, since if it can't be created, the source branch can't be successfully merged into the target branch. |
I see, thanks for the explanation |
This is to avoid git determining merge base of given ref and current head
Summary
Related to #4589 but for
rush change --verify --no-fetch --target-branch <ref>
In CI, I have the SHA of merge base given by the CI runner at env variable (
$CI_MERGE_REQUEST_DIFF_BASE_SHA
), so I run the following command.However that will fail with the following output:
Details
I figured out that the failing codes was here in the getChangedFiles function:
rushstack/libraries/rush-lib/src/logic/Git.ts
Lines 277 to 294 in 49d93a9
From my understanding from reading
git diff
docs and this StackOverflow question, to retrieve a list of changed files between the given ref (branch/commit/tag/etc) and current HEAD, I do not need to retrieve the merge base (and hence do not need to be in the style ofA...B
which Git will determine the merge base)From my investigation of Rush's history, it seemed like it was added this way since 2016, so I'm not sure what's the intention there:
4a4de9a#diff-26c82d4767d5297bb1f9e2d6c4329b25a0f60cd6b99c5d8d59c4ac7e14a04320R22
How it was tested
Manual test: I build Rush locally with this change and able to get that command (
rush change --verify --no-fetch --target-branch <<REDACTED_MERGE_BASE>>
) working on my local repro.Impacted documentation