[cocci] [PATCH] Exit with non-zero status when spatch on a directory fails

[cocci] [PATCH] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

When an error occurs during patching of a file, the process terminates
with a 255 exit code.

    spatch docs/dev/file-path-error.cocci docs/dev/test1.c

This was not the case when trying to apply the patch on a directory:

    spatch docs/dev/file-path-error.cocci docs/dev/

The process would print errors for each file individually,
and then finish with a zero (success) exit code.
Additionally, it would write out files for files that succeeded.

Let’s exit with 255 when failure occurs during the mass patching
as well, and prevent writing the changes of successful transformations
to avoid the chance of accidentally not noticing the failure.
---
 enter.ml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/enter.ml b/enter.ml
index 53d4753f..56a4d4c6 100644
--- a/enter.ml
+++ b/enter.ml
@@ -1083,6 +1083,7 @@ let rec main_action xs =
   | _ -> failwith "only one .cocci file allowed");
   Iteration.base_file_list := xs;
   previous_merges := ([], []);
+  let patching_failed = ref false in
   let rec toploop = function
       [] -> failwith "no C files provided"
     | x::xs ->
@@ -1366,6 +1367,7 @@ singleton lists are then just appended to each other during the merge. *)
 					    (Printexc.to_string e)
 					    all_cfiles;
 					  flush stderr;
+					  patching_failed := true;
 					  prev (* *)
 					end
 					else raise e) in
@@ -1414,6 +1416,10 @@ singleton lists are then just appended to each other during the merge. *)
 		  (x,xs,cocci_infos,outfiles)
 		end) in
       let (x,xs,cocci_infos,outfiles) = toploop xs in
+      if !patching_failed then begin
+        Printf.fprintf stderr "An error occurred when attempting to transform some files.\n";
+        raise (UnixExit (-1))
+      end;
       Common.profile_code "Main.result analysis" (fun () ->
 	Ctlcocci_integration.print_bench();
 	generate_outfiles outfiles x xs;
-- 
2.37.2

Re: [cocci] [PATCH] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]



On Mon, 5 Sep 2022, Jan Tojnar wrote:

> When an error occurs during patching of a file, the process terminates
> with a 255 exit code.
>
>     spatch docs/dev/file-path-error.cocci docs/dev/test1.c
>
> This was not the case when trying to apply the patch on a directory:
>
>     spatch docs/dev/file-path-error.cocci docs/dev/
>
> The process would print errors for each file individually,
> and then finish with a zero (success) exit code.
> Additionally, it would write out files for files that succeeded.
>
> Let’s exit with 255 when failure occurs during the mass patching
> as well, and prevent writing the changes of successful transformations
> to avoid the chance of accidentally not noticing the failure.

It looks like this breaks the testing infrastructure.

> ---
>  enter.ml | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/enter.ml b/enter.ml
> index 53d4753f..56a4d4c6 100644
> --- a/enter.ml
> +++ b/enter.ml
> @@ -1083,6 +1083,7 @@ let rec main_action xs =
>    | _ -> failwith "only one .cocci file allowed");
>    Iteration.base_file_list := xs;
>    previous_merges := ([], []);
> +  let patching_failed = ref false in
>    let rec toploop = function
>        [] -> failwith "no C files provided"
>      | x::xs ->
> @@ -1366,6 +1367,7 @@ singleton lists are then just appended to each other during the merge. *)
>  					    (Printexc.to_string e)
>  					    all_cfiles;
>  					  flush stderr;
> +					  patching_failed := true;
>  					  prev (* *)
>  					end
>  					else raise e) in
> @@ -1414,6 +1416,10 @@ singleton lists are then just appended to each other during the merge. *)
>  		  (x,xs,cocci_infos,outfiles)
>  		end) in
>        let (x,xs,cocci_infos,outfiles) = toploop xs in
> +      if !patching_failed then begin
> +        Printf.fprintf stderr "An error occurred when attempting to transform some files.\n";
> +        raise (UnixExit (-1))
> +      end;

Maybe put:

if !patching_failed && not !Flag_ctl.bench && not !compare_with_expected
then
  begin
    ...
  end

Please add the appropriate:

Signed-off-by: Random J Developer <random@developer.example.org>

See readme.txt at the root of the source directory.

thanks,
julia

>        Common.profile_code "Main.result analysis" (fun () ->
>  	Ctlcocci_integration.print_bench();
>  	generate_outfiles outfiles x xs;
> --
> 2.37.2
>
>

[cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

When an error occurs during patching of a file, the process terminates
with a 255 exit code.

    spatch docs/dev/file-path-error.cocci docs/dev/test1.c

This was not the case when trying to apply the patch on a directory:

    spatch docs/dev/file-path-error.cocci docs/dev/

The process would print errors for each file individually,
and then finish with a zero (success) exit code.
Additionally, it would write out files for files that succeeded.

Let’s exit with 255 when failure occurs during the mass patching
as well, and prevent writing the changes of successful transformations
to avoid the chance of accidentally not noticing the failure.

Let’s also introduce a `--keep-going` flag that reverts to the previous
behaviour. And also do the same with `--bench` and `--compare-with-expected`
since the testing infrastructure relies on it.

Signed-off-by: Jan Tojnar <jtojnar@gmail.com>
---

Thansk, updated as per your instructions.

Though, I think, ideally, the test infrastructure would be updated
to use the newly introduced `--keep-going` flag and the magic
behaviour of `--bench` and `--compare-with-expected` would
be removed. I would do that but was not able to find any
uses of the two flags in the repo.

 enter.ml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/enter.ml b/enter.ml
index 53d4753f..b4fac617 100644
--- a/enter.ml
+++ b/enter.ml
@@ -28,6 +28,7 @@ let preprocess = ref false     (* run the C preprocessor before cocci *)
 let compat_mode = ref false
 let ignore_unknown_opt = ref false
 let profile_per_file = ref false
+let keep_going = ref false
 
 let dir = ref false
 let ignore = ref []
@@ -729,6 +730,8 @@ let other_options = [
     "   option to set if launch spatch in ocamldebug";
     "--disable-once",       Arg.Set Common.disable_pr2_once,
     "   to print more messages";
+    "--keep-going",         Arg.Set keep_going,
+    "     write transformations and exit with zero status, even if some files fail";
     "--show-trace-profile", Arg.Set Common.show_trace_profile,
     "   show trace";
     "--save-tmp-files",     Arg.Set Common.save_tmp_files,   " ";
@@ -1083,6 +1086,7 @@ let rec main_action xs =
   | _ -> failwith "only one .cocci file allowed");
   Iteration.base_file_list := xs;
   previous_merges := ([], []);
+  let patching_failed = ref false in
   let rec toploop = function
       [] -> failwith "no C files provided"
     | x::xs ->
@@ -1366,6 +1370,7 @@ singleton lists are then just appended to each other during the merge. *)
 					    (Printexc.to_string e)
 					    all_cfiles;
 					  flush stderr;
+					  patching_failed := true;
 					  prev (* *)
 					end
 					else raise e) in
@@ -1414,6 +1419,12 @@ singleton lists are then just appended to each other during the merge. *)
 		  (x,xs,cocci_infos,outfiles)
 		end) in
       let (x,xs,cocci_infos,outfiles) = toploop xs in
+      if !patching_failed && not !keep_going && !Flag_ctl.bench == 0 && !compare_with_expected == None
+      then
+        begin
+          Printf.fprintf stderr "An error occurred when attempting to transform some files.\n";
+          raise (UnixExit (-1))
+        end;
       Common.profile_code "Main.result analysis" (fun () ->
 	Ctlcocci_integration.print_bench();
 	generate_outfiles outfiles x xs;
-- 
2.37.2

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]



On Mon, 5 Sep 2022, Jan Tojnar wrote:

> When an error occurs during patching of a file, the process terminates
> with a 255 exit code.
>
>     spatch docs/dev/file-path-error.cocci docs/dev/test1.c
>
> This was not the case when trying to apply the patch on a directory:
>
>     spatch docs/dev/file-path-error.cocci docs/dev/
>
> The process would print errors for each file individually,
> and then finish with a zero (success) exit code.
> Additionally, it would write out files for files that succeeded.
>
> Let’s exit with 255 when failure occurs during the mass patching
> as well, and prevent writing the changes of successful transformations
> to avoid the chance of accidentally not noticing the failure.
>
> Let’s also introduce a `--keep-going` flag that reverts to the previous
> behaviour. And also do the same with `--bench` and `--compare-with-expected`
> since the testing infrastructure relies on it.
>
> Signed-off-by: Jan Tojnar <jtojnar@gmail.com>
> ---
>
> Thansk, updated as per your instructions.
>
> Though, I think, ideally, the test infrastructure would be updated
> to use the newly introduced `--keep-going` flag and the magic
> behaviour of `--bench` and `--compare-with-expected` would
> be removed. I would do that but was not able to find any
> uses of the two flags in the repo.

I'm not sure to understand the point.  --bench and --compare-with expected
don't mean at all the same thing as just keep going.  --bench prints some
statistics about the CTL mating, and --compare-with-expected checks that
the result is the same as the one indicated in a .res file.

julia

>
>  enter.ml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/enter.ml b/enter.ml
> index 53d4753f..b4fac617 100644
> --- a/enter.ml
> +++ b/enter.ml
> @@ -28,6 +28,7 @@ let preprocess = ref false     (* run the C preprocessor before cocci *)
>  let compat_mode = ref false
>  let ignore_unknown_opt = ref false
>  let profile_per_file = ref false
> +let keep_going = ref false
>
>  let dir = ref false
>  let ignore = ref []
> @@ -729,6 +730,8 @@ let other_options = [
>      "   option to set if launch spatch in ocamldebug";
>      "--disable-once",       Arg.Set Common.disable_pr2_once,
>      "   to print more messages";
> +    "--keep-going",         Arg.Set keep_going,
> +    "     write transformations and exit with zero status, even if some files fail";
>      "--show-trace-profile", Arg.Set Common.show_trace_profile,
>      "   show trace";
>      "--save-tmp-files",     Arg.Set Common.save_tmp_files,   " ";
> @@ -1083,6 +1086,7 @@ let rec main_action xs =
>    | _ -> failwith "only one .cocci file allowed");
>    Iteration.base_file_list := xs;
>    previous_merges := ([], []);
> +  let patching_failed = ref false in
>    let rec toploop = function
>        [] -> failwith "no C files provided"
>      | x::xs ->
> @@ -1366,6 +1370,7 @@ singleton lists are then just appended to each other during the merge. *)
>  					    (Printexc.to_string e)
>  					    all_cfiles;
>  					  flush stderr;
> +					  patching_failed := true;
>  					  prev (* *)
>  					end
>  					else raise e) in
> @@ -1414,6 +1419,12 @@ singleton lists are then just appended to each other during the merge. *)
>  		  (x,xs,cocci_infos,outfiles)
>  		end) in
>        let (x,xs,cocci_infos,outfiles) = toploop xs in
> +      if !patching_failed && not !keep_going && !Flag_ctl.bench == 0 && !compare_with_expected == None
> +      then
> +        begin
> +          Printf.fprintf stderr "An error occurred when attempting to transform some files.\n";
> +          raise (UnixExit (-1))
> +        end;
>        Common.profile_code "Main.result analysis" (fun () ->
>  	Ctlcocci_integration.print_bench();
>  	generate_outfiles outfiles x xs;
> --
> 2.37.2
>
>

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

On Mon, 5 Sept 2022 at 12:48, Julia Lawall <julia.lawall@inria.fr> wrote:
>
>
> I'm not sure to understand the point.  --bench and --compare-with expected
> don't mean at all the same thing as just keep going.  --bench prints some
> statistics about the CTL mating, and --compare-with-expected checks that
> the result is the same as the one indicated in a .res file.

I would expect that when an error occurs during transforming one of the files,
transforming any other files does not make sense since the SmPL patch
probably makes incorrect assumptions about the source code.

In my case, I made the Python script inside the semantic patch raise
an Exception
when the source code does not meet certain preconditions, to detect that
the underlying code changed in a way that the .cocci file needs adjusting.

I guess that failures might make sense for tests (--compare-with-expected
seems to only be relevant for tests as far as I can tell). But I would expect
benchmarking to be useful for people creating patches and there, silently
suppressing failures could be unexpected.

But you are more knowledgeable about the use cases so I will leave it up to you.
to profile failing

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

On Mon, 5 Sep 2022, Jan Tojnar wrote:

> On Mon, 5 Sept 2022 at 12:48, Julia Lawall <julia.lawall@inria.fr> wrote:
> >
> >
> > I'm not sure to understand the point.  --bench and --compare-with expected
> > don't mean at all the same thing as just keep going.  --bench prints some
> > statistics about the CTL mating, and --compare-with-expected checks that
> > the result is the same as the one indicated in a .res file.
>
> I would expect that when an error occurs during transforming one of the files,
> transforming any other files does not make sense since the SmPL patch
> probably makes incorrect assumptions about the source code.

Each test case applies to a unique file, so this problem doesn't occur.

Furthermore, the original use of Coccinelle was to create a patch, not to
update the file.  One can update the file using the patch afterwards, if
the result is satisfactory.  But I don't know what options most people
use.

Thanks,
julia

>
> In my case, I made the Python script inside the semantic patch raise
> an Exception
> when the source code does not meet certain preconditions, to detect that
> the underlying code changed in a way that the .cocci file needs adjusting.
>
> I guess that failures might make sense for tests (--compare-with-expected
> seems to only be relevant for tests as far as I can tell). But I would expect
> benchmarking to be useful for people creating patches and there, silently
> suppressing failures could be unexpected.
>
> But you are more knowledgeable about the use cases so I will leave it up to you.
> to profile failing
>

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

On Mon, 5 Sep 2022, Jan Tojnar wrote:

> On Mon, 5 Sept 2022 at 14:29, Julia Lawall <julia.lawall@inria.fr> wrote:
> > Furthermore, the original use of Coccinelle was to create a patch, not to
> > update the file.  One can update the file using the patch afterwards, if
> > the result is satisfactory.
>
> Right, that was what I was doing as well. The spatch command succeeded
> and the patch looked fine at a glance. Only later, I noticed that the Python
> script failed on some files and the resulting patch was incomplete.
>
> So my motivation for this patch submission is avoiding this situation
> in the future,
> by always returning with exit status and not producing any patches/changes
> when some file fails, unless user explicitly requests the partial result
> with --keep-going flag.

But Coccinelle is not so fast.  It would be unfortunate to run it for
several hours on something complex, have it fail in one case, and get no
feedback at all.

I think you want some python function that aborts in a more aggressive
way, not to have that be the default behavior.

Most of the time when Coccinelle doesn't do what you want, it is because
the semantic patch is doing something, but it is just designed in the
wrong way.  Not because of something that Coccinelle will perceive as an
error.  So I think that an exit error status is of limited usefulness.
But it could happen that some script code is written badly, such that some
input values cause a crash.  I'm not sure at all that that means that one
doesn't want to see any information about all of the other cases where the
semantic patch applied.

Maybe a --crash-on-script-failure option would be reasonable.

julia

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

On Mon, 5 Sept 2022 at 14:51, Julia Lawall <julia.lawall@inria.fr> wrote:
> But Coccinelle is not so fast.  It would be unfortunate to run it for
> several hours on something complex, have it fail in one case, and get no
> feedback at all.

Unless the user runs it with --quiet, there should be feedback.
This patch only terminates it once all transformation happen so that
user can see all failures without having to run Coccinelle repeatedly.

> I think you want some python function that aborts in a more aggressive
> way, not to have that be the default behavior.

I would say that raising an exception is as aggressive as one can get
without interfering with user’s environment, like shutting down their computer.

We could limit ourselves to only e.g. FatalPycocciException and
catch any other exceptions. But I would again argue that any exception
already signifies an abnormal behaviour, and any spatch run where
one occurs should be considered tainted and the exception should
be resolved before user can continue.

> Most of the time when Coccinelle doesn't do what you want, it is because
> the semantic patch is doing something, but it is just designed in the
> wrong way.  Not because of something that Coccinelle will perceive as an
> error.  So I think that an exit error status is of limited usefulness.

Indeed, and there is not much we can do about those cases.
But here the code explicitly fails by calling raising a Failure (using
failwith function)
and will terminate immediately when working with a single file:
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/d5f94f9bdca5ba23aabf515072fa7fe67239e221/enter.ml#L1362-1371
This behaviour implies that user is supposed to notice the failure,
yet with the current behaviour, it is easy to overlook it.

> But it could happen that some script code is written badly, such that some
> input values cause a crash.  I'm not sure at all that that means that one
> doesn't want to see any information about all of the other cases where the
> semantic patch applied.

Again, all the information is still there, only the changes are not written when
`--in-place` or `--out-place` is used. We could move the exiting at the end of
the main_action so that everything would be as before, except for exit
status code.

But I would say that when such failure occurs, all bets are already off and
users would need to clear the repo any way, as scripts that contain errors
are hardly guaranteed to be idempotent.

And I just noticed the exit code is still zero when running spatch in
parallel mode
but it is not clear to me why. Maybe the patching_failed ref is not accessible
from parmap created “threads”.

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

The proposed behavior seems basically ok.  It seems to have no impact if
one is not writing things to files.

But I wonder why not just check for !patching_failed before calling
generate_outfiles, and then putting the choice of exit status after the
whole "Main results analysis" block?

I don't think that the keep going flag is a good idea.  It makes it sound
like that, without it, as soon as spatch fails on one file, it aborts
completely.

The correct patching_failed value is indeed not seen in the case of parmap
because modifications are not propagated back to the caller.  One has to
add the value to the result.

julia

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

The patch has been applied, followed by another one that dropped th e
--keep-trying option and that addresses the parmap issue.

thanks,
julia

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

On Mon, 5 Sept 2022 at 21:51, Julia Lawall <julia.lawall@inria.fr> wrote:
>
> The patch has been applied, followed by another one that dropped th e
> --keep-trying option and that addresses the parmap issue.

Thanks, that will work for us, except that you probably accidentally
made it so that patching only happens when a failure occurs – there
should be `not` before `patching_failed` on this line:
https://github.com/coccinelle/coccinelle/blob/5448bb2bd03491ffec356bf7bd6ddcdbf4d36bc9/enter.ml#L1419

Additionally, it looks like you keep introducing more tab characters
into the indentation, resulting in mixed tabs and spaces. This makes
diffs more noisy than necessary and the code can end up displayed
misaligned in some text editors.

May I suggest adding the following .editorconfig file to the root of
the repository? Hopefully, your editor has support for that standard
and will use consistent formatting:
https://editorconfig.org/#pre-installed

root = true

[*]
end_of_line = lf
insert_final_newline = true

[*.{ml,mli}]
indent_style = space
indent_size = 2
tab_width = 8

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]



On Mon, 5 Sep 2022, Jan Tojnar wrote:

> On Mon, 5 Sept 2022 at 21:51, Julia Lawall <julia.lawall@inria.fr> wrote:
> >
> > The patch has been applied, followed by another one that dropped th e
> > --keep-trying option and that addresses the parmap issue.
>
> Thanks, that will work for us, except that you probably accidentally
> made it so that patching only happens when a failure occurs – there
> should be `not` before `patching_failed` on this line:
> https://github.com/coccinelle/coccinelle/blob/5448bb2bd03491ffec356bf7bd6ddcdbf4d36bc9/enter.ml#L1419

Oops, thanks for noticing it.  It will be fixed shortly.

>
> Additionally, it looks like you keep introducing more tab characters
> into the indentation, resulting in mixed tabs and spaces. This makes
> diffs more noisy than necessary and the code can end up displayed
> misaligned in some text editors.
>
> May I suggest adding the following .editorconfig file to the root of
> the repository? Hopefully, your editor has support for that standard
> and will use consistent formatting:
> https://editorconfig.org/#pre-installed
>
> root = true
>
> [*]
> end_of_line = lf
> insert_final_newline = true
>
> [*.{ml,mli}]
> indent_style = space
> indent_size = 2
> tab_width = 8

Not so clear what the impact of this is.  I prefer tabs to be used when
possible.

julia

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]



On Mon, 5 Sep 2022, Jan Tojnar wrote:

> On Mon, 5 Sept 2022 at 21:51, Julia Lawall <julia.lawall@inria.fr> wrote:
> >
> > The patch has been applied, followed by another one that dropped th e
> > --keep-trying option and that addresses the parmap issue.
>
> Thanks, that will work for us, except that you probably accidentally
> made it so that patching only happens when a failure occurs – there
> should be `not` before `patching_failed` on this line:
> https://github.com/coccinelle/coccinelle/blob/5448bb2bd03491ffec356bf7bd6ddcdbf4d36bc9/enter.ml#L1419

Fixed.

julia

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Jan Tojnar]

On Mon, 2022-09-05 at 22:35 +0200, Julia Lawall wrote:
> Not so clear what the impact of this is.  I prefer tabs to be used
> when possible.

I very much prefer tabs myself but the choice of tabs vs spaces is not
important. As long as they are used consistently. Unfortunately, that
is not currently the case, which leads to various issues:

Most importantly, when a developer edits a code with mixed indentation,
they will almost certainly adjust the indentation in one direction or
another. This means that the patches will include extra changes
unrelaated to the main contribution, making them more difficult to
review and retrospectively analyze.

Additionally, different contributors may have different visual tab
widths, which can cause indentation to “jump” making it harder to grasp
the code nesting. Editors often allow to adjust this but users
would need to do it in their text editor, Git client, e-mail client
and maybe even GitLab. For example, the following image is what I saw 
when I reviewed the patch earlier from this thread in my Git client:

https://imgur.com/VeM6jbN
(I highlighted the actual nesting of the syntax tree with red arrows.)

And that assumes all contributors use the same visual tab width.
If that is not the case, the nesting will be mangled no matter
how the developer viewing the code sets the width in their editor.

----

It looks like the original codebase used two spaces for indentation (as
seems to be the convention for OCaml [1]) but later, tabs started to
creep in.

[1]: https://ocaml.org/docs/guidelines#using-tab-stops
Though, their argument against tabs is as fallacious as always.
Their only valid point actually argues against mixed indentation,
which could just as well be used to deny using spaces for indentation.

Cheers,

Jan

Re: [cocci] [PATCH v2] Exit with non-zero status when spatch on a directory fails

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]



On Sat, 22 Oct 2022, Jan Tojnar wrote:

> On Mon, 2022-09-05 at 22:35 +0200, Julia Lawall wrote:
> > Not so clear what the impact of this is.  I prefer tabs to be used
> > when possible.
>
> I very much prefer tabs myself but the choice of tabs vs spaces is not
> important. As long as they are used consistently. Unfortunately, that
> is not currently the case, which leads to various issues:
>
> Most importantly, when a developer edits a code with mixed indentation,
> they will almost certainly adjust the indentation in one direction or
> another. This means that the patches will include extra changes
> unrelaated to the main contribution, making them more difficult to
> review and retrospectively analyze.
>
> Additionally, different contributors may have different visual tab
> widths, which can cause indentation to “jump” making it harder to grasp
> the code nesting. Editors often allow to adjust this but users
> would need to do it in their text editor, Git client, e-mail client
> and maybe even GitLab. For example, the following image is what I saw
> when I reviewed the patch earlier from this thread in my Git client:
>
> https://imgur.com/VeM6jbN
> (I highlighted the actual nesting of the syntax tree with red arrows.)
>
> And that assumes all contributors use the same visual tab width.
> If that is not the case, the nesting will be mangled no matter
> how the developer viewing the code sets the width in their editor.
>
> ----
>
> It looks like the original codebase used two spaces for indentation (as
> seems to be the convention for OCaml [1]) but later, tabs started to
> creep in.
>
> [1]: https://ocaml.org/docs/guidelines#using-tab-stops
> Though, their argument against tabs is as fallacious as always.
> Their only valid point actually argues against mixed indentation,
> which could just as well be used to deny using spaces for indentation.

Thanks for your thoughts, but I'm sorry nothing is likely to happen on
this.  We don't have the development resources for this to be a priority.

julia