Fossil diff shows error even though the diff is correctly displayed. This was working fine until I updated to 2.26

fossil diff --command 'diff -u --color=always' application.py
Index: application.py
==================================================================
--- /var/tmp/application~orig.py	2025-05-31 10:09:55.946934103 -0400
+++ /home/fran/bin/tijeras-repo/application.py	2025-05-31 10:05:30.636918139 -0400
@@ -122,3 +122,32 @@
 
+
+class line_position:
+	def __init__(self, *position):
+		self.pos = list(position) if position else [0]
...
...
External diff command failed: diff -u --color=always /var/tmp/application~orig.py /home/fran/bin/tijeras-repo/application.py

Fossil diff shows error even though the diff is correctly displayed.

That happens because your diff program is exiting with non-0, which is the conventional way for C-invoked apps to report an error.

Edit: that warning was added recently because it's otherwise very difficult to track down problems caused by the diff program not starting.

I think it depends... most application yes, but programs like gnu diff will only use 0 as exit status when files are identical.

see on the bottom of this diff manpage:

       ...
       Exit status is 0 if inputs are the same, 1 if different, 2 if
       trouble.

BSD Diff (what is installed on MacOS) is similar:

EXIT STATUS
     The diff utility exits with one of the following values:

           0       No differences were found.
           1       Differences were found.
           >1      An error occurred.

As a "generic" solution (because may be some other program may return 1 to report error),
perhaps we could capture stderr output, and if it's not empty, we could print the warning.
together with the output of stderr..

BTW: The diff command could be prefxed with cmd /c to get CMD.EXE behavior.

POC TODO: close returned process and thread handles.

... but I think fossil_system() won't allow such fanciness, anyway.

Indeed: as soon as the command contains any redirection or boolean operators, Fossil aborts with Unsafe command string: .... So using complex shell commands hasn't been possible for a while, already.

POC TODO II: expand environment variables.

I have a patch to address this on Windows (pasted at the bottom):

Examples:

• diff is the diff utility
• DIFF_NOT_FOUND is an invalid command
• A and B are files containing their names

Old behavior on Windows:

C:\...>fossil test-fossil-system
system-test> diff A B
cmd: [diff A B]
1c1
< A
---
> B
result: 1
system-test> DIFF_NOT_FOUND A B
cmd: [DIFF_NOT_FOUND A B]
'DIFF_NOT_FOUND' is not recognized as an internal or external command,
operable program or batch file.
result: 1
system-test>

• The exit code isn't helpful to spot invalid commands.

Patched behavior on Windows:

C:\...>fossil test-fossil-system
system-test> diff A B
cmd: [diff A B]
1c1
< A
---
> B
result: 1
system-test> DIFF_NOT_FOUND A B
cmd: [DIFF_NOT_FOUND A B]
result: -1
system-test>

• Failure to invoke the diff utility now returns exit code -1.

But I get the same problem on FreeBSD:

$ ./fossil test-fossil-system
system-test> diff A B
cmd: [diff A B]
1c1
< A
---
> B
result: 256
system-test> DIFF_NOT_FOUND A B
cmd: [DIFF_NOT_FOUND A B]
sh: DIFF_NOT_FOUND: not found
result: 32512
system-test>

• The exit code (i.e. zero vs. non-zero) isn't helpful to spot invalid commands, either.

I think the other suggested solution about checking stderr doesn't help either, as both error messages of the shell about invalid commands, as well as command errors, go to the same channel.

While digging into this, I found that the error messages are clear:

'DIFF_NOT_FOUND' is not recognized as an internal or external command,
operable program or batch file.

sh: DIFF_NOT_FOUND: not found

Users replacing their standard shells may see different error messages, but they're likely to recognize them, too?

I somehow think this problem is too rare to be worth addressing. Maybe just add a hint to use --systemtrace in case of diff problems to the diff-command help?

---

PATCH TO FOSSIL [9b04150885]:

Modify `fossil_system()' to bypass the shell on Windows, so that the returned
value can be used to distinguish failure to launch the external diff tool from
the external diff tool running successfully but returning a failure code. Keep
forwarding commands beginning with `start ' to the shell, so that URLs and web
browsers can still be launched without blocking.

Index: src/diffcmd.c
==================================================================
--- src/diffcmd.c
+++ src/diffcmd.c
@@ -667,11 +667,13 @@
       blob_append_escaped_arg(&cmd, blob_str(&nameFile1), 1);
       blob_append_escaped_arg(&cmd, zFile2, 1);
     }

     /* Run the external diff command */
-    fossil_system(blob_str(&cmd));
+    if( fossil_system(blob_str(&cmd))==-1 ){
+      fossil_warning("External diff command failed: %b\n", &cmd);
+    }

     /* Delete the temporary file and clean up memory used */
     if( useTempfile ) file_delete(blob_str(&nameFile1));
     blob_reset(&nameFile1);
     blob_reset(&cmd);
@@ -760,11 +762,13 @@
     blob_append(&cmd, pCfg->zDiffCmd, -1);
     blob_append_escaped_arg(&cmd, blob_str(&temp1), 1);
     blob_append_escaped_arg(&cmd, blob_str(&temp2), 1);

     /* Run the external diff command */
-    fossil_system(blob_str(&cmd));
+    if( fossil_system(blob_str(&cmd))==-1 ){
+      fossil_warning("External diff command failed: %b\n", &cmd);
+    }

     /* Delete the temporary file and clean up memory used */
     if( useTempfile1 ) file_delete(blob_str(&temp1));
     if( useTempfile2 ) file_delete(blob_str(&temp2));

Index: src/util.c
==================================================================
--- src/util.c
+++ src/util.c
@@ -338,22 +338,37 @@
 ** This function implements a cross-platform "system()" interface.
 */
 int fossil_system(const char *zOrigCmd){
   int rc;
 #if defined(_WIN32)
-  /* On windows, we have to put double-quotes around the entire command.
-  ** Who knows why - this is just the way windows works.
+  /* Commands prefixed with "start " are assumed to call the CMD.EXE built-in
+  ** `start' cmdlet and are handed to `_wsystem()'.  For executable and batch
+  ** files using the same name, a file name extension needs to be added, such
+  ** as in `start.exe', or `start.cmd'.  Commands without the "start " prefix
+  ** are executed directly, bypassing the shell, so that invalid commands can
+  ** be distinguished from valid commands returning a failure code.
   */
-  char *zNewCmd = mprintf("\"%s\"", zOrigCmd);
-  wchar_t *zUnicode = fossil_utf8_to_unicode(zNewCmd);
-  if( g.fSystemTrace ) {
-    fossil_trace("SYSTEM: %s\n", zNewCmd);
-  }
-  fossil_assert_safe_command_string(zOrigCmd);
-  rc = _wsystem(zUnicode);
-  fossil_unicode_free(zUnicode);
-  free(zNewCmd);
+  if( fossil_strnicmp(zOrigCmd,"start ",6)!=0 ){
+    if( g.fSystemTrace ) {
+      fossil_trace("SYSTEM: %s\n", zOrigCmd);
+    }
+    fossil_assert_safe_command_string(zOrigCmd);
+    rc = win32_exec(zOrigCmd);
+  }else{
+    /* On windows, we have to put double-quotes around the entire command.
+    ** See the `CMD.EXE /?' help screen for explanations of this behavior.
+    */
+    char *zNewCmd = mprintf("\"%s\"", zOrigCmd);
+    wchar_t *zUnicode = fossil_utf8_to_unicode(zNewCmd);
+    if( g.fSystemTrace ) {
+      fossil_trace("SYSTEM: %s\n", zNewCmd);
+    }
+    fossil_assert_safe_command_string(zOrigCmd);
+    rc = _wsystem(zUnicode);
+    fossil_unicode_free(zUnicode);
+    free(zNewCmd);
+  }
 #else
   /* On unix, evaluate the command directly.
   */
   if( g.fSystemTrace ) fprintf(stderr, "SYSTEM: %s\n", zOrigCmd);
   fossil_assert_safe_command_string(zOrigCmd);
@@ -371,10 +386,72 @@
   fossil_limit_memory(1);
 #endif
   return rc;
 }

+#ifdef _WIN32
+/*
+** On Windows, execute a command directly through CreateProcess(), bypassing the
+** shell, so that invalid commands can be distinguished from valid commands that
+** return a failure code.  This allows for better error messages if the external
+** diff command is misconfigured.
+*/
+int win32_exec(const char *zCmd){
+  int rc;
+  STARTUPINFOW si;
+  PROCESS_INFORMATION pi;
+  wchar_t *wzCmd = fossil_utf8_to_unicode(zCmd);
+  wchar_t *wzCmdExp = NULL;
+  DWORD cchCmdExp;
+  ZeroMemory(&si,sizeof(STARTUPINFOW));
+  ZeroMemory(&pi,sizeof(PROCESS_INFORMATION));
+  cchCmdExp = ExpandEnvironmentStringsW(wzCmd,NULL,0);
+  wzCmdExp = fossil_malloc(sizeof(wchar_t)*cchCmdExp);
+  if( cchCmdExp<ExpandEnvironmentStringsW(wzCmd,wzCmdExp,cchCmdExp) ){
+    rc = -1;  /* Failure expanding environment variables. */
+    goto exit;
+  }
+  si.dwFlags = STARTF_USESTDHANDLES;
+  si.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
+  si.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
+  si.hStdError = GetStdHandle(STD_ERROR_HANDLE);
+  if( CreateProcessW(
+        NULL,       /* lpApplicationName */
+        wzCmdExp,   /* lpCommandLine */
+        NULL,       /* lpProcessAttributes */
+        NULL,       /* lpThreadAttributes */
+        TRUE,       /* bInheritHandles */
+        CREATE_DEFAULT_ERROR_MODE,  /* dwCreationFlags */
+        NULL,       /* lpEnvironment */
+        NULL,       /* lpCurrentDirectory */
+        &si,        /* lpStartupInfo */
+        &pi         /* lpProcessInformation */) ){
+      DWORD dwResult;
+      dwResult = WaitForSingleObject(pi.hProcess,INFINITE);
+      if( dwResult!=WAIT_OBJECT_0 ){
+        rc = -1;  /* Failure waiting for child process to exit. */
+        goto exit;
+      }
+      if( GetExitCodeProcess(pi.hProcess,&dwResult) ){
+        rc = (int)dwResult;
+      }else{
+        rc = -1;  /* Failure retrieving child process exit code. */
+      }
+      goto exit;
+  }else{
+    rc = -1;  /* Failure launching child process. */
+    goto exit;
+  }
+exit:
+  fossil_unicode_free(wzCmd);
+  fossil_unicode_free(wzCmdExp);
+  if( pi.hProcess) CloseHandle(pi.hProcess);
+  if( pi.hThread) CloseHandle(pi.hThread);
+  return rc;
+}
+#endif /* _WIN32 */
+
 /*
 ** Like "fossil_system()" but does not check the command-string for
 ** potential security problems.
 */
 int fossil_unsafe_system(const char *zOrigCmd){

Edit: that warning was added recently because it's otherwise very difficult to track down problems caused by the diff program not starting.

Citation: it was added here based on this forum thread.