[pve-devel] [PATCH v6 pve-storage 0/1] FreeNAS storage plugin

[Fabian Grünbichler]

On Mon, Jun 19, 2017 at 08:28:57AM +0200, Michael Rasmussen wrote:
> Hi Fabian,
> If I understand you correctly you want a separate patch for every fix comment?
> 

no. please re-read the rather lengthy part of my e-mail in response to
v5 (quoted in full below).

a short summary:
- individual commits/patches should reflect logical, self-contained
  changes, together with a clear subject line and description
- if you fix something introduced in patch #1, update patch #1 and don't
  introduce a new commit/patch for the fix (see below for how!)
- if patches no longer make sense in their current form, split them up /
  combine them / re-organize them using the above criteria
- ideally, keep a changelog for the whole patch series (high level) and
  for individual patches (low level), depending on the amount of changes

for example, see the following three versions of a patch series by
Dominik:

v1: https://pve.proxmox.com/pipermail/pve-devel/2017-April/026164.html
v2: https://pve.proxmox.com/pipermail/pve-devel/2017-May/026241.html
v3: https://pve.proxmox.com/pipermail/pve-devel/2017-May/026302.html

note how between v1 and v2 a new patch was added, but otherwise the
changes were included in updated versions of the existing commits.

I know this feels awkward when introducing completely new code (it is
bascially one single file after all, so why split it up?), but it makes
both reviewing and integrating reviews so much easier because we can
focus on individual, easily digestable chunks of code at a time..

thanks!

> If/When you send a v6, please follow the following guide lines (to make
> both reviewing and incorporating the reviews easier):
> - don't change the cover letter subject when sending new versions of a
>   patch series, only bump the version (e.g., send your patch series as
>   "add FreeNAS storage plugin" "v2 add FreeNAS storage plugin" etc -
>   some of us rely on this to filter patch series, and every mail
>   deviating from this generates extra effort for manual processing)
> - don't add new patches fixing stuff in older patches, but split your
>   changes in a logical manner instead of chronologically
> 
> the latter means, instead of doing
> - big patch introducing plugin
> - fix issue one in previous commit
> - fix issue two in first commit
> - fix issue three in second commit
> - ...
> - fix issue seventeen in commit fourteen which attempted to fix issue
>   eight in commit five which attempted to fix issue four in commit four
>   which attempted to fix issue three in commit two which attempted to
>   fix issue one in huge first commit
> 
> you should do
> - add plugin file with name and options
> - add FreeNAS API access code
> - add OS iSCSI interaction code
> - implement foo
> - implement bar
> - add regression tests
> - ...
> 
> otherwise, I/we have to review huge changesets over and over, which is a
> PITA ;) bonus points for marking which patches have changed and which
> haven't, and giving a short summary of the changes between versions
> broken down by individual patches.
> 
> e.g., if you get a review saying "there is issue XYZ with your
> implementation of foo", you just need to update the (hopefully small)
> commit which introduced this issue. the usual way to do this is to have
> a branch for v1, and then when you get reviews you can check it out as
> v2, and either use fixup commits[1] or an editing rebase workflow[2] to
> incorporate changes. once you are happy with your v2, you can easily
> diff it with v1 (both are separate branches), generate a new patch
> series and re-send it as v2.
> 
> 1: "git commit --fixup COMMITYOUWANTTOFIX; git rebase -i --autosquash FIRSTCOMMITOFYOURPATCHSERIES^"
> 2: "git rebase -i FIRSTCOMMITOFYOURPATCHSERIES^" and following the instructions ;)
> 
> but of course, any other work flow that works for you is okay on your
> side, as long as the end result fits the criteria ;) note that it is
> okay to add new patches in a new version of a patch series if it makes
> sense (e.g., because between version x and y a new feature was added
> that warrants to get its own patch/commit, or because you split a bigger
> patch into two or more smaller ones).
> 
> keep in mind that the end result of the iterative code and review
> process should be a patch series that can be applied, and not a change
> log of how the final version of the code was iteratively improved.