fix: diff action missing files

(diff ..) should know to distinguish between empty and missing files

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

Author: rgrinberg

doc/changes/fixed/13696.md

@@ -0,0 +1,5 @@
+- Improve handling of empty files in the `diff` action. These are now correctly
+  distinguished from *empty* files. (#13696, @rgrinberg)
+
+- Pass `/dev/null` to `--diff-command` instead of non-existent files (#13696,
+  @rgrinberg)

src/dune_engine/diff_action.ml

@@ -6,11 +6,19 @@ let compare_files = function
   | Text -> Io.compare_text_files
 ;;
 
-let diff_eq_files { Diff.optional; mode; file1; file2 } =
-  let file1 = if Fpath.exists (Path.to_string file1) then file1 else Dev_null.path in
-  let file2 = Path.build file2 in
-  (optional && not (Fpath.exists (Path.to_string file2)))
-  || compare_files mode file1 file2 = Eq
+let compare_files { Diff.optional; mode; file1; file2 } =
+  let file2_exists = lazy (Fpath.exists (Path.Build.to_string file2)) in
+  if optional && not (Lazy.force file2_exists)
+  then
+    (* Small optimization to avoid stat'ing file1 *)
+    `Eq true
+  else (
+    let file1_exists = Fpath.exists (Path.to_string file1) in
+    match file1_exists, Lazy.force file2_exists with
+    | true, true -> `Eq (compare_files mode file1 (Path.build file2) = Eq)
+    | false, false -> `Eq true
+    | true, false -> if optional then `Eq true else `Delete
+    | false, true -> `Create)
 ;;
 
 let exec loc ({ Diff.optional; file1; file2; mode } as diff) =
@@ -20,11 +28,11 @@ let exec loc ({ Diff.optional; file1; file2; mode } as diff) =
       try Fpath.unlink_exn (Path.Build.to_string file2) with
       | Unix.Unix_error (ENOENT, _, _) -> ())
   in
-  if diff_eq_files diff
-  then (
+  match compare_files diff with
+  | `Eq true ->
     remove_intermediate_file ();
-    Fiber.return ())
-  else (
+    Fiber.return ()
+  | `Eq false | `Create | `Delete ->
     let is_copied_from_source_tree file =
       match Path.extract_build_context_dir_maybe_sandboxed file with
       | None -> false
@@ -73,5 +81,5 @@ let exec loc ({ Diff.optional; file1; file2; mode } as diff) =
            then register `Copy
          | true ->
            if in_source_or_target then register `Move else remove_intermediate_file ());
-        Fiber.return ()))
+        Fiber.return ())
 ;;

src/dune_engine/print_diff.ml

@@ -97,16 +97,14 @@ let make_metadata ~has_embedded_location promotion loc =
 module External = struct
   let which prog = Bin.which ~path:(Env_path.path Env.initial) prog
 
-  let diff ~skip_trailing_cr ~dir promotion loc file1 file2 =
+  let diff ~skip_trailing_cr ~dir promotion loc (label1, file1) (label2, file2) =
     which "diff"
     |> Option.map ~f:(fun prog ->
       let relative = Path.reach ~from:dir in
-      let file1 = relative file1 in
-      let file2 = relative file2 in
       let args =
-        [ "-u"; "--label"; file1; "--label"; file2 ]
+        [ "-u"; "--label"; relative label1; "--label"; relative label2 ]
         @ (if skip_trailing_cr then [ "--strip-trailing-cr" ] else [])
-        @ [ file1; file2 ]
+        @ [ relative file1; relative file2 ]
       in
       { dir
       ; prog
@@ -158,6 +156,8 @@ module External = struct
   ;;
 end
 
+let noent_to_dev_null f = if Fpath.exists (Path.to_string f) then f else Dev_null.path
+
 let prepare ~skip_trailing_cr promotion path1 path2 =
   let dir, loc =
     let dir, file1 =
@@ -170,15 +170,19 @@ let prepare ~skip_trailing_cr promotion path1 path2 =
     in
     dir, Loc.in_file file1
   in
+  let label1 = Path.drop_optional_sandbox_root path1 in
+  let label2 = Path.drop_optional_sandbox_root path2 in
+  let path1 = noent_to_dev_null path1 in
+  let path2 = noent_to_dev_null path2 in
   let fallback =
     With_fallback.fail
       (User_error.make
          ~loc
          ?promotion
          [ Pp.textf
              "Files %s and %s differ."
-             (Path.to_string_maybe_quoted (Path.drop_optional_sandbox_root path1))
-             (Path.to_string_maybe_quoted (Path.drop_optional_sandbox_root path2))
+             (Path.to_string_maybe_quoted (Path.drop_optional_sandbox_root label1))
+             (Path.to_string_maybe_quoted (Path.drop_optional_sandbox_root label2))
          ])
   in
   match !Clflags.diff_command with
@@ -215,7 +219,9 @@ let prepare ~skip_trailing_cr promotion path1 path2 =
       | None -> fallback
       | Some diff -> With_fallback.run diff ~fallback
     in
-    let diff = External.diff ~skip_trailing_cr ~dir promotion loc path1 path2 in
+    let diff =
+      External.diff ~skip_trailing_cr ~dir promotion loc (label1, path1) (label2, path2)
+    in
     if Execution_env.inside_dune
     then or_fallback ~fallback diff
     else (

test/blackbox-tests/test-cases/promote/non-existent-dir.t

@@ -18,7 +18,7 @@ dune promote should be able to promote into directories that don't exist
 
   $ dune build ./foo --diff-command "$SHELL $PWD/diff.sh"
   File "dir/foo", line 1, characters 0-0:
-  a: dir/foo
+  a: /dev/null
   b: foo
   [1]
 

test/blackbox-tests/test-cases/promote/non-existent-empty.t/run.t

@@ -6,18 +6,27 @@ Tests for promoting with non existent reference
   x-non-existent-empty missing
 
   $ dune build @blah-non-existent-empty
+  File "x-non-existent-empty", line 1, characters 0-0:
+  Error: Files _build/default/x-non-existent-empty and _build/default/x.gen
+  differ.
+  [1]
 
   $ dune promote
+  Promoting _build/default/x.gen to x-non-existent-empty.
 
   $ file_status x-non-existent-empty
-  x-non-existent-empty missing
+  x-non-existent-empty exists
 
   $ file_status x-non-existent-non-empty
   x-non-existent-non-empty missing
 
   $ dune build @blah-non-existent-non-empty 2>&1 | dune_cmd subst '^.+/diff:' 'diff:'
   File "x-non-existent-non-empty", line 1, characters 0-0:
-  diff: x-non-existent-non-empty: No such file or directory
+  --- x-non-existent-non-empty
+  +++ y.gen
+  @@ -0,0 +1 @@
+  +foobar
+  \ No newline at end of file
   [1]
 
   $ dune promote