Warren, I'm glad you like the idea... and thanks also for making a stab at bringing the branch up-to-date: I'll try and give it a go in the morning (it's late here at the moment).

Regarding your "concerns":

My main response is that I didn't think of that branch as being "merge ready", but at the stage where some discussions about it could be had: the result of those discussions would then be used to make it "merge ready".

• From memory and a quick skim of info.c just now, I think most of the conditional blocks can be eliminated: when writing the code, there were several things (like dividers between diff-blocks) that I thought improved things, but as a newbie to Fossil development didn't feel qualified to make wholesale decisions on without discussion... hence the big comment-block at the top of info.c. The "plan" was that after discussion, most of the #ifdef blocks would go (either completely if the change wasn't liked, or become "normal" code if liked).

• Many of the if/else blocks could certainly change to use the ternary operator: I think I kept them separate mainly because (a) I didn't know how extensive the changes for local-mode would need to be at any given point, and (b) I wanted to make sure the non-local code remained intact while I was fumbling my way around the new stuff.

• And I agree that all the TODO:LD would want to go before a merge: they're either "aide memoires" to me as to what I was thinking at the time (as I worked out how the Fossil framework worked), or mark points that would be affected by the above mentioned discussions.

Fossil has diff --tk,

Perhaps the branch has this already (i got lost in the huge diff!), but it might be useful to add a --ui flag to the diff command which simply exec's fossil ui --page X, where X is the new page name. That would make it easier for --tk users to migrate to this new approach.

No, at this stage I had not modified the diff command, although adding a --ui option was one possible way I'd considered.

One possible reason not to (at least at the stage I got to) is that the diff command can compare so many different "pairs of things", but only "tip vs. the local checkout" would work in UI mode. Having --ui only work for local-mode might be considered "wrong".

(At some stage, having all text-mode diff's options work in the GUI would/could be nice, but that would be a "phase 2" project!)

You should definitely be able to say things like fossil diff --ui --from trunk --to prev to skip over the current checkout version and diff the prior commit against tip-of-trunk.

That isn't a blocker for this branch, but being able to use Fossil's web diff instead of installing third-party GUI diffing tools has been on my wish list for a long time.

I brought his branch up to date, and it seems to work, though I don't know enough about what all it does to properly evaluate whether it's all working properly.

I've tried your up-to-date commit and while most things seem to be working OK, I have found one thing that has broken. In a "diff block":

Local changes of aa6.txt from [a7ffc6f8bf] to [local file]

The "[local file]" link was supposed to display the local (current checkout) version of the file in question: at the moment it just gives ERR_EMPTY_RESPONSE... I'll look into that, but in the meantime...

As I've said, my last check-in was at a point where all things I think should be in a local-diff function were there, but there were a few questions that I wanted feedback on before it would potentially be ready for merging: answering these will allow most (if not all) of the #ifdef blocks to be removed, and other "verbose" bits to be simplified (e.g. by using the ternary operator).

#define LOCAL_DIFF_MAX_EXTRAS (5)

At the top of the list of differences, I show "missing" files (roughly similar fossil extras). To stop the output being overwhelmed by having forgotten to add a suitable entry to ignore-glob/clean-glob/keep-glob, I limit the list of "extra" files unless the user asks for them all.

I've been using my local-diff function fairly constantly since I wrote it, and five seems to be about the right number: if the various -glob settings are up-to-date, it's sufficient to alert you if you've created a new file and forgotten to fossil add it. I'll leave it at that unless there's any objections.

#define LOCAL_DIFF_EXTRAS_MODE ("")

This controls whether the "extras" are shown by default, or whether they have to be manually requested. Again from personal use, I find displaying them by default very useful (to catch the forgotten "fossil add" scenario), but would go with the consensus if it's against.

A possible follow-up question is should this (and possibly the limit on how many are shown) be just compile-time settings, or should they be available as run-time configuration options?

#define LOCAL_DIFF_USE_TWO_PASSES (1)

One of the things I noticed while developing the local-diff function was that it was easy to not notice single-line reports (e.g. "Added file5.c but not committed.") if there were large diff-blocks either side. With this option enabled, two passes are made over the list of differing files: the first just collects "single-line" reports; the second performs the full-blown text-diff on the remainder.

Again(!) from personal use, doing this makes things much clearer. Possible reasons not to are (a) it alters the order of entries (which seem to be in alphabetic order), and (b) making two passes over the list of differences might be thought "wasteful" (but I've never seen a "response time problem").

If thought useful, a follow-on question is whether the change should be added to "normal" check-in page. (I didn't attempt to do this, but from memory, it didn't look like it would be problematic).

#define LOCAL_DIFF_ADD_DIVIDER (1)

My last change was to add a solid divider between diff-blocks from different files, again in the name of clarity (as an extension of the dotted-line the existing code shows between two "chunks" of difference within a single file). Without it, when skimming diffs, I sometimes didn't notice that the file being compared had changed.

I guess one concern is whether it "breaks" any of the skins: something I've not really played around with much.

Automated Tests

While I've done manual testing of my changes (and they break none of the existing test suite), I admit I've not really looked at adding any tests: I had a (very) quick look at the "/test" directory, but didn't immediately see anything from which I could work out "how to write a test for GUI functions". Do GUI functions have automated tests? Can someone point me in the right direction if some should be created for my changes?

I guess one concern is whether it "breaks" any of the skins: something I've not really played around with much.

Those are, since very recently, much easier to test: the /skins page lists the built-in skins and following each link will prepend its skin name to the URL, which is now a way to switch skins dynamically. Prepending the appropriate skin name to your URL, e.g. /skn_darkmode or /skn_xekri, will let you quickly try out a page in multiple skins.

Do GUI functions have automated tests?

We don't have any mechanisms in place for automated UI tests.

I've found the problem that was stopping the merge-from-trunk viewing local files: there was a call at line 3198 of src/file.c to mimetype_from_name() that looks like it has been moved from there to several places above, depending on "various things", with the result that it wasn't being called in local-diff mode. I've made a change locally:

  if( !bLocalMode ){
    style_submenu_element("Download", "%R/raw/%s?at=%T", zUuid, file_tail(zName));
    if( db_exists("SELECT 1 FROM mlink WHERE fid=%d", rid) ){
      style_submenu_element("Check-ins Using", "%R/timeline?uf=%s", zUuid);
    }
  } else {
    zMime = mimetype_from_name(blob_str(&downloadName));
  }
  if( zMime ){

to reinstate the call in local mode, and that seems to work.

However, I've not been able to push the change as Fossil says I'm not authorised to write:

Push to https://fossil-scm.org/home
Round-trips: 1   Artifacts sent: 0  received: 0
Error: not authorized to write
Round-trips: 1   Artifacts sent: 0  received: 0
Push done, sent: 1933  received: 221  ip: 45.33.6.223

I don't know whether permission was revoked, or I've got something wrong at my end. Is someone able to check whether use graham has write permission?

I don't know whether permission was revoked, or I've got something wrong at my end. Is someone able to check whether use graham has write permission?

Your account has developer rights, which includes checkin. i just made a few checkins as well, so it seems to be working. i notice that your sync URL does not include your name:

Push to https://fossil-scm.org/home

and suspect that you're using a clone pulled anonymously. Try:

fossil pull https://graham@fossil-scm.org/home

That will prompt for your password, when you should be able to push.

Thanks, the missing username was the problem and I've pushed the change that restores my original functionality.

Shouldn’t that sync URL have “graham@” at the start? It’s an anonymous sync otherwise. Fix with the “?URL?” arg to “fossil sync”.

I have found one thing that has broken.

I'm not surprised. There were a bunch of merge conflicts, and I had to patch them up without completely understanding what the original did, across about 3/4 of a year of trunk changes.

answering these will allow most (if not all) of the #ifdef blocks to be removed

My worry about all the conditional code isn't so much these #ifdefs, it's all the if (bLocalMode) stuff the branch adds. Unchecked, this sort of thing can cause a combinatorial explosion where even the developers most intimately familiar with the code have trouble figuring out what the result will be without tediously tracing the code paths.

Obviously you have a well-justified mode here, but what I'm suggesting is that you try to constrain it to affect the least amount of code in info.c as possible. Try to abstract things to a higher level so the code common to both modes is less affected.

I saw some of this in the code already, where you'd do something like pull a file from disk into a Blob structure and then pass it along to a bit of code that was originally written with the assumption that the Blob came from the repo DB. This sort of thing is good: it reuses the existing mechanisms unchanged, affecting the result only in the preparation of the input data, so the processing and output stages can remain unchanged.

All I'm saying is, "More of this, please!"

#define LOCAL_DIFF_MAX_EXTRAS

I have no strong feeling on this, but since it's only used in a single place, I don't see the need for a named constant. Or rather, a doubly-named constant. Simply say

  static const int maxExtrasToShow = 5;

and be done with it.

#define constants are best used when they're needed in multiple places.

#define LOCAL_DIFF_EXTRAS_MODE

This and the other two modes seem like changes you should just decide on as the feature's creator and defend them, as you've done here.

Given that you aren't getting push-back on these, I'd say you should take the current defaults as fixed and drop the conditional mode to simplify the patch.

But again, this is the smaller part of my concern over reducing conditionals. Having decided this minor matter, move on to reducing all of that if( bLocalMode ) stuff as much as you can.

Good work so far!

Thanks for the feedback, which all makes sense.

This and the other two modes seem like changes you should just decide on as the feature's creator and defend them, as you've done here.

Ok. I'll shortly do this, and remove associated #ifdef blocks. I'll also purge many of the "TODO:LD" blocks, many of which – now I've looked over the code again – are more "notes to myself" that I added as I was stumbling my way around the original code and trying to work out what bits did what (which actually was quite a pleasant experience: I was prepared to be far more confused than I was :-))

But again, this is the smaller part of my concern over reducing conditionals. Having decided this minor matter, move on to reducing all of that if( bLocalMode ) stuff as much as you can.

Once I've done the first stage, I'll revisit my changes and see what I can do in this regard (some simplifications are obvious; some might have to be thought about in a bit more detail...).

I've found a bug in the current version: this URL shows the file's commit manifest in the left-hand pane, not the old file version's diffs:

  http://localhost:8080/localdiff?name=src/info.c&from=trunk

Thanks for the report. Can you tell me whether you got to that URL by following links, or by manual editing?

I've not had a chance to look at the code, but from a very quick experiment in a local repository, if I start from "http://localhost:8080/local", select "Hide Diffs" and then view an individual diff, I get a URL like:

http://localhost:8080/localdiff?name=handlers/market.js&from=00e4173a04a87b29

and if I replace "00e4173a04a87b29" with "trunk" I get the same effect.

What I can't remember is whether it is supposed (for some definition of "supposed") to accept anything other than hashes at that point... I'll try and have a look, but have been tied up with other things recently, so may be a day or so before I get a chance.

I went directly there, composing the URL from the "fossil help /localdiff" output.

I've merged trunk into this branch once more, but beware, it's in danger of being closed due to lack of interest. It builds and runs, but it doesn't seem to function as expected, and there's a lot of to-do type stuff needed to get this into a mergeable state.

If Graham isn't coming back to finish this, this feature branch needs a new champion.

On mobile, so can't check anything at the moment, but I'll try to have a look at what you've merged to see if something can be done.