Noissue - Add helper script to import changes from Rietveld reviews

patchconv/rapply.sh

-#!/bin/bash

[Sebastian Noack, 2017/08/23 15:52:33]

It seems the only reason this script requires bash is so that you can use a
regular expression to detect HTTP(S) URLs. Alternatively, you could use [ -f
filename ] to check whether it is a file, which works with every POSIX shell.

[Vasily Kuznetsov, 2017/08/23 16:29:39]

Done.

+#!/bin/sh
 #
 # This file is part of Adblock Plus <https://adblockplus.org/>,
-# Copyright (C) 2006-2017 eyeo GmbH

[Sebastian Noack, 2017/08/23 15:52:33]

We don't put the current year into the copyright header anymore, but instead use
"(C) 2006-present eyeo GmbH".

[Vasily Kuznetsov, 2017/08/23 16:29:39]

Done.

+# Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 

[Sebastian Noack, 2017/08/23 15:52:32]

It is best practice to put `set -eu` at the top of every shell script, as it
mitigates a wide range of bugs. -e causes the script to abort as soon as any
command whose exit status isn't handled fails. -u causes the script to abort
when accessing undefined variables (instead of falling back to an empty string).

[Vasily Kuznetsov, 2017/08/23 16:29:39]

Done.

-if [[ "$1" == "" ]]; then
+set -eu
+
+if [ $# -lt 1 ]; then
     echo "usage: rapply.sh PATCH"
     exit 1
 fi
 
-SRCFILE="$1"
-TMPFILE="$TMPDIR/`basename $SRCFILE`"

[Sebastian Noack, 2017/08/23 15:52:32]

Where does the variable TMPDIR comes from? I don't see it being defined in this
script, neither is it defined by default (in bash on Debian).

[Vasily Kuznetsov, 2017/08/23 16:29:39]

It comes from POSIX (see https://en.wikipedia.org/wiki/TMPDIR), but anyway, we
don't need it anymore because piping is better.

-
 if git st >/dev/null 2>&1; then
     IMPORT="git apply"
 elif hg st >/dev/null 2>&1; then
-    IMPORT="hg import --no-commit"
+    IMPORT="hg import --no-commit -"
 else
     echo "No repository found in `pwd`"
     exit 2
 fi
 
-if [[ "$SRCFILE" =~ ^https?:// ]]; then
-    curl "$SRCFILE" | patchconv >"$TMPFILE" || exit 1
-else
-    patchconv <"$SRCFILE" >"$TMPFILE" || exit 1

[Sebastian Noack, 2017/08/23 15:52:33]

Nit: Please add a space before and after the angle brackets for readability and
consistency.

[Vasily Kuznetsov, 2017/08/23 16:29:39]

Done.

+BASEDIR=$(dirname "$0")
+if [ -f "$BASEDIR/patchconv.py" ]; then
+    alias patchconv="$BASEDIR/patchconv.py"
 fi
 
-$IMPORT "$TMPFILE"  

[Sebastian Noack, 2017/08/23 15:52:32]

It seems the temporary file is not cleaned up. But why creating a temporary file
in the first place? Couldn't you just pipe the output directly to `hg import` or
`git apply`?

[Vasily Kuznetsov, 2017/08/23 16:29:39]

You're right, that's better.

Done.

+if [ -f "$1" ]; then
+    patchconv < "$1" | $IMPORT
+else
+    curl "$1" | patchconv | $IMPORT
+fi