I just tried to build fossil under Cygwin. fossil ui revision f3c1a27646c, which I self-built from source, starts the browser.

Fossil version 999d838de2 reports that it is unable to start the browser. Also fossil --www --checkin tip (which is why I was building that version) doesn't start the browser.

fossil set | grep web shows web-browser unset.

Bisecting shows revision f3acbe429aa460c8 is where it breaks. Last good revision was 56a40e3b9d803ae8 and the checkin for the breaking revision says:

Enhance the "fossil ui" command so that the REPOSITORY argument can be on a remote system.

so at least it's changing code in the right area. Is there a debug flag or something that I can enable to get more output to help debug this?

I also get a few warnings:

pikchr.y:4984:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]
pikchr.y:4985:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]

but I can't see how they are related.

For MSVC builds, the following patch is required for the new --www option to start the web browser, analogous to this code in main.c. Not sure if this is also required for Cygwin, however.

Index: src/diffcmd.c
==================================================================
--- src/diffcmd.c
+++ src/diffcmd.c
@@ -321,11 +321,15 @@
       fossil_print("<script>\n%s</script>\n", zJs);
     }
     fossil_print("%s", zWebpageEnd);
   }
   if( (diffFlags & DIFF_WWW)!=0 && nErr==0 ){
+#ifdef _WIN32
+    char *zCmd = mprintf("%s %s", fossil_web_browser(), tempDiffFilename);
+#else
     char *zCmd = mprintf("%$ %$", fossil_web_browser(), tempDiffFilename);
+#endif
     fclose(diffOut);
     diffOut = freopen(NULL_DEVICE, "wb", stdout);
     fossil_system(zCmd);
     fossil_free(zCmd);
     diffOut = 0;

I also noticed that starting with [5380333f63] there's now always horizontal scrollbars for the whole diff page, not only for the individual side-by-side diff panels, in both Chromium-based browsers and IE. But the horizontal scrolling range is just minimal, i.e. a fraction of the full page width, and unnecessary, i.e. there's no clipped contents that can be scrolled into view. I'm not sure how to fix this, however.

Or, maybe better with quotes around tempDiffFilename, in case the path contains spaces.

Index: src/diffcmd.c
==================================================================
--- src/diffcmd.c
+++ src/diffcmd.c
@@ -321,11 +321,15 @@
       fossil_print("<script>\n%s</script>\n", zJs);
     }
     fossil_print("%s", zWebpageEnd);
   }
   if( (diffFlags & DIFF_WWW)!=0 && nErr==0 ){
+#ifdef _WIN32
+    char *zCmd = mprintf("%s \"%s\"", fossil_web_browser(), tempDiffFilename);
+#else
     char *zCmd = mprintf("%$ %$", fossil_web_browser(), tempDiffFilename);
+#endif
     fclose(diffOut);
     diffOut = freopen(NULL_DEVICE, "wb", stdout);
     fossil_system(zCmd);
     fossil_free(zCmd);
     diffOut = 0;

What is the failure mode here? It works as written for me on Window10 compiled with MSVC. I cannot get it to fail. Why do you think that the filename should be unquoted on Windows? What if there is a space in the temporary filename path?

What is the failure mode here? It works as written for me on Windows 10 compiled with MSVC. I cannot get it to fail.

Yes, I can confirm that things work fine with the default settings.

My interpretation of the web-browser setting was that this can be a sequence of commands (the online help says: "A shell command ...") passed down to fossil_system(), rather than just one single-word command. At least that's how it works with the fossil ui command:

> fossil set web-browser "start \"\" cmd /k echo"
> fossil set web-browser
web-browser          (local)  start "" cmd /k echo
> fossil ui

(** New console window **)
http://localhost:8080/timeline?c=current

But not with:

> fossil diff --by
The system cannot find the path specified.

Maybe my interpretation of the web-browser setting is wrong, but I thought for best compatibility with existing settings that work for fossil ui, maybe the same approach should be used?

Why do you think that the filename should be unquoted on Windows? What if there is a space in the temporary filename path?

See the 2nd version of the patch with quotes around the temporary file name. Simple quoting seems sufficient because a file name should never end with a backslash to escape the trailing quote.

---

Also, shouldn't fossil diff --b[y] abort before launching a web browser with an empty page, if the requested diff is empty? Or maybe only if no explicit --from, --to or --checkin options are present?

Oh, and yes, the patch above requires this 2nd part to make the pedantic «start "" ...» variant the default, or the quoted URL would be interpreted as the window title!

So this 2nd part also changes the built-in default -- and hence breaks all repositories with the old form of the start command without the empty quotes for the window title in the settings.

So, perhaps not a good idea to distribute this widely?

--------------------------------------------------------------------------------

Fossil baseline: [1422b02222]

Index: src/util.c
==================================================================
--- src/util.c
+++ src/util.c
@@ -834,11 +834,11 @@
 ** Return the name of a command that will launch a web-browser.
 */
 const char *fossil_web_browser(void){
   const char *zBrowser = 0;
 #if defined(_WIN32)
-  zBrowser = db_get("web-browser", "start");
+  zBrowser = db_get("web-browser", "start \"\"");
 #elif defined(__DARWIN__) || defined(__APPLE__) || defined(__HAIKU__)
   zBrowser = db_get("web-browser", "open");
 #else
   zBrowser = db_get("web-browser", 0);
   if( zBrowser==0 ){

--------------------------------------------------------------------------------

Thanks, @drh, for your work -- your solution is even better and more safe than mine!

My solution predates the introduction of the (handy!) %$ printf type, and doesn't work with URLs themselves containing double quotes -- but that was a compromise I was willing to make, since URLs with " were much rarer than URLs with & when using the --page option of fossil ui.

I also noticed that starting with [5380333f63] there's now always horizontal scrollbars for the whole diff page

I'm not able to reproduce this problem. I've tried:

• Linux: Firefox and Chrome
• MacOS: Firefox, Chrome, and Safari
• Win10: Firefox and Edge

I don't ever get a horizontal scrollbars on the whole diff page. I do, of course, get the smaller column-wide horizontal scrollbars on side-by-side diffs when the column is too narrow, but I don't think that is what you are talking about is it? You mean a big horizontal scrollbar at the very bottom of the page that spans the whole window, right? I don't ever see that.

Suggestions on how to repro are welcomed.

Hi,

I see there are new preview builds of Fossil available from the 21st of Aug. would it be possible to make new ones, specifically for Windows? I’d like to try the new features out and see what breaks on Windows.

No rush, though. I won’t be back at a computer until Friday morning.

You mean a big horizontal scrollbar at the very bottom of the page that spans the whole window, right?

Yes, in all my browsers, I extra bottom scrollbars similar to this example:

https://i.postimg.cc/gjJhd8T3/scrollbars.png

I'm running Windows 10 on a laptop with a 1920x1080 pixels monitor at the default 96 DPI (100%).

Without deeper understanding of this CSS mystery:

With these two lines removed (i.e. setting just maxWidth, but not width), the unnecessary page-level horizontal scrollbar at the bottom disappears, but the individual side-by-side or unified diff blocks still have horizontal scrollbars if necessary to show all clipped contents.

Does the feature still work as intended like this, and also with Firefox? Or is there a CSS wizard with a better (and probably more correct) solution?

If yes, I'd be glad if this could be fixed, as it affects all my diff views of the canonical Fossil web site.

If yes, I'd be glad if this could be fixed, as it affects all my diff views of the canonical Fossil web site.

Mine, too, with both FF and Chrome. It seems to be resolved now by setting the width to 98% instead of a pixel value.

Thanks for having a look at this! It works better for me, now.

My gut feeling tells me that there should be a CSS-only solution -- but my repeated attempts to find one with the help of the browser developer tools have come to nothing.

I fired up the debugger on 56a40e3b9d803ae8(good) and f3acbe429aa460c8(bad). I set a breakpoint at fossil_web_browser() in ./src/util.c.

It looks like f3ac doesn't have _WIN32 defined for the cygwin case. It gets the value of zBrowser from the else clause:

 zBrowser = db_get("web-browser", 0);

and reports warning: cannot start browser

In comparison, 56a40 has the _WIN32 branch and uses:

zBrowser = db_get("web-browser", "start");

Not sure why _WIN32 isn't defined for f3ac.

Same compiler (using gcc-10.2.0 (64-bit)), same Cygwin environment. Diffing config.h created with f3ac against 56a40 shows:

diff -u src/config.h src/config.h.good
--- src/config.h        2021-08-26 14:07:55.034564300 -0400
+++ src/config.h.good   2021-08-26 14:07:45.210494900 -0400
@@ -68,12 +68,6 @@
 #  ifndef _WIN32
 #    define _WIN32
 #  endif
-# include <io.h>
-# include <fcntl.h>
-# undef popen
-# define popen _popen
-# undef pclose
-# define pclose _pclose
 #else
 # include <sys/types.h>
 # include <signal.h>

so those lines are present in the bad file but not the good file. I don't see any redefines of _WIN32 in those included files or their includes.

gcc -E -dM --include src/config.h - < /dev/null| grep WIN | wc -l returns the same 21 lines when run using config.h or config.h.good.

Ideas?

pikchr.y:4984:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]
pikchr.y:4985:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]

I've tried building with every version of gcc and clang and MSVC that I have at hand, using -Wall -Wextra, but I still get no such warnings. And the line-numbers are wrong on the report so they are no help either.

Can you please explain how to repro these (harmless) warnings so that I can fix them?

Hi Richard, got me. I type make and it throws the warnings.

I tried to make it with some additional verbosity and I get:

In file included from ./src/pikchr.c:122:
pikchr.y: In function ‘pik_parse_macro_args’:
pikchr.y:4984:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]
pikchr.y:4985:36: warning: array subscript has type ‘char’ [-Wchar-subscripts]

In my copy of that file the line is #include <ctype.h>. I use gcc version 10.2.0. Not sure if that helps.

Any ideas on why fossil ui fails to work?

I configure with: fossil/configure --json --with-tcl-private-stubs --with-th1-hooks --with-openssl=auto --with-zlib=auto

Edited to append casting precaution.

It appears that the implementation of isspace() is defective for the compiler you are using. The error message is referring to lines 7592 and 7593 of pikchr.c which, due to a #line directive, are annotated as coming from lines 4984 and 4985 of pikchr.y and read: while( t->n>0 && isspace(t->z[0]) ){ t->n--; t->z++; } while( t->n>0 && isspace(t->z[t->n-1]) ){ t->n--; } , where the isspace macro argument begins at column 32.

Would you be willing to help confirm this by making those lines read: while( t->n>0 && isspace((int)(t->z[0])) ){ t->n--; t->z++; } while( t->n>0 && isspace((int)(t->z[t->n-1])) ){ t->n--; } ?

The argument is a char in those lines, which the isspace macro expansion should accept without back-talk. I surmise that the macro expands to a simple use of the argument to index into the flags array that ctype.h uses to classify chars.

If you can confirm that the casts I inserted cure the complaint, the cure will be easy to apply to pikchr.y, with confidence.

CAUTION: On platforms where a plain char is signed, that simple cast to int may be incorrect. It would be prudent to write the cast as (int)(unsigned char) so that the result conforms to the C standard requirement that the char classification argument is between 0 and 255 inclusive.

Hi Larry:

The casts do remove the warning.

I didn't see any #line 4984 directive in src/pikchr.c though. How did you track down the location?

-- rouilj

At line 3372 of the pikchr.c I have (which is likely the same as your copy), there is a preprocessor directive which is: #line 765 "pikchr.y" . That tells the compiler to report errors in the code following as having originated from pikchr.y, with line numbers calculated by offset from that #line line. I merely replicated the same arithmetic, using your error report, and confirmed that two successive lines possibly could generate such an error, given only a slightly naive definition of isspace(). You have confirmed the naivety.