Skip to content

Move firmware_version to protocol/#74

Merged
cjevans-google merged 5 commits intogoogle:mainfrom
stevenportley:fw_version
Feb 3, 2025
Merged

Move firmware_version to protocol/#74
cjevans-google merged 5 commits intogoogle:mainfrom
stevenportley:fw_version

Conversation

@stevenportley
Copy link
Collaborator

This PR de-couples the EC_CMD_GET_VERSION wrapper from htool and moves it to protocol/. It also adds a setup adding mocks to libhoth_device for testing protocols.

@cjevans-google
Copy link
Collaborator

Meta-nitpick: The commits in a pull request should either be squashed (ie, "fix meson build"), or else each commit should complete and consistent in itself. I suspect this PR should be just one commit.

@xorptr
Copy link
Collaborator

xorptr commented Jan 31, 2025

Meta-nitpick: The commits in a pull request should either be squashed (ie, "fix meson build"), or else each commit should complete and consistent in itself. I suspect this PR should be just one commit.

@rkr35 mentioned that this repository uses squash-then-merge strategy for merging PR changes. So I thought it was okay to have multiple commits when PR is under review (and I followed the same approach for my PRs since I prefer not to use force push).

@cjevans-google
Copy link
Collaborator

Both strategies are enabled -- I mildly prefer to rebase-then-merge, as it results in slightly cleaner history [1], and authors can arrange their commit sequence exactly as it should appear in the history.

[1] Example: 751413a for PR #73 shows part of the commit message for "fix meson project", which is meaningless after the PR is reviewed and merged.

@stevenportley
Copy link
Collaborator Author

Both strategies are enabled -- I mildly prefer to rebase-then-merge, as it results in slightly cleaner history [1], and authors can arrange their commit sequence exactly as it should appear in the history.

[1] Example: 751413a for PR #73 shows part of the commit message for "fix meson project", which is meaningless after the PR is reviewed and merged.

I've always preferred squash-then-merge. In my person opinion, if you feel the need to have multiple commits included in upstream they should probably be separate independently reviewed PRs. For that "fix meson project", I should have updated the merge commit to remove it but I overlooked that. I generally do not like squashing then force-pushing to public branches after review as started since it messes with the history and make following changes to a PR as its reviewed more difficult.

@stevenportley stevenportley force-pushed the fw_version branch 3 times, most recently from 19e64e8 to 1c6132c Compare February 1, 2025 01:27
@cjevans-google
Copy link
Collaborator

Fwiw, I'm more concerned about the readability of the git commit history (which we'll refer to in the distant future), than the convenience of review in github's review tool (which does not handle force-push gracefully). An argument for another time.

@cjevans-google cjevans-google merged commit c854e47 into google:main Feb 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants