Really stick to new nomenclature, plus …

[Olf0]

1. Fix help output in src/bin/patchmanager-daemon/main.cpp, lines 51-61.
2. Fix and slightly extend argument evaluation logic in src/bin/patchmanager-daemon/patchmanagerobject.cpp, lines 941-953.
3. For the numerous changes due to the new nomenclature, see, e.g., Patchmanager's Wiki page Terms-and-wording, entry "enable".
4. "Sneak in change" to alter the timeout back from 3 to 1 minutes for CI runs on MRs to the master branch.

[Olf0]

Open questions:

1. What is the technical difference between calling patchmanager (old: "run as daemon, new: "Run Patchmanager as daemon") and calling patchmanager --daemon (old: "daemonize", new: "Daemonize Patchmanager")?
2. How to express / differentiate that concisely in these two statements? Currently they sound confusingly similar.

[nephros]

1. What is the technical difference between calling patchmanager and calling patchmanager --daemon

patchmanager --daemon is how it is launched from the d-bus service so it backgrounds and starts to accept calls via that service.

Calling patchmanager actually does not do anything, it just checks for the root user and refuses to run if it isn't root, otherwise it will just output the help info.

Therefore I conclude that the first line in the usage output is plain wrong.

https://github.com/sailfishos-patches/patchmanager/blob/master/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L941

[Olf0]

1. What is the technical difference between calling patchmanager and calling patchmanager --daemon

patchmanager --daemon is how it is launched from the d-bus service so it backgrounds and starts to accept calls via that service.

https://github.com/sailfishos-patches/patchmanager/blob/master/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L941

Thanks, this is what I failed to find quickly yesterday night.

So "Starts patchmanager-daemon" would be a simpler and better explanation of the option --daemon, right?

Calling patchmanager actually does not do anything, it just checks for the root user and refuses to run if it isn't root, otherwise it will just output the help info.

Oh, obviously args.count() == 1 is not handled at all.

Therefore I conclude that the first line in the usage output is plain wrong.

So I will change it to "Print this help text.", O.K.?

I also think that there should be an option --help, which achieves the same. Currently any unknown argument(s) results in it / them being sent to the daemon via D-Bus, printing [D] unknown:0 - void PatchmanagerObject::process() Has arguments, sending D-Bus message and quit.
So inserting
} else if (args.count() == 2 && args[1] == QStringLiteral("--help")) {
return ;
between lines 946 and 947 will do, right?

Three more things I am considering:

1. I never called patchmanager-daemon [-a|-u|--unapply-all], always used patchmanager [-a|-u|--unapply-all]. AFAICS (and tried) calling patchmanager-daemon is wrong and yields nothing. Hence the -daemon suffix shall be dropped throughout the help text, correct?
2. Document the option --reset-system here. "Fully reset Patchmanager" would be my first take, unless you can easily think of a better or more specific phrase. I know you once researched what --reset-system does, if you do not remember, never mind, IMO a very abstract phrase is better than nothing.
3. Conceptually, I always modelled the patchmanager command as a CLI front-end for the patchmanager-daemon. I plan to denote that above all options, where it currently solely states patchmanager-daemon, which does not make much sense IMO.

[Olf0]

Oh, line 943 should be
} else if (args.count() == 2 && args[1] == QStringLiteral("--reset-system")) {
, right?

Actually then it would make sense to replace lines 941-947 with

if (args.count() == 2) {
    if (args[1] == QStringLiteral("--daemon")) {
        initialize();
    } else if (args[1] == QStringLiteral("--reset-system")) {
        resetSystem();
        QCoreApplication::exit(2);
        return;
    } else if (args[1] == QStringLiteral("--help")) {
        return;
    }
}
else if (args.count() == 1) {
    return;
}
else if (args.count() > 2) {
…

O.K.?

Side note: For me this cries for a case statement (both, outer and inner if levels), but "alter little" is paramount for me in C++ (although this is basically C here), otherwise I easily introduce flaws due to a lack of practice.

[nephros]

On the topic of --reset-system, I'd much prefer if it stayed undocumented (or rather, not exposed to users).

It causes a rather radical noninteractive change, (it determines the rpms owning any files changed by patches, downloads them and force-installs them). I guess it's supposed to fix issues caused by modifying files on disk.
As such it's a legacy from the days before the ld-preload method and unlikely to be needed any more.

Ref:

• reset system function
• Issue Secret functionality: --reset-system #204

[nephros]

Three more things I am considering:

1. I never called patchmanager-daemon [-a|-u|--unapply-all], always used patchmanager [-a|-u|--unapply-all]. AFAICS (and tried) calling patchmanager-daemon is wrong and yields nothing. Hence the -daemon suffix shall be dropped throughout the help text, correct?
   [...]

2. Conceptually, I always modelled the patchmanager command as a CLI front-end for the patchmanager-daemon. I plan to denote that above all options, where it currently solely states patchmanager-daemon, which does not make much sense IMO.

"There is no Dana patchmanager-daemon. There is only Zuulpatchmanager ".

patchmanager-daemon does not exist as a binary. It's also not used anywhere in the output AFAICS. There is only the source directory called that.

The patchmanager executable can operate in both (dbus-) daemon and client mode (when called with any option other than --daemon). Even when in client mode, it uses a running daemon instance to actually do anything (triggered by a dbus message).

[nephros]

Even when in client mode, it uses a running daemon instance to actually do anything (triggered by a dbus message).

Heh except for --reset-system which does not go through dbus.

[Olf0]

[…] Even when in client mode, it uses a running daemon instance to actually do anything (triggered by a dbus message).

This is what confused me into guessing that there must be a separate daemon when reading the help text.

Even when in client mode, it uses a running daemon instance to actually do anything (triggered by a dbus message).

Heh except for --reset-system which does not go through dbus.

… as the lines 941-947 and my code-snippet show.

Does your approval also cover this code-snippet? I.e., does it look O.K.?

[Olf0]

There is no Dana patchmanager-daemon, there is only Zuul patchmanager.

That did ring a bell, but I had no clue which, so I looked it up: O.K., that was in 1984!
Did not know though, that this phrase has become a meme. IIRC it is one of the truly cool scenes in this movie, but much of it is "Klamauk" (I guess "a whimsy" comes close, but is not really an equivalent).

[nephros]

… as the lines 941-947 and my code-snippet show.

Does your approval also cover this code-snippet? I.e., does it look O.K.?

Yep, LGTM.