Use granular commits?

[hrw]

I understand that Arm may want to develop ACS as inside project using separate repository.

But a way changes land in public repo could be improved. Granular commits changing single things make it easier to follow development.

Instead we get "this commit does X, Y, Z, updates typos and change (C) years" code drops.

This is one of reasons I am only reporting issues instead of doing pull requests.

[chetan-rathore]

Hi @hrw,

Based on your previous feedback received, ACS has recently made changes to the commit process for bug fixes (external or internal).
We are trying to have commits at a granular level. Each commit must have

• Issue id linking the issue for which changes are raised.
• Description explaining the changes
• Should only have changes relevant only to the issue for which it is raised.

Can you please share feedback on recent commits which can be further improved?

Your suggestions and "PR" are welcomed.

Thanks,
ACS

[hrw]

ae6d9fc7e6ed3d8cb80e1aba4ef0f0d26104ec5c is nice example:

S_L4PCI_2 test moved to SBSA L4

- Fix for issues https://github.com/ARM-software/sbsa-acs/issues/297 and https://github.com/ARM-software/sbsa-acs/issues/317.
- Moved test_p062 to L4 of SBSA under TEST_ID 601
- Included all device types in PCIe BDF table.
- PCIe test rule mapping fixes.
- PCIe BDF table used to recognise exercisers also.

issue #297 is one change
issue #317 is another change
each of next lines in commit message is also separate change

Summary info can be put into PR description and will end in merge commit.

I had own branches with changes in past and rebasing them over changes like above was always taking more time than needed.

[hrw]

917f63640f77a487d502bd2481573cde2922d635 was a nightmare one:

"Showing 182 changed files with 8,140 additions and 4,099 deletions."

 SBSA ACS 7.1.1 Updates:

- Baremetal support for SBSA 7.1 spec updates
  including RAS, PMU, MPAM
- Initialise SMMU eventq handler
- Change of order of modules run to sync with
  the order of modules run for BSA
- Better messaging and info details
- Removal of Exerciser tests common to SBSA and BSA
- Documentation and README updates

Signed-off-by: Sujana M <sujana.m@arm.com>

Co-authored-by: Chetan Rathore <chetan.rathore@arm.com>
Co-authored-by: Gowtham Siddarth <gowtham.siddarth@arm.com>
Co-authored-by: Srikar Josyula <srikar.josyula@arm.com>
Co-authored-by: Rajat Goyal <rajat.goyal@arm.com>
Co-authored-by: Amrathesh <amrathesh@arm.com>
Co-authored-by: Balaji Gontumukkala <balaji.gontumukkala@arm.com>
Co-authored-by: Sujana M <sujana.m@arm.com>

Eight persons worked on this...

[chetan-rathore]

Hi @hrw,

Thanks for sharing the commits.

Some comments from the ACS side. ACS has two types of commits.

1. Development commits ( ex 917f636 ), are currently upstreamed once a major internal milestone is reached, to make sure changes are stable.
   The commit 917f636 was Pre-Silicon SBSA 7.1 BETA release, hence it had bulk
   changes.
   A couple of improvements ACS is targeting for development commits are:

   1. Use a common fork/branch and every developer moves his changes directly in that fork so that it has a history of individual commits.
   2. Upstream any development changes after a BETA release directly to github.

2. Bug fix commits (ex ae6d9fc), these are upstreamed directly on github on an individual basis.

For issue #317, it was a very minor change of updating the RULE, that lead to the merging of this issue fix with the #297 fix.
In the future, this will be avoided.

Issue #297 needed PCIe infra changes, which lead to changes in most of PCIe tests and val files, but all the file changes are related to the issue fix.

We will discuss internally how to better upstream changes for bug fixes that result in more than 5-6 file changes.
Suggestions/feedback from your side is welcomed.

Thanks,
ACS team

[hrw]

The 917f636 was a commit which made me stop developing anything for ACS. Rebasing local changes against set of patches allows to go up step by step.

Rebasing against abominations like 917f636 are worthless as it is easier to just rewrite everything from scratch.