ci: Fix COMMIT_RANGE variable value for PRs by hebasto · Pull Request #20697 · bitcoin/bitcoin

hebasto · 2020-12-18T12:19:20Z

This PR:

is a ci: Move linter task to cirrus #20658 and ci: Install missing lint packages #20682 followup
set the COMMIT_RANGE variable correctly for PRs
cleans up Travis-specific code
prints COMMIT_RANGE value to the log for convenience as it was in Travis CI

maflcko

this still "fails" because master doesn't exist when the repo was cloned with go. I think creating the master branch after git fetch will do.

maflcko · 2020-12-18T12:29:21Z

ci/lint/06_script.sh

+  COMMIT_RANGE="$CIRRUS_BASE_BRANCH..HEAD"
  test/lint/commit-script-check.sh $COMMIT_RANGE
+else
+  COMMIT_RANGE="$(git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_LAST_GREEN_CHANGE && git merge-base $CIRRUS_LAST_GREEN_CHANGE HEAD)..HEAD"


we don't run the linters on any branch because it is too late to fixup any issues that have been merged

Correct. But this is for forked repos and personal Cirrus accounts.

test/lint/lint-git-commit-check.sh and test/lint/lint-whitespace.sh work in all scenarios now.

Maybe submit this as a separate pull? ;)

Thanks! Done.

ci/lint/05_before_script.sh

maflcko · 2020-12-18T13:20:07Z

ci/lint/06_script.sh

+  COMMIT_RANGE="$CIRRUS_BASE_BRANCH..HEAD"
  test/lint/commit-script-check.sh $COMMIT_RANGE
+else
+  COMMIT_RANGE="$(git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_LAST_GREEN_CHANGE && git merge-base $CIRRUS_LAST_GREEN_CHANGE HEAD)..HEAD"


Maybe submit this as a separate pull? ;)

ci/lint/05_before_script.sh

DrahtBot · 2020-12-19T03:25:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

hebasto · 2020-12-20T12:19:31Z

Updated 144250e -> 83c07d1 (pr20697.03 -> pr20697.04):

addressed @MarcoFalke's comment:

Maybe submit this as a separate pull? ;)

See: #20728.

addressed @MarcoFalke's comment:

needs to be guarded by if [ -n "$CIRRUS_PR" ]; then

added printing of the COMMIT_RANGE variable value to the log as it was in Travis CI

maflcko

ACK

maflcko · 2020-12-21T07:06:55Z

ci/lint/06_script.sh

-  # by a pull request this is the name of the branch targeted by the pull request.
-  # https://cirrus-ci.org/guide/writing-tasks/#environment-variables
-  COMMIT_RANGE="$CIRRUS_BRANCH..HEAD"
+  git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_BASE_SHA


Is this needed?

Actually, it replaces the git fetch command from the removed ci/lint/05_before_script.sh

Oh, I see. The CIRRUS_BASE_SHA must already be in the commit history for pulls, right?
I'll push another change without this command.

Thanks! Updated.

hebasto · 2020-12-21T09:24:48Z

Updated 83c07d1 -> b4cd93d (pr20697.04 -> pr20697.05, diff):

addressed @MarcoFalke's comment
fixed COMMIT_RANGE value when a changed PR is force pushed into the base branch that has already grown, i.e., the pull is effectively based on the non-top commit of the base branch
added git log command to the lint task log

maflcko · 2020-12-21T09:55:07Z

ci/lint/06_script.sh

-  # by a pull request this is the name of the branch targeted by the pull request.
-  # https://cirrus-ci.org/guide/writing-tasks/#environment-variables
-  COMMIT_RANGE="$CIRRUS_BRANCH..HEAD"
+  COMMIT_RANGE="$(git merge-base $CIRRUS_BASE_SHA $CIRRUS_CHANGE_IN_REPO)..$GIT_HEAD"


Why is this changed again? Now it will include master commits, because master has already been merged in for pull requests (see .cirrus.yml)

COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" was correct.

... master has already been merged in for pull requests (see .cirrus.yml)

Right.

For described case "when a changed PR is force pushed into the base branch that has already grown, i.e., the pull is effectively based on the non-top commit of the base branch" I think COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" is wrong, as $CIRRUS_BASE_SHA is not the commit on which the pull is based on.

$CIRRUS_BASE_SHA is not the commit on which the pull is based on.

It is the commit that this pull is de-facto based on, assuming that git merge A is approximately doing the same like git rebase A.

As I mentioned previously the current version is broken and will include all commits that have been pushed to master since. See:

https://cirrus-ci.com/task/5396234322051072?command=lint#L931

In .cirrus.yml the base branch is merged into the pull one. That's why I think that listed commits are correct COMMIT_RANGE to lint.

OTOH, we could merge the pull branch into the base one. In that case $CIRRUS_BASE_SHA will work as documented.

That's why I think that listed commits are correct COMMIT_RANGE to lint.

It is not the way it worked on travis

Agree.

Is it acceptable to s/git merge A/git rebase A/ ?

I've tested COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD". It works for all cases.
git is smart enough to know what to do :)

But for a human the value of COMMIT_RANGE is a bit cryptic (regarding to pull commits) in cases of a pull based on the non-top commit of the base branch. So going to skip output of it.

Is it acceptable to s/git merge A/git rebase A/ ?

In theory yes, in practise it doesn't work with subtree bumps.

maflcko · 2020-12-21T11:07:00Z

ACK 3c2478c

3c2478c ci: Print COMMIT_RANGE to the log as it was in Travis CI (Hennadii Stepanov) c123892 ci: Drop Travis-specific workaround for shellcheck (Hennadii Stepanov) 10af252 ci: Drop Travis-specific way to set COMMIT_RANGE variable (Hennadii Stepanov) 93504da ci: Fix COMMIT_RANGE variable value for PRs (Hennadii Stepanov) Pull request description: This PR: - is a #20658 and #20682 followup - set the `COMMIT_RANGE` variable correctly for PRs - cleans up Travis-specific code - prints COMMIT_RANGE value to the log for convenience as it was in Travis CI ACKs for top commit: MarcoFalke: ACK 3c2478c Tree-SHA512: beb933352b10fd5eb3e66373ddb62439e4f3a03b50fb037ee89fa92c0706cec41d05f2d307f15bb18d1e634e6464f4e123b7e2f88703c8edfd145d8d6eff0b1a

hebasto · 2020-12-21T11:10:15Z

Updated b4cd93d -> f89d374 (pr20697.05 -> pr20697.07, diff):

addressed @MarcoFalke's comment:

Why is this changed again? Now it will include master commits, because master has already been merged in for pull requests (see .cirrus.yml)

COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" was correct.

maflcko · 2020-12-21T11:11:48Z

This was merged two seconds before you force pushed 😅

hebasto · 2020-12-21T11:12:32Z

This was merged two seconds before you force pushed sweat_smile

slow typing :)

3c2478c ci: Print COMMIT_RANGE to the log as it was in Travis CI (Hennadii Stepanov) c123892 ci: Drop Travis-specific workaround for shellcheck (Hennadii Stepanov) 10af252 ci: Drop Travis-specific way to set COMMIT_RANGE variable (Hennadii Stepanov) 93504da ci: Fix COMMIT_RANGE variable value for PRs (Hennadii Stepanov) Pull request description: This PR: - is a bitcoin#20658 and bitcoin#20682 followup - set the `COMMIT_RANGE` variable correctly for PRs - cleans up Travis-specific code - prints COMMIT_RANGE value to the log for convenience as it was in Travis CI ACKs for top commit: MarcoFalke: ACK 3c2478c Tree-SHA512: beb933352b10fd5eb3e66373ddb62439e4f3a03b50fb037ee89fa92c0706cec41d05f2d307f15bb18d1e634e6464f4e123b7e2f88703c8edfd145d8d6eff0b1a

hebasto mentioned this pull request Dec 18, 2020

ci: Install missing lint packages #20682

Merged

hebasto marked this pull request as draft December 18, 2020 12:24

maflcko reviewed Dec 18, 2020

View reviewed changes

hebasto force-pushed the 201218-range branch from 7f8643b to 9d17a14 Compare December 18, 2020 12:33

maflcko reviewed Dec 18, 2020

View reviewed changes

ci/lint/05_before_script.sh Outdated Show resolved Hide resolved

hebasto force-pushed the 201218-range branch from 9d17a14 to 144250e Compare December 18, 2020 13:08

maflcko reviewed Dec 18, 2020

View reviewed changes

DrahtBot added the Tests label Dec 18, 2020

This was referenced Dec 18, 2020

ci: Fix COMMIT_RANGE variable value for cloned repos #20716

Closed

ci: Fix COMMIT_RANGE variable value for cloned repos #20717

Closed

hebasto mentioned this pull request Dec 20, 2020

ci: Provide COMMIT_RANGE variable for non-PRs #20728

Closed

hebasto force-pushed the 201218-range branch from 144250e to 83c07d1 Compare December 20, 2020 12:03

hebasto marked this pull request as ready for review December 20, 2020 12:22

hebasto changed the title ~~ci: Fix COMMIT_RANGE variable value~~ ci: Fix COMMIT_RANGE variable value for PRs Dec 20, 2020

maflcko approved these changes Dec 21, 2020

View reviewed changes

hebasto force-pushed the 201218-range branch from 83c07d1 to b4cd93d Compare December 21, 2020 09:16

maflcko reviewed Dec 21, 2020

View reviewed changes

hebasto force-pushed the 201218-range branch from b4cd93d to 3c2478c Compare December 21, 2020 11:02

hebasto added 4 commits December 21, 2020 13:07

ci: Fix COMMIT_RANGE variable value for PRs

1b16ac3

ci: Drop Travis-specific way to set COMMIT_RANGE variable

46c96d6

ci: Drop Travis-specific workaround for shellcheck

31cb012

ci: Print COMMIT_RANGE to the log as it was in Travis CI

f89d374

hebasto force-pushed the 201218-range branch from 3c2478c to f89d374 Compare December 21, 2020 11:09

maflcko closed this Dec 21, 2020

bitcoin locked and limited conversation to collaborators Feb 22, 2021

Conversation

hebasto commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Dec 20, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 21, 2020

Uh oh!

hebasto commented Dec 21, 2020

Uh oh!

maflcko commented Dec 21, 2020

Uh oh!

hebasto commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hebasto commented Dec 18, 2020 •

edited

Loading

hebasto Dec 18, 2020 •

edited

Loading

DrahtBot commented Dec 19, 2020 •

edited

Loading

hebasto Dec 21, 2020 •

edited

Loading