must_send 0 causes bogus monitor messages for dropped commands to be sent to client scripts, which makes comc usage in scripts (without sync, which I haven't experimented with) touchy and unreliable. This patch moves script_monitor to only the code paths where the command is actually issued.
The comment, "// moved here to avoid having to reimplement must_send in scripts" can be omitted.
This patch was based on commit 17769c06c8b815c65fdb310c2500bb6da6f21477 from 2021, but I didn't get any merge conflicts when applying it to a recent (couple of weeks ago) source.
Thanks for the patch. This looks fine to me. Though I wonder, what do you mean by "comc usage in scripts touchy and unreliable?" Each
ncomcommand has a number that the server sends back incomcto confirm completion of that command, so you can always track which commands have been executed.Sorry, instead of "touchy and unreliable" I meant "practically impossible". When the client drops a sent command but still notifies scripts as if it had been sent, any script that increments its own ncom counter (because you cannot ever know the client's actual ncom state) will go out of sync unrecoverably. From what I've seen glancing at the client source, sync is supposed to deal with this, but I haven't looked into how it works yet. The scripting interface should have sent ncom with each monitored command (-1 in old protocols) instead of forcing scripts guess ncom, which would also make this functionality a lot more discoverable.
Committed, with changes. Thanks!