You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by pwendell <gi...@git.apache.org> on 2014/11/08 08:27:25 UTC
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
GitHub user pwendell opened a pull request:
https://github.com/apache/spark/pull/3165
SPARK-3648: Provide a script for fetching remote PR's for review
This is a simple script I wrote for fetching remote pull requests.
I found this very useful during code reviews and I think other
developers would as well.
One nice thing is this allows you to avoid creating local refs for
all pull requests, which can be annoying since there is high churn
in PR activity.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pwendell/spark fetch-pr
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3165.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3165
----
commit aacb814cf8c785298d03389624913be376f79fb5
Author: Patrick Wendell <pw...@gmail.com>
Date: 2014-11-08T07:23:37Z
SPARK-3648: Provide a script for fetching remote PR's for review
This is a simple script I wrote for fetching remote pull requests.
I found this very useful during code reviews and I think other
developers would as well.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62279046
Why not using hub? It is super easy and we don't need to duplicate the work.
https://github.com/github/hub
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249112
[Test build #23092 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23092/consoleFull) for PR 3165 at commit [`aacb814`](https://github.com/apache/spark/commit/aacb814cf8c785298d03389624913be376f79fb5).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62251086
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23092/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on a diff in the pull request:
https://github.com/apache/spark/pull/3165#discussion_r20050584
--- Diff: dev/fetch-pr ---
@@ -0,0 +1,53 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# A utility script for fetching local copies of a pull request.
+
+set -e
+set -o pipefail
+
+usage(){
+ echo "Utility for fetching a specific pull request into a temporary ref."
+ echo "Usage: ./dev/fetch-pr <pr-num>"
+ exit 1
+}
+
+case "$1" in
+ "") usage ;;
+ -h) usage ;;
+ h) usage ;;
+ --help) usage ;;
+ -help) usage ;;
+ help) usage ;;
+esac
+
+pr_num=$1
+# Find remote corresponding to github
+remote_name=$(git remote -v |grep apache/spark\.git | grep fetch | \
+ awk '{ print $1; }')
--- End diff --
Some minor style comments:
You can format this so that each command in the pipeline is on a separate line.
```shell
remote_name=$(
git remote -v \
| grep "apache/spark\.git" \
| grep "(fetch)$" \
| awk '{ print $1; }'
)
```
May I also suggest quoting the search terms and restricting the search for `fetch` to lines ending in the word.
Finally, if there is any chance this filter might return more than one result, we should probably add `-m 1` to the first grep statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249570
On the other hand, I guess that `hub` pollutes your local git branch namespace by creating local tracking branches for each PR that you checkout, which could quickly get annoying at the scale that we've been working at.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249644
This looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3165#discussion_r20050579
--- Diff: dev/fetch-pr ---
@@ -0,0 +1,53 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# A utility script for fetching local copies of a pull request.
+
+set -e
+set -o pipefail
+
+usage(){
+ echo "Utility for fetching a specific pull request into a temporary ref."
+ echo "Usage: ./dev/fetch-pr <pr-num>"
+ exit 1
+}
+
+case "$1" in
+ "") usage ;;
+ -h) usage ;;
+ h) usage ;;
+ --help) usage ;;
+ -help) usage ;;
+ help) usage ;;
+esac
+
+pr_num=$1
+# Find remote corresponding to github
+remote_name=$(git remote -v |grep apache/spark\.git | grep fetch | \
--- End diff --
It turns out that I have multiple remotes that point to the Apache Spark Github:
```
git remote -v |grep apache/spark\.git | grep fetch
apache-github git://github.com/apache/spark.git (fetch)
origin git@github.com:apache/spark.git (fetch)
```
This caused the checkout to fail:
```
./dev/fetch-pr 3000
Detected Apache github remote 'apache-github
origin'
Fetching remote ref 'pull/3000/head'
fatal: Couldn't find remote ref origin
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell closed the pull request at:
https://github.com/apache/spark/pull/3165
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249465
In order to try out this PR's script, I used `hub checkout https://github.com/apache/spark/pull/3165` to fetch this PR, which created a new branch named `pwendell-fetch-pr`. Do you think that we still need this if [`hub`](https://github.com/github/hub) exists?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249778
Yeah, it looks like there was a race condition. Good style point about separating the pipeline commands onto their own lines; I always overlook these kind of bash readability tricks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3165#discussion_r20050583
--- Diff: dev/fetch-pr ---
@@ -0,0 +1,53 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# A utility script for fetching local copies of a pull request.
+
+set -e
+set -o pipefail
+
+usage(){
+ echo "Utility for fetching a specific pull request into a temporary ref."
+ echo "Usage: ./dev/fetch-pr <pr-num>"
+ exit 1
+}
+
+case "$1" in
+ "") usage ;;
+ -h) usage ;;
+ h) usage ;;
+ --help) usage ;;
+ -help) usage ;;
+ help) usage ;;
+esac
+
+pr_num=$1
+# Find remote corresponding to github
+remote_name=$(git remote -v |grep apache/spark\.git | grep fetch | \
--- End diff --
Tossing a `head -n1` into this pipeline solves this problem, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62279935
Just realized Josh also pointed this out earlier. I've been using hub and it's pretty good. It does require a local branch, but that's pretty easy to do.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62251084
[Test build #23092 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23092/consoleFull) for PR 3165 at commit [`aacb814`](https://github.com/apache/spark/commit/aacb814cf8c785298d03389624913be376f79fb5).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62249680
Whoops, I missed Josh's comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62248992
@JoshRosen mind taking a quick look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62288892
I didn't use hub - but I preferred this since I can just create a temporary ref that gets thrown away and I never have to worry about cleaning it up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62288954
I'm actually happy to just drop this though if we can update the documentation in our wiki to suggest people use hub. @JoshRosen or @rxin would one of you guys be able to put a few lines in https://cwiki.apache.org/confluence/display/SPARK/Useful+Developer+Tools with the process you use? I can then verify and if it's all good I can just drip this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3648: Provide a script for fetching remo...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/3165#issuecomment-62313063
Okay i'm gonna close this. If one of you guys could quickly add docs on our wiki, that would be great.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org