Re: [PATCH for-4.17 v3 07/15] CODING_STYLE(tools/ocaml): add 'make format' and remove tabs

[Julien Grall <julien@xxxxxxx>]

On 08/11/2022 17:02, Edwin Torok wrote:
> > On 8 Nov 2022, at 16:03, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 08/11/2022 15:33, Edwin Török wrote:
> > > See CODING_STYLE: Xen uses spaces, not tabs.
> > > * OCaml code:
> > > Using `ocp-indent` for now to just make minimal modifications in
> > > tabs vs spaces and get the right indentation.
> > > We can introduce `ocamlformat` later.
> > > * C stubs:
> > > just replace tabs with spaces now, using `indent` or `clang-format`
> > > would change code too much for 4.17.
> > > This avoids perpetuating a formatting style that is inconsistent with
> > > the rest of Xen, and that makes preparing and submitting patches more
> > > difficult (OCaml indentation tools usually only support spaces, not tabs).
> > > Contains a bugfix for `abi-check` script to handle the change in the
> > > amount of whitespace.
> > > No functional change.
> > > Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
> > > --
> > > Reason for inclusion in 4.17:
> > > - makes it easier to backport changes from master to 4.17
> > Right, but you will have the problem when backporting to 4.16 and older. So the 
> > overhead will always be there for a couple of years.
>
> There will always be more than one Xen release in support, which means we'd 
> never be able to fix this.
Note that I haven't said this should never be done. This just need to be 
correctly timed. Doing it in the middle of a deep freeze doesn't look 
right to me.
[...]

> > > - avoid perpetuating a different coding style (I thought tabs were
> > >    mandated by Xen, and was about to fix up my editor config to match
> > >    when I realized Xen already mandates the use of spaces)
> > > - should make submitting patches for OCaml easier (OCaml indentation
> > >    tools know only about spaces, so I either can't use them, or have to
> > >    manually adjust indentation every time I submit a patch)
> > > - it can be verified that the only change here is the Makefile change
> > >    for the new rule, 'git log -p -1 -w' should be otherwise empty
> > While I understand the goal and support, this seems to be a bit too late to do 
> > it in Xen 4.17 (we are only a couple of weeks away). At this stage of the 
> > release we should only do bug fix.
> I think it can be fairly easily proven that there is no functional change by 
> rerunning the make format command manually, and by looking at the diff with 
> ignore whitespace as suggested above.
That's not really the point here. The point is that if we start to allow 
large coding style change (whether automatic or manual) very late in the 
release then it will be hard to reject it in the future.
In fact we already have guidelines for that. If you look at [1], only 
bug fixes should be done past the code freeze (23rd September).
As I wrote before, this patch only seem to be a cosmetic/quality 
improvement. IOW this is not a bug fix and would not qualify for 4.17.
> I understand the reluctance in including it (which is why I was not sure 
> whether to post it in the first place), but I think it might be beneficial to 
> do it.
> There is a large backlog of work in oxenstored that got piled up during the 
> past couple of years of XSA work, and it'd be a lot easier to update and 
> upstream those if we wouldn't have to worry
> about indentation at all.
This is an argument for including this patch in Xen 4.18. As I wrote 
above, I am not against that.
> Usually patches on LCM and security branches are avoided to reduce the risk of 
> breaking anything, but a reindentation patch should not really break anything 
> (well other than the abi-check script in the build, but I fixed that to accept 
> both ways).
What does LCM stands for?

> One alternative would be that I add another step after reformat that runs sed 
> and turns spaces back into tabs for now, and that way I can still run 'make 
> format' at each step while preparing patches for master, or 4.17 or security 
> patches and get something consistent, and that minimizes other whitespace 
> changes, but it wouldn't completely eliminate them (e.g. there are pieces of 
> code that are just wrongly indented, so there'd be at least a diff to fix all 
> that).
I would view this as a feature. Which again doesn't qualify for Xen 4.17 
release. This doesn't mean the patch couldn't be backported afterwards.
Cheers,

[1] 
AS8PR08MB7991145C8063D6939AFFED8F92829@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> Best regards,
> --Edwin
>
> > Cheers,
>
> > --
> > Julien Grall
--
Julien Grall