Add check for gnu sed inplace replace #21020

1. 
   ghost commented at 7:45 am on January 28, 2021: none

   Fixes #19815

   Changes interpreter to ‘bash’ since it uses ‘command_not_found_handle’

2. implemented 8746338abd
3. 
   MarcoFalke commented at 7:53 am on January 28, 2021: member

   I am not convinced it is worth to add 37 lines of obscure bash that no one understands (or a regexp with similar complexity), when a simple documentation (printed as part of the fail-error) can have the same benefit. See #21002 (review)
4. updated check 3fa487b424
5. 
   ghost commented at 9:03 am on January 28, 2021: none

   @MarcoFalke

   sed -i file1 -e ’s/world/WORLD/’ file2

   This command will work without throwing error on both gnu and bsd sed, but will do different things.

   This approach removes all commands from shell environment, adds a function that runs in place of sed and checks if the arguments are compatible. Second commit adds a check that prevents this issue. ‘sed’ function can be modified to check for any incompatibilities between sed versions.

6. DrahtBot added the label Tests on Jan 28, 2021
7. in test/lint/commit-script-check.sh:69 in 3fa487b424

   61@@ -28,7 +62,11 @@ for commit in $(git rev-list --reverse $1); do
   62             echo "Error: missing script for: $commit"
   63             echo "Failed"
   64             RET=1
   65+        elif verify_script "$SCRIPT"; then
   66+            echo ERROR: You are using BSD sed syntax for "-i" option, which is incompatible with GNU sed
   67+            RET=1
   68         else
   69+            verify_script "$SCRIPT"
   

   ---

   ---

   

   ajtowns commented at 3:39 pm on February 15, 2021:

   Why run verify_script twice when it fails (in both the elif condition and else branch)?
8. in test/lint/commit-script-check.sh:29 in 3fa487b424

   24+  (
   25+  export PATH='/invalid'
   26+  # ignore commands other than 'sed'
   27+  function command_not_found_handle () {
   28+    return 0;
   29+  }
   

   ---

   ---

   

   ajtowns commented at 3:46 pm on February 15, 2021:

   I don’t see any examples where it’d be a problem, but that won’t capture any builtin commands.

   This will block processing of git ls | xargs sed so those cases won’t be checked.

9. 
   ajtowns commented at 3:48 pm on February 15, 2021: member

   This seems like overkill to me
10. trongham approved
11. 
    laanwj commented at 12:23 pm on March 3, 2021: member

    I am not convinced it is worth to add 37 lines of obscure bash that no one understands (or a regexp with similar complexity),

    I tend to agree. I cannot review this at least, and very few active developers here can, so the general tendency is to keep the amount of advanced shell script in bitcoin core down. Usually in favor of Python, but maybe that’s not really an option here.

    So -0 on this.

12. 
    fanquake commented at 9:07 am on April 1, 2021: member

    @fctorial thanks for the contribution. However I don’t think this is the approach we are going to take to solve this particular issue, so I’m going to close this PR.
13. fanquake closed this on Apr 1, 2021

    ---

14. DrahtBot locked this on Aug 16, 2022

Contributors
ghost MarcoFalke ajtowns trongham laanwj fanquake

Labels
Tests