> On 8 Nov 2022, at 16:03, Julien Grall <jul...@xen.org> 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.to...@citrix.com> >> -- >> 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. At some point we need to decide to go ahead and just do it (which may or may not be 4.17) The whitespace fixup needs to happen somewhere anyway, there are a couple of options: * use git rebase/cherry-pick -Xignore-space-change to avoid backport failing to apply * do a reformat on 4.16 branch too (backport the couple of lines of Makefile change and run 'make format' and commit the result) * include the reformat as a prerequisite of the next security patch (I'd prefer not to, hence including it in 4.17) * do the whitespace fixup prior to submission, turning a correctly formatted patch with spaces into an incorrectly formatted one with tabs. I wouldn't have minded doing this if the coding style said tabs, I could've run my reformat tool locally and fix it up with 'sed' prior to submission. However this seems unnecessary overhead if doing this works actively against the existing coding style. However if we put this into 4.17 and there are no new security issues discovered before 4.16 goes out of support then we'll be in a much better position for 4.18 onward. There is still quite a lot of work outstanding in testing of oxenstored (updating my fuzzer for one), so I can't really predict at this point whether there are more security bugs in oxenstored or not, so maybe we need to take this decision at 4.18 time? Not sure. > >> - 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. 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. 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). 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). > > This is clearly only a comesmetic change and there I would argue this should > be deferred to 4.18. That said the last call is from the RM. Best regards, --Edwin > > Cheers, > > > -- > Julien Grall
