Differential D4279 Diff 13983 contrib/arcanist/autopatch.sh

Changeset View

Standalone View

contrib/arcanist/autopatch.sh

This file was added.

Property	Old Value	New Value
File Mode	null	100755

				#!/usr/bin/env bash
				# Copyright (c) 2019 The Bitcoin developers
				# Distributed under the MIT software license, see the accompanying
				# file COPYING or http://www.opensource.org/licenses/mit-license.php.

				export LC_ALL=C.UTF-8

				set -euxo pipefail

				DEFAULT_PATCH_ARGS="--skip-dependencies"
				DEFAULT_REMOTE="origin"
				DEFAULT_BRANCH="master"

				help_message() {
				set +x
				echo "Apply a patch from Phabricator cleanly on latest master."
				echo ""
				echo "Options:"
				echo "-b, --branch The git branch to fetch and rebase onto. Default: '${DEFAULT_BRANCH}'"
				echo "-h, --help Display this help message."
				echo "-o, --remote The git remote to fetch latest from. Default: '${DEFAULT_REMOTE}'"
				echo "-p, --patch-args Args to pass to 'arc patch'. Default: '${DEFAULT_PATCH_ARGS}'"
				echo "-r, --revision The Differential revision ID used in Phabricator that you want to land. (ex: D1234)"
				echo " This argument is required if --patch-args does not specify a revision or diff ID."
				set -x
				}

				BRANCH="${DEFAULT_BRANCH}"
				PATCH_ARGS="${DEFAULT_PATCH_ARGS}"
				REMOTE="${DEFAULT_REMOTE}"
				REVISION=""

				# Parse command line arguments
				while [[ $# -gt 0 ]]; do
				case $1 in
				-b\|--branch)
				BRANCH="$2"
				shift # shift past argument
				shift # shift past value
				;;
				-h\|--help)
				help_message
				exit 0
				shift # shift past argument
				;;
				-o\|--remote)
				REMOTE="$2"
				shift # shift past argument
				shift # shift past value
				;;
				-p\|--patch-args)
				PATCH_ARGS="$2"
				shift # shift past argument
				shift # shift past value
				;;
				-r\|--revision)
				REVISION="$2"
				shift # shift past argument
				shift # shift past value
				;;
				*)
				echo "Unknown argument: $1"
				help_message
				exit 1
				shift # shift past argument
				;;
				esac
				done

				PATCH_ARGS="${REVISION} ${PATCH_ARGS}"

				# Stash unstaged changes, just incase this script is being run locally
				FabienUnsubmitted Not Done Inline Actions Nit: `incase` => `in case` Fabien: Nit: `incase` => `in case`
				git stash --include-untracked
				FabienUnsubmitted Not Done Inline Actions Could it fail if another stash exists with the same default name ? Maybe just warn about dirty tree and bail out is a better solution here, as this is what arcanist does in such a circumstance. Fabien: Could it fail if another stash exists with the same default name ? Maybe just warn about dirty…
				jasonbcoxAuthorUnsubmitted Done Inline Actions I suppose that's theoretically possible, but my understanding is stash always pushes down on a stack. ie. if you have: stash@{0} - some message existing and call `git stash` then you end up with: stash@{0} - some new message stash@{1} - some message Regardless, I agree that mucking about with the source tree less is better than more. jasonbcox: I suppose that's theoretically possible, but my understanding is stash always pushes down on a…

				# Fetch and checkout latest changes, ignoring local commits
				git fetch "${REMOTE}" "${BRANCH}:origin/${BRANCH}"
				FabienUnsubmitted Not Done Inline Actions `origin/${BRANCH}` should be `${REMOTE}/${BRANCH}` ? Fabien: `origin/${BRANCH}` should be `${REMOTE}/${BRANCH}` ?
				jasonbcoxAuthorUnsubmitted Done Inline Actions you're right. I made this assumption because ${REMOTE} wasn't working for local file paths. As it turns out, the simple fix for this is to add it as a remote (thereby giving it a name) before using it. jasonbcox: you're right. I made this assumption because ${REMOTE} wasn't working for local file paths. As…
				git checkout "${BRANCH}"
				git reset --hard "origin/${BRANCH}"
				FabienUnsubmitted Not Done Inline Actions Why is this necessary ? Fabien: Why is this necessary ?
				jasonbcoxAuthorUnsubmitted Done Inline Actions For the case where you're on master and may have modified it with changes that do not belong in the diff that you're patching. Without this step, it's possible to do the following: `M (origin/master) <- A (master) <- P (arc patch Dxxxx)` and then `arc land` on master and A + P could be landed. jasonbcox: For the case where you're on master and may have modified it with changes that do not belong in…
				FabienUnsubmitted Not Done Inline Actions I don't think this is a good solution: It will eventually lose some of the host commits, as stated in the below `echo` It only fix the case where master if ahead of origin/master. If master is behind you may want to pull instead, checking that the path is fast-forward first. There are multiple way to check if master has some change ahead of origin/master, and I'm in favor of bailing out in this case. Thought ? Fabien: I don't think this is a good solution: - It will eventually lose some of the host commits, as…
				jasonbcoxAuthorUnsubmitted Done Inline Actions I think bailing out is also an option, though I didn't initially intend for users to call this script directly, so that wasn't the primary use case. I don't have a good reason to prevent this behavior though. If origin/master is ahead, the `git reset --hard` takes care of that. But if there's a better way to test for fast-forwardability, I would prefer that. I'll ping you offline for details. jasonbcox: I think bailing out is also an option, though I didn't initially intend for users to call this…
				echo "
				INFO: Local commits built on remote branch '${BRANCH}' not been branched
				or tagged. See 'git reflog' to recover any lost commits."

				(
				# If arc fails, there may be a dangling branch. Clean it up before exiting.
				cleanup() {
				CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD)
				git checkout "${BRANCH}"
				git branch -D "${CURRENT_BRANCH}" \|\| true
				}
				trap "cleanup" ERR

				# Note: `: \| arc ...` pipes an empty string to stdin incase arcanist prompts
				# for user input. This fails and is treated as an error.
				: \| arc patch ${PATCH_ARGS}
				FabienUnsubmitted Not Done Inline Actions `arc patch --skip-dependencies` can succeed if there are dependencies that are not landed yet (intended that there is no conflict), I'm not sure that's the intended behavior. If a user want to land a diff declared with dependencies, it's probably a better idea to request him to clean up it's dependencies prior to landing. Fabien: `arc patch --skip-dependencies` can succeed if there are dependencies that are not landed yet…
				jasonbcoxAuthorUnsubmitted Done Inline Actions The really unfortunate consequence of this is that stacks will be discouraged on phab. This is because any diff with dependencies will need to be rebased, tests run again (on phab), and then landed. That requires more manual work on part of the user, which is something this script aims to solve so it's kind of self-defeating to require this. An alternative solution would be to check all dependencies and see that they're simply closed on phab. This isn't error-proof, but an improvement on the script as it is now without negatively impacting UX. jasonbcox: The really unfortunate consequence of this is that stacks will be discouraged on phab. This is…
				)

				git rebase "${BRANCH}"