feat: compare all branch authors when deciding if a branch is modified #21558

Keysox · 2023-04-17T16:43:34Z

Changes

Previously, when Renovate tried to determine if a branch was modified before force-pushing, it only looked at the last commit, which can result in losing commit history. This PR changes the behavior to compare all of the commits to the default branch.

Context

Pull Request was force-pushed even though the PR was edited and a rebase was not requested #19994

Documentation (please check one with an [x])

I have updated the documentation, or
No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

Code inspection only, or
Newly added/modified unit tests, or
No unit tests but ran on a real repository, or
Both unit tests + ran on a real repository

…don't have to use the non-null assertion operator

…d so we don't have to use the non-null assertion operator" This reverts commit 1786954.

… even when there are multiple base branches

…iption for more info on the special notation

…orking in jest

rarkins · 2023-05-23T08:08:13Z

Hi @Keysox thanks for resubmitting this functionality. Could you describe your confidence about whether the problem which occurred previously has been resolved and how?

viceice

last time it was totally broken, are those issues now resolved?

lib/util/git/index.ts

Keysox · 2023-05-23T16:06:24Z

@rarkins @viceice, the original one used git log --pretty=format:%ae origin/main...origin/multiple_commits and this one does git log --pretty=format:%ae origin/main..origin/multiple_commits (with two dots). https://git-scm.com/docs/git-log#_description has an explanation of the differences but testing locally with a few branches, the new behavior shows only the newly added commits to a branch whereas the other was a total difference, which no longer worked if a branch wasn't up to date since it would pull in every change.

Do you have any suggestions on how to add unit tests? The last PR had them but when I tried a month ago, I wasn't able to this time around since this new approach relied on the branch being pushed, which I wasn't able to figure out how to do with jest. Will poke at it again!

rarkins · 2023-09-16T07:53:16Z

@Keysox do you have the time to add the requested test?

lib/util/git/index.ts

Co-authored-by: Rhys Arkins <rhys@arkins.net>

renovate-release · 2023-10-17T09:01:37Z

🎉 This PR is included in version 37.26.0 🎉

The release is available on:

GitHub release
37.26.0

Your semantic-release bot 📦🚀

kindlich · 2023-10-17T11:06:15Z

lib/util/git/index.ts

+            await git.raw([
+              'log',
+              '--pretty=format:%ae',
+              `origin/${branchName}..origin/${baseBranch}`,


The syntax here is the wrong way.
This check means "all from baseBranch except those on branchName".

So basically this would return all commits that are on the base but NOT on the renovate-bot.

what would be the right command?

@viceice Either switch the operands

Suggested change

`origin/${branchName}..origin/${baseBranch}`,

`origin/${baseBranch}..origin/${branchName}`,

Or use the explicit

Suggested change

`origin/${branchName}..origin/${baseBranch}`,

`origin/${branchName} ^origin/${baseBranch}`,

Both should show all commits on the branchName (e.g. renovate/dep-abc), but without any commits on the baseBranch (e.g. main).

So if we were to have the following git tree (I hope the mermaid rendering is supported in all browsers):

gitGraph commit id: "A" commit id: "B" commit id: "C" branch renovate/x checkout main commit id: "D" commit id: "E" checkout renovate/x commit id: "X"

Then baseBranch would be main and branchName would be renovate/x
We'd probably only want to look at the commit X, but the way this MR was written would return the D and E

Then the result would be:

command result

main..renovate/x X ✅

renovate/x..main D, E ❌

renovate/x ^main X ✅

main ^renovate/x D, E ❌

Of course, if there were some rebasing done in between, it would be a bit different.
Let's say we rebased C, D, E on the main branch (if for whatever reason you were to rebase on main).
Or well, pretty much anything that rewrites history, like someone else force-pushing over someone else's changes.

gitGraph commit id: "A" commit id: "B" branch renovate/x checkout renovate/x commit id: "C" commit id: "X" checkout main commit id: "C - rebased" commit id: "D - rebased" commit id: "E - rebased"

Then the result would be:

command result

main..renovate/x C, X

renovate/x..main C - rebased, D - rebased, E - rebased

renovate/x ^main C, X

main ^renovate/x C - rebased, D - rebased, E - rebased

I don't know what the expected behaviour should be in the case the main was force-pushed, but you can at least use these tables as reference if you wanted this feature again 😉

rebase would be an issue now too. I'm not too worried about that. it shouldn't be a normal workflow with renovate. can we detect that?

maybe I'll try to create this PR again with proper tests

muffl0n · 2023-10-17T11:08:57Z

This just closed all our merge requests in Bitbucket Server. 🥲

   INFO: Comment added (repository=US/gcp-helmfile, branch=renovate/defaults-external-secrets-0.x)
         "prNo": 687,
         "topic": "Edited/Blocked Notification"

viceice · 2023-10-17T11:22:23Z

@rarkins revert again?

bobvandevijver · 2023-10-17T11:26:20Z

Yeah, renovate is marking all our open MRs as edited/blocked incorrectly as well with this update.

… modified (#21558)" This reverts commit c2cb93c.

rarkins · 2023-10-17T11:28:11Z

This just closed all our merge requests in Bitbucket Server. 🥲

   INFO: Comment added (repository=US/gcp-helmfile, branch=renovate/defaults-external-secrets-0.x)
         "prNo": 687,
         "topic": "Edited/Blocked Notification"

@mufflon it closed them? Or added a comment?

muffl0n · 2023-10-17T11:46:17Z

That was wrong: It did not close them, it posted this message. Sorry for the confusion!

rarkins · 2023-10-17T12:18:20Z

Now reverted, please upgrade to 37.26.1 once it's built and available. Sorry for the trouble everyone!

bobvandevijver · 2023-10-17T12:20:27Z

Now reverted, please upgrade to 37.26.0 once it's built and available. Sorry for the trouble everyone!

37.26.1 you mean😸 Thanks for the quick action! 👍🏻

rarkins · 2023-10-17T12:26:35Z

Yes, fixed now thanks!

ben-foxmoore · 2023-10-17T13:21:13Z

With the rollback, is there any straightforward way to recover PRs that have been marked in this way, without having to go through and checking each individual box?

rarkins · 2023-10-17T13:25:12Z

Renovate should treat them as unmodified now, although I'm not sure if it will remove those comments

ben-foxmoore · 2023-10-17T14:18:28Z

OK - as long as the comment itself doesn't have some significance to Renovate. I'll keep an eye on the affected PRs. Thanks @rarkins

chezsmithy · 2023-10-18T02:57:34Z

The MRs affected by this issue don't seem to be auto resolving with newer builds. Is anyone seeing anything different?

I'm trying to force a rebase now to get some of them unblocked.

rarkins · 2023-10-18T07:01:36Z

@chezsmithy please copy/paste the logs for one such branch.

My expectation is that the comment remains, but that Renovate should be back to its old functionality and consider the branch to be unmodified

muffl0n · 2023-10-18T07:34:09Z

Can confirm this: Renovatebot proceeds to update the PRs. The comment remains, though.

chezsmithy · 2023-10-18T20:02:22Z

@chezsmithy please copy/paste the logs for one such branch.

My expectation is that the comment remains, but that Renovate should be back to its old functionality and consider the branch to be unmodified

@rarkins Also seeing what @muffl0n is reporting. The comment remaining was the unexpected part.

renovatebot#21558) Co-authored-by: Rhys Arkins <rhys@arkins.net>

Keysox added 17 commits March 3, 2023 11:34

fix: isBranchModified now compares commit authors to defaultBranch

1f9eae1

Switch to using baseBranch instead of defaultBranch

b932f56

Update unit tests with isBranchModified's first parameter

98964bb

Update types for baseBranch/defaultBranch to be always defined so we …

1786954

…don't have to use the non-null assertion operator

Revert "Update types for baseBranch/defaultBranch to be always define…

bddb6c0

…d so we don't have to use the non-null assertion operator" This reverts commit 1786954.

Simplify branchIsModified logic

18f2597

Merge branch 'main' into 19994

df774b5

Refactor to for-of loop

adb0698

Use old isBranchModified logic if baseBranch is unknown

f679af9

Update types and improve debug message

a4c5dc7

Don't use baseBranch for cleanUpBranches since it's only called once,…

4f32999

… even when there are multiple base branches

Commit suggested changes

75ddd0b

Merge branch 'main' into 19994

62905b0

Merge branch 'main' into 19994

11bf388

Fixes renovatebot#21504, see https://git-scm.com/docs/git-log\#_descr…

ab74915

…iption for more info on the special notation

Revert test since '..' relies on branches being pushed, which isn't w…

64e8c96

…orking in jest

ignore coverage for if block since .. relies on a git push

6a513b2

Keysox marked this pull request as ready for review April 17, 2023 17:35

Merge branch 'main' into 19994

2a03f7d

viceice reviewed May 23, 2023

View reviewed changes

lib/util/git/index.ts Outdated Show resolved Hide resolved

Merge branch 'main' into 19994

0ae4756

Add test per @rarkins and @viceice

5794b9e

rarkins requested a review from viceice September 19, 2023 04:47

rarkins requested changes Sep 23, 2023

View reviewed changes

lib/util/git/index.ts Show resolved Hide resolved

rarkins requested changes Sep 23, 2023

View reviewed changes

lib/util/git/index.ts Show resolved Hide resolved

Keysox and others added 2 commits September 25, 2023 09:21

Apply suggestions from code review

362caee

Co-authored-by: Rhys Arkins <rhys@arkins.net>

pnpm run prettier-fix

6db9a13

newlineRegex per @viceice

3d486eb

Keysox dismissed rarkins’s stale review via 3d486eb October 16, 2023 15:14

viceice approved these changes Oct 16, 2023

View reviewed changes

viceice requested a review from rarkins October 16, 2023 15:21

Merge branch 'main' into 19994

3524e70

rarkins enabled auto-merge October 17, 2023 08:39

rarkins added this pull request to the merge queue Oct 17, 2023

Merged via the queue into renovatebot:main with commit c2cb93c Oct 17, 2023
36 checks passed

kindlich reviewed Oct 17, 2023

View reviewed changes

rarkins added a commit that referenced this pull request Oct 17, 2023

Revert "feat: compare all branch authors when deciding if a branch is…

10366ff

… modified (#21558)" This reverts commit c2cb93c.

rarkins mentioned this pull request Oct 17, 2023

fix: Revert "feat: compare all branch authors when deciding if a branch is modified" #25254

Merged

jon4hz pushed a commit to jon4hz/renovate that referenced this pull request Nov 9, 2023

feat: compare all branch authors when deciding if a branch is modified (

189dfab

renovatebot#21558) Co-authored-by: Rhys Arkins <rhys@arkins.net>

github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: compare all branch authors when deciding if a branch is modified #21558

feat: compare all branch authors when deciding if a branch is modified #21558

Keysox commented Apr 17, 2023 •

edited by viceice

rarkins commented May 23, 2023

viceice left a comment

Keysox commented May 23, 2023

rarkins commented Sep 16, 2023

renovate-release commented Oct 17, 2023

kindlich Oct 17, 2023

viceice Oct 17, 2023

kindlich Oct 17, 2023 •

edited

viceice Oct 17, 2023

viceice Oct 17, 2023

muffl0n commented Oct 17, 2023

viceice commented Oct 17, 2023

bobvandevijver commented Oct 17, 2023 •

edited

rarkins commented Oct 17, 2023

muffl0n commented Oct 17, 2023

rarkins commented Oct 17, 2023 •

edited

bobvandevijver commented Oct 17, 2023

rarkins commented Oct 17, 2023

ben-foxmoore commented Oct 17, 2023

rarkins commented Oct 17, 2023

ben-foxmoore commented Oct 17, 2023

chezsmithy commented Oct 18, 2023

rarkins commented Oct 18, 2023

muffl0n commented Oct 18, 2023

chezsmithy commented Oct 18, 2023 •

edited

	`origin/${branchName}..origin/${baseBranch}`,
	`origin/${baseBranch}..origin/${branchName}`,

	`origin/${branchName}..origin/${baseBranch}`,
	`origin/${branchName} ^origin/${baseBranch}`,

command	result
`main..renovate/x`	`X`	✅
`renovate/x..main`	`D, E`	❌
`renovate/x ^main`	`X`	✅
`main ^renovate/x`	`D, E`	❌

command	result
`main..renovate/x`	`C, X`
`renovate/x..main`	`C - rebased, D - rebased, E - rebased`
`renovate/x ^main`	`C, X`
`main ^renovate/x`	`C - rebased, D - rebased, E - rebased`

feat: compare all branch authors when deciding if a branch is modified #21558

feat: compare all branch authors when deciding if a branch is modified #21558

Conversation

Keysox commented Apr 17, 2023 • edited by viceice

Changes

Context

Documentation (please check one with an [x])

How I've tested my work (please select one)

rarkins commented May 23, 2023

viceice left a comment

Choose a reason for hiding this comment

Keysox commented May 23, 2023

rarkins commented Sep 16, 2023

renovate-release commented Oct 17, 2023

kindlich Oct 17, 2023

Choose a reason for hiding this comment

viceice Oct 17, 2023

Choose a reason for hiding this comment

kindlich Oct 17, 2023 • edited

Choose a reason for hiding this comment

viceice Oct 17, 2023

Choose a reason for hiding this comment

viceice Oct 17, 2023

Choose a reason for hiding this comment

muffl0n commented Oct 17, 2023

viceice commented Oct 17, 2023

bobvandevijver commented Oct 17, 2023 • edited

rarkins commented Oct 17, 2023

muffl0n commented Oct 17, 2023

rarkins commented Oct 17, 2023 • edited

bobvandevijver commented Oct 17, 2023

rarkins commented Oct 17, 2023

ben-foxmoore commented Oct 17, 2023

rarkins commented Oct 17, 2023

ben-foxmoore commented Oct 17, 2023

chezsmithy commented Oct 18, 2023

rarkins commented Oct 18, 2023

muffl0n commented Oct 18, 2023

chezsmithy commented Oct 18, 2023 • edited

Keysox commented Apr 17, 2023 •

edited by viceice

kindlich Oct 17, 2023 •

edited

bobvandevijver commented Oct 17, 2023 •

edited

rarkins commented Oct 17, 2023 •

edited

chezsmithy commented Oct 18, 2023 •

edited