Fossil Forum

RFC: Warning policy for commits and merges
Login

RFC: Warning policy for commits and merges

RFC: Warning policy for commits and merges

(1.1) By Preben Guldberg (preben) on 2023-10-18 07:37:12 edited from 1.0 [link] [source]

I am looking at implementing conditional warnings. The scenarios for could be committing to trunk when a release is pending, or merging a private branch into a publich branch as the history may be lost.

A somewhat similar concept was discussed in the past as a MOTD feature, e.g. on cloning.

I have updated the warn-on-merging-private-branch branch with an approach that introduces:

  1. A new propagating setting type, which is sent from server to client when the client pulls information (be it as part of a commit, merge, sync, etc.). FWIW, I experiemented on sending when the client holds a ci-lock, but that only worked for committing.

  2. A warning-policy setting with a JSON syntax described below. The policy is currently limited to commit and merge but can be expanded.

I have tried to include commit/merge oriented conditions that might be broadly useful - but may also have gone too far.

New settings and commands:

    % fossil help warning-policy
    Setting: "warning-policy" (default: {}) (propagating)

    Policy for showing warnings under certain conditions.

    The policy is a JSON object where the following names are recognised:

      commit:        Used when committing.  A list of objects with names in
                     (message, branch, except-branch, users, except-users).
      merge:         Used when merging.  A List of objects with names in
                     (message, branch, except-branch, from, except-from,
                     users, except-users, history-loss).
                     users, except-users, unpublished).
      match-style:   If "regexp", patterns use REGEXP, otherwise GLOB.

    Meaning of names used in lists above:

      message: MESSAGE          Required: Message to show.
      branch: PATTERN           Apply branch match PATTERN (default any).
      except-branch: PATTERN    Exclude when in a branch matching PATTERN.
      from: PATTERN             Apply if merging from PATTERN (default any).
      except-from: PATTERN      Exclude when merging from PATTERN.
      history-loss: true        If true, only show message when history loss
                                would occur; that is, when merging a private
                                branch into a public branch.
      unpublished: true         If true, only show when merging from a private
                                branch into a public branch.
      users: LIST               Show only for users in LIST (default any).
      except-users: LIST        Users in LIST will not be shown the messages.

    Example:

      {
        "commit": [
          { "message": "Release pending, proceed with caution.",
            "branch": "trunk",
            "except-users": [ "owner", "admin" ] }
        ],
        "merge": [
          { "message": "Please use 'fossil publish' before merging private to public",
            "except-branch": "rebased-branch-*",
            "history-loss": true },
            "unpublished": true },
          { "message": "Updates to release branches should be merged from rc.",
            "branch": "release-*",
            "except-from": "rc-*" }
        ]
      }
    %
    % fossil help test-warning-policy
    Usage:  fossil test-warning-policy EVENT ?OPTIONS?

    Test what messages would be shown for a specific scenario.
    Use the global -U|--user option to test for a specific user.

    Options:
      --json JSON

    Options for "commit" event:
      -b|--branch BRANCH   Test commit to BRANCH.

    Options for "merge" event:
      -b|--branch BRANCH   Test merge to BRANCH.
      -f|--from BRANCH     Test merge from BRANCH.
      -hl|--history-loss   Test merging from a private to a public branch.
      -u|--unpublished     Test merging from a private to a public branch.

Sample output when committing:

    % fossil commit -m test
    Pull from file:///Users/preben/tmp/fossil/warning-policy/base.fossil
    Round-trips: 1   Artifacts sent: 0  received: 1
    Pull done, wire bytes sent: 364  received: 1546  remote:
    Policy warnings:
        Release pending, proceed with caution.
    Continue anyway (y/N)? n

I'd appreicate feedback on the idea, approach, implementation or whatever selse comes to mind.

(2) By John Rouillard (rouilj) on 2023-10-17 17:44:46 in reply to 1.0 [link] [source]

I like the idea, but I am trying to figure out how this works in a distributed setting?

Is the warning policy synched from (one of possible multiple) upstream remotes?

Let's say I merge to trunk on my local repo. A warning is set on my upstream repository for that merge operation.

What happens when I sync? IIUC fossil sync just transfers any blob whose ID is not already in the upstream repo. It doesn't know at sync time anything about branches etc. It's why I can't push just a branch upstream, I push all (except private) artifacts in my repo.

It discovers branch/merge etc. info after the sync as it incorporates the artifacts.

So what would happen? Would I just see a message "Hey you weren't supposed to merge to trunk!"? If so isn't it too late, I already messed up trunk on the upstream repo.

(3) By Warren Young (wyoung) on 2023-10-17 22:55:23 in reply to 2 [link] [source]

What happens when I sync?

There are several parts of the repo config that are copied once on the first clone, then may diverge until you say “fossil config pull FOO”. I assume this JSON config data will be treated the same way.

(10) By Preben Guldberg (preben) on 2023-10-18 07:04:39 in reply to 3 [link] [source]

The pull I referred to was the pull that happens when you do a sync, pull, commit, merge, etc. Situations where an updated policy might have to apply.

My impression is that config pull is used by some, but certainly not all. So if a project maintainer would like, possibly temporarily, to have warnings show on some condition, that's not a reliable means to propagate the setting. So instead, I let the server send it to clients as part of the regular flow.

(12) By Preben Guldberg (preben) on 2023-10-18 08:01:03 in reply to 2 [link] [source]

I like the idea, but I am trying to figure out how this works in a distributed setting?

Is the warning policy synched from (one of possible multiple) upstream remotes?

The setting is sent from servers to clients, when the clients do a SYNC_PULL (done my sync, pull, commit, merge, probably more).

The client usually only accept config items that they requested (the various config pull areas), but propagating settings are accepted.

If you have multiple upstreams, the client could get different settings, depending on which upstream was last used. If upstreams are all tied to some common central repository (and have pulled), they may all send the same policy.

Let's say I merge to trunk on my local repo. A warning is set on my upstream repository for that merge operation.

What happens when I sync? IIUC fossil sync just transfers any blob whose ID is not already in the upstream repo. It doesn't know at sync time anything about branches etc. It's why I can't push just a branch upstream, I push all (except private) artifacts in my repo.

When you sync, the client receives the policy. No warnings are shown then, though, as no policy for sync can be defined. (At least presently, but if done it would be very similar in nature to the earlier MOTD discussion.)

It discovers branch/merge etc. info after the sync as it incorporates the artifacts.

So what would happen? Would I just see a message "Hey you weren't supposed to merge to trunk!"? If so isn't it too late, I already messed up trunk on the upstream repo.

On a commit or merge, the client attempts an autosync early on in the internal steps. Here the client would receive the policy to apply.

After other checks, like time skew, the policy is checked and any applicable warnings are shown, followed by a "Commit anyway (y/N)?" message (except commit --no-prompt which treats it as fatal.)

If using --force, the warnings are only shown after the commit/merge succeeds so as not to disrupt the flow, but still inform the user. At least as currently implemented.

(13) By Warren Young (wyoung) on 2023-10-18 08:13:21 in reply to 12 [link] [source]

Does your implementation recheck the config between autosync and commit? If not, it won’t catch a policy update until the next commit attempt.

(4) By Andy Bradford (andybradford) on 2023-10-18 01:32:30 in reply to 1.0 [link] [source]

> I am looking at implementing conditional warnings.

My first gut  reaction is, why? This kind of  functionality almost seems
misplaced with  respect to Fossil's laissez-faire  approach to procedure
and "policy".
 

> or merging a  private branch into a publich branch  as the history may
> be lost.

What's this? This is the first  I've heard of Fossil losing any history.
How does merging a private branch into a public branch lose history?

Thanks,

Andy

(5) By John Rouillard (rouilj) on 2023-10-18 02:48:28 in reply to 4 [link] [source]

IIUC when you merge a private branch into a public branch and push, none of the artifacts except the merge artifact is synced to the upstream server.

All of the private artifacts that made up the private branch are kept locally. So you "lose" the series of commits on the private branch.

(6) By John Rouillard (rouilj) on 2023-10-18 02:54:58 in reply to 4 [link] [source]

My first gut reaction is, why? This kind of functionality almost seems misplaced with respect to Fossil's laissez-faire approach to procedure and "policy".

I think this was driven by an accidental push that occurred a couple of weeks ago after the trunk was supposed to have stabilized. The developer had merged his changes to trunk. The merge to trunk may have been unintentional and was intended to merge trunk into a development branch and then push the development branch for review.

So it's kind of a guardrail or safety. Given a distributed environment, it may be a local-only option that a developer can add friction to accidentally merging in the wrong direction.

(14) By Preben Guldberg (preben) on 2023-10-18 08:13:38 in reply to 6 [link] [source]

So it's kind of a guardrail or safety. Given a distributed environment, it may be a local-only option that a developer can add friction to accidentally merging in the wrong direction.

Certainly meant as a guardrail.

As implemented, the setting would propagate from upstream to downstream, allowing project admins to update the policy in the main repository and let it propagate.

Currently it can be set as a local option, but if you pull from upstream, it is overridden. One could add checks not to do it if any remotes are defined, but that would make it harder to set it locally and perform a config push project to push it to the main repository.

That said, I could see the benefit of a local version of the same concept (instead of or in addition to the propagating version).

(8.1) By mark on 2023-10-18 03:33:06 edited from 8.0 in reply to 4 [link] [source]

DISCLAIMER: I haven't caught up with /chat so am not apprised of any previous
discussion yet.

I am looking at implementing conditional warnings.

My first gut reaction is, why? This kind of functionality almost seems misplaced with respect to Fossil's laissez-faire approach to procedure and "policy".

I share your same sentiments, tbh.

Project policy can be documented in tracked docs rather than annoying the user
on stdout and the project maintainer with more config. The onus is on developers
to be cognisant of such policy. Plus with /chat and /forum, notices can easily
be announced. Admittedly, the maintainer can still use embedded docs, chat,
and the forum, and simply ignore this feature, but I don't think this warrants
the code cost.

And in the accidental commit to the wrong branch case, fossil makes it very easy
to move such accidents to a new branch. Fossil also makes it easy to disable
autosync, which is already a builtin safety mechanism: you can correct any local
mistakes before pushing them upstream.

or merging a private branch into a publich branch as the history may be lost.

What's this? This is the first I've heard of Fossil losing any history. How does merging a private branch into a public branch lose history?

I suspect this is an incorrect use of "lost" if, as John remarks, this refers
to antecedent commits on the private branch not making it to the public branch
along with the merge commit. But, ime, this is a very useful point of private
branches. In the absence of histedit or rebase, private branches are a great
compromise to allow devs who work intermittently on larger features to treat
commands like fossil commit as :wq and still save the project from myriad
wip: doesn't build yet\n\nlist of current state and changes since the last "wip
commit"
, which can be unwanted even on feature branches let alone trunk. And
the eventual merge is simply a commit that contains the product of that private
branch and would only ever be considered a commit to all its reviewers if the
comment doesn't say things like merge x from private branch.

ETA: s/descendant/antecedent

(9) By Florian Balmer (florian.balmer) on 2023-10-18 06:29:29 in reply to 4 [source]

How does merging a private branch into a public branch lose history?

No history is lost, but the private merge parent is not recorded. Following is a copy of a message posted to chat:

Attached image: https://i.imgur.com/cYlLaD2.png

I'd also say the publish command is the best way to make a private branch public.

Private branches used to be marked by tag, but later the private tag was abandoned (but still handled for backwards compatibility) and private commits (and file versions exclusively appearing in private commits) were only recorded in the PRIVATE table (which already existed before as a cache, IIRC).

So as not to generate missing artifact references on peer repositories without the private branch, the merge parent is not recorded when merging the private branch into a public branch, and the timeline shows no merge arrow. That's how private branches are useful for "squash" or "rebase" commits: the result looks like a single direct descendant of the non-merge parent, without any references to the private branch, and generates no missing artifact references/phantoms on clones lacking the private branch. Therefore, such commits should probably not be called "merges" in the commit message, as no merge operation is really recorded. Also see the fine print in Private Branches → Publishing Private Changes.

An alternative to publishing a whole private branch with all the drafting is to merge from a private branch to a new public branch, first, to make a "squash" commit for review, testing and upcoming public changes, and later merge from the public branch to trunk, with the merge operation public-helper-branch-to-trunk recorded the usual way (see attachment).

(11) By Preben Guldberg (preben) on 2023-10-18 07:19:44 in reply to 4 [link] [source]

What's this? This is the first I've heard of Fossil losing any history.

Others have already explained what's happening. That was poor wording on my part.

I understand your reaction and see that it needs a different name ("unpublished" perhaps) and just explain that the condition is "merging a private branch into a public branch".

(7) By sean (jungleboogie) on 2023-10-18 03:25:28 in reply to 1.0 [link] [source]

I'd appreicate feedback on the idea, approach, implementation or whatever else comes to mind.

Fossil has a chat and a forum. I think an admin of a repo could post a message in either/both of those places to convey a release is pending and to be cautious about committing to trunk. There are also wiki pages and tech notes that could be leveraged.

Alternatively/additionally, what if a commit to trunk had some kind of visible UI element preset on the web UI timeline that would signal to committers to cautiously merge to trunk? Maybe some kind of padlock symbol or something. Perhaps it could also work on the cli as well with the timeline view.

I understand these ideas would require the developer to conscientiously check the chat, forum, tech/wiki notes, timeline, and not have the tool tell the developer of pending doom.

(15) By Florian Balmer (florian.balmer) on 2023-10-19 06:10:19 in reply to 1.1 [link] [source]

I still think: overengineering.

Moreover, the feature seems to rely on auto-sync to work properly. I'm way too cautious to work with auto-sync enabled, especially when dealing with the main Fossil repository--so I hope we can still get "pencils down" announcements and similar on the forum.

(16) By Florian Balmer (florian.balmer) on 2023-12-07 19:16:53 in reply to 1.1 [link] [source]

What's the status of this feature?

I remember drh expressing his interest about a "policy" or "guardrail" feature in chat.

Are the open questions answered:

  • wyoung: Does your implementation recheck the config between autosync and commit? If not, it won’t catch a policy update until the next commit attempt.

  • florain.balmer (me): Is the feature useful in any way for people working with autosync disabled?

We have a lot of open branches, at the moment, but most of them look like they're ready to merge, and I'd love to see them land on trunk soon for a "daily driver" to play around with.

The warn-on-merging-private-branch branch seems to be the only one with open questions, and I'd love to hear at least whether this is going to be pursued or abandoned.

(17) By Daniel Dumitriu (danield) on 2023-12-08 13:59:36 in reply to 16 [link] [source]

We have a lot of open branches, at the moment, but most of them look like they're ready to merge, and I'd love to see them land on trunk soon for a "daily driver" to play around with.

For example, I've just re-discovered Florian's timeline and diff keyboard navigation branches, which look like a lot of invested effort. Is there any interest in landing them on trunk?

I guess there is some branch triaging work lying ahead...

(18) By Florian Balmer (florian.balmer) on 2023-12-08 18:19:32 in reply to 17 [link] [source]

I guess there is some branch triaging work lying ahead...

I can understand very well that Richars's days probably don't have enough hours to closely follow, review and test every contributed commit on every branch in real time, and to regularly take care of pulling in all the new stuff, so things not actively pushed by contributors are sometimes left lying around.

But my impression from all the years is that it's very rare for Richard to actively reject contributions, as long as the main program flows remain streamlined, and the contributed features are not obvious "feature creep"--on the contrary, I think if some contribution solves a problem for somebody, it's usually accepted. (IIRC, an example is the legacy import and export commands, AFAIK not used by Richard and later superseded by his git commands when needed, but even if not used by the project architect they were accepted and now retained in the code base for people depending on them.)

What I noted is that Stephan has been very involved with the SQLite project, lately, logically with some decrease in his work for Fossil. And by that I also mean that Stephan--being Richard's fully trusted deputee--used to answer a lot of questions about contributions, de facto giving an "official go for merge" in case of favorable comments.

Yes, all this means (unpaid) efforts (in spare time)!

For example, I've just re-discovered Florian's timeline and diff keyboard navigation branches, which look like a lot of invested effort. Is there any interest in landing them on trunk?

I have been using these features in my private builds from the beginning, that's for more than a year now, without any clashes with other web UI JS stuff.

When browsing a repository, I sometimes forget about the timeline navigation hotkeys, but as soon as keyboard mode is activated by pressing any hotkey for the first time, I find it useful to switch between timeline and info views, and also to copy hashes and branch names without using the mouse. Moreover, when doing some "continuous work" on a series of check-ins, it's nice to use the keyboard focus indicator as a bookmark. Maybe not everything is super useful, but it's "lightweight" (as a feature, not necessarily as a served JS resource, see below) and useful enough to have, I think.

The diff hotkeys are just awesome! Going from side-by-side to unified and back with B and Shift+B is great--even if this causes a page reload and resets the scrolling position and the shown/hidden states of the diff blocks. The features to show and hide the individual file changes are my favorites, because especially for larger branch merges I may not be interested in every file, so I can press I to hide them all, then only unfold what I want to see. Or, even better, I can repeatedly press P to show file change after file change, so I'm always aware of the current file I'm in--this has become my standard way to do diff reviews.

But somehow I started to think that this may be "feature creep", not complying to the Fossil way of keeping things simple. Also, depending on the configuration, this adds almost 20 KB of JS source code (measured before comments are stripped and spaces are trimmed, and without HTTP compression taken into account) to each and every Fossil web page request, so it has impact on ALL Fossil users, even those not interested in the feature!

Moreover, it's using vanilla JS and not the Fossil JS Framework. When tracking down some browser differences, I realized there may be some maintenance burden with such legacy code (that stuff even works with my IE-compatible1 Fossil build). On the other hand, it was fun and interesting to hunt down the browser quirks, and I'm sure it will also be fun and interesting to fix future browser quirks, if required.

So yes, they're ready to merge. There's a note somewhere about a possible simplification in case both branches are merged together, something I would take care of, of course!


  1. ^ With some (simple) trickery, IE11 can still be launched as a standalone web browser program on Windows 11! ;-)

(19) By Preben Guldberg (preben) on 2023-12-11 15:18:06 in reply to 16 [link] [source]

What's the status of this feature?

For what it attempted to provicde, I think functional.

But, I think the general input is that it is not that desirable in the community.

I remember drh expressing his interest about a "policy" or "guardrail" feature in chat.

Indeed. I wanted to follow up on this and a couple of other branches, if dry would approve or not. Been a bit busy elsewhere of late, though.