Replace promote actions by diff actions + promote command

Makefile

@@ -23,6 +23,11 @@ reinstall: uninstall reinstall
 test:
 	$(BIN) runtest --dev
 
+promote:
+	$(BIN) promote
+
+accept-corrections: promote
+
 all-supported-ocaml-versions:
 	$(BIN) build --dev @install @runtest --workspace jbuild-workspace.dev --root .
 
@@ -34,13 +39,8 @@ doc:
 	cd doc && sphinx-build . _build
 
 update-jbuilds: $(BIN)
-	$(BIN) build --dev @jbuild --promote copy
-
-accept-corrections:
-	for i in `find . -name \*.corrected`; do \
-	  cp $$i $${i%.corrected}; \
-	done
+	$(BIN) build --dev @doc/runtest --auto-promote
 
 .DEFAULT_GOAL := default
 .PHONY: default install uninstall reinstall clean test doc
-.PHONY: accept-corrections
+.PHONY: promote accept-corrections

bin/main.ml

@@ -23,7 +23,7 @@ type common =
   ; capture_outputs  : bool
   ; x                : string option
   ; diff_command     : string option
-  ; promote_mode     : Clflags.Promote_mode.t
+  ; auto_promote     : bool
   ; (* Original arguments for the external-lib-deps hint *)
     orig_args        : string list
   }
@@ -42,7 +42,7 @@ let set_common c ~targets =
     Sys.chdir c.root;
   Clflags.workspace_root := Sys.getcwd ();
   Clflags.diff_command := c.diff_command;
-  Clflags.promote_mode := c.promote_mode;
+  Clflags.auto_promote := c.auto_promote;
   Clflags.external_lib_deps_hint :=
     List.concat
       [ ["jbuilder"; "external-lib-deps"; "--missing"]
@@ -161,7 +161,8 @@ let common =
         no_buffer
         workspace_file
         diff_command
-        (root, only_packages, promote_mode, orig)
+        auto_promote
+        (root, only_packages, orig)
         x
     =
     let root, to_cwd =
@@ -188,7 +189,7 @@ let common =
     ; orig_args
     ; target_prefix = String.concat ~sep:"" (List.map to_cwd ~f:(sprintf "%s/"))
     ; diff_command
-    ; promote_mode
+    ; auto_promote
     ; only_packages =
         Option.map only_packages
           ~f:(fun s -> String_set.of_list (String.split s ~on:','))
@@ -279,23 +280,12 @@ let common =
                     targets given on the command line. It is only intended
                     for scripts.|})
   in
-  let promote =
-    let mode =
-      Arg.(conv
-             (Arg.parser_of_kind_of_string ~kind:"promotion mode"
-                Clflags.Promote_mode.of_string,
-              fun ppf mode ->
-                Format.pp_print_string ppf
-                  (Clflags.Promote_mode.to_string mode)))
-    in
+  let auto_promote =
     Arg.(value
-         & opt (some mode) None
-         & info ["promote"] ~docs
-             ~doc:"How to interpret promote actions. $(b,copy) means to print
-                   a diff and copy the generated files to the source tree when
-                   they differ. $(b,copy) is the default. $(b,check) means to
-                   only print a diff without copying files. $(b,ignore) means
-                   to ignore promote action altogether.")
+         & flag
+         & info ["auto-promote"] ~docs
+             ~doc:"Automatically promote files. This is similar to running
+                   $(b,jbuilder promote) after the build.")
   in
   let for_release = "for-release-of-packages" in
   let frop =
@@ -308,38 +298,32 @@ let common =
                     packages as well as getting reproducible builds.|})
   in
   let root_and_only_packages =
-    let merge root only_packages promote release =
+    let merge root only_packages release =
       let fail opt =
         `Error (true,
                 sprintf
                   "Cannot use -p/--%s and %s simultaneously"
                   for_release opt)
       in
-      match release, root, only_packages, promote with
-      | Some _, Some _, _, _ -> fail "--root"
-      | Some _, _, Some _, _ -> fail "--only-packages"
-      | Some _, _, _, Some _ -> fail "--promote"
-      | Some pkgs, None, None, None ->
+      match release, root, only_packages with
+      | Some _, Some _, _ -> fail "--root"
+      | Some _, _, Some _ -> fail "--only-packages"
+      | Some pkgs, None, None ->
         `Ok (Some ".",
              Some pkgs,
-             Clflags.Promote_mode.Ignore,
              ["-p"; pkgs]
             )
-      | None, _, _, _ ->
+      | None, _, _ ->
         `Ok (root,
              only_packages,
-             Option.value promote ~default:Clflags.Promote_mode.Copy,
              List.concat
                [ dump_opt "--root" root
                ; dump_opt "--only-packages" only_packages
-               ; dump_opt "--promote"
-                   (Option.map promote ~f:Clflags.Promote_mode.to_string)
                ])
     in
     Term.(ret (const merge
                $ root
                $ only_packages
-               $ promote
                $ frop))
   in
   let x =
@@ -364,6 +348,7 @@ let common =
         $ no_buffer
         $ workspace_file
         $ diff_command
+        $ auto_promote
         $ root_and_only_packages
         $ x
        )
@@ -1088,6 +1073,28 @@ let utop =
           $ Arg.(value & pos_right 0 string [] (Arg.info [] ~docv:"ARGS")))
   , Term.info "utop" ~doc ~man )
 
+let promote =
+  let doc = "Promote files from the last run" in
+  let man =
+    [ `S "DESCRIPTION"
+    ; `P {|Considering all actions of the form $(b,(diff a b)) that failed
+           in the last run of jbuilder, $(b,jbuilder promote) does the following:
+
+           If $(b,a) is present in the source tree but $(b,b) isn't, $(b,b) is
+           copied over to $(b,a) in the source tree. The idea behind this is that
+           you might use $(b,(diff file.expected file.generated)) and then call
+           $(b,jbuilder promote) to promote the generated file.
+         |}
+    ; `Blocks help_secs
+    ] in
+  let go common =
+    set_common common ~targets:[];
+    Action.Promotion.promote_files_registered_in_last_run ()
+  in
+  ( Term.(const go
+          $ common)
+  , Term.info "promote" ~doc ~man )
+
 let all =
   [ installed_libraries
   ; external_lib_deps
@@ -1100,6 +1107,7 @@ let all =
   ; subst
   ; rules
   ; utop
+  ; promote
   ]
 
 let default =

doc/jbuild

@@ -16,5 +16,5 @@
   (run bash ${path:update-jbuild.sh} ${bin:jbuilder})))
 
 (alias
- ((name jbuild)
-  (action (promote (jbuild.inc.gen as jbuild.inc)))))
+ ((name runtest)
+  (action (diff jbuild.inc jbuild.inc.gen))))

doc/jbuild.inc

@@ -53,6 +53,15 @@
  ((section man)
   (files (jbuilder-installed-libraries.1))))
 
+(rule
+ ((targets (jbuilder-promote.1))
+  (action  (with-stdout-to ${@}
+            (run ${bin:jbuilder} promote --help=groff)))))
+
+(install
+ ((section man)
+  (files (jbuilder-promote.1))))
+
 (rule
  ((targets (jbuilder-rules.1))
   (action  (with-stdout-to ${@}

doc/jbuild.rst

@@ -555,15 +555,15 @@ For instance:
     (rule (with-stdout-to jbuild.inc.gen (run ./gen-jbuild.exe)))
 
     (alias
-     ((name   jbuild)
-      (action (promote (jbuild.inc.gen as jbuild.inc)))))
+     ((name   runtest)
+      (action (diff jbuild.inc jbuild.inc.gen))))
 
 With this jbuild file, running jbuilder as follow will replace the
 ``jbuild.inc`` file in the source tree by the generated one:
 
 .. code:: shell
 
-    $ jbuilder build @jbuild
+    $ jbuilder build @runtest --auto-promote
 
 Common items
 ============
@@ -1014,13 +1014,12 @@ The following constructions are available:
   and ``cmd`` on Windows
 - ``(bash <cmd>)`` to execute a command using ``/bin/bash``. This is obviously
   not very portable
-- ``(promote <files-to-promote>)`` copy generated files to the source
-  tree. See `Promotion`_ for more details
-- ``(promote-if <files-to-promote>)`` is the same as ``(promote
-  <files-to-promote>)`` except that a form ``(<a> as <b>)`` is ignored
-  when ``<a>`` doesn't exists. Additionally, ``<a>`` won't be copied
-  if ``<b>`` doesn't already exist. This can be used with command that
-  only produce a correction when differences are found
+- ``(diff <file1> <file2>)`` is similar to ``(run diff <file1>
+  <file2>)`` but is better and allows promotion.  See `Diffing and
+  promotion`_ for more details
+- ``(diff? <file1> <file2>)`` is the same as ``(diff <file1>
+  <file2>)`` except that it is ignored when ``<file1>`` or ``<file2>``
+  doesn't exists
 
 As mentioned ``copy#`` inserts a line directive at the beginning of
 the destination file. More precisely, it inserts the following line:
@@ -1138,37 +1137,69 @@ is global to all build contexts, simply use an absolute filename:
 
 .. _ocaml-syntax:
 
+Diffing and promotion
+---------------------
+
+``(diff <file1> <file2>)`` is very similar to ``(run diff <file1>
+<file2>)``. In particular it behaves in the same way:
+
+- when ``<file1>`` and ``<file2>`` are equal, it doesn't nothing
+- when they are not, the differences are shown and the action fails
+
+However, it is different for the following reason:
+
+- the exact command used to diff files can be configured via the
+  ``--diff-command`` command line argument. Note that it is only
+  called when the files are not byte equals
+
+- by default, it will use ``patdiff`` if it is installed. ``patdiff``
+  is a better diffing program. You can install it via opam with:
+
+  .. code:: sh
+
+     $ opam install patdiff
+
+- since ``(diff a b)`` is a builtin action, Jbuilder knowns that ``a``
+  and ``b`` are needed and so you don't need to specify them
+  explicitly as dependencies
+
+- you can use ``(diff? a b)`` after a command that might or might not
+  produce ``b``. For cases where commands optionally produce a
+  *corrected* file
+
+- it allows promotion. See below
+
 Promotion
----------
+~~~~~~~~~
+
+Whenever an action ``(diff <file1> <file2>)`` or ``(diff?  <file1>
+<file2>)`` fails because the two files are different, jbuilder allows
+you to promote ``<file2>`` as ``<file1>`` if ``<file1>`` is a source
+file and ``<file2>`` is a generated file.
+
+More precisely, let's consider the following jbuild file:
+
+.. code:: scheme
+
+   (rule
+    (with-stdout-to data.out (run ./test.exe)))
+
+   (alias
+    ((name   runtest)
+     (action (diff data.expected data.out))))
+
+Where ``data.expected`` is a file committed in the source
+repository. You can use the following workflow to update your test:
+
+- update the code of your test
+- run ``jbuilder runtest``. The diff action will fail and a diff will
+  be printed
+- check the diff to make sure it is what you expect
+- run ``jbuilder promote``. This will copy the generated ``data.out``
+  file to ``data.expected`` directly in the source tree
 
-The ``(promote (<file1> as <file2>) (<file3> as <file4>) ...)`` and
-``(promote-if (<file1> as <file2>) (<file3> as <file4>) ...)`` actions
-can be used to copy generated files to the source tree.
-
-This method is used when one wants to commit a generated file that is
-independent of the systems where it is generated. Typically this can
-be used to:
-
-- cut dependencies and/or speed up the build in release mode: we use
-  the file in the source tree rather than re-generate it
-- support bootstrap cycles
-- simplify the review when the generated code is easier to review than
-  the generator
-
-How jbuilder interprets promotions can be controlled using the
-``--promote`` command line argument. The following behaviors are
-available:
-
-- ``--promote copy``: when the two files given in a ``(<a> as <b>)``
-  form are different, jbuilder prints a diff and copies ``<a>`` to
-  ``<b>`` directly in the source
-   tree. This is the default
-- ``--promote check``: Jbuilder just checks that the two files are
-  equal and print a diff when there are not
-- ``--promote ignore``: ``promote`` actions are simply ignored
-
-Note that ``-p/--for-release-of-packages`` implies ``--promote
-ignore``.
+You can also use ``jbuilder runtest --auto-promote`` which will
+automatically do the promotion.
 
 OCaml syntax
 ============