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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 8 Nov 2022, at 17:26, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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?

LCM is what we internally call the long term release branch where we backport 
fixes (i.e. 4.16 is an LCM branch after it is released). I think it'd be 
equivalent to the stable branch of Xen/Linux, I should've used that 
terminology. (I think it is an abbreviation of software lifecycle management)

> 
>> 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.


Ah ok, if this can be backported to 4.17.1, or applied for 4.18 that might work 
too. (I just thought the rules around backport would be even stricter than what 
can go into a release)
I'll try to find a short term workaround for the spaces vs tabs issues 
meanwhile.


> 
> Cheers,
> 
> [1] 
> AS8PR08MB7991145C8063D6939AFFED8F92829@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx


Hmm I thought that is my Outlook rewriting the link, but the archive at 
lore.kernel.org seems to have this mangled URL as well which I cannot open.
Could you send it in such a way that it is not encoded when being sent (e.g. 
base64 encode it...)

> 
>> Best regards,
>> --Edwin
>>> 
>>> Cheers,
>>> 
>>> 
>>> -- 
>>> Julien Grall
> 
> -- 
> Julien Grall