Support multiple -*notify commands #22372

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:multinotify changing 3 files +43 −32

luke-jr commented at 7:14 pm on June 29, 2021: member

Useful, intuitive, and avoids creating issues with #22354
DrahtBot added the label Validation on Jun 29, 2021
DrahtBot added the label Wallet on Jun 29, 2021
DrahtBot commented at 11:39 am on June 30, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in src/wallet/wallet.cpp:996 in 71adc97c73 outdated

958-        // https://github.com/bitcoin/bitcoin/pull/13339#issuecomment-537384875
959-        // A few ways it could be implemented in the future are described in:
960-        // https://github.com/bitcoin/bitcoin/pull/13339#issuecomment-461288094
961-        boost::replace_all(strCmd, "%w", ShellEscape(GetName()));
962+    if (gArgs.IsArgSet("-walletnotify")) {
963+#ifdef WIN32

kristapsk commented at 0:03 am on July 1, 2021:

Comment below #ifndef WIN32 from previous code about why it’s not supported currently should be copied here IMO.

luke-jr commented at 6:54 pm on December 17, 2021:

Copied

kristapsk commented at 0:27 am on July 1, 2021: contributor

Concept ACK
benthecarman commented at 5:23 pm on July 3, 2021: contributor

Concept ACK
benthecarman commented at 5:25 pm on July 3, 2021: contributor

Looks like this doesn’t touch -startupnotify, does that support multiple already?
luke-jr force-pushed on Jul 3, 2021
luke-jr commented at 6:05 pm on July 3, 2021: member

Whoops, fixed.

in src/init.cpp:686 in 54b25a8c51 outdated

682@@ -683,9 +683,8 @@ static void CleanupBlockRevFiles()
683 #if HAVE_SYSTEM
684 static void StartupNotify(const ArgsManager& args)
685 {
686-    std::string cmd = args.GetArg("-startupnotify", "");
687-    if (!cmd.empty()) {
688-        std::thread t(runCommand, cmd);
689+    for (std::string command : gArgs.GetArgs("-startupnotify")) {

maflcko commented at 6:43 pm on November 10, 2021:

0    for (std::string cmd : args.GetArgs("-startupnotify")) {

Seems odd to use gArgs when args is passed in. Also, why change the symbol name to increase the diff?

luke-jr commented at 6:54 pm on December 17, 2021:

gArgs: Fixed.

Symbol name: It doesn’t increase the diff to adjust it to our current style.

DrahtBot added the label Needs rebase on Nov 10, 2021
luke-jr referenced this in commit c83d8d7a76 on Dec 14, 2021
luke-jr referenced this in commit 6c7304ff6e on Dec 14, 2021

scripted-diff: test: rename `strCmd` to `command`

-BEGIN VERIFY SCRIPT-
sed -i 's/\bstrCmd\b/command/g' $(git grep -l strCmd)
-END VERIFY SCRIPT-

5498850040

Support multiple -*notify commands 041b1ed8b7
luke-jr force-pushed on Dec 17, 2021
luke-jr commented at 6:55 pm on December 17, 2021: member

Rebased and addressed review comments
DrahtBot removed the label Needs rebase on Dec 17, 2021
DrahtBot added the label Needs rebase on May 6, 2022
DrahtBot commented at 8:51 am on May 6, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
achow101 commented at 6:34 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Oct 12, 2022
kristapsk commented at 2:25 pm on November 23, 2022: contributor

Guess this was closed just because of no rebase after May 6 and lack of ACKs? Seems useful in context of #25975.
luke-jr commented at 11:41 pm on November 26, 2022: member

Feel free to reopen. As with most of my PRs, I continue to maintain it for Knots.
bitcoin locked this on Nov 26, 2023

Contributors
luke-jr DrahtBot kristapsk benthecarman maflcko achow101

Labels
Wallet Validation Needs rebase

Support multiple -*notify commands #22372

Conflicts