feat(git)!: determine branch modification based on all branch commits #28225

AaronMoat · 2024-04-04T00:34:30Z

Changes

Extract all authors from the git log of the changes in the branch for a given PR against master and use those in isBranchModified

Context

Currently, the documentation for gitIgnoredAuthors states:

Specify commit authors ignored by Renovate.

By default, Renovate will treat any PR as modified if another Git author has added to the branch. When a PR is considered modified, Renovate won't perform any further commits such as if it's conflicted or needs a version update. If you have other bots which commit on top of Renovate PRs, and don't want Renovate to treat these PRs as modified, then add the other Git author(s) to gitIgnoredAuthors.

The current implementation, however, only looks at the latest commit. If you have a flow like:

Renovate commit
Human commit
Bot commit (e.g. formatting)

This will mean isBranchModified returns false and the rebase flow throws away commits 2 and 3.

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

CLAassistant · 2024-04-04T00:34:36Z

All committers have signed the CLA.

lib/util/git/index.ts

Co-authored-by: Sam Chung <[email protected]>

rarkins

We had a couple of attempts at this same concept and both times it caused unintended consequences causing it to be rolled back. It's important we understand why that happened and how this fresh attempt avoids the same problem before merging

AaronMoat · 2024-04-04T13:21:55Z

Good call @rarkins I hadn't thought about checking closed issues/discussions so I hadn't seen that context.

I have found #21505 - this comment stands out to me #21504 (comment) - and I support the assessment because the attempt used origin/${branchName}...origin/${baseBranch}. This is wrong: it includes all commits that are different between the two branches, not just the new ones. https://stackoverflow.com/a/24186641/16823396

Are you able to find what the other issue was?

AaronMoat · 2024-04-04T13:26:24Z

Ah, was this the second time? https://github.com/renovatebot/renovate/pull/21558/files#r1361930608

I think this attempt also doesn't have that problem.

That being said, if it's been wrong twice, who's to say it can't be a third time. What do you recommend to gain confidence?

RahulGautamSingh · 2024-04-04T15:06:53Z

That being said, if it's been wrong twice, who's to say it can't be a third time. What do you recommend to gain confidence?

This PR does things correctly I think ie. instead of the wrong way origin/${branchName}...origin/${baseBranch} it uses the correct one origin/${baseBranch}...origin/${branchName}.

In both the previous attempts the wrong way was merged. I think some real runs will help us gain confidence.
If you can first test all the cases which made the previous attempts fail , then we can discuss from there.

Some example test runs:

simple update PR (up-to-date with base branch and no other commits except bot's)
simple update PR (behind base branch and no other commits except bot's)
modified update PR
conflicted update PR
rebased update PR but no external commits
rebased updated PR but with external commits (ie. a modified PR which has been rebased after user requested)
feat: compare all branch authors when deciding if a branch is modified #20739 (comment)

Some other mixes like simple update PR with commits from ignored authors.

AaronMoat · 2024-04-06T04:56:25Z

This is the test cases without using the gitIgnoredAuthors configuration.

✅ Up to date, clean PR happily ignored (1.run-on-update-pr.log) AaronMoat/renovate-testing@85e6cfe
✅ PR that's behind is happy ignored (2.run-on-update-pr-when-behind.log) or rebased with behind-base-branch (2.behind-base-with-rebasewhen-behind-base-branch.log - AaronMoat/renovate-testing@d95d200)
✅ PR that's been modified successfully gets the "Edited/Blocked Notification". renamed - Update dependency graphql to v16.8.1 AaronMoat/renovate-testing#2 (comment) (3.modified-pr.log)
✅ rebased (4.conflicted.log, AaronMoat/renovate-testing@460a58f)
✅ Update dependency axios to v1.6.8 - recreate AaronMoat/renovate-testing#4 commit remade and force pushed. See logs 👇
✅ Update dependency chalk to v5.3.0 - recreate AaronMoat/renovate-testing#5 same, custom changes thrown away.
5-6.rebase.log
✅ It shows as modified: Update dependency graphql to v16.8.1 - abandoned AaronMoat/renovate-testing#3 (comment) (7.github-ui-update.log)

I will do some testing with the gitIgnoredAuthors configuration once we're satisfied that this hasn't broken any of the standard behaviour.

lib/util/git/index.ts

RahulGautamSingh · 2024-04-06T15:56:44Z

Sorry, I'm a bit confused. What does rebased update PR mean?

In update PR created by the bot. There is a checkbox present which user can check to rebase the PR.
[] If you want to rebase/retry this PR, check this box

Reading: https://docs.renovatebot.com/updating-rebasing/

AaronMoat · 2024-04-07T04:57:31Z

Cool, rebase is fine too (updated prior comment).

As well,

Update dependency koa to v2.15.2 AaronMoat/renovate-testing#7 -> committed with a bot author and ran, got isModified = false (and no "blocked" message), gitIgnoredAuthor-latest.log
Update dependency chalk to v5.3.0 - recreate AaronMoat/renovate-testing#5 -> committed with a human then a bot, got isModified = true (and a "blocked" message),
gitIgnoredAuthor-with-human-then-bot.log

This all seems to be working as expected for the cases I have tested.

lib/modules/platform/local/scm.ts

lib/util/git/index.ts

lib/modules/platform/types.ts

rarkins

Make baseBranch mandatory

Removes fallback to checking depName for all matchPackageX and excludePackageX rules. BREAKING CHANGE: matchPackageNames and related functions no longer fall back to checking depName. Rewrite packageRules to use matchDepNames instead.

lib/util/git/index.ts

rarkins

Does this make or require any change to caching?

…for tag lookups (renovatebot#28400) Changes default Docker Hub lookups from index.docker.io to hub.docker.com, which is more efficient. If you are configuring a Docker Hub token for docker.io then you should now configure it for docker.com as well. Closes renovatebot#24666 BREAKING CHANGE: Docker Hub lookups prefer hub.docker.com over index.docker.io. Set RENOVATE_X_DOCKER_HUB_TAGS_DISABLE=true in env to revert behavior.

AaronMoat · 2024-04-21T22:32:50Z

Hmm, good question on caching changes.

One potential thing is that the cache is solely based on the branch name and not the base branch. I guess if you now call isBranchModified for multiple different base branches then you could get into trouble? Is there a use case for this? I'll take your guidance on if we need to fix or not.

lib/util/git/index.ts

rarkins · 2024-04-22T10:18:07Z

If the cashing is based on branchName + commitSha then it might already work fine. If a branch SHA was "unmodified" on a previous run then it shouldn't change to modified even if the base branch has received commits. There's potentially an edge case where a modified one changes to unmodified due to creative git use but in that case they can request a rebase/retry

AaronMoat · 2024-04-23T10:49:02Z

Thanks all for the work on this 🙏 is someone happy to merge for me? Looks like I can't myself 😄

…#28225)

Make ignoredAuthors analyse all commits, not latest

abc1758

AaronMoat added 2 commits April 4, 2024 11:37

Remove FIXME

bf5894b

Remove other changes

487f6cf

samchungy reviewed Apr 4, 2024

View reviewed changes

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

Update lib/util/git/index.ts

df66c06

Co-authored-by: Sam Chung <[email protected]>

AaronMoat changed the title ~~Make ignoredAuthors analyse all commits, not latest~~ fix: Make ignoredAuthors analyse all commits, not latest Apr 4, 2024

AaronMoat added 2 commits April 4, 2024 20:34

Add a test for this case

15eb9e4

100% coverage

eb39cfb

AaronMoat requested a review from samchungy April 4, 2024 09:55

Merge branch 'main' into ignoredAuthors-all-commits

d6be5cb

AaronMoat marked this pull request as ready for review April 4, 2024 09:55

rarkins requested changes Apr 4, 2024

View reviewed changes

AaronMoat requested a review from rarkins April 6, 2024 04:56

viceice reviewed Apr 6, 2024

View reviewed changes

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

Pass undefined explicitly

5f278c0

Fix logging

a693607

Merge branch 'main' into ignoredAuthors-all-commits

d8207e4

AaronMoat requested a review from viceice April 8, 2024 23:27

viceice reviewed Apr 9, 2024

View reviewed changes

lib/modules/platform/local/scm.ts Show resolved Hide resolved

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

lib/modules/platform/types.ts Outdated Show resolved Hide resolved

rarkins changed the title ~~fix: Make ignoredAuthors analyse all commits, not latest~~ fix(git): Make ignoredAuthors analyse all commits, not latest Apr 13, 2024

Merge branch 'main' into ignoredAuthors-all-commits

fa9c292

rarkins requested changes Apr 13, 2024

View reviewed changes

Make baseBranch mandatory

32df340

RahulGautamSingh and others added 3 commits April 21, 2024 12:08

feat(logging)!: initialize file logging via env (renovatebot#28104)

dd59178

Merge branch 'v38' into ignoredAuthors-all-commits

750b685

rarkins reviewed Apr 21, 2024

View reviewed changes

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

rarkins reviewed Apr 21, 2024

View reviewed changes

Merge branch 'v38' into ignoredAuthors-all-commits

c80f000

rarkins requested changes Apr 22, 2024

View reviewed changes

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

rarkins force-pushed the v38 branch from c550a3d to 3e6fd6d Compare April 22, 2024 07:36

AaronMoat added 2 commits April 22, 2024 19:17

Move details to trace

9b01e3c

Merge branch 'v38' into ignoredAuthors-all-commits

89fe338

AaronMoat requested review from rarkins and viceice April 22, 2024 09:17

rarkins approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'v38' into ignoredAuthors-all-commits

cfbf0d8

viceice approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'v38' into ignoredAuthors-all-commits

f1684ca

rarkins merged commit 2ff1569 into renovatebot:v38 Apr 23, 2024
34 checks passed

rarkins pushed a commit that referenced this pull request Apr 26, 2024

feat(git)!: determine branch modification based on all branch commits (…

988a609

…#28225)

rarkins pushed a commit that referenced this pull request Apr 29, 2024

feat(git)!: determine branch modification based on all branch commits (…

73df318

…#28225)

rarkins pushed a commit that referenced this pull request May 3, 2024

feat(git)!: determine branch modification based on all branch commits (…

9a36287

…#28225)

rarkins pushed a commit that referenced this pull request May 4, 2024

feat(git)!: determine branch modification based on all branch commits (…

6a73ec3

…#28225)

rarkins pushed a commit that referenced this pull request May 5, 2024

feat(git)!: determine branch modification based on all branch commits (…

58494a9

…#28225)

rarkins pushed a commit that referenced this pull request May 6, 2024

feat(git)!: determine branch modification based on all branch commits (…

148d871

…#28225)

rarkins pushed a commit that referenced this pull request May 11, 2024

feat(git)!: determine branch modification based on all branch commits (…

e7596ad

…#28225)

rarkins pushed a commit that referenced this pull request May 19, 2024

feat(git)!: determine branch modification based on all branch commits (…

917bc18

…#28225)

github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(git)!: determine branch modification based on all branch commits #28225

feat(git)!: determine branch modification based on all branch commits #28225

AaronMoat commented Apr 4, 2024 •

edited

Loading

CLAassistant commented Apr 4, 2024 •

edited

Loading

rarkins left a comment

AaronMoat commented Apr 4, 2024

AaronMoat commented Apr 4, 2024 •

edited

Loading

RahulGautamSingh commented Apr 4, 2024 •

edited

Loading

AaronMoat commented Apr 6, 2024 •

edited

Loading

RahulGautamSingh commented Apr 6, 2024 •

edited

Loading

AaronMoat commented Apr 7, 2024

rarkins left a comment

rarkins left a comment

AaronMoat commented Apr 21, 2024

rarkins commented Apr 22, 2024

AaronMoat commented Apr 23, 2024

feat(git)!: determine branch modification based on all branch commits #28225

feat(git)!: determine branch modification based on all branch commits #28225

Conversation

AaronMoat commented Apr 4, 2024 • edited Loading

Changes

Context

Documentation (please check one with an [x])

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

CLAassistant commented Apr 4, 2024 • edited Loading

rarkins left a comment

Choose a reason for hiding this comment

AaronMoat commented Apr 4, 2024

AaronMoat commented Apr 4, 2024 • edited Loading

RahulGautamSingh commented Apr 4, 2024 • edited Loading

AaronMoat commented Apr 6, 2024 • edited Loading

RahulGautamSingh commented Apr 6, 2024 • edited Loading

AaronMoat commented Apr 7, 2024

rarkins left a comment

Choose a reason for hiding this comment

rarkins left a comment

Choose a reason for hiding this comment

AaronMoat commented Apr 21, 2024

rarkins commented Apr 22, 2024

AaronMoat commented Apr 23, 2024

AaronMoat commented Apr 4, 2024 •

edited

Loading

CLAassistant commented Apr 4, 2024 •

edited

Loading

AaronMoat commented Apr 4, 2024 •

edited

Loading

RahulGautamSingh commented Apr 4, 2024 •

edited

Loading

AaronMoat commented Apr 6, 2024 •

edited

Loading

RahulGautamSingh commented Apr 6, 2024 •

edited

Loading