Page MenuHomePhabricator

Add a check for unstaged changes when generating automated commits
ClosedPublic

Authored by jasonbcox on Sep 17 2020, 18:50.

Details

Summary

While this check is not particularly helpful when the automation runs
in production, this will help prevent blowing away changes by mistake when
testing this script locally.

It also turns out that the forced git checkout on master isn't really necessary anymore.
While land-patch.sh still assumes working on master, better separation of responsibilities
makes this script easier to test locally without specifying a given parent commit.

Test Plan
COMMIT_TYPE=dummy ./automated-commits.sh ./patch-recipes/update-aur-version.sh bitcoin-abc   # Still bails early since there was no change

echo "" >> CMakeLists.txt
COMMIT_TYPE=archive-release-notes ./automated-commits.sh  # should error instead of blowing away the change

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix requested changes to this revision.Sep 18 2020, 00:47
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
contrib/source-control-tools/automated-commits.sh
87 ↗(On Diff #23567)

Then why the git checkout dance? It seems superflous now.

This revision now requires changes to proceed.Sep 18 2020, 00:47
jasonbcox added inline comments.
contrib/source-control-tools/automated-commits.sh
87 ↗(On Diff #23567)

It's superfluous if PARENT_COMMIT is the default. But it makes sense to keep this when testing on a stack of diffs on your local machine.

contrib/source-control-tools/automated-commits.sh
87 ↗(On Diff #23567)

What problem does this solve?

jasonbcox added inline comments.
contrib/source-control-tools/automated-commits.sh
87 ↗(On Diff #23567)

While writing my response as to why it was necessary, I realized the assumptions within this script have changed. While land-patch.sh enforces master as the only branch it will work on, testing this script locally doesn't care since the change will not be landed anyway. It looks like I can cleanup the PARENT_COMMIT entirely.

Cleanup unneeded commit parent logic

jasonbcox edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Sep 21 2020, 23:30