[land-bot] Add a sanity rebase after running smoke tests
ClosedPublic
Actions

Authored by jasonbcox on Sep 17 2020, 00:24.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC12d862c0dffa: [land-bot] Add a sanity rebase after running smoke tests

Summary

This ensures that any non-colliding changes pushed upstream within the few minute
window that smoke tests were running will not break the git push. While we do make
the assumption that tests still pass after this, breaking changes from automated
diffs should be exceedingly rare. For human-authored changes, we have a land queue to
help mitigate this.

This is copied from automated-commits.sh to bring this script up to
par with the sanity checking we already do there. The next step is to cleanup
automated-commits.sh so that it calls land-patch.sh directly.

Depends on D7467 and D7911

Test Plan

cmake -GNinja -DENABLE_SOURCE_CONTROL_TOOLS_TESTS=ON ..
ninja check-source-control-tools

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Sep 17 2020, 00:24

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 17 2020, 00:24

jasonbcox requested review of this revision.Sep 17 2020, 00:24

Harbormaster completed remote builds in B12763: Diff 23543.Sep 17 2020, 00:48

I don't see what problem this solves. The description doesn't explains this.

This revision now requires changes to proceed.Sep 17 2020, 13:03

jasonbcox requested review of this revision.Sep 17 2020, 15:33

jasonbcox edited the summary of this revision. (Show Details)

This does not solve the problem mentioned, because there might be more changes in between the rebase and the push. This is fundamentally broken and cannot be fixed with this approach.

If git push cannot handle this, then you are bound to rebase in a loop for as long as push is non fast forward - but still fail if the push fails for other reasons, or if the rebase fails. However I'd be very surprised if git did not include a way to do this properly already.

This revision now requires changes to proceed.Sep 17 2020, 15:49

You want something like:

do {
  PUSH_OUTPUT=$(git push --porcelain)
  PUSH_SUCCESS=$?
  if ($PUSH_SUCCESS) {
    exit(SUCCESS)
  }
} while (echo "$PUSH_OUTPUT" | grep -qF "To gitRepoURL ! refs/heads/master:refs/heads/master [rejected] (non-fast-forward) Done");

exit(FAILURE)

jasonbcox mentioned this in D7911: [land-bot] Add basic test coverage.Oct 13 2020, 22:07

Make it more robust and add tests (now depends on D7911)

jasonbcox edited the summary of this revision. (Show Details)Oct 13 2020, 22:20

jasonbcox edited the test plan for this revision. (Show Details)

jasonbcox added a parent revision: D7911: [land-bot] Add basic test coverage.

Harbormaster completed remote builds in B13388: Diff 24606.Oct 13 2020, 22:39

jasonbcox mentioned this in rABC3b4e698c7e84: [land-bot] Add basic test coverage.Oct 14 2020, 16:29

deadalnix accepted this revision.Oct 16 2020, 16:29

This revision is now accepted and ready to land.Oct 16 2020, 16:29

Closed by commit rABC12d862c0dffa: [land-bot] Add a sanity rebase after running smoke tests (authored by jasonbcox). · Explain WhyOct 16 2020, 17:27

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rABC12d862c0dffa: [land-bot] Add a sanity rebase after running smoke tests.