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.

+#
+# 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.

+#
+# 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
+    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"
+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.

+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.