Issue 29524903: Noissue - Add helper script to import changes from Rietveld reviews (Closed)
|
Created:
Aug. 23, 2017, 2:50 p.m. by Vasily Kuznetsov Modified:
Aug. 28, 2017, 3:30 p.m. CC:
Jon Sonesen Visibility:
Public. |
DescriptionThis is a script that I use for local testing of the changes from reviews. Thought it could be useful for others too.
Repository: https://hg.adblockplus.org/codingtools
Base revision: 9eec8dfdcbcb
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comment 4 #
Total comments: 4
Patch Set 3 : Make rapply.sh commit the changes and work without installing patchconv.py #
Total comments: 6
Patch Set 4 : Revert the committing logic and simplify patchconv discovery #MessagesTotal messages: 15
Hi! Here's a script that I'm using for applying changes from Rietveld reviews to my local copy. It should be working both with Mercurial and Git, I've done a bit of testing, but perhaps you'll find something that is missing. Jon, Julian: I added you as CC since I thought you'd be interested in this too. Cheers, Vasily
Cool idea but wouldn't it make sense to just put the patch fixing logic in our Rietveld fork so that patches are always provided in the correct format?
On 2017/08/23 15:04:23, kzar wrote: > Cool idea but wouldn't it make sense to just put the patch fixing logic in our > Rietveld fork so that patches are always provided in the correct format? If somebody patches Rietveld, the script from this review wouldn't need to use `patchconv` and would perhaps become too trivial to be worth including in `codingtools`. As long as we don't have patched Rietveld, this script is useful for me and I though I'd share it with the others.
On 2017/08/23 15:16:14, Vasily Kuznetsov wrote: > On 2017/08/23 15:04:23, kzar wrote: > > Cool idea but wouldn't it make sense to just put the patch fixing logic in our > > Rietveld fork so that patches are always provided in the correct format? > > If somebody patches Rietveld, the script from this review wouldn't need to use > `patchconv` and would perhaps become too trivial to be worth including in > `codingtools`. As long as we don't have patched Rietveld, this script is useful > for me and I though I'd share it with the others. I would rather not patch Rietveld. There might be a reason why it uses the standard/traditional patch format instead of than the advanced format Git and Mercurial uses. Also, we probably don't want to maintain our own Rietveld fork with custom features, but merely forked it in order to do minimal changes to run it on our infrastructure, and in the mid-term, we probably have to replace Rietveld anyway. Moreover, having a standalone script that converts these patches is potentially more universally useful, not only for patches downloaded from codereview.adblockplus.org. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:1: #!/bin/bash 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. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:4: # Copyright (C) 2006-2017 eyeo GmbH We don't put the current year into the copyright header anymore, but instead use "(C) 2006-present eyeo GmbH". https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:17: 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). https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:24: TMPFILE="$TMPDIR/`basename $SRCFILE`" 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). https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:38: patchconv <"$SRCFILE" >"$TMPFILE" || exit 1 Nit: Please add a space before and after the angle brackets for readability and consistency. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:41: $IMPORT "$TMPFILE" 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`?
Thanks for the comments, Sebastian, here's a new improved version. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:1: #!/bin/bash On 2017/08/23 15:52:33, Sebastian Noack wrote: > 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. Done. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:4: # Copyright (C) 2006-2017 eyeo GmbH On 2017/08/23 15:52:33, Sebastian Noack wrote: > We don't put the current year into the copyright header anymore, but instead use > "(C) 2006-present eyeo GmbH". Done. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:17: On 2017/08/23 15:52:32, Sebastian Noack wrote: > 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). Done. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:24: TMPFILE="$TMPDIR/`basename $SRCFILE`" On 2017/08/23 15:52:32, Sebastian Noack wrote: > 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). It comes from POSIX (see https://en.wikipedia.org/wiki/TMPDIR), but anyway, we don't need it anymore because piping is better. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:38: patchconv <"$SRCFILE" >"$TMPFILE" || exit 1 On 2017/08/23 15:52:33, Sebastian Noack wrote: > Nit: Please add a space before and after the angle brackets for readability and > consistency. Done. https://codereview.adblockplus.org/29524903/diff/29524904/patchconv/rapply.sh... patchconv/rapply.sh:41: $IMPORT "$TMPFILE" On 2017/08/23 15:52:32, Sebastian Noack wrote: > 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`? You're right, that's better. Done.
LGTM
https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh... patchconv/rapply.sh:28: IMPORT="hg import --no-commit -" There is no --no-commit option for hg import? (I'm using the latest version of mercurial provided by homebrew)
https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh... patchconv/rapply.sh:28: IMPORT="hg import --no-commit -" On 2017/08/25 12:07:12, juliandoucette wrote: > There is no --no-commit option for hg import? > > (I'm using the latest version of mercurial provided by homebrew) There is, at least in Mercurial 4.0, which is included in Debian Stretch. But IMO this script would be more useful if the changes get automatically committed anyway.
https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh... patchconv/rapply.sh:28: IMPORT="hg import --no-commit -" On 2017/08/25 12:12:18, Sebastian Noack wrote: > On 2017/08/25 12:07:12, juliandoucette wrote: > > There is no --no-commit option for hg import? > > > > (I'm using the latest version of mercurial provided by homebrew) > > There is, at least in Mercurial 4.0, which is included in Debian Stretch. But > IMO this script would be more useful if the changes get automatically committed > anyway. I'm using hg --version Mercurial Distributed SCM (version 4.3.1) (see https://mercurial-scm.org for more information) Copyright (C) 2005-2017 Matt Mackall and others This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. I agree with you. But the import doesn't succeed because mercurial complains about there being no --no-commit option.
On 2017/08/25 12:21:28, juliandoucette wrote: > https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh > File patchconv/rapply.sh (right): > > https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh... > patchconv/rapply.sh:28: IMPORT="hg import --no-commit -" > On 2017/08/25 12:12:18, Sebastian Noack wrote: > > On 2017/08/25 12:07:12, juliandoucette wrote: > > > There is no --no-commit option for hg import? > > > > > > (I'm using the latest version of mercurial provided by homebrew) > > > > There is, at least in Mercurial 4.0, which is included in Debian Stretch. But > > IMO this script would be more useful if the changes get automatically > committed > > anyway. > > I'm using hg --version > Mercurial Distributed SCM (version 4.3.1) > (see https://mercurial-scm.org for more information) > > Copyright (C) 2005-2017 Matt Mackall and others > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > I agree with you. But the import doesn't succeed because mercurial complains > about there being no --no-commit option. I added --no-commit in order for the behavior to be consistent between Git and Mercurial. But if it causes problems and you guys prefer the committing version, I'm happy to make it always commit instead (that's actually what my old script did anyway).
https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29524910/patchconv/rapply.sh... patchconv/rapply.sh:28: IMPORT="hg import --no-commit -" On 2017/08/25 13:44:52, Vasily Kuznetsov wrote: > I added --no-commit in order for the behavior to be consistent between Git and > Mercurial. But if it causes problems and you guys prefer the committing version, > I'm happy to make it always commit instead (that's actually what my old script > did anyway). I'm still confused why the --no-commit option is causing problems for Julian, since he is on an even newer version than me, and according to the documentation that option hasn't been removed or even deprecated. But yeah, making the script always commit seems preferable to me, anyway.
Here we go. The current version always commits the patch and can use the `patchconv.py` that's located next to it.
https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:27: alias IMPORT="git apply --index && git commit -m '$MESSAGE'" As per the discussion on IRC, sorry, for some reason I assumed we have the metadata (including the commit message) in the converted patch. But of course, we don't. Hence having rapply.sh commit the changes doesn't make much sense, in the end. In fact it increases the risk that people push commits imported from Rietveld with wrong commit message and author. https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:37: alias PATCHCONV="python $BASEDIR/patchconv.py" patchconv.py is executable. Specifying the interpreter is redundant. https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:41: echo "Can't find patchconv in the PATH or in $BASEDIR." This error message and the `which ...` check above seems superfluous. Just fail with the native error, you get when trying to call patchconv?
Hi Sebastian, I made it not commit again (sorry, Julian, you will need to fix your hg :/ ) and simplified the code related to patchconv path discovery. Cheers, Vasily https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh File patchconv/rapply.sh (right): https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:27: alias IMPORT="git apply --index && git commit -m '$MESSAGE'" On 2017/08/25 16:40:38, Sebastian Noack wrote: > As per the discussion on IRC, sorry, for some reason I assumed we have the > metadata (including the commit message) in the converted patch. But of course, > we don't. Hence having rapply.sh commit the changes doesn't make much sense, in > the end. In fact it increases the risk that people push commits imported from > Rietveld with wrong commit message and author. Done. https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:37: alias PATCHCONV="python $BASEDIR/patchconv.py" On 2017/08/25 16:40:38, Sebastian Noack wrote: > patchconv.py is executable. Specifying the interpreter is redundant. Done. https://codereview.adblockplus.org/29524903/diff/29527782/patchconv/rapply.sh... patchconv/rapply.sh:41: echo "Can't find patchconv in the PATH or in $BASEDIR." On 2017/08/25 16:40:38, Sebastian Noack wrote: > This error message and the `which ...` check above seems superfluous. Just fail > with the native error, you get when trying to call patchconv? Done.
LGTM 
|
