improve scripted-diff check #19815

1. 
   amitiuttarwar commented at 3:12 am on August 27, 2020: contributor

   Context:

   Scripted diffs are a way to automate some types of reformatting or refactoring changes. When a commit message matches a specific format (begins with scripted-diff:), CI will run commit-script-check.sh.

   sed is platform dependent. One significant difference is when using the -i flag. On BSD (eg. macOS), you have to insert an empty string, but with GNU sed (used by Travis), this is compatible.

   example of a working command for these two platforms- macOS: sed -i '' 's/a oneshot/an addrfetch/g' src/chainparams.cpp Travis: sed -i 's/a oneshot/an addrfetch/g' src/chainparams.cpp

   Task:

   Add logic to the commit-script-check.sh to detect if the scripted diff is using the BSD sed syntax and print a helpful error message.

   Useful skills:

   Bash

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. fanquake added the label good first issue on Aug 27, 2020
3. 
   khuei commented at 11:29 am on August 27, 2020: none

   You can also use append empty string directly after -i to make sed -i work on both GNU and BSD sed:

   sed -i'' 's/a oneshot/an addrfetch/g' src/chainparams.cpp

   I want to work on this, but I think it is good to recommend people to do the above for portability for now.

4. 
   amitiuttarwar commented at 5:22 am on August 28, 2020: contributor

   @Z5483 hi! thanks for taking a look & opening a PR!

   I tried this trick out locally (macOS), but got an error… actually, I’m getting different errors on each try 👀
   (but doing -i '' is working fine)

   example 1: sed -i'' 's/MempoolUnbroadcastTest/LaLaLaTest/g' test/functional/mempool_unbroadcast.py error: sed: 1: "test/functional/mempool ...": undefined label 'est/functional/mempool_unbroadcast.py'

   example 2: sed -i'' 's/CMainParams/SuperImportantParams/g' src/chainparams.cpp error: sed: 1: "src/chainparams.cpp": unterminated substitute in regular expression

   example 3: sed -i'' 's/MAX_FEELER_CONNECTIONS/MAX_FEELS/g' src/net.h error sed: 1: "src/net.h": unterminated substitute pattern

   but I see you’ve opened a patch to update the script, so I’ll check that out :)

5. 
   robot-dreams commented at 10:32 pm on September 16, 2020: contributor

   @Z5483 Thanks for working on this! I’ve ran into some scripted-diff issues as well, and I’m very excited to see that you’re improving it :) @amitiuttarwar I’m a MacOS user as well, and I ran into the following issue; did you ever encounter this as well?

   0$ ./test/lint/commit-script-check.sh 85165d4~..85165d4
   1sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
   2Error: missing script for: 85165d4332b0f72d30e0c584b476249b542338e6
   3Failed
   

   I fixed it locally by installing gsed and creating a symbolic link. But do you think it would make sense to add something like the following to help future MacOS users?

   After making this change, here’s the new output:

   0$ ./test/lint/commit-script-check.sh 85165d4~..85165d4
   1sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
   2Error: Incompatible version of sed; MacOS users should install gsed
   3Failed
   

6. fanquake deleted a comment on Sep 19, 2020
7. fanquake deleted a comment on Sep 19, 2020
8. 
   bitkarrot commented at 2:32 am on December 6, 2020: none

   I’m signing up for this task (if still open) because @bliotti wants me to work on it as “I’m way overdue for a bitcoin commit” as a Jimmy Song Alumni, using my public face here.
9. 
   bliotti commented at 2:50 am on December 6, 2020: contributor

   @bitkarrot Awesome! Looks like a good one. @amitiuttarwar or @Z5483 where does this one stand? We would like to pick this one up if still available. Thanks!
10. 
    khuei commented at 5:53 am on December 6, 2020: none

    Feel free to work on this. I scrapped my PR because I didn’t put time into it so it’s horrible. I’m busy with schools for the rest of the school year so I don’t think I will pick this up again anytime soon. I hope you guys can produce something better than mine.
11. 
    ghost commented at 11:34 am on December 6, 2020: none

    Catching sed syntax usage that leads to unexpected results is not a low hanging fruit I guess :)

    Maybe you should pimp your own script like so:

     0kernel_name="$(uname -s)"
     1
     2case "${kernel_name}" in
     3    Linux*)     sed_option='-i'
     4                ;;
     5    Darwin*)    sed_option='-i ""'
     6                ;;
     7    *)          echo "Unknown kernel name: '${kernel_name}'"
     8                exit 1
     9esac
    10
    11sed $sed_option 's/a oneshot/an addrfetch/g' src/chainparams.cpp
    

    With that, you can define all specialities needed for different environments. You could pimp it even more like evaluating the sed version etc.

    Maybe you could create a PR to add this as a template to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#suggestions-and-examples

12. 
    bitkarrot commented at 0:06 am on December 21, 2020: none

    Looking into it but also going on holiday vacation; will be back in the new year
13. 
    rysolv-bot commented at 2:44 am on January 5, 2021: none

    themanmaran has contributed $50.00 to this issue on Rysolv. The total bounty is now $50.00. Solve this issue on Rysolv to earn this bounty.
14. 
    kvkenyon commented at 10:59 pm on February 18, 2021: none

    Anyone working on this now? @bitkarrot @amitiuttarwar
15. 
    amitiuttarwar commented at 7:11 pm on February 22, 2021: contributor

    @kvkenyon looks like #21002 and #21020 are open as proposals to improve this. would be helpful to review those :)
16. 
    bliotti commented at 7:05 am on May 17, 2021: contributor

    @Z5483 Thanks for working on this! I’ve ran into some scripted-diff issues as well, and I’m very excited to see that you’re improving it :)

    @amitiuttarwar I’m a MacOS user as well, and I ran into the following issue; did you ever encounter this as well?

    0$ ./test/lint/commit-script-check.sh 85165d4~..85165d4
    1sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
    2Error: missing script for: 85165d4332b0f72d30e0c584b476249b542338e6
    3Failed
    

    I fixed it locally by installing gsed and creating a symbolic link. But do you think it would make sense to add something like the following to help future MacOS users?

    After making this change, here’s the new output:

    0$ ./test/lint/commit-script-check.sh 85165d4~..85165d4
    1sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
    2Error: Incompatible version of sed; MacOS users should install gsed
    3Failed
    

    What was wrong with solution, with a simple error message to use gsed? or will #21002 ultimately replace this Issue.

17. 
    anouarkappitou commented at 1:46 pm on November 18, 2021: contributor

    Hello guys, i would love to work on this issue.
18. MarcoFalke referenced this in commit b3122e167a on Jan 24, 2022
19. MarcoFalke closed this on Jan 24, 2022

    ---

20. sidhujag referenced this in commit bd8973ed4c on Jan 28, 2022
21. PastaPastaPasta referenced this in commit 01361608e9 on Apr 7, 2022
22. PastaPastaPasta referenced this in commit 4e9dc7e046 on Apr 11, 2022
23. DrahtBot locked this on Jan 24, 2023

Contributors
amitiuttarwar khuei robot-dreams bitkarrot bliotti ghost rysolv-bot kvkenyon anouarkappitou

Labels
good first issue