[PATCH 00/14] Make git-pull a builtin

[PATCH 00/14] Make git-pull a builtin

[Paul Tan]

This series directly depends on the changes made in jc/merge and
pt/pull-tests. The commit messages assume that the behavioral changes
proposed in pt/pull-log-n, pt/pull-tags-error-diag, pt/pull-ff-vs-merge-ff,
and [1] have been introduced.

git-pull is a commonly executed command to check for new changes in the
upstream repository and, if there are, fetch and integrate them into the
current branch. Currently it is implemented by the shell script git-pull.sh.
However, compared to C, shell scripts have certain deficiencies:

1. Usually have to rely on spawning a lot of processes, which can be slow on
   systems that do not implement copy-on-write semantics.

2. They introduce a lot of dependencies, especially on non-POSIX systems.

3. They cannot take advantage of git's C API and internal caches.

This series rewrites git-pull.sh into a C builtin, thus improving its
performance and portability. It is part of my GSoC project to rewrite git-pull
and git-am into builtins[2].

[1] http://thread.gmane.org/gmane.comp.version-control.git/269249
[2] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1

Paul Tan (14):
  pull: implement fetch + merge
  pull: pass verbosity, --progress flags to fetch and merge
  pull: pass git-merge's options to git-merge
  pull: pass git-fetch's options to git-fetch
  pull: error on no merge candidates
  pull: support pull.ff config
  pull: check if in unresolved merge state
  pull: fast-forward working tree if head is updated
  pull: implement pulling into an unborn branch
  pull: set reflog message
  pull: teach git pull about --rebase
  pull: configure --rebase via branch.<name>.rebase or pull.rebase
  pull --rebase: exit early when the working directory is dirty
  pull --rebase: error on no merge candidate cases

 Makefile                     |   2 +-
 advice.c                     |   8 +
 advice.h                     |   1 +
 builtin.h                    |   1 +
 builtin/pull.c               | 919 +++++++++++++++++++++++++++++++++++++++++++
 contrib/examples/git-pull.sh | 339 ++++++++++++++++
 git-pull.sh                  | 339 ----------------
 git.c                        |   1 +
 8 files changed, 1270 insertions(+), 340 deletions(-)
 create mode 100644 builtin/pull.c
 create mode 100755 contrib/examples/git-pull.sh
 delete mode 100755 git-pull.sh

-- 
2.1.4

[PATCH 01/14] pull: implement fetch + merge

[Paul Tan]

Start off the rewrite of git-pull.sh to C by implementing a simple
git-pull builtin that:

1. Parses the command line arguments into [<repo> [<refspecs>...]]

2. Runs git-fetch, passing the repo and refspecs to it.

3. Runs git-merge FETCH_HEAD.

Due to missing features, several tests are disabled:

* Passing flags to git fetch, rebase, merge:

  * t5521: verbosity flags -v, -s

* Passing flags to git-merge, git-rebase:

  * t5150, t5572: Pass --ff, --no-ff, --ff-only to git-merge

  * t5520, t5521, t7406: Support --rebase, branch.*.rebase and
    pull.rebase config

  * t6029, t4013: -s/--strategy

  * t6037: -X/--strategy-option

  * t5524: --log

* Passing flags to git-fetch:

  * t5521: --dry-run

  * t5500: --depth

  * t5521: --force

  * t5521: --all

  * t7403: --no-recurse-submodules

* t7601: pull.ff config sets --ff, --no-ff, --ff-only

* t5520: branch.*.rebase or pull.rebase sets --rebase, --rebase=preserve

* t5520: Pulling into void

* t5520: appropriate user-friendly error messages on no merge candidates
  case

* t5520: appropriate user-friendly error messages if index has
  unresolved entries.

* t5520: fast-forward working tree if branch head is updated by
  git-fetch

* t5520: set reflog action to "pull args..."

These features will be re-implemented in succeeding patches.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile                     |   2 +-
 builtin.h                    |   1 +
 builtin/pull.c               |  83 +++++++++++
 contrib/examples/git-pull.sh | 339 +++++++++++++++++++++++++++++++++++++++++++
 git-pull.sh                  | 339 -------------------------------------------
 git.c                        |   1 +
 t/t4013-diff-various.sh      |   3 +
 t/t5150-request-pull.sh      |   2 +-
 t/t5500-fetch-pack.sh        |  10 +-
 t/t5520-pull.sh              |  68 ++++-----
 t/t5521-pull-options.sh      |  20 +--
 t/t5524-pull-msg.sh          |   2 +-
 t/t5572-pull-submodule.sh    |   4 +
 t/t6029-merge-subtree.sh     |   6 +-
 t/t6037-merge-ours-theirs.sh |   2 +-
 t/t7403-submodule-sync.sh    |   8 +-
 t/t7406-submodule-update.sh  |   2 +
 t/t7601-merge-pull-config.sh |   4 +-
 18 files changed, 495 insertions(+), 401 deletions(-)
 create mode 100644 builtin/pull.c
 create mode 100755 contrib/examples/git-pull.sh
 delete mode 100755 git-pull.sh

diff --git a/Makefile b/Makefile
index 36655d5..51087dc 100644
--- a/Makefile
+++ b/Makefile
@@ -474,7 +474,6 @@ SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
-SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
@@ -876,6 +875,7 @@ BUILTIN_OBJS += builtin/pack-refs.o
 BUILTIN_OBJS += builtin/patch-id.o
 BUILTIN_OBJS += builtin/prune-packed.o
 BUILTIN_OBJS += builtin/prune.o
+BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/receive-pack.o
diff --git a/builtin.h b/builtin.h
index b87df70..ea3c834 100644
--- a/builtin.h
+++ b/builtin.h
@@ -98,6 +98,7 @@ extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);
 extern int cmd_patch_id(int argc, const char **argv, const char *prefix);
 extern int cmd_prune(int argc, const char **argv, const char *prefix);
 extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
+extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/pull.c b/builtin/pull.c
new file mode 100644
index 0000000..0b771b9
--- /dev/null
+++ b/builtin/pull.c
@@ -0,0 +1,83 @@
+/*
+ * Builtin "git pull"
+ *
+ * Based on git-pull.sh by Junio C Hamano
+ *
+ * Fetch one or more remote refs and merge it/them into the current HEAD.
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const pull_usage[] = {
+	N_("git pull [options] [<repo> [<refspec>...]]"),
+	NULL
+};
+
+static struct option pull_options[] = {
+	OPT_END()
+};
+
+/**
+ * Parses argv into [<repo> [<refspecs>...]], returning their values in `repo`
+ * as a string and `refspecs` as a null-terminated array of strings. If `repo`
+ * is not provided in argv, it is set to NULL.
+ */
+static void parse_repo_refspecs(int argc, const char **argv, const char **repo, const char ***refspecs)
+{
+	if (argc > 0) {
+		*repo = *argv++;
+		argc--;
+	} else
+		*repo = NULL;
+	*refspecs = argv;
+}
+
+/**
+ * Runs git-fetch, returning its exit status. `repo` and `refspecs` are the
+ * repository and refspecs to fetch, or NULL if they are not provided.
+ */
+static int run_fetch(const char *repo, const char **refspecs)
+{
+	struct argv_array args = ARGV_ARRAY_INIT;
+	int ret;
+
+	argv_array_pushl(&args, "fetch", "--update-head-ok", NULL);
+	if (repo)
+		argv_array_push(&args, repo);
+	while (*refspecs)
+		argv_array_push(&args, *refspecs++);
+	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
+	argv_array_clear(&args);
+	return ret;
+}
+
+/**
+ * Runs git-merge, returning its exit status.
+ */
+static int run_merge(void)
+{
+	int ret;
+	struct argv_array args = ARGV_ARRAY_INIT;
+
+	argv_array_pushl(&args, "merge", NULL);
+	argv_array_push(&args, "FETCH_HEAD");
+	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
+	argv_array_clear(&args);
+	return ret;
+}
+
+int cmd_pull(int argc, const char **argv, const char *prefix)
+{
+	const char *repo, **refspecs;
+
+	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
+	parse_repo_refspecs(argc, argv, &repo, &refspecs);
+
+	if (run_fetch(repo, refspecs))
+		return 1;
+
+	return run_merge();
+}
diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
new file mode 100755
index 0000000..5ff4545
--- /dev/null
+++ b/contrib/examples/git-pull.sh
@@ -0,0 +1,339 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+# Fetch one or more remote refs and merge it/them into the current HEAD.
+
+USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [<fetch-options>] <repo> <head>...'
+LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
+SUBDIRECTORY_OK=Yes
+OPTIONS_SPEC=
+. git-sh-setup
+. git-sh-i18n
+set_reflog_action "pull${1+ $*}"
+require_work_tree_exists
+cd_to_toplevel
+
+
+die_conflict () {
+    git diff-index --cached --name-status -r --ignore-submodules HEAD --
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "$(gettext "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution and make a commit.")"
+    else
+	die "$(gettext "Pull is not possible because you have unmerged files.")"
+    fi
+}
+
+die_merge () {
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "$(gettext "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge.")"
+    else
+	die "$(gettext "You have not concluded your merge (MERGE_HEAD exists).")"
+    fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
+
+bool_or_string_config () {
+	git config --bool "$1" 2>/dev/null || git config "$1"
+}
+
+strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
+merge_args= edit= rebase_args=
+curr_branch=$(git symbolic-ref -q HEAD)
+curr_branch_short="${curr_branch#refs/heads/}"
+rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
+if test -z "$rebase"
+then
+	rebase=$(bool_or_string_config pull.rebase)
+fi
+
+# Setup default fast-forward options via `pull.ff`
+pull_ff=$(git config pull.ff)
+case "$pull_ff" in
+false)
+	no_ff=--no-ff
+	;;
+only)
+	ff_only=--ff-only
+	;;
+esac
+
+
+dry_run=
+while :
+do
+	case "$1" in
+	-q|--quiet)
+		verbosity="$verbosity -q" ;;
+	-v|--verbose)
+		verbosity="$verbosity -v" ;;
+	--progress)
+		progress=--progress ;;
+	--no-progress)
+		progress=--no-progress ;;
+	-n|--no-stat|--no-summary)
+		diffstat=--no-stat ;;
+	--stat|--summary)
+		diffstat=--stat ;;
+	--log|--log=*|--no-log)
+		log_arg="$1" ;;
+	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
+		no_commit=--no-commit ;;
+	--c|--co|--com|--comm|--commi|--commit)
+		no_commit=--commit ;;
+	-e|--edit)
+		edit=--edit ;;
+	--no-edit)
+		edit=--no-edit ;;
+	--sq|--squ|--squa|--squas|--squash)
+		squash=--squash ;;
+	--no-sq|--no-squ|--no-squa|--no-squas|--no-squash)
+		squash=--no-squash ;;
+	--ff)
+		no_ff=--ff ;;
+	--no-ff)
+		no_ff=--no-ff ;;
+	--ff-only)
+		ff_only=--ff-only ;;
+	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
+		--strateg=*|--strategy=*|\
+	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
+		case "$#,$1" in
+		*,*=*)
+			strategy=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
+		1,*)
+			usage ;;
+		*)
+			strategy="$2"
+			shift ;;
+		esac
+		strategy_args="${strategy_args}-s $strategy "
+		;;
+	-X*)
+		case "$#,$1" in
+		1,-X)
+			usage ;;
+		*,-X)
+			xx="-X $(git rev-parse --sq-quote "$2")"
+			shift ;;
+		*,*)
+			xx=$(git rev-parse --sq-quote "$1") ;;
+		esac
+		merge_args="$merge_args$xx "
+		;;
+	-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*)
+		rebase="${1#*=}"
+		;;
+	-r|--r|--re|--reb|--reba|--rebas|--rebase)
+		rebase=true
+		;;
+	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
+		rebase=false
+		;;
+	--recurse-submodules)
+		recurse_submodules=--recurse-submodules
+		;;
+	--recurse-submodules=*)
+		recurse_submodules="$1"
+		;;
+	--no-recurse-submodules)
+		recurse_submodules=--no-recurse-submodules
+		;;
+	--verify-signatures)
+		verify_signatures=--verify-signatures
+		;;
+	--no-verify-signatures)
+		verify_signatures=--no-verify-signatures
+		;;
+	--gpg-sign|-S)
+		gpg_sign_args=-S
+		;;
+	--gpg-sign=*)
+		gpg_sign_args=$(git rev-parse --sq-quote "-S${1#--gpg-sign=}")
+		;;
+	-S*)
+		gpg_sign_args=$(git rev-parse --sq-quote "$1")
+		;;
+	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
+		dry_run=--dry-run
+		;;
+	-h|--help-all)
+		usage
+		;;
+	*)
+		# Pass thru anything that may be meant for fetch.
+		break
+		;;
+	esac
+	shift
+done
+
+case "$rebase" in
+preserve)
+	rebase=true
+	rebase_args=--preserve-merges
+	;;
+true|false|'')
+	;;
+*)
+	echo "Invalid value for --rebase, should be true, false, or preserve"
+	usage
+	exit 1
+	;;
+esac
+
+error_on_no_merge_candidates () {
+	exec >&2
+	for opt
+	do
+		case "$opt" in
+		-t|--t|--ta|--tag|--tags)
+			echo "It doesn't make sense to pull all tags; you probably meant:"
+			echo "  git fetch --tags"
+			exit 1
+		esac
+	done
+
+	if test true = "$rebase"
+	then
+		op_type=rebase
+		op_prep=against
+	else
+		op_type=merge
+		op_prep=with
+	fi
+
+	upstream=$(git config "branch.$curr_branch_short.merge")
+	remote=$(git config "branch.$curr_branch_short.remote")
+
+	if [ $# -gt 1 ]; then
+		if [ "$rebase" = true ]; then
+			printf "There is no candidate for rebasing against "
+		else
+			printf "There are no candidates for merging "
+		fi
+		echo "among the refs that you just fetched."
+		echo "Generally this means that you provided a wildcard refspec which had no"
+		echo "matches on the remote end."
+	elif [ $# -gt 0 ] && [ "$1" != "$remote" ]; then
+		echo "You asked to pull from the remote '$1', but did not specify"
+		echo "a branch. Because this is not the default configured remote"
+		echo "for your current branch, you must specify a branch on the command line."
+	elif [ -z "$curr_branch" -o -z "$upstream" ]; then
+		. git-parse-remote
+		error_on_missing_default_upstream "pull" $op_type $op_prep \
+			"git pull <remote> <branch>"
+	else
+		echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'"
+		echo "from the remote, but no such ref was fetched."
+	fi
+	exit 1
+}
+
+test true = "$rebase" && {
+	if ! git rev-parse -q --verify HEAD >/dev/null
+	then
+		# On an unborn branch
+		if test -f "$(git rev-parse --git-path index)"
+		then
+			die "$(gettext "updating an unborn branch with changes added to the index")"
+		fi
+	else
+		require_clean_work_tree "pull with rebase" "Please commit or stash them."
+	fi
+	oldremoteref= &&
+	test -n "$curr_branch" &&
+	. git-parse-remote &&
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	oldremoteref=$(git merge-base --fork-point "$remoteref" $curr_branch 2>/dev/null)
+}
+orig_head=$(git rev-parse -q --verify HEAD)
+git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
+test -z "$dry_run" || exit 0
+
+curr_head=$(git rev-parse -q --verify HEAD)
+if test -n "$orig_head" && test "$curr_head" != "$orig_head"
+then
+	# The fetch involved updating the current branch.
+
+	# The working tree and the index file is still based on the
+	# $orig_head commit, but we are merging into $curr_head.
+	# First update the working tree to match $curr_head.
+
+	eval_gettextln "Warning: fetch updated the current branch head.
+Warning: fast-forwarding your working tree from
+Warning: commit \$orig_head." >&2
+	git update-index -q --refresh
+	git read-tree -u -m "$orig_head" "$curr_head" ||
+		die "$(eval_gettext "Cannot fast-forward your working tree.
+After making sure that you saved anything precious from
+$ git diff \$orig_head
+output, run
+$ git reset --hard
+to recover.")"
+
+fi
+
+merge_head=$(sed -e '/	not-for-merge	/d' \
+	-e 's/	.*//' "$GIT_DIR"/FETCH_HEAD | \
+	tr '\012' ' ')
+
+case "$merge_head" in
+'')
+	error_on_no_merge_candidates "$@"
+	;;
+?*' '?*)
+	if test -z "$orig_head"
+	then
+		die "$(gettext "Cannot merge multiple branches into empty head")"
+	fi
+	if test true = "$rebase"
+	then
+		die "$(gettext "Cannot rebase onto multiple branches")"
+	fi
+	;;
+esac
+
+# Pulling into unborn branch: a shorthand for branching off
+# FETCH_HEAD, for lazy typers.
+if test -z "$orig_head"
+then
+	# Two-way merge: we claim the index is based on an empty tree,
+	# and try to fast-forward to HEAD.  This ensures we will not
+	# lose index/worktree changes that the user already made on
+	# the unborn branch.
+	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	git read-tree -m -u $empty_tree $merge_head &&
+	git update-ref -m "initial pull" HEAD $merge_head "$curr_head"
+	exit
+fi
+
+if test true = "$rebase"
+then
+	o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
+	if test "$oldremoteref" = "$o"
+	then
+		unset oldremoteref
+	fi
+fi
+
+case "$rebase" in
+true)
+	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
+	eval="$eval $gpg_sign_args"
+	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
+	;;
+*)
+	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
+	eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress"
+	eval="$eval $gpg_sign_args"
+	eval="$eval FETCH_HEAD"
+	;;
+esac
+eval "exec $eval"
diff --git a/git-pull.sh b/git-pull.sh
deleted file mode 100755
index 5ff4545..0000000
--- a/git-pull.sh
+++ /dev/null
@@ -1,339 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Junio C Hamano
-#
-# Fetch one or more remote refs and merge it/them into the current HEAD.
-
-USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [<fetch-options>] <repo> <head>...'
-LONG_USAGE='Fetch one or more remote refs and integrate it/them with the current HEAD.'
-SUBDIRECTORY_OK=Yes
-OPTIONS_SPEC=
-. git-sh-setup
-. git-sh-i18n
-set_reflog_action "pull${1+ $*}"
-require_work_tree_exists
-cd_to_toplevel
-
-
-die_conflict () {
-    git diff-index --cached --name-status -r --ignore-submodules HEAD --
-    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
-	die "$(gettext "Pull is not possible because you have unmerged files.
-Please, fix them up in the work tree, and then use 'git add/rm <file>'
-as appropriate to mark resolution and make a commit.")"
-    else
-	die "$(gettext "Pull is not possible because you have unmerged files.")"
-    fi
-}
-
-die_merge () {
-    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
-	die "$(gettext "You have not concluded your merge (MERGE_HEAD exists).
-Please, commit your changes before you can merge.")"
-    else
-	die "$(gettext "You have not concluded your merge (MERGE_HEAD exists).")"
-    fi
-}
-
-test -z "$(git ls-files -u)" || die_conflict
-test -f "$GIT_DIR/MERGE_HEAD" && die_merge
-
-bool_or_string_config () {
-	git config --bool "$1" 2>/dev/null || git config "$1"
-}
-
-strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules= verify_signatures=
-merge_args= edit= rebase_args=
-curr_branch=$(git symbolic-ref -q HEAD)
-curr_branch_short="${curr_branch#refs/heads/}"
-rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
-if test -z "$rebase"
-then
-	rebase=$(bool_or_string_config pull.rebase)
-fi
-
-# Setup default fast-forward options via `pull.ff`
-pull_ff=$(git config pull.ff)
-case "$pull_ff" in
-false)
-	no_ff=--no-ff
-	;;
-only)
-	ff_only=--ff-only
-	;;
-esac
-
-
-dry_run=
-while :
-do
-	case "$1" in
-	-q|--quiet)
-		verbosity="$verbosity -q" ;;
-	-v|--verbose)
-		verbosity="$verbosity -v" ;;
-	--progress)
-		progress=--progress ;;
-	--no-progress)
-		progress=--no-progress ;;
-	-n|--no-stat|--no-summary)
-		diffstat=--no-stat ;;
-	--stat|--summary)
-		diffstat=--stat ;;
-	--log|--log=*|--no-log)
-		log_arg="$1" ;;
-	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
-		no_commit=--no-commit ;;
-	--c|--co|--com|--comm|--commi|--commit)
-		no_commit=--commit ;;
-	-e|--edit)
-		edit=--edit ;;
-	--no-edit)
-		edit=--no-edit ;;
-	--sq|--squ|--squa|--squas|--squash)
-		squash=--squash ;;
-	--no-sq|--no-squ|--no-squa|--no-squas|--no-squash)
-		squash=--no-squash ;;
-	--ff)
-		no_ff=--ff ;;
-	--no-ff)
-		no_ff=--no-ff ;;
-	--ff-only)
-		ff_only=--ff-only ;;
-	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
-		--strateg=*|--strategy=*|\
-	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
-		case "$#,$1" in
-		*,*=*)
-			strategy=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
-		1,*)
-			usage ;;
-		*)
-			strategy="$2"
-			shift ;;
-		esac
-		strategy_args="${strategy_args}-s $strategy "
-		;;
-	-X*)
-		case "$#,$1" in
-		1,-X)
-			usage ;;
-		*,-X)
-			xx="-X $(git rev-parse --sq-quote "$2")"
-			shift ;;
-		*,*)
-			xx=$(git rev-parse --sq-quote "$1") ;;
-		esac
-		merge_args="$merge_args$xx "
-		;;
-	-r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*)
-		rebase="${1#*=}"
-		;;
-	-r|--r|--re|--reb|--reba|--rebas|--rebase)
-		rebase=true
-		;;
-	--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
-		rebase=false
-		;;
-	--recurse-submodules)
-		recurse_submodules=--recurse-submodules
-		;;
-	--recurse-submodules=*)
-		recurse_submodules="$1"
-		;;
-	--no-recurse-submodules)
-		recurse_submodules=--no-recurse-submodules
-		;;
-	--verify-signatures)
-		verify_signatures=--verify-signatures
-		;;
-	--no-verify-signatures)
-		verify_signatures=--no-verify-signatures
-		;;
-	--gpg-sign|-S)
-		gpg_sign_args=-S
-		;;
-	--gpg-sign=*)
-		gpg_sign_args=$(git rev-parse --sq-quote "-S${1#--gpg-sign=}")
-		;;
-	-S*)
-		gpg_sign_args=$(git rev-parse --sq-quote "$1")
-		;;
-	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
-		dry_run=--dry-run
-		;;
-	-h|--help-all)
-		usage
-		;;
-	*)
-		# Pass thru anything that may be meant for fetch.
-		break
-		;;
-	esac
-	shift
-done
-
-case "$rebase" in
-preserve)
-	rebase=true
-	rebase_args=--preserve-merges
-	;;
-true|false|'')
-	;;
-*)
-	echo "Invalid value for --rebase, should be true, false, or preserve"
-	usage
-	exit 1
-	;;
-esac
-
-error_on_no_merge_candidates () {
-	exec >&2
-	for opt
-	do
-		case "$opt" in
-		-t|--t|--ta|--tag|--tags)
-			echo "It doesn't make sense to pull all tags; you probably meant:"
-			echo "  git fetch --tags"
-			exit 1
-		esac
-	done
-
-	if test true = "$rebase"
-	then
-		op_type=rebase
-		op_prep=against
-	else
-		op_type=merge
-		op_prep=with
-	fi
-
-	upstream=$(git config "branch.$curr_branch_short.merge")
-	remote=$(git config "branch.$curr_branch_short.remote")
-
-	if [ $# -gt 1 ]; then
-		if [ "$rebase" = true ]; then
-			printf "There is no candidate for rebasing against "
-		else
-			printf "There are no candidates for merging "
-		fi
-		echo "among the refs that you just fetched."
-		echo "Generally this means that you provided a wildcard refspec which had no"
-		echo "matches on the remote end."
-	elif [ $# -gt 0 ] && [ "$1" != "$remote" ]; then
-		echo "You asked to pull from the remote '$1', but did not specify"
-		echo "a branch. Because this is not the default configured remote"
-		echo "for your current branch, you must specify a branch on the command line."
-	elif [ -z "$curr_branch" -o -z "$upstream" ]; then
-		. git-parse-remote
-		error_on_missing_default_upstream "pull" $op_type $op_prep \
-			"git pull <remote> <branch>"
-	else
-		echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'"
-		echo "from the remote, but no such ref was fetched."
-	fi
-	exit 1
-}
-
-test true = "$rebase" && {
-	if ! git rev-parse -q --verify HEAD >/dev/null
-	then
-		# On an unborn branch
-		if test -f "$(git rev-parse --git-path index)"
-		then
-			die "$(gettext "updating an unborn branch with changes added to the index")"
-		fi
-	else
-		require_clean_work_tree "pull with rebase" "Please commit or stash them."
-	fi
-	oldremoteref= &&
-	test -n "$curr_branch" &&
-	. git-parse-remote &&
-	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
-	oldremoteref=$(git merge-base --fork-point "$remoteref" $curr_branch 2>/dev/null)
-}
-orig_head=$(git rev-parse -q --verify HEAD)
-git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok "$@" || exit 1
-test -z "$dry_run" || exit 0
-
-curr_head=$(git rev-parse -q --verify HEAD)
-if test -n "$orig_head" && test "$curr_head" != "$orig_head"
-then
-	# The fetch involved updating the current branch.
-
-	# The working tree and the index file is still based on the
-	# $orig_head commit, but we are merging into $curr_head.
-	# First update the working tree to match $curr_head.
-
-	eval_gettextln "Warning: fetch updated the current branch head.
-Warning: fast-forwarding your working tree from
-Warning: commit \$orig_head." >&2
-	git update-index -q --refresh
-	git read-tree -u -m "$orig_head" "$curr_head" ||
-		die "$(eval_gettext "Cannot fast-forward your working tree.
-After making sure that you saved anything precious from
-$ git diff \$orig_head
-output, run
-$ git reset --hard
-to recover.")"
-
-fi
-
-merge_head=$(sed -e '/	not-for-merge	/d' \
-	-e 's/	.*//' "$GIT_DIR"/FETCH_HEAD | \
-	tr '\012' ' ')
-
-case "$merge_head" in
-'')
-	error_on_no_merge_candidates "$@"
-	;;
-?*' '?*)
-	if test -z "$orig_head"
-	then
-		die "$(gettext "Cannot merge multiple branches into empty head")"
-	fi
-	if test true = "$rebase"
-	then
-		die "$(gettext "Cannot rebase onto multiple branches")"
-	fi
-	;;
-esac
-
-# Pulling into unborn branch: a shorthand for branching off
-# FETCH_HEAD, for lazy typers.
-if test -z "$orig_head"
-then
-	# Two-way merge: we claim the index is based on an empty tree,
-	# and try to fast-forward to HEAD.  This ensures we will not
-	# lose index/worktree changes that the user already made on
-	# the unborn branch.
-	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-	git read-tree -m -u $empty_tree $merge_head &&
-	git update-ref -m "initial pull" HEAD $merge_head "$curr_head"
-	exit
-fi
-
-if test true = "$rebase"
-then
-	o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
-	if test "$oldremoteref" = "$o"
-	then
-		unset oldremoteref
-	fi
-fi
-
-case "$rebase" in
-true)
-	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
-	eval="$eval $gpg_sign_args"
-	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
-	;;
-*)
-	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
-	eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress"
-	eval="$eval $gpg_sign_args"
-	eval="$eval FETCH_HEAD"
-	;;
-esac
-eval "exec $eval"
diff --git a/git.c b/git.c
index 44374b1..e7a7713 100644
--- a/git.c
+++ b/git.c
@@ -445,6 +445,7 @@ static struct cmd_struct commands[] = {
 	{ "pickaxe", cmd_blame, RUN_SETUP },
 	{ "prune", cmd_prune, RUN_SETUP },
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
+	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP },
 	{ "receive-pack", cmd_receive_pack },
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6ec6072..a11e48b 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -7,6 +7,9 @@ test_description='Various diff formatting options'
 
 . ./test-lib.sh
 
+skip_all='setup for this test suite requires git pull -s'
+test_done
+
 LF='
 '
 
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 82c33b8..f5ea605 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -132,7 +132,7 @@ test_expect_success 'pull request when forgot to push' '
 
 '
 
-test_expect_success 'pull request after push' '
+test_expect_failure 'pull request after push' '
 
 	rm -fr downstream.git &&
 	git init --bare downstream.git &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 3a9b775..213b231 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -226,14 +226,14 @@ test_expect_success 'add two more (part 2)' '
 	add B69 $B68
 '
 
-test_expect_success 'deepening pull in shallow repo' '
+test_expect_failure 'deepening pull in shallow repo' '
 	(
 		cd shallow &&
 		git pull --depth 4 .. B
 	)
 '
 
-test_expect_success 'clone shallow object count' '
+test_expect_failure 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
@@ -248,7 +248,7 @@ test_expect_success 'deepening fetch in shallow repo' '
 	)
 '
 
-test_expect_success 'clone shallow object count' '
+test_expect_failure 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
@@ -272,11 +272,11 @@ test_expect_success 'additional simple shallow deepenings' '
 	)
 '
 
-test_expect_success 'clone shallow depth count' '
+test_expect_failure 'clone shallow depth count' '
 	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 11
 '
 
-test_expect_success 'clone shallow object count' '
+test_expect_failure 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 62dbfb5..f32b8cb 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -26,7 +26,7 @@ test_expect_success 'pulling into void' '
 	test_cmp file cloned/file
 '
 
-test_expect_success 'pulling into void using master:master' '
+test_expect_failure 'pulling into void using master:master' '
 	git init cloned-uho &&
 	(
 		cd cloned-uho &&
@@ -76,7 +76,7 @@ test_expect_success 'pulling into void does not remove new staged files' '
 	)
 '
 
-test_expect_success 'pulling into void must not create an octopus' '
+test_expect_failure 'pulling into void must not create an octopus' '
 	git init cloned-octopus &&
 	(
 		cd cloned-octopus &&
@@ -85,7 +85,7 @@ test_expect_success 'pulling into void must not create an octopus' '
 	)
 '
 
-test_expect_success 'test . as a remote' '
+test_expect_failure 'test . as a remote' '
 	git branch copy master &&
 	git config branch.copy.remote . &&
 	git config branch.copy.merge refs/heads/master &&
@@ -101,7 +101,7 @@ test_expect_success 'test . as a remote' '
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-test_expect_success 'the default remote . should not break explicit pull' '
+test_expect_failure 'the default remote . should not break explicit pull' '
 	git checkout -b second master^ &&
 	echo modified >file &&
 	git commit -a -m modified &&
@@ -116,7 +116,7 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-test_expect_success 'fail if wildcard spec does not match any refs' '
+test_expect_failure 'fail if wildcard spec does not match any refs' '
 	git checkout -b test copy^ &&
 	test_when_finished "git checkout -f copy && git branch -D test" &&
 	test "$(cat file)" = file &&
@@ -125,7 +125,7 @@ test_expect_success 'fail if wildcard spec does not match any refs' '
 	test "$(cat file)" = file
 '
 
-test_expect_success 'fail if no branches specified with non-default remote' '
+test_expect_failure 'fail if no branches specified with non-default remote' '
 	git remote add test_remote . &&
 	test_when_finished "git remote remove test_remote" &&
 	git checkout -b test copy^ &&
@@ -137,7 +137,7 @@ test_expect_success 'fail if no branches specified with non-default remote' '
 	test "$(cat file)" = file
 '
 
-test_expect_success 'fail if not on a branch' '
+test_expect_failure 'fail if not on a branch' '
 	git remote add origin . &&
 	test_when_finished "git remote remove origin" &&
 	git checkout HEAD^ &&
@@ -148,7 +148,7 @@ test_expect_success 'fail if not on a branch' '
 	test "$(cat file)" = file
 '
 
-test_expect_success 'fail if no configuration for current branch' '
+test_expect_failure 'fail if no configuration for current branch' '
 	git remote add test_remote . &&
 	test_when_finished "git remote remove test_remote" &&
 	git checkout -b test copy^ &&
@@ -160,7 +160,7 @@ test_expect_success 'fail if no configuration for current branch' '
 	test "$(cat file)" = file
 '
 
-test_expect_success 'fail if upstream branch does not exist' '
+test_expect_failure 'fail if upstream branch does not exist' '
 	git checkout -b test copy^ &&
 	test_when_finished "git checkout -f copy && git branch -D test" &&
 	test_config branch.test.remote . &&
@@ -171,7 +171,7 @@ test_expect_success 'fail if upstream branch does not exist' '
 	test "$(cat file)" = file
 '
 
-test_expect_success 'fail if the index has unresolved entries' '
+test_expect_failure 'fail if the index has unresolved entries' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
@@ -191,7 +191,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_cmp expected file
 '
 
-test_expect_success 'fast-forwards working tree if branch head is updated' '
+test_expect_failure 'fast-forwards working tree if branch head is updated' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
@@ -201,7 +201,7 @@ test_expect_success 'fast-forwards working tree if branch head is updated' '
 	test "$(git rev-parse third)" = "$(git rev-parse second)"
 '
 
-test_expect_success 'fast-forward fails with conflicting work tree' '
+test_expect_failure 'fast-forward fails with conflicting work tree' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
@@ -212,7 +212,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
 	test "$(git rev-parse third)" = "$(git rev-parse second)"
 '
 
-test_expect_success '--rebase' '
+test_expect_failure '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
 	git commit -m file file &&
@@ -226,7 +226,7 @@ test_expect_success '--rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
-test_expect_success '--rebase fails with multiple branches' '
+test_expect_failure '--rebase fails with multiple branches' '
 	git reset --hard before-rebase &&
 	test_must_fail git pull --rebase . copy master 2>err &&
 	test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
@@ -234,7 +234,7 @@ test_expect_success '--rebase fails with multiple branches' '
 	test modified = "$(git show HEAD:file)"
 '
 
-test_expect_success 'pull.rebase' '
+test_expect_failure 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
@@ -242,7 +242,7 @@ test_expect_success 'pull.rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
-test_expect_success 'branch.to-rebase.rebase' '
+test_expect_failure 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
@@ -280,7 +280,7 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success 'pull.rebase=true flattens keep-merge' '
+test_expect_failure 'pull.rebase=true flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
@@ -288,7 +288,7 @@ test_expect_success 'pull.rebase=true flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
+test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase 1 &&
 	git pull . copy &&
@@ -296,7 +296,7 @@ test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
+test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull . copy &&
@@ -304,13 +304,13 @@ test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
 	test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
 '
 
-test_expect_success 'pull.rebase=invalid fails' '
+test_expect_failure 'pull.rebase=invalid fails' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase invalid &&
 	! git pull . copy
 '
 
-test_expect_success '--rebase=false create a new merge commit' '
+test_expect_failure '--rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=false . copy &&
@@ -319,7 +319,7 @@ test_expect_success '--rebase=false create a new merge commit' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success '--rebase=true rebases and flattens keep-merge' '
+test_expect_failure '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase=true . copy &&
@@ -327,7 +327,7 @@ test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success '--rebase=preserve rebases and merges keep-merge' '
+test_expect_failure '--rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=preserve . copy &&
@@ -340,7 +340,7 @@ test_expect_success '--rebase=invalid fails' '
 	! git pull --rebase=invalid . copy
 '
 
-test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase . copy &&
@@ -348,7 +348,7 @@ test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_success '--rebase with rebased upstream' '
+test_expect_failure '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
 	git checkout copy &&
@@ -366,7 +366,7 @@ test_expect_success '--rebase with rebased upstream' '
 
 '
 
-test_expect_success '--rebase with rebased default upstream' '
+test_expect_failure '--rebase with rebased default upstream' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git checkout --track -b to-rebase2 me/copy &&
@@ -377,7 +377,7 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
-test_expect_success 'rebased upstream + fetch + pull --rebase' '
+test_expect_failure 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
@@ -390,7 +390,7 @@ test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 '
 
-test_expect_success 'pull --rebase dies early with dirty working directory' '
+test_expect_failure 'pull --rebase dies early with dirty working directory' '
 
 	git checkout to-rebase &&
 	git update-ref refs/remotes/me/copy copy^ &&
@@ -409,7 +409,7 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
 
 '
 
-test_expect_success 'pull --rebase works on branch yet to be born' '
+test_expect_failure 'pull --rebase works on branch yet to be born' '
 	git rev-parse master >expect &&
 	mkdir empty_repo &&
 	(cd empty_repo &&
@@ -420,7 +420,7 @@ test_expect_success 'pull --rebase works on branch yet to be born' '
 	test_cmp expect actual
 '
 
-test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
+test_expect_failure 'pull --rebase fails on unborn branch with staged changes' '
 	test_when_finished "rm -rf empty_repo2" &&
 	git init empty_repo2 &&
 	(
@@ -456,14 +456,14 @@ test_expect_success 'setup for detecting upstreamed changes' '
 	)
 '
 
-test_expect_success 'git pull --rebase detects upstreamed changes' '
+test_expect_failure 'git pull --rebase detects upstreamed changes' '
 	(cd dst &&
 	 git pull --rebase &&
 	 test -z "$(git ls-files -u)"
 	)
 '
 
-test_expect_success 'setup for avoiding reapplying old patches' '
+test_expect_failure 'setup for avoiding reapplying old patches' '
 	(cd dst &&
 	 test_might_fail git rebase --abort &&
 	 git reset --hard origin/master
@@ -485,14 +485,14 @@ test_expect_success 'setup for avoiding reapplying old patches' '
 	)
 '
 
-test_expect_success 'git pull --rebase does not reapply old patches' '
+test_expect_failure 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
 	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
 	)
 '
 
-test_expect_success 'git pull --rebase against local branch' '
+test_expect_failure 'git pull --rebase against local branch' '
 	git checkout -b copy2 to-rebase-orig &&
 	git pull --rebase . to-rebase &&
 	test "conflicting modification" = "$(cat file)" &&
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 56e7377..0f6094b 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -11,7 +11,7 @@ test_expect_success 'setup' '
 	 git commit -m one)
 '
 
-test_expect_success 'git pull -q' '
+test_expect_failure 'git pull -q' '
 	mkdir clonedq &&
 	(cd clonedq && git init &&
 	git pull -q "../parent" >out 2>err &&
@@ -19,7 +19,7 @@ test_expect_success 'git pull -q' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -q --rebase' '
+test_expect_failure 'git pull -q --rebase' '
 	mkdir clonedqrb &&
 	(cd clonedqrb && git init &&
 	git pull -q --rebase "../parent" >out 2>err &&
@@ -38,7 +38,7 @@ test_expect_success 'git pull' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull --rebase' '
+test_expect_failure 'git pull --rebase' '
 	mkdir clonedrb &&
 	(cd clonedrb && git init &&
 	git pull --rebase "../parent" >out 2>err &&
@@ -46,7 +46,7 @@ test_expect_success 'git pull --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v' '
+test_expect_failure 'git pull -v' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
 	git pull -v "../parent" >out 2>err &&
@@ -54,7 +54,7 @@ test_expect_success 'git pull -v' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v --rebase' '
+test_expect_failure 'git pull -v --rebase' '
 	mkdir clonedvrb &&
 	(cd clonedvrb && git init &&
 	git pull -v --rebase "../parent" >out 2>err &&
@@ -62,7 +62,7 @@ test_expect_success 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v -q' '
+test_expect_failure 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&
 	git pull -v -q "../parent" >out 2>err &&
@@ -70,7 +70,7 @@ test_expect_success 'git pull -v -q' '
 	test_must_be_empty err)
 '
 
-test_expect_success 'git pull -q -v' '
+test_expect_failure 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
 	git pull -q -v "../parent" >out 2>err &&
@@ -78,7 +78,7 @@ test_expect_success 'git pull -q -v' '
 	test -s err)
 '
 
-test_expect_success 'git pull --force' '
+test_expect_failure 'git pull --force' '
 	mkdir clonedoldstyle &&
 	(cd clonedoldstyle && git init &&
 	cat >>.git/config <<-\EOF &&
@@ -99,7 +99,7 @@ test_expect_success 'git pull --force' '
 	)
 '
 
-test_expect_success 'git pull --all' '
+test_expect_failure 'git pull --all' '
 	mkdir clonedmulti &&
 	(cd clonedmulti && git init &&
 	cat >>.git/config <<-\EOF &&
@@ -117,7 +117,7 @@ test_expect_success 'git pull --all' '
 	)
 '
 
-test_expect_success 'git pull --dry-run' '
+test_expect_failure 'git pull --dry-run' '
 	test_when_finished "rm -rf clonedry" &&
 	git init clonedry &&
 	(
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index eebb8c9..1ba8054 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -25,7 +25,7 @@ test_expect_success setup '
 	git commit -m "do not clobber $dollar signs"
 '
 
-test_expect_success pull '
+test_expect_failure pull '
 (
 	cd cloned &&
 	git pull --log &&
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index accfa5c..408c886 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -5,6 +5,10 @@ test_description='pull can handle submodules'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
+KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
+KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
+KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
+
 reset_branch_to_HEAD () {
 	git branch -D "$1" &&
 	git checkout -b "$1" HEAD &&
diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh
index 73fc240..54af64e 100755
--- a/t/t6029-merge-subtree.sh
+++ b/t/t6029-merge-subtree.sh
@@ -61,7 +61,7 @@ test_expect_success 'initial merge' '
 	test_cmp expected actual
 '
 
-test_expect_success 'merge update' '
+test_expect_failure 'merge update' '
 	cd ../git-gui &&
 	echo git-gui2 > git-gui.sh &&
 	o3=$(git hash-object git-gui.sh) &&
@@ -95,7 +95,7 @@ test_expect_success 'initial ambiguous subtree' '
 	test_cmp expected actual
 '
 
-test_expect_success 'merge using explicit' '
+test_expect_failure 'merge using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
 	git pull -Xsubtree=git-gui gui master2 &&
@@ -108,7 +108,7 @@ test_expect_success 'merge using explicit' '
 	test_cmp expected actual
 '
 
-test_expect_success 'merge2 using explicit' '
+test_expect_failure 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
 	git pull -Xsubtree=git-gui2 gui master2 &&
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 3889eca..876e23b 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -65,7 +65,7 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 	git diff --exit-code master HEAD -- file
 '
 
-test_expect_success 'pull passes -X to underlying merge' '
+test_expect_failure 'pull passes -X to underlying merge' '
 	git reset --hard master && git pull -s recursive -Xours . side &&
 	git reset --hard master && git pull -s recursive -X ours . side &&
 	git reset --hard master && git pull -s recursive -Xtheirs . side &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 79bc135..b448562 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -96,7 +96,7 @@ test_expect_success 'change submodule url' '
 	)
 '
 
-test_expect_success '"git submodule sync" should update submodule URLs' '
+test_expect_failure '"git submodule sync" should update submodule URLs' '
 	(
 		cd super-clone &&
 		git pull --no-recurse-submodules &&
@@ -121,7 +121,7 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
 	)
 '
 
-test_expect_success '"git submodule sync --recursive" should update all submodule URLs' '
+test_expect_failure '"git submodule sync --recursive" should update all submodule URLs' '
 	(
 		cd super-clone &&
 		(
@@ -149,7 +149,7 @@ test_expect_success 'reset submodule URLs' '
 	reset_submodule_urls super-clone
 '
 
-test_expect_success '"git submodule sync" should update submodule URLs - subdirectory' '
+test_expect_failure '"git submodule sync" should update submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		git pull --no-recurse-submodules &&
@@ -177,7 +177,7 @@ test_expect_success '"git submodule sync" should update submodule URLs - subdire
 	)
 '
 
-test_expect_success '"git submodule sync --recursive" should update all submodule URLs - subdirectory' '
+test_expect_failure '"git submodule sync --recursive" should update all submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		(
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..1ffd837 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -11,6 +11,8 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 
 . ./test-lib.sh
 
+skip_all='skipping submodule update tests, requires git pull --rebase'
+test_done
 
 compare_head()
 {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index f768c90..7a846a2 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -45,7 +45,7 @@ test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' '
 	test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
 '
 
-test_expect_success 'fast-forward pull creates merge with "false" in pull.ff' '
+test_expect_failure 'fast-forward pull creates merge with "false" in pull.ff' '
 	git reset --hard c0 &&
 	test_config pull.ff false &&
 	git pull . c1 &&
@@ -53,7 +53,7 @@ test_expect_success 'fast-forward pull creates merge with "false" in pull.ff' '
 	test "$(git rev-parse HEAD^2)" = "$(git rev-parse c1)"
 '
 
-test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
+test_expect_failure 'pull prevents non-fast-forward with "only" in pull.ff' '
 	git reset --hard c1 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c3
-- 
2.1.4

[PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

[Paul Tan]

7f87aff (Teach/Fix pull/fetch -q/-v options, 2008-11-15) taught git-pull
to accept the verbosity -v and -q options and pass them to git-fetch and
git-merge.

Re-implement support for the verbosity flags by adding it to the options
list and introducing argv_push_verbosity() to push the flags into the
argv array used to execute git-fetch and git-merge.

9839018 (fetch and pull: learn --progress, 2010-02-24) and bebd2fd
(pull: propagate --progress to merge, 2011-02-20) taught git-pull to
accept the --progress option and pass it to git-fetch and git-merge.

Re-implement support for this flag by introducing the option callback
handler parse_opt_passthru(). This callback is used to pass the
"--progress" or "--no-progress" command-line switch to git-fetch and
git-merge.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c          | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5521-pull-options.sh |  8 +++----
 2 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 0b771b9..a4d9c92 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -11,16 +11,64 @@
 #include "argv-array.h"
 #include "run-command.h"
 
+/**
+ * Given an option opt, where opt->value points to a char* and opt->defval is a
+ * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is
+ * NULL, then opt->value is set to "--$defval". If unset is true, then
+ * opt->value is set to "--no-$defval".
+ */
+static int parse_opt_passthru(const struct option *opt, const char *arg, int unset)
+{
+	struct strbuf sb = STRBUF_INIT;
+	char **opt_value = opt->value;
+
+	assert(opt->defval);
+	strbuf_addstr(&sb, unset ? "--no-" : "--");
+	strbuf_addstr(&sb, (const char*) opt->defval);
+	if (arg) {
+		strbuf_addch(&sb, '=');
+		strbuf_addstr(&sb, arg);
+	}
+	if (*opt_value)
+		free(*opt_value);
+	*opt_value = strbuf_detach(&sb, NULL);
+	return 0;
+}
+
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repo> [<refspec>...]]"),
 	NULL
 };
 
+/* Shared options */
+static int opt_verbosity;
+static char *opt_progress;
+
 static struct option pull_options[] = {
+	/* Shared options */
+	OPT__VERBOSITY(&opt_verbosity),
+	{ OPTION_CALLBACK, 0, "progress", &opt_progress, NULL,
+	  N_("force progress reporting"),
+	  PARSE_OPT_NOARG, parse_opt_passthru},
+
 	OPT_END()
 };
 
 /**
+ * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
+ */
+static void argv_push_verbosity(struct argv_array *arr)
+{
+	int verbosity;
+
+	for (verbosity = opt_verbosity; verbosity > 0; verbosity--)
+		argv_array_push(arr, "-v");
+
+	for (verbosity = opt_verbosity; verbosity < 0; verbosity++)
+		argv_array_push(arr, "-q");
+}
+
+/**
  * Parses argv into [<repo> [<refspecs>...]], returning their values in `repo`
  * as a string and `refspecs` as a null-terminated array of strings. If `repo`
  * is not provided in argv, it is set to NULL.
@@ -45,6 +93,12 @@ static int run_fetch(const char *repo, const char **refspecs)
 	int ret;
 
 	argv_array_pushl(&args, "fetch", "--update-head-ok", NULL);
+
+	/* Shared options */
+	argv_push_verbosity(&args);
+	if (opt_progress)
+		argv_array_push(&args, opt_progress);
+
 	if (repo)
 		argv_array_push(&args, repo);
 	while (*refspecs)
@@ -63,6 +117,12 @@ static int run_merge(void)
 	struct argv_array args = ARGV_ARRAY_INIT;
 
 	argv_array_pushl(&args, "merge", NULL);
+
+	/* Shared options */
+	argv_push_verbosity(&args);
+	if (opt_progress)
+		argv_array_push(&args, opt_progress);
+
 	argv_array_push(&args, "FETCH_HEAD");
 	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
 	argv_array_clear(&args);
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 0f6094b..89e2104 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -11,7 +11,7 @@ test_expect_success 'setup' '
 	 git commit -m one)
 '
 
-test_expect_failure 'git pull -q' '
+test_expect_success 'git pull -q' '
 	mkdir clonedq &&
 	(cd clonedq && git init &&
 	git pull -q "../parent" >out 2>err &&
@@ -46,7 +46,7 @@ test_expect_failure 'git pull --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_failure 'git pull -v' '
+test_expect_success 'git pull -v' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
 	git pull -v "../parent" >out 2>err &&
@@ -62,7 +62,7 @@ test_expect_failure 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_failure 'git pull -v -q' '
+test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&
 	git pull -v -q "../parent" >out 2>err &&
@@ -70,7 +70,7 @@ test_expect_failure 'git pull -v -q' '
 	test_must_be_empty err)
 '
 
-test_expect_failure 'git pull -q -v' '
+test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
 	git pull -q -v "../parent" >out 2>err &&
-- 
2.1.4

[PATCH 03/14] pull: pass git-merge's options to git-merge

[Paul Tan]

Specify git-merge's options in the option list, and pass any specified
options to git-merge.

These options are:

* -n, --stat, --summary: since d8abe14 (merge, pull: introduce
  '--(no-)stat' option, 2008-04-06)

* --log: since efb779f (merge, pull: add '--(no-)log' command line
  option, 2008-04-06)

* --squash: since 7d0c688 (git-merge --squash, 2006-06-23)

* --commit: since 5072a32 (Teach git-pull about --[no-]ff, --no-squash
  and --commit, 2007-10-29)

* --edit: since 8580830 ("git pull" doesn't know "--edit", 2012-02-11)

* --ff, --ff-only: since 5072a32 (Teach git-pull about --[no-]ff,
  --no-squash and --commit, 2007-10-29)

* --verify-signatures: since efed002 (merge/pull: verify GPG signatures
  of commits being merged, 2013-03-31)

* -s, --strategy: since 60fb5b2 (Use git-merge in git-pull (second
  try)., 2005-09-25)

* -X, --strategy-option: since ee2c795 (Teach git-pull to pass
  -X<option> to git-merge, 2009-11-25)

* -S, --gpg-sign: since ea230d8 (pull: add the --gpg-sign option.,
  2014-02-10)

While most options can be implemented through the parse_opt_passthru()
callback, --strategy and --strategy-option are implemented by
introducing argv_push_strategies() and argv_push_strategy_opts() as they
can be specified multiple times.

Also allow --log to take a value <n>, thus making the failing test
'--log=1 limits shortlog length' pass.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c               | 94 ++++++++++++++++++++++++++++++++++++++++++++
 t/t4013-diff-various.sh      |  3 --
 t/t5150-request-pull.sh      |  2 +-
 t/t5524-pull-msg.sh          |  2 +-
 t/t5572-pull-submodule.sh    |  4 --
 t/t6029-merge-subtree.sh     |  6 +--
 t/t6037-merge-ours-theirs.sh |  2 +-
 7 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a4d9c92..573e4f6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -44,6 +44,18 @@ static const char * const pull_usage[] = {
 static int opt_verbosity;
 static char *opt_progress;
 
+/* Options passed to git-merge */
+static char *opt_diffstat;
+static char *opt_log;
+static char *opt_squash;
+static char *opt_commit;
+static char *opt_edit;
+static char *opt_ff;
+static char *opt_verify_signatures;
+static struct string_list opt_strategies = STRING_LIST_INIT_NODUP;
+static struct string_list opt_strategy_opts = STRING_LIST_INIT_NODUP;
+static char *opt_gpg_sign;
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -51,6 +63,47 @@ static struct option pull_options[] = {
 	  N_("force progress reporting"),
 	  PARSE_OPT_NOARG, parse_opt_passthru},
 
+	/* Options passed to git-merge */
+	OPT_GROUP(N_("Options related to merging")),
+	{ OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL,
+	  N_("do not show a diffstat at the end of the merge"),
+	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "no-stat" },
+	{ OPTION_CALLBACK, 0, "stat", &opt_diffstat, NULL,
+	  N_("show a diffstat at the end of the merge"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "stat" },
+	{ OPTION_CALLBACK, 0, "summary", &opt_diffstat, NULL,
+	  N_("(synonym to --stat)"),
+	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, parse_opt_passthru, (intptr_t) "stat" },
+	{ OPTION_CALLBACK, 0, "log", &opt_log, N_("n"),
+	  N_("add (at most <n>) entries from shortlog to merge commit message"),
+	  PARSE_OPT_OPTARG, parse_opt_passthru, (intptr_t) "log" },
+	{ OPTION_CALLBACK, 0, "squash", &opt_squash, NULL,
+	  N_("create a single commit instead of doing a merge"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "squash" },
+	{ OPTION_CALLBACK, 0, "commit", &opt_commit, NULL,
+	  N_("perform a commit if the merge succeeds (default)"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "commit" },
+	{ OPTION_CALLBACK, 0, "edit", &opt_edit, NULL,
+	  N_("edit message before committing"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "edit" },
+	{ OPTION_CALLBACK, 0, "ff", &opt_ff, NULL,
+	  N_("allow fast-forward"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "ff" },
+	{ OPTION_CALLBACK, 0, "ff-only", &opt_ff, NULL,
+	  N_("abort if fast-forward is not possible"),
+	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "ff-only" },
+	{ OPTION_CALLBACK, 0, "verify-signatures", &opt_verify_signatures, NULL,
+	  N_("verify that the named commit has a valid GPG signature"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "verify-signatures" },
+	OPT_STRING_LIST('s', "strategy", &opt_strategies, N_("strategy"),
+			N_("merge strategy to use")),
+	OPT_STRING_LIST('X', "strategy-option", &opt_strategy_opts,
+			N_("option=value"),
+			N_("option for selected merge strategy")),
+	{ OPTION_CALLBACK, 'S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
+	  N_("GPG sign commit"),
+	  PARSE_OPT_OPTARG, parse_opt_passthru, (intptr_t) "gpg-sign" },
+
 	OPT_END()
 };
 
@@ -69,6 +122,27 @@ static void argv_push_verbosity(struct argv_array *arr)
 }
 
 /**
+ * Pushes opt_strategies as "-s <strategy>" flags into arr.
+ */
+static void argv_push_strategies(struct argv_array *arr)
+{
+	struct string_list_item *item;
+
+	for_each_string_list_item(item, &opt_strategies)
+		argv_array_pushl(arr, "-s", item->string, NULL);
+}
+
+/**
+ * Pushes opt_strategy_opts as "-X <option>" flags into arr.
+ */
+static void argv_push_strategy_opts(struct argv_array *arr)
+{
+	struct string_list_item *item;
+	for_each_string_list_item(item, &opt_strategy_opts)
+		argv_array_pushl(arr, "-X", item->string, NULL);
+}
+
+/**
  * Parses argv into [<repo> [<refspecs>...]], returning their values in `repo`
  * as a string and `refspecs` as a null-terminated array of strings. If `repo`
  * is not provided in argv, it is set to NULL.
@@ -123,6 +197,26 @@ static int run_merge(void)
 	if (opt_progress)
 		argv_array_push(&args, opt_progress);
 
+	/* Options passed to git-merge */
+	if (opt_diffstat)
+		argv_array_push(&args, opt_diffstat);
+	if (opt_log)
+		argv_array_push(&args, opt_log);
+	if (opt_squash)
+		argv_array_push(&args, opt_squash);
+	if (opt_commit)
+		argv_array_push(&args, opt_commit);
+	if (opt_edit)
+		argv_array_push(&args, opt_edit);
+	if (opt_ff)
+		argv_array_push(&args, opt_ff);
+	if (opt_verify_signatures)
+		argv_array_push(&args, opt_verify_signatures);
+	argv_push_strategies(&args);
+	argv_push_strategy_opts(&args);
+	if (opt_gpg_sign)
+		argv_array_push(&args, opt_gpg_sign);
+
 	argv_array_push(&args, "FETCH_HEAD");
 	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
 	argv_array_clear(&args);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index a11e48b..6ec6072 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -7,9 +7,6 @@ test_description='Various diff formatting options'
 
 . ./test-lib.sh
 
-skip_all='setup for this test suite requires git pull -s'
-test_done
-
 LF='
 '
 
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index f5ea605..82c33b8 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -132,7 +132,7 @@ test_expect_success 'pull request when forgot to push' '
 
 '
 
-test_expect_failure 'pull request after push' '
+test_expect_success 'pull request after push' '
 
 	rm -fr downstream.git &&
 	git init --bare downstream.git &&
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 1ba8054..eebb8c9 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -25,7 +25,7 @@ test_expect_success setup '
 	git commit -m "do not clobber $dollar signs"
 '
 
-test_expect_failure pull '
+test_expect_success pull '
 (
 	cd cloned &&
 	git pull --log &&
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 408c886..accfa5c 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -5,10 +5,6 @@ test_description='pull can handle submodules'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
-KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
-
 reset_branch_to_HEAD () {
 	git branch -D "$1" &&
 	git checkout -b "$1" HEAD &&
diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh
index 54af64e..73fc240 100755
--- a/t/t6029-merge-subtree.sh
+++ b/t/t6029-merge-subtree.sh
@@ -61,7 +61,7 @@ test_expect_success 'initial merge' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'merge update' '
+test_expect_success 'merge update' '
 	cd ../git-gui &&
 	echo git-gui2 > git-gui.sh &&
 	o3=$(git hash-object git-gui.sh) &&
@@ -95,7 +95,7 @@ test_expect_success 'initial ambiguous subtree' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'merge using explicit' '
+test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
 	git pull -Xsubtree=git-gui gui master2 &&
@@ -108,7 +108,7 @@ test_expect_failure 'merge using explicit' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'merge2 using explicit' '
+test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard master2 &&
 	git pull -Xsubtree=git-gui2 gui master2 &&
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 876e23b..3889eca 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -65,7 +65,7 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 	git diff --exit-code master HEAD -- file
 '
 
-test_expect_failure 'pull passes -X to underlying merge' '
+test_expect_success 'pull passes -X to underlying merge' '
 	git reset --hard master && git pull -s recursive -Xours . side &&
 	git reset --hard master && git pull -s recursive -X ours . side &&
 	git reset --hard master && git pull -s recursive -Xtheirs . side &&
-- 
2.1.4

[PATCH 04/14] pull: pass git-fetch's options to git-fetch

[Paul Tan]

git-pull.sh passed all unknown options to git-fetch, thus supporting
git-fetch's options.

However, this led to problems as git-pull was not aware of the specifics
of the options passed to git-fetch. For example, running

	git-pull --all --dry-run

would pass the --dry-fetch option to git-fetch, but would still run
git-merge. This is because git-pull.sh is not aware of the --dry-run
option.

Fix this by parsing all of git-fetch's options at git-pull. The options
are:

* --all

* -a, --append

* --upload-pack

* -f, --force

* -t, --tags

* -p, --prune

* --recurse-submodules

* --dry-run

* -k, --keep

* --depth

* --unshallow

* --update-shallow

* --refmap

Since 29609e6 (pull: do nothing on --dry-run, 2010-05-25) git-pull
supported the --dry-run option, exiting after git-fetch if --dry-run is
set. Re-implement this behavior.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c            | 95 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t5500-fetch-pack.sh     | 10 ++---
 t/t5521-pull-options.sh   |  6 +--
 t/t7403-submodule-sync.sh |  8 ++--
 4 files changed, 107 insertions(+), 12 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 573e4f6..07ad783 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -56,6 +56,21 @@ static struct string_list opt_strategies = STRING_LIST_INIT_NODUP;
 static struct string_list opt_strategy_opts = STRING_LIST_INIT_NODUP;
 static char *opt_gpg_sign;
 
+/* Options passed to git-fetch */
+static char *opt_all;
+static char *opt_append;
+static char *opt_upload_pack;
+static int opt_force;
+static char *opt_tags;
+static char *opt_prune;
+static char *opt_recurse_submodules;
+static int opt_dry_run;
+static char *opt_keep;
+static char *opt_depth;
+static char *opt_unshallow;
+static char *opt_update_shallow;
+static char *opt_refmap;
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -104,6 +119,46 @@ static struct option pull_options[] = {
 	  N_("GPG sign commit"),
 	  PARSE_OPT_OPTARG, parse_opt_passthru, (intptr_t) "gpg-sign" },
 
+	/* Options passed to git-fetch */
+	OPT_GROUP(N_("Options related to fetching")),
+	{ OPTION_CALLBACK, 0, "all", &opt_all, 0,
+	  N_("fetch from all remotes"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "all" },
+	{ OPTION_CALLBACK, 'a', "append", &opt_append, 0,
+	  N_("append to .git/FETCH_HEAD instead of overwriting"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "append" },
+	{ OPTION_CALLBACK, 0, "upload-pack", &opt_upload_pack, N_("path"),
+	  N_("path to upload pack on remote end"),
+	  0, parse_opt_passthru, (intptr_t) "upload-pack" },
+	OPT__FORCE(&opt_force, N_("force overwrite of local branch")),
+	{ OPTION_CALLBACK, 't', "tags", &opt_tags, 0,
+	  N_("fetch all tags and associated objects"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "tags" },
+	{ OPTION_CALLBACK, 'p', "prune", &opt_prune, 0,
+	  N_("prune remote-tracking branches no longer on remote"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "prune" },
+	{ OPTION_CALLBACK, 0, "recurse-submodules", &opt_recurse_submodules,
+	  N_("on-demand"),
+	  N_("control recursive fetching of submodules"),
+	  PARSE_OPT_OPTARG, parse_opt_passthru, (intptr_t) "recurse-submodules" },
+	OPT_BOOL(0, "dry-run", &opt_dry_run,
+		N_("dry run")),
+	{ OPTION_CALLBACK, 'k', "keep", &opt_keep, 0,
+	  N_("keep downloaded pack"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "keep" },
+	{ OPTION_CALLBACK, 0, "depth", &opt_depth, N_("depth"),
+	  N_("deepen history of shallow clone"),
+	  0, parse_opt_passthru, (intptr_t) "depth" },
+	{ OPTION_CALLBACK, 0, "unshallow", &opt_unshallow, 0,
+	  N_("convert to a complete repository"),
+	  PARSE_OPT_NONEG | PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "unshallow" },
+	{ OPTION_CALLBACK, 0, "update-shallow", &opt_update_shallow, 0,
+	  N_("accept refs that update .git/shallow"),
+	  PARSE_OPT_NOARG, parse_opt_passthru, (intptr_t) "update-shallow" },
+	{ OPTION_CALLBACK, 0, "refmap", &opt_refmap, N_("refmap"),
+	  N_("specify fetch refmap"),
+	  PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "refmap" },
+
 	OPT_END()
 };
 
@@ -143,6 +198,16 @@ static void argv_push_strategy_opts(struct argv_array *arr)
 }
 
 /**
+ * Pushes "-f" switches into arr to match the opt_force level.
+ */
+static void argv_push_force(struct argv_array *arr)
+{
+	int force = opt_force;
+	while (force-- > 0)
+		argv_array_push(arr, "-f");
+}
+
+/**
  * Parses argv into [<repo> [<refspecs>...]], returning their values in `repo`
  * as a string and `refspecs` as a null-terminated array of strings. If `repo`
  * is not provided in argv, it is set to NULL.
@@ -173,6 +238,33 @@ static int run_fetch(const char *repo, const char **refspecs)
 	if (opt_progress)
 		argv_array_push(&args, opt_progress);
 
+	/* Options passed to git-fetch */
+	if (opt_all)
+		argv_array_push(&args, opt_all);
+	if (opt_append)
+		argv_array_push(&args, opt_append);
+	if (opt_upload_pack)
+		argv_array_push(&args, opt_upload_pack);
+	argv_push_force(&args);
+	if (opt_tags)
+		argv_array_push(&args, opt_tags);
+	if (opt_prune)
+		argv_array_push(&args, opt_prune);
+	if (opt_recurse_submodules)
+		argv_array_push(&args, opt_recurse_submodules);
+	if (opt_dry_run)
+		argv_array_push(&args, "--dry-run");
+	if (opt_keep)
+		argv_array_push(&args, opt_keep);
+	if (opt_depth)
+		argv_array_push(&args, opt_depth);
+	if (opt_unshallow)
+		argv_array_push(&args, opt_unshallow);
+	if (opt_update_shallow)
+		argv_array_push(&args, opt_update_shallow);
+	if (opt_refmap)
+		argv_array_push(&args, opt_refmap);
+
 	if (repo)
 		argv_array_push(&args, repo);
 	while (*refspecs)
@@ -233,5 +325,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (run_fetch(repo, refspecs))
 		return 1;
 
+	if (opt_dry_run)
+		return 0;
+
 	return run_merge();
 }
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 213b231..3a9b775 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -226,14 +226,14 @@ test_expect_success 'add two more (part 2)' '
 	add B69 $B68
 '
 
-test_expect_failure 'deepening pull in shallow repo' '
+test_expect_success 'deepening pull in shallow repo' '
 	(
 		cd shallow &&
 		git pull --depth 4 .. B
 	)
 '
 
-test_expect_failure 'clone shallow object count' '
+test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
@@ -248,7 +248,7 @@ test_expect_success 'deepening fetch in shallow repo' '
 	)
 '
 
-test_expect_failure 'clone shallow object count' '
+test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
@@ -272,11 +272,11 @@ test_expect_success 'additional simple shallow deepenings' '
 	)
 '
 
-test_expect_failure 'clone shallow depth count' '
+test_expect_success 'clone shallow depth count' '
 	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 11
 '
 
-test_expect_failure 'clone shallow object count' '
+test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 89e2104..4176e11 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -78,7 +78,7 @@ test_expect_success 'git pull -q -v' '
 	test -s err)
 '
 
-test_expect_failure 'git pull --force' '
+test_expect_success 'git pull --force' '
 	mkdir clonedoldstyle &&
 	(cd clonedoldstyle && git init &&
 	cat >>.git/config <<-\EOF &&
@@ -99,7 +99,7 @@ test_expect_failure 'git pull --force' '
 	)
 '
 
-test_expect_failure 'git pull --all' '
+test_expect_success 'git pull --all' '
 	mkdir clonedmulti &&
 	(cd clonedmulti && git init &&
 	cat >>.git/config <<-\EOF &&
@@ -117,7 +117,7 @@ test_expect_failure 'git pull --all' '
 	)
 '
 
-test_expect_failure 'git pull --dry-run' '
+test_expect_success 'git pull --dry-run' '
 	test_when_finished "rm -rf clonedry" &&
 	git init clonedry &&
 	(
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index b448562..79bc135 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -96,7 +96,7 @@ test_expect_success 'change submodule url' '
 	)
 '
 
-test_expect_failure '"git submodule sync" should update submodule URLs' '
+test_expect_success '"git submodule sync" should update submodule URLs' '
 	(
 		cd super-clone &&
 		git pull --no-recurse-submodules &&
@@ -121,7 +121,7 @@ test_expect_failure '"git submodule sync" should update submodule URLs' '
 	)
 '
 
-test_expect_failure '"git submodule sync --recursive" should update all submodule URLs' '
+test_expect_success '"git submodule sync --recursive" should update all submodule URLs' '
 	(
 		cd super-clone &&
 		(
@@ -149,7 +149,7 @@ test_expect_success 'reset submodule URLs' '
 	reset_submodule_urls super-clone
 '
 
-test_expect_failure '"git submodule sync" should update submodule URLs - subdirectory' '
+test_expect_success '"git submodule sync" should update submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		git pull --no-recurse-submodules &&
@@ -177,7 +177,7 @@ test_expect_failure '"git submodule sync" should update submodule URLs - subdire
 	)
 '
 
-test_expect_failure '"git submodule sync --recursive" should update all submodule URLs - subdirectory' '
+test_expect_success '"git submodule sync --recursive" should update all submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		(
-- 
2.1.4

[PATCH 05/14] pull: error on no merge candidates

[Paul Tan]

Commit a8c9bef (pull: improve advice for unconfigured error case,
2009-10-05) fully established the current advices given by git-pull for
the different cases where git-fetch will not have anything marked for
merge:

1. We fetched from a specific remote, and a refspec was given, but it
   ended up not fetching anything. This is usually because the user
   provided a wildcard refspec which had no matches on the remote end.

2. We fetched from a non-default remote, but didn't specify a branch to
   merge. We can't use the configured one because it applies to the
   default remote, and thus the user must specify the branches to merge.

3. We fetched from the branch's or repo's default remote, but:

   a. We are not on a branch, so there will never be a configured branch
      to merge with.

   b. We are on a branch, but there is no configured branch to merge
      with.

4. We fetched from the branch's or repo's default remote, but the
   configured branch to merge didn't get fetched (either it doesn't
   exist, or wasn't part of the configured fetch refspec)

Re-implement the above behavior by implementing get_merge_heads() to
parse the heads in FETCH_HEAD for merging, and implementing
die_no_merge_candidates(), which will be called when FETCH_HEAD has no
heads for merging.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5520-pull.sh |  10 ++---
 2 files changed, 137 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 07ad783..8982fdf 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -10,6 +10,8 @@
 #include "parse-options.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "sha1-array.h"
+#include "remote.h"
 
 /**
  * Given an option opt, where opt->value points to a char* and opt->defval is a
@@ -207,6 +209,130 @@ static void argv_push_force(struct argv_array *arr)
 		argv_array_push(arr, "-f");
 }
 
+struct known_remote {
+	struct known_remote *next;
+	struct remote *remote;
+};
+
+/**
+ * Use this callback with for_each_remote() to get the configured remotes as
+ * a singly linked known_remote list. cb_data must be a pointer to a
+ * struct known_remote*, which must be initialized to NULL. For example,
+ * For example:
+ *
+ *     struct known_remote *list = NULL;
+ *     for_each_remote(add_known_remote, &list);
+ */
+static int add_known_remote(struct remote *remote, void *cb_data)
+{
+	struct known_remote **list = cb_data;
+	struct known_remote *item;
+
+	item = xmalloc(sizeof(*item));
+	item->remote = remote;
+	item->next = *list;
+	*list = item;
+	return 0;
+}
+
+/**
+ * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
+ * into merge_heads.
+ */
+static void get_merge_heads(struct sha1_array *merge_heads)
+{
+	const char *filename = git_path("FETCH_HEAD");
+	FILE *fp;
+	struct strbuf sb = STRBUF_INIT;
+	unsigned char sha1[GIT_SHA1_RAWSZ];
+
+	if (!(fp = fopen(filename, "r")))
+		die_errno(_("could not open '%s' for reading"), filename);
+	while(strbuf_getline(&sb, fp, '\n') != EOF) {
+		if (get_sha1_hex(sb.buf, sha1))
+			continue;  /* invalid line: does not start with SHA1 */
+		if (starts_with(sb.buf + GIT_SHA1_HEXSZ, "\tnot-for-merge"))
+			continue;  /* ref is not-for-merge */
+		sha1_array_append(merge_heads, sha1);
+	}
+	fclose(fp);
+	strbuf_release(&sb);
+}
+
+/**
+ * Dies with the appropriate reason for why there are no merge candidates:
+ *
+ * 1. We fetched from a specific remote, and a refspec was given, but it ended
+ *    up not fetching anything. This is usually because the user provided a
+ *    wildcard refspec which had no matches on the remote end.
+ *
+ * 2. We fetched from a non-default remote, but didn't specify a branch to
+ *    merge. We can't use the configured one because it applies to the default
+ *    remote, thus the user must specify the branches to merge.
+ *
+ * 3. We fetched from the branch's or repo's default remote, but:
+ *
+ *    a. We are not on a branch, so there will never be a configured branch to
+ *       merge with.
+ *
+ *    b. We are on a branch, but there is no configured branch to merge with.
+ *
+ * 4. We fetched from the branch's or repo's default remote, but the configured
+ *    branch to merge didn't get fetched. (Either it doesn't exist, or wasn't
+ *    part of the configured fetch refspec.)
+ */
+static void NORETURN die_no_merge_candidates(const char *repo, const char **refspecs)
+{
+	struct branch *curr_branch = branch_get("HEAD");
+	const char *remote = curr_branch ? curr_branch->remote_name : NULL;
+
+	if (*refspecs) {
+		fprintf(stderr,
+			_("There are no candidates for merging among the refs that you just fetched.\n"
+			"Generally this means that you provided a wildcard refspec which had no\n"
+			"matches on the remote end.\n"));
+	} else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
+		fprintf(stderr,
+			_("You asked to pull from the remote '%s', but did not specify\n"
+			"a branch. Because this is not the default configured remote\n"
+			"for your current branch, you must specify a branch on the command line.\n"),
+			repo);
+	} else if (!curr_branch) {
+		fprintf(stderr,
+			_("You are not currently on a branch. Please specify which\n"
+			"branch you want to merge with. See git-pull(1) for details.\n"
+			"\n"
+			"    git pull <remote> <branch>\n"
+			"\n"));
+	} else if (!curr_branch->merge_nr) {
+		struct known_remote *remotes = NULL;
+		const char *remote_name = "<remote>";
+
+		for_each_remote(add_known_remote, &remotes);
+		if (remotes && !remotes->next)
+			remote_name = remotes->remote->name;
+
+		fprintf(stderr,
+			_("There is no tracking information for the current branch.\n"
+			"Please specify which branch you want to merge with.\n"
+			"See git-pull(1) for details.\n"
+			"\n"
+			"    git pull <remote> <branch>\n"
+			"\n"
+			"If you wish to set tracking information for this branch you can do so with:\n"
+			"\n"
+			"    git branch --set-upstream-to=%s/<branch> %s\n"
+			"\n"),
+			remote_name, curr_branch->name);
+	} else
+		fprintf(stderr,
+			_("Your configuration specifies to merge with the ref '%s'\n"
+			"from the remote, but no such ref was fetched.\n"
+			"\n"),
+			*curr_branch->merge_name);
+	exit(1);
+}
+
 /**
  * Parses argv into [<repo> [<refspecs>...]], returning their values in `repo`
  * as a string and `refspecs` as a null-terminated array of strings. If `repo`
@@ -318,6 +444,7 @@ static int run_merge(void)
 int cmd_pull(int argc, const char **argv, const char *prefix)
 {
 	const char *repo, **refspecs;
+	struct sha1_array merge_heads = SHA1_ARRAY_INIT;
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
@@ -328,5 +455,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_dry_run)
 		return 0;
 
+	get_merge_heads(&merge_heads);
+
+	if (!merge_heads.nr)
+		die_no_merge_candidates(repo, refspecs);
+
 	return run_merge();
 }
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index f32b8cb..0ff5df3 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -116,7 +116,7 @@ test_expect_failure 'the default remote . should not break explicit pull' '
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-test_expect_failure 'fail if wildcard spec does not match any refs' '
+test_expect_success 'fail if wildcard spec does not match any refs' '
 	git checkout -b test copy^ &&
 	test_when_finished "git checkout -f copy && git branch -D test" &&
 	test "$(cat file)" = file &&
@@ -125,7 +125,7 @@ test_expect_failure 'fail if wildcard spec does not match any refs' '
 	test "$(cat file)" = file
 '
 
-test_expect_failure 'fail if no branches specified with non-default remote' '
+test_expect_success 'fail if no branches specified with non-default remote' '
 	git remote add test_remote . &&
 	test_when_finished "git remote remove test_remote" &&
 	git checkout -b test copy^ &&
@@ -137,7 +137,7 @@ test_expect_failure 'fail if no branches specified with non-default remote' '
 	test "$(cat file)" = file
 '
 
-test_expect_failure 'fail if not on a branch' '
+test_expect_success 'fail if not on a branch' '
 	git remote add origin . &&
 	test_when_finished "git remote remove origin" &&
 	git checkout HEAD^ &&
@@ -148,7 +148,7 @@ test_expect_failure 'fail if not on a branch' '
 	test "$(cat file)" = file
 '
 
-test_expect_failure 'fail if no configuration for current branch' '
+test_expect_success 'fail if no configuration for current branch' '
 	git remote add test_remote . &&
 	test_when_finished "git remote remove test_remote" &&
 	git checkout -b test copy^ &&
@@ -160,7 +160,7 @@ test_expect_failure 'fail if no configuration for current branch' '
 	test "$(cat file)" = file
 '
 
-test_expect_failure 'fail if upstream branch does not exist' '
+test_expect_success 'fail if upstream branch does not exist' '
 	git checkout -b test copy^ &&
 	test_when_finished "git checkout -f copy && git branch -D test" &&
 	test_config branch.test.remote . &&
-- 
2.1.4

[PATCH 06/14] pull: support pull.ff config

[Paul Tan]

Since b814da8 (pull: add pull.ff configuration, 2014-01-15), git-pull.sh
would lookup the configuration value of "pull.ff", and set the flag
"--ff" if its value is "true", "--no-ff" if its value is "false" and
"--ff-only" if its value is "only".

Re-implement this behavior.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c               | 25 +++++++++++++++++++++++++
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 8982fdf..b305a47 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -209,6 +209,28 @@ static void argv_push_force(struct argv_array *arr)
 		argv_array_push(arr, "-f");
 }
 
+/**
+ * If pull.ff is "true", returns "--ff". If pull.ff is "false", returns
+ * "--no-ff". If pull.ff is "only", returns "--ff-only". Otherwise, returns
+ * NULL.
+ */
+static const char *config_get_ff(void)
+{
+	const char *value;
+
+	if (git_config_get_value("pull.ff", &value))
+		return NULL;
+	switch (git_config_maybe_bool("pull.ff", value)) {
+		case 0:
+			return "--no-ff";
+		case 1:
+			return "--ff";
+	}
+	if (!strcmp("pull.ff", "only"))
+		return "--ff-only";
+	die(_("Invalid value for pull.ff: %s"), value);
+}
+
 struct known_remote {
 	struct known_remote *next;
 	struct remote *remote;
@@ -449,6 +471,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
+	if (!opt_ff)
+		opt_ff = xstrdup_or_null(config_get_ff());
+
 	if (run_fetch(repo, refspecs))
 		return 1;
 
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 7a846a2..f768c90 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -45,7 +45,7 @@ test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' '
 	test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
 '
 
-test_expect_failure 'fast-forward pull creates merge with "false" in pull.ff' '
+test_expect_success 'fast-forward pull creates merge with "false" in pull.ff' '
 	git reset --hard c0 &&
 	test_config pull.ff false &&
 	git pull . c1 &&
@@ -53,7 +53,7 @@ test_expect_failure 'fast-forward pull creates merge with "false" in pull.ff' '
 	test "$(git rev-parse HEAD^2)" = "$(git rev-parse c1)"
 '
 
-test_expect_failure 'pull prevents non-fast-forward with "only" in pull.ff' '
+test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
 	git reset --hard c1 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c3
-- 
2.1.4

[PATCH 07/14] pull: check if in unresolved merge state

[Paul Tan]

Since, commit d38a30d (Be more user-friendly when refusing to do
something because of conflict., 2010-01-12), git-pull will error out
with user-friendly advices if the user is in the middle of a merge or
has unmerged files.

Re-implement this behavior. While the "has unmerged files" case can be
handled by die_resolve_conflict(), we introduce a new function
die_conclude_merge() for printing a different error message for when
there are no unmerged files but the merge has not been finished.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 advice.c        | 8 ++++++++
 advice.h        | 1 +
 builtin/pull.c  | 9 +++++++++
 t/t5520-pull.sh | 2 +-
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index 575bec2..4965686 100644
--- a/advice.c
+++ b/advice.c
@@ -96,6 +96,14 @@ void NORETURN die_resolve_conflict(const char *me)
 	die("Exiting because of an unresolved conflict.");
 }
 
+void NORETURN die_conclude_merge(void)
+{
+	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
+	if (advice_resolve_conflict)
+		advise(_("Please, commit your changes before you can merge."));
+	die(_("Exiting because of unfinished merge."));
+}
+
 void detach_advice(const char *new_name)
 {
 	const char fmt[] =
diff --git a/advice.h b/advice.h
index 5ecc6c1..b341a55 100644
--- a/advice.h
+++ b/advice.h
@@ -24,6 +24,7 @@ __attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
 int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
+void NORETURN die_conclude_merge(void);
 void detach_advice(const char *new_name);
 
 #endif /* ADVICE_H */
diff --git a/builtin/pull.c b/builtin/pull.c
index b305a47..9b06dfa 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "sha1-array.h"
 #include "remote.h"
+#include "dir.h"
 
 /**
  * Given an option opt, where opt->value points to a char* and opt->defval is a
@@ -471,6 +472,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
+	git_config(git_default_config, NULL);
+
+	if (read_cache_unmerged())
+		die_resolve_conflict("Pull");
+
+	if (file_exists(git_path("MERGE_HEAD")))
+		die_conclude_merge();
+
 	if (!opt_ff)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 0ff5df3..4c52175 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -171,7 +171,7 @@ test_expect_success 'fail if upstream branch does not exist' '
 	test "$(cat file)" = file
 '
 
-test_expect_failure 'fail if the index has unresolved entries' '
+test_expect_success 'fail if the index has unresolved entries' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
-- 
2.1.4

[PATCH 08/14] pull: fast-forward working tree if head is updated

[Paul Tan]

Since b10ac50 (Fix pulling into the same branch., 2005-08-25), git-pull,
upon detecting that git-fetch updated the current head, will
fast-forward the working tree to the updated head commit.

Re-implement this behavior.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 30 ++++++++++++++++++++++++++++++
 t/t5520-pull.sh |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9b06dfa..3b7029f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -468,6 +468,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 {
 	const char *repo, **refspecs;
 	struct sha1_array merge_heads = SHA1_ARRAY_INIT;
+	unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
@@ -483,12 +484,41 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!opt_ff)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
+	if (get_sha1("HEAD", orig_head))
+		hashclr(orig_head);
+
 	if (run_fetch(repo, refspecs))
 		return 1;
 
 	if (opt_dry_run)
 		return 0;
 
+	if (get_sha1("HEAD", curr_head))
+		hashclr(curr_head);
+
+	if (!is_null_sha1(orig_head) && !is_null_sha1(curr_head) &&
+			hashcmp(orig_head, curr_head)) {
+		/*
+		 * The fetch involved updating the current branch.
+		 *
+		 * The working tree and the index file are still based on
+		 * orig_head commit, but we are merging into curr_head.
+		 * Update the working tree to match curr_head.
+		 */
+
+		warning(_("fetch updated the current branch head.\n"
+			"fast-forwarding your working tree from\n"
+			"commit %s."), sha1_to_hex(orig_head));
+
+		if (checkout_fast_forward(orig_head, curr_head, 0))
+			die(_("Cannot fast-forward your working tree.\n"
+				"After making sure that you saved anything precious from\n"
+				"$ git diff %s\n"
+				"output, run\n"
+				"$ git reset --hard\n"
+				"to recover."), sha1_to_hex(orig_head));
+	}
+
 	get_merge_heads(&merge_heads);
 
 	if (!merge_heads.nr)
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4c52175..3645a59 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -191,7 +191,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_cmp expected file
 '
 
-test_expect_failure 'fast-forwards working tree if branch head is updated' '
+test_expect_success 'fast-forwards working tree if branch head is updated' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
@@ -201,7 +201,7 @@ test_expect_failure 'fast-forwards working tree if branch head is updated' '
 	test "$(git rev-parse third)" = "$(git rev-parse second)"
 '
 
-test_expect_failure 'fast-forward fails with conflicting work tree' '
+test_expect_success 'fast-forward fails with conflicting work tree' '
 	git checkout -b third second^ &&
 	test_when_finished "git checkout -f copy && git branch -D third" &&
 	test "$(cat file)" = file &&
-- 
2.1.4

[PATCH 09/14] pull: implement pulling into an unborn branch

[Paul Tan]

b4dc085 (pull: merge into unborn by fast-forwarding from empty
tree, 2013-06-20) established git-pull's current behavior of pulling
into an unborn branch by fast-forwarding the work tree from an empty
tree to the merge head, then setting HEAD to the merge head.

Re-implement this behavior by introducing pull_into_void() which will be
called instead of run_merge() if HEAD is invalid.

Helped-by: Stephen Robin <stephen.robin@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 29 ++++++++++++++++++++++++++++-
 t/t5520-pull.sh |  4 ++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3b7029f..ba2ff01 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -13,6 +13,7 @@
 #include "sha1-array.h"
 #include "remote.h"
 #include "dir.h"
+#include "refs.h"
 
 /**
  * Given an option opt, where opt->value points to a char* and opt->defval is a
@@ -424,6 +425,27 @@ static int run_fetch(const char *repo, const char **refspecs)
 }
 
 /**
+ * "Pulls into void" by branching off merge_head.
+ */
+static int pull_into_void(unsigned char merge_head[GIT_SHA1_RAWSZ],
+		unsigned char curr_head[GIT_SHA1_RAWSZ])
+{
+	/*
+	 * Two-way merge: we claim the index is based on an empty tree,
+	 * and try to fast-forward to HEAD. This ensures we will not lose
+	 * index/worktree changes that the user already made on the unborn
+	 * branch.
+	 */
+	if (checkout_fast_forward(EMPTY_TREE_SHA1_BIN, merge_head, 0))
+		return 1;
+
+	if (update_ref("initial pull", "HEAD", merge_head, curr_head, 0, UPDATE_REFS_DIE_ON_ERR))
+		return 1;
+
+	return 0;
+}
+
+/**
  * Runs git-merge, returning its exit status.
  */
 static int run_merge(void)
@@ -524,5 +546,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!merge_heads.nr)
 		die_no_merge_candidates(repo, refspecs);
 
-	return run_merge();
+	if (is_null_sha1(orig_head)) {
+		if (merge_heads.nr > 1)
+			die(_("Cannot merge multiple branches into empty head."));
+		return pull_into_void(*merge_heads.sha1, curr_head);
+	} else
+		return run_merge();
 }
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 3645a59..2131749 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -26,7 +26,7 @@ test_expect_success 'pulling into void' '
 	test_cmp file cloned/file
 '
 
-test_expect_failure 'pulling into void using master:master' '
+test_expect_success 'pulling into void using master:master' '
 	git init cloned-uho &&
 	(
 		cd cloned-uho &&
@@ -76,7 +76,7 @@ test_expect_success 'pulling into void does not remove new staged files' '
 	)
 '
 
-test_expect_failure 'pulling into void must not create an octopus' '
+test_expect_success 'pulling into void must not create an octopus' '
 	git init cloned-octopus &&
 	(
 		cd cloned-octopus &&
-- 
2.1.4

[PATCH 10/14] pull: set reflog message

[Paul Tan]

f947413 (Use GIT_REFLOG_ACTION environment variable instead.,
2006-12-28) established git-pull's method for setting the reflog
message, which is to set the environment variable GIT_REFLOG_ACTION to
the evaluation of "pull${1+ $*}" if it has not already been set.

Re-implement this behavior.

Signed-off-by: Stephen Robin <stephen.robin@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 23 +++++++++++++++++++++++
 t/t5520-pull.sh |  4 ++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index ba2ff01..81e31a1 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -212,6 +212,26 @@ static void argv_push_force(struct argv_array *arr)
 }
 
 /**
+ * Sets the GIT_REFLOG_ACTION environment variable to the concatenation of argv
+ */
+static void set_reflog_message(int argc, const char **argv)
+{
+	int i;
+	struct strbuf msg = STRBUF_INIT;
+
+	for (i = 0; i < argc; i++) {
+		strbuf_addstr(&msg, argv[i]);
+		strbuf_addch(&msg, ' ');
+	}
+
+	strbuf_rtrim(&msg);
+
+	setenv("GIT_REFLOG_ACTION", msg.buf, 0);
+
+	strbuf_release(&msg);
+}
+
+/**
  * If pull.ff is "true", returns "--ff". If pull.ff is "false", returns
  * "--no-ff". If pull.ff is "only", returns "--ff-only". Otherwise, returns
  * NULL.
@@ -492,6 +512,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct sha1_array merge_heads = SHA1_ARRAY_INIT;
 	unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
 
+	if (!getenv("GIT_REFLOG_ACTION"))
+		set_reflog_message(argc, argv);
+
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 2131749..9414cc1 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -85,7 +85,7 @@ test_expect_success 'pulling into void must not create an octopus' '
 	)
 '
 
-test_expect_failure 'test . as a remote' '
+test_expect_success 'test . as a remote' '
 	git branch copy master &&
 	git config branch.copy.remote . &&
 	git config branch.copy.merge refs/heads/master &&
@@ -101,7 +101,7 @@ test_expect_failure 'test . as a remote' '
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-test_expect_failure 'the default remote . should not break explicit pull' '
+test_expect_success 'the default remote . should not break explicit pull' '
 	git checkout -b second master^ &&
 	echo modified >file &&
 	git commit -a -m modified &&
-- 
2.1.4

[PATCH 11/14] pull: teach git pull about --rebase

[Paul Tan]

Since cd67e4d (Teach 'git pull' about --rebase, 2007-11-28), if the
--rebase option is set, git-rebase is run instead of git-merge.

Re-implement this by introducing run_rebase(), which is called instead
of run_merge() if opt_rebase is a true value.

Since c85c792 (pull --rebase: be cleverer with rebased upstream
branches, 2008-01-26), git-pull handles the case where the upstream
branch was rebased since it was last fetched. The fork point (old remote
ref) of the branch from the upstream branch is calculated before fetch,
and then rebased from onto the new remote head (merge_head) after fetch.

Re-implement this by introducing get_merge_branch_2() and
get_merge_branch_1() to find the upstream branch for the
specified/current branch, and get_rebase_fork_point() which will find
the fork point between the upstream branch and current branch.

However, the above change created a problem where git-rebase cannot
detect commits that are already upstream, and thus may result in
unnecessary conflicts. cf65426 (pull --rebase: Avoid spurious conflicts
and reapplying unnecessary patches, 2010-08-12) fixes this by ignoring
the above old remote ref if it is contained within the merge base of the
merge head and the current branch.

This is re-implemented in run_rebase() where fork_point is not used if
it is the merge base returned by get_octopus_merge_base().

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c              | 227 +++++++++++++++++++++++++++++++++++++++++++-
 t/t5520-pull.sh             |  26 ++---
 t/t5521-pull-options.sh     |   6 +-
 t/t7406-submodule-update.sh |   2 -
 4 files changed, 241 insertions(+), 20 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 81e31a1..f18a21c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -39,6 +39,42 @@ static int parse_opt_passthru(const struct option *opt, const char *arg, int uns
 	return 0;
 }
 
+enum rebase_type {
+	REBASE_FALSE,
+	REBASE_TRUE,
+	REBASE_PRESERVE
+};
+
+/**
+ * Parses the value of --rebase, branch.*.rebase or pull.rebase. If value is a
+ * false value, returns REBASE_FALSE. If value is a true value, returns
+ * REBASE_TRUE. If value is "preserve", returns REBASE_PRESERVE. Otherwise,
+ * returns -1 to signify an invalid value.
+ */
+static int parse_config_rebase(const char *value)
+{
+	int v = git_config_maybe_bool("pull.rebase", value);
+	if (v >= 0)
+		return v;
+	if (!strcmp(value, "preserve"))
+		return REBASE_PRESERVE;
+	return -1;
+}
+
+/**
+ * Callback for --rebase, which parses arg with parse_config_rebase().
+ */
+static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
+{
+	int *value = (int*) opt->value;
+
+	if (arg)
+		*value = parse_config_rebase(arg);
+	else
+		*value = unset ? REBASE_FALSE : REBASE_TRUE;
+	return *value >= 0 ? 0 : -1;
+}
+
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repo> [<refspec>...]]"),
 	NULL
@@ -48,7 +84,8 @@ static const char * const pull_usage[] = {
 static int opt_verbosity;
 static char *opt_progress;
 
-/* Options passed to git-merge */
+/* Options passed to git-merge or git-rebase */
+static int opt_rebase;
 static char *opt_diffstat;
 static char *opt_log;
 static char *opt_squash;
@@ -82,8 +119,12 @@ static struct option pull_options[] = {
 	  N_("force progress reporting"),
 	  PARSE_OPT_NOARG, parse_opt_passthru},
 
-	/* Options passed to git-merge */
+	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
+	{ OPTION_CALLBACK, 'r', "rebase", &opt_rebase,
+	  N_("false|true|preserve"),
+	  N_("incorporate changes by rebasing rather than merging"),
+	  PARSE_OPT_OPTARG, parse_opt_rebase, 0},
 	{ OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL,
 	  N_("do not show a diffstat at the end of the merge"),
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "no-stat" },
@@ -506,11 +547,185 @@ static int run_merge(void)
 	return ret;
 }
 
+/**
+ * Returns the merge branch for the current branch. Returns NULL if repo is not
+ * a valid remote, HEAD does not point to a branch, repo is not the branch's
+ * configured remote or the branch does not have any configured merge branch.
+ */
+static char *get_merge_branch_1(const char *repo)
+{
+	struct remote *rm;
+	struct branch *curr_branch;
+
+	if (repo && !(rm = remote_get(repo)))
+		return NULL;
+	if (!(curr_branch = branch_get("HEAD")))
+		return NULL;
+	if (repo && curr_branch->remote != rm)
+		return NULL;
+	if (!curr_branch->merge_nr)
+		return NULL;
+	return xstrdup(curr_branch->merge[0]->dst);
+}
+
+/**
+ * Given a refspec, returns the merge branch. Returns NULL if the refspec src
+ * does not refer to a branch.
+ *
+ * FIXME: It should return the tracking branch. Currently only works with the
+ * default mapping.
+ */
+static char *get_merge_branch_2(const char *repo, const char *refspec)
+{
+	struct refspec *spec;
+	const char *remote;
+	char *merge_branch;
+
+	spec = parse_fetch_refspec(1, &refspec);
+	remote = spec->src;
+	if (!*remote || !strcmp(remote, "HEAD"))
+		remote = "HEAD";
+	else if (skip_prefix(remote, "heads/", &remote))
+		;
+	else if (skip_prefix(remote, "refs/heads/", &remote))
+		;
+	else if (starts_with(remote, "refs/") ||
+		starts_with(remote, "tags/") ||
+		starts_with(remote, "remotes/"))
+		remote = "";
+
+	if (*remote) {
+		if (!strcmp(repo, "."))
+			merge_branch = mkpathdup("refs/heads/%s", remote);
+		else
+			merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
+	} else
+		merge_branch = NULL;
+
+	free_refspec(1, spec);
+	return merge_branch;
+}
+
+/**
+ * Sets fork_point to the point at which the current branch forked from its
+ * remote merge branch. Returns 0 on success, -1 on failure.
+ */
+static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
+		const char *repo, const char *refspec)
+{
+	int ret;
+	struct branch *curr_branch;
+	char *remote_merge_branch;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!(curr_branch = branch_get("HEAD")))
+		return -1;
+
+	if (refspec)
+		remote_merge_branch = get_merge_branch_2(repo, refspec);
+	else
+		remote_merge_branch = get_merge_branch_1(repo);
+
+	if (!remote_merge_branch)
+		return -1;
+
+	argv_array_pushl(&args, "merge-base", "--fork-point",
+			remote_merge_branch, curr_branch->name, NULL);
+	cp.argv = args.argv;
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.git_cmd = 1;
+
+	if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
+		goto cleanup;
+
+	if ((ret = get_sha1_hex(sb.buf, fork_point)))
+		goto cleanup;
+
+cleanup:
+	free(remote_merge_branch);
+	strbuf_release(&sb);
+	return ret ? -1 : 0;
+}
+
+/**
+ * Sets merge_base to the octopus merge base of curr_head, merge_head and
+ * fork_point. Returns 0 if a merge base is found, 1 otherwise.
+ */
+static int get_octopus_merge_base(unsigned char merge_base[GIT_SHA1_HEXSZ],
+		unsigned char curr_head[GIT_SHA1_RAWSZ],
+		unsigned char merge_head[GIT_SHA1_RAWSZ],
+		unsigned char fork_point[GIT_SHA1_RAWSZ])
+{
+	struct commit_list *revs = NULL, *result;
+
+	commit_list_insert(lookup_commit_reference(curr_head), &revs);
+	commit_list_insert(lookup_commit_reference(merge_head), &revs);
+	if (!is_null_sha1(fork_point))
+		commit_list_insert(lookup_commit_reference(fork_point), &revs);
+
+	result = reduce_heads(get_octopus_merge_bases(revs));
+	free_commit_list(revs);
+	if (!result)
+		return 1;
+
+	hashcpy(merge_base, result->item->object.sha1);
+	return 0;
+}
+
+/**
+ * Given the current HEAD SHA1, the merge head returned from git-fetch and the
+ * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
+ * appropriate arguments and returns its exit status.
+ */
+static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
+		unsigned char merge_head[GIT_SHA1_RAWSZ],
+		unsigned char fork_point[GIT_SHA1_RAWSZ])
+{
+	int ret;
+	unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
+	struct argv_array args = ARGV_ARRAY_INIT;
+
+	if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head, fork_point))
+		if (!is_null_sha1(fork_point) && !hashcmp(oct_merge_base, fork_point))
+			hashclr(fork_point);
+
+	argv_array_push(&args, "rebase");
+
+	/* Shared options */
+	argv_push_verbosity(&args);
+
+	/* Options passed to git-rebase */
+	if (opt_rebase == REBASE_PRESERVE)
+		argv_array_push(&args, "--preserve-merges");
+	if (opt_diffstat)
+		argv_array_push(&args, opt_diffstat);
+	argv_push_strategies(&args);
+	argv_push_strategy_opts(&args);
+	if (opt_gpg_sign)
+		argv_array_push(&args, opt_gpg_sign);
+
+	argv_array_push(&args, "--onto");
+	argv_array_push(&args, sha1_to_hex(merge_head));
+
+	if (!is_null_sha1(fork_point))
+		argv_array_push(&args, sha1_to_hex(fork_point));
+	else
+		argv_array_push(&args, sha1_to_hex(merge_head));
+
+	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
+	argv_array_clear(&args);
+	return ret;
+}
+
 int cmd_pull(int argc, const char **argv, const char *prefix)
 {
 	const char *repo, **refspecs;
 	struct sha1_array merge_heads = SHA1_ARRAY_INIT;
 	unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
+	unsigned char rebase_fork_point[GIT_SHA1_RAWSZ];
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -532,6 +747,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_sha1("HEAD", orig_head))
 		hashclr(orig_head);
 
+	if (opt_rebase)
+		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
+			hashclr(rebase_fork_point);
+
 	if (run_fetch(repo, refspecs))
 		return 1;
 
@@ -573,6 +792,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(*merge_heads.sha1, curr_head);
+	} else if (opt_rebase) {
+		if (merge_heads.nr > 1)
+			die(_("Cannot rebase onto multiple branches."));
+		return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
 	} else
 		return run_merge();
 }
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9414cc1..3798b96 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -212,7 +212,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
 	test "$(git rev-parse third)" = "$(git rev-parse second)"
 '
 
-test_expect_failure '--rebase' '
+test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
 	git commit -m file file &&
@@ -226,7 +226,7 @@ test_expect_failure '--rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
-test_expect_failure '--rebase fails with multiple branches' '
+test_expect_success '--rebase fails with multiple branches' '
 	git reset --hard before-rebase &&
 	test_must_fail git pull --rebase . copy master 2>err &&
 	test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
@@ -310,7 +310,7 @@ test_expect_failure 'pull.rebase=invalid fails' '
 	! git pull . copy
 '
 
-test_expect_failure '--rebase=false create a new merge commit' '
+test_expect_success '--rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=false . copy &&
@@ -319,7 +319,7 @@ test_expect_failure '--rebase=false create a new merge commit' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure '--rebase=true rebases and flattens keep-merge' '
+test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase=true . copy &&
@@ -327,7 +327,7 @@ test_expect_failure '--rebase=true rebases and flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure '--rebase=preserve rebases and merges keep-merge' '
+test_expect_success '--rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=preserve . copy &&
@@ -340,7 +340,7 @@ test_expect_success '--rebase=invalid fails' '
 	! git pull --rebase=invalid . copy
 '
 
-test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
+test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase . copy &&
@@ -348,7 +348,7 @@ test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-m
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure '--rebase with rebased upstream' '
+test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
 	git checkout copy &&
@@ -366,7 +366,7 @@ test_expect_failure '--rebase with rebased upstream' '
 
 '
 
-test_expect_failure '--rebase with rebased default upstream' '
+test_expect_success '--rebase with rebased default upstream' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git checkout --track -b to-rebase2 me/copy &&
@@ -377,7 +377,7 @@ test_expect_failure '--rebase with rebased default upstream' '
 
 '
 
-test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
@@ -409,7 +409,7 @@ test_expect_failure 'pull --rebase dies early with dirty working directory' '
 
 '
 
-test_expect_failure 'pull --rebase works on branch yet to be born' '
+test_expect_success 'pull --rebase works on branch yet to be born' '
 	git rev-parse master >expect &&
 	mkdir empty_repo &&
 	(cd empty_repo &&
@@ -456,14 +456,14 @@ test_expect_success 'setup for detecting upstreamed changes' '
 	)
 '
 
-test_expect_failure 'git pull --rebase detects upstreamed changes' '
+test_expect_success 'git pull --rebase detects upstreamed changes' '
 	(cd dst &&
 	 git pull --rebase &&
 	 test -z "$(git ls-files -u)"
 	)
 '
 
-test_expect_failure 'setup for avoiding reapplying old patches' '
+test_expect_success 'setup for avoiding reapplying old patches' '
 	(cd dst &&
 	 test_might_fail git rebase --abort &&
 	 git reset --hard origin/master
@@ -485,7 +485,7 @@ test_expect_failure 'setup for avoiding reapplying old patches' '
 	)
 '
 
-test_expect_failure 'git pull --rebase does not reapply old patches' '
+test_expect_success 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
 	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 4176e11..56e7377 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -19,7 +19,7 @@ test_expect_success 'git pull -q' '
 	test_must_be_empty out)
 '
 
-test_expect_failure 'git pull -q --rebase' '
+test_expect_success 'git pull -q --rebase' '
 	mkdir clonedqrb &&
 	(cd clonedqrb && git init &&
 	git pull -q --rebase "../parent" >out 2>err &&
@@ -38,7 +38,7 @@ test_expect_success 'git pull' '
 	test_must_be_empty out)
 '
 
-test_expect_failure 'git pull --rebase' '
+test_expect_success 'git pull --rebase' '
 	mkdir clonedrb &&
 	(cd clonedrb && git init &&
 	git pull --rebase "../parent" >out 2>err &&
@@ -54,7 +54,7 @@ test_expect_success 'git pull -v' '
 	test_must_be_empty out)
 '
 
-test_expect_failure 'git pull -v --rebase' '
+test_expect_success 'git pull -v --rebase' '
 	mkdir clonedvrb &&
 	(cd clonedvrb && git init &&
 	git pull -v --rebase "../parent" >out 2>err &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1ffd837..dda3929 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -11,8 +11,6 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 
 . ./test-lib.sh
 
-skip_all='skipping submodule update tests, requires git pull --rebase'
-test_done
 
 compare_head()
 {
-- 
2.1.4

[PATCH 12/14] pull: configure --rebase via branch.<name>.rebase or pull.rebase

[Paul Tan]

Since cd67e4d (Teach 'git pull' about --rebase, 2007-11-28),
fetch+rebase could be set by default by defining the config variable
branch.<name>.rebase. This setting can be overriden on the command line
by --rebase and --no-rebase.

Since 6b37dff (pull: introduce a pull.rebase option to enable --rebase,
2011-11-06), git-pull --rebase can also be configured via the
pull.rebase configuration option.

Re-implement support for these two configuration settings by introducing
config_get_rebase() which is called before parse_options() to set the
default value of opt_rebase.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 35 +++++++++++++++++++++++++++++++++++
 t/t5520-pull.sh | 12 ++++++------
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index f18a21c..a0958a7 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -294,6 +294,39 @@ static const char *config_get_ff(void)
 	die(_("Invalid value for pull.ff: %s"), value);
 }
 
+/**
+ * Returns the default configured value for --rebase. It first looks for the
+ * value of "branch.$curr_branch.rebase", where $curr_branch is the current
+ * branch, and if HEAD is detached or the configuration key does not exist,
+ * looks for the value of "pull.rebase". If both configuration keys do not
+ * exist, returns REBASE_FALSE.
+ */
+static int config_get_rebase(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct branch *curr_branch = branch_get("HEAD");
+	const char *key, *str;
+	int ret = -1, value;
+
+	if (curr_branch) {
+		strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
+		key = sb.buf;
+		ret = git_config_get_value(sb.buf, &str);
+	}
+	if (ret) {
+		key = "pull.rebase";
+		ret = git_config_get_value(key, &str);
+	}
+	if (ret) {
+		strbuf_release(&sb);
+		return REBASE_FALSE;
+	}
+	if ((value = parse_config_rebase(str)) < 0)
+		die(_("Invalid value for %s: %s"), key, str);
+	strbuf_release(&sb);
+	return value;
+}
+
 struct known_remote {
 	struct known_remote *next;
 	struct remote *remote;
@@ -730,6 +763,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
+	opt_rebase = config_get_rebase();
+
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 3798b96..17254ee 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -234,7 +234,7 @@ test_expect_success '--rebase fails with multiple branches' '
 	test modified = "$(git show HEAD:file)"
 '
 
-test_expect_failure 'pull.rebase' '
+test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
@@ -242,7 +242,7 @@ test_expect_failure 'pull.rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
-test_expect_failure 'branch.to-rebase.rebase' '
+test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
@@ -280,7 +280,7 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure 'pull.rebase=true flattens keep-merge' '
+test_expect_success 'pull.rebase=true flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
@@ -288,7 +288,7 @@ test_expect_failure 'pull.rebase=true flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
+test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase 1 &&
 	git pull . copy &&
@@ -296,7 +296,7 @@ test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	test file3 = "$(git show HEAD:file3.t)"
 '
 
-test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
+test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull . copy &&
@@ -304,7 +304,7 @@ test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
 	test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
 '
 
-test_expect_failure 'pull.rebase=invalid fails' '
+test_expect_success 'pull.rebase=invalid fails' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase invalid &&
 	! git pull . copy
-- 
2.1.4

[PATCH 13/14] pull --rebase: exit early when the working directory is dirty

[Paul Tan]

Re-implement the behavior introduced by f9189cf (pull --rebase: exit
early when the working directory is dirty, 2008-05-21).

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t5520-pull.sh |  6 ++---
 2 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a0958a7..c8d673d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -14,6 +14,8 @@
 #include "remote.h"
 #include "dir.h"
 #include "refs.h"
+#include "revision.h"
+#include "lockfile.h"
 
 /**
  * Given an option opt, where opt->value points to a char* and opt->defval is a
@@ -327,6 +329,73 @@ static int config_get_rebase(void)
 	return value;
 }
 
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(const char *prefix)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, prefix);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(const char *prefix)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, prefix);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+static void die_on_unclean_work_tree(const char *prefix)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+	int do_die = 0;
+
+	hold_locked_index(lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	update_index_if_able(&the_index, lock_file);
+	rollback_lock_file(lock_file);
+
+	if (has_unstaged_changes(prefix)) {
+		error(_("Cannot pull with rebase: You have unstaged changes."));
+		do_die = 1;
+	}
+
+	if (has_uncommitted_changes(prefix)) {
+		if (do_die)
+			error(_("Additionally, your index contains uncommitted changes."));
+		else
+			error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
+		do_die = 1;
+	}
+
+	if (do_die)
+		exit(1);
+}
+
 struct known_remote {
 	struct known_remote *next;
 	struct remote *remote;
@@ -782,9 +851,15 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_sha1("HEAD", orig_head))
 		hashclr(orig_head);
 
-	if (opt_rebase)
+	if (opt_rebase) {
+		if (is_null_sha1(orig_head) && !is_cache_unborn())
+			die(_("Updating an unborn branch with changes added to the index."));
+
+		die_on_unclean_work_tree(prefix);
+
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
+	}
 
 	if (run_fetch(repo, refspecs))
 		return 1;
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 17254ee..62dbfb5 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -390,7 +390,7 @@ test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 '
 
-test_expect_failure 'pull --rebase dies early with dirty working directory' '
+test_expect_success 'pull --rebase dies early with dirty working directory' '
 
 	git checkout to-rebase &&
 	git update-ref refs/remotes/me/copy copy^ &&
@@ -420,7 +420,7 @@ test_expect_success 'pull --rebase works on branch yet to be born' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'pull --rebase fails on unborn branch with staged changes' '
+test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
 	test_when_finished "rm -rf empty_repo2" &&
 	git init empty_repo2 &&
 	(
@@ -492,7 +492,7 @@ test_expect_success 'git pull --rebase does not reapply old patches' '
 	)
 '
 
-test_expect_failure 'git pull --rebase against local branch' '
+test_expect_success 'git pull --rebase against local branch' '
 	git checkout -b copy2 to-rebase-orig &&
 	git pull --rebase . to-rebase &&
 	test "conflicting modification" = "$(cat file)" &&
-- 
2.1.4

[PATCH 14/14] pull --rebase: error on no merge candidate cases

[Paul Tan]

Tweak the error messages printed by die_no_merge_candidates() to take
into account that we may be "rebasing against" rather than "merging
with".

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index c8d673d..15b65a0 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -474,10 +474,12 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 	const char *remote = curr_branch ? curr_branch->remote_name : NULL;
 
 	if (*refspecs) {
-		fprintf(stderr,
-			_("There are no candidates for merging among the refs that you just fetched.\n"
-			"Generally this means that you provided a wildcard refspec which had no\n"
-			"matches on the remote end.\n"));
+		if (opt_rebase)
+			fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
+		else
+			fputs(_("There are no candidates for merging among the refs that you just fetched."), stderr);
+		fputs(_("Generally this means that you provided a wildcard refspec which had no\n"
+			"matches on the remote end."), stderr);
 	} else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
 		fprintf(stderr,
 			_("You asked to pull from the remote '%s', but did not specify\n"
@@ -485,12 +487,15 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			"for your current branch, you must specify a branch on the command line.\n"),
 			repo);
 	} else if (!curr_branch) {
-		fprintf(stderr,
-			_("You are not currently on a branch. Please specify which\n"
-			"branch you want to merge with. See git-pull(1) for details.\n"
-			"\n"
-			"    git pull <remote> <branch>\n"
-			"\n"));
+		fputs(_("You are not currently on a branch."), stderr);
+		if (opt_rebase)
+			fputs(_("Please specify which branch you want to rebase against."), stderr);
+		else
+			fputs(_("Please specify which branch you want to merge with."), stderr);
+		fputs(_("See git-pull(1) for details."), stderr);
+		fputs("", stderr);
+		fputs("    git pull <remote> <branch>", stderr);
+		fputs("", stderr);
 	} else if (!curr_branch->merge_nr) {
 		struct known_remote *remotes = NULL;
 		const char *remote_name = "<remote>";
@@ -499,14 +504,17 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 		if (remotes && !remotes->next)
 			remote_name = remotes->remote->name;
 
+		fputs(_("There is no tracking information for the current branch."), stderr);
+		if (opt_rebase)
+			fputs(_("Please specify which branch you want to rebase against."), stderr);
+		else
+			fputs(_("Please specify which branch you want to merge with."), stderr);
+		fputs(_("See git-pull(1) for details."), stderr);
+		fputs("", stderr);
+		fputs("    git pull <remote> <branch>", stderr);
+		fputs("", stderr);
 		fprintf(stderr,
-			_("There is no tracking information for the current branch.\n"
-			"Please specify which branch you want to merge with.\n"
-			"See git-pull(1) for details.\n"
-			"\n"
-			"    git pull <remote> <branch>\n"
-			"\n"
-			"If you wish to set tracking information for this branch you can do so with:\n"
+			_("If you wish to set tracking information for this branch you can do so with:\n"
 			"\n"
 			"    git branch --set-upstream-to=%s/<branch> %s\n"
 			"\n"),
-- 
2.1.4

Re: [PATCH 01/14] pull: implement fetch + merge

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:05, Paul Tan wrote:
> Start off the rewrite of git-pull.sh to C by implementing a simple
> git-pull builtin that:
> 
> 1. Parses the command line arguments into [<repo> [<refspecs>...]]
> 
> 2. Runs git-fetch, passing the repo and refspecs to it.
> 
> 3. Runs git-merge FETCH_HEAD.
> 
> Due to missing features, several tests are disabled:
> 
> * Passing flags to git fetch, rebase, merge:
> 
>   * t5521: verbosity flags -v, -s
> 
> * Passing flags to git-merge, git-rebase:
> 
>   * t5150, t5572: Pass --ff, --no-ff, --ff-only to git-merge
> 
>   * t5520, t5521, t7406: Support --rebase, branch.*.rebase and
>     pull.rebase config
> 
>   * t6029, t4013: -s/--strategy
> 
>   * t6037: -X/--strategy-option
> 
>   * t5524: --log
> 
> * Passing flags to git-fetch:
> 
>   * t5521: --dry-run
> 
>   * t5500: --depth
> 
>   * t5521: --force
> 
>   * t5521: --all
> 
>   * t7403: --no-recurse-submodules
> 
> * t7601: pull.ff config sets --ff, --no-ff, --ff-only
> 
> * t5520: branch.*.rebase or pull.rebase sets --rebase, --rebase=preserve
> 
> * t5520: Pulling into void
> 
> * t5520: appropriate user-friendly error messages on no merge candidates
>   case
> 
> * t5520: appropriate user-friendly error messages if index has
>   unresolved entries.
> 
> * t5520: fast-forward working tree if branch head is updated by
>   git-fetch
> 
> * t5520: set reflog action to "pull args..."
> 
> These features will be re-implemented in succeeding patches.
> 
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  Makefile                     |   2 +-
>  builtin.h                    |   1 +
>  builtin/pull.c               |  83 +++++++++++
>  contrib/examples/git-pull.sh | 339 +++++++++++++++++++++++++++++++++++++++++++
>  git-pull.sh                  | 339 -------------------------------------------
>  git.c                        |   1 +
>  t/t4013-diff-various.sh      |   3 +
>  t/t5150-request-pull.sh      |   2 +-
>  t/t5500-fetch-pack.sh        |  10 +-
>  t/t5520-pull.sh              |  68 ++++-----
>  t/t5521-pull-options.sh      |  20 +--
>  t/t5524-pull-msg.sh          |   2 +-
>  t/t5572-pull-submodule.sh    |   4 +
>  t/t6029-merge-subtree.sh     |   6 +-
>  t/t6037-merge-ours-theirs.sh |   2 +-
>  t/t7403-submodule-sync.sh    |   8 +-
>  t/t7406-submodule-update.sh  |   2 +
>  t/t7601-merge-pull-config.sh |   4 +-

This is an excellent way to develop the patch series. For the final patch series, I would like to propose that `git-pull.sh` is replaced by the builtin with the very last patch, though. That way, the builtin can be implemented in the way you did, but without touching the tests. In the final form, the builtin git-pull will be a drop-in replacement for the shell script, and the final patch will just make that switch.

In other words, patch 1/N would leave SCRIPT_SH in the Makefile untouched, and leave out the "pull" line from `git.c`.

Oh, by the way, if you pass the `-M` option to `git format-patch`, the diffstat will report the move rather than an added and a deleted git-pull.sh.

Ciao,
Dscho

Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:05, Paul Tan wrote:

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 0b771b9..a4d9c92 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -11,16 +11,64 @@
>  #include "argv-array.h"
>  #include "run-command.h"
>  
> +/**
> + * Given an option opt, where opt->value points to a char* and opt->defval is a
> + * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is
> + * NULL, then opt->value is set to "--$defval". If unset is true, then
> + * opt->value is set to "--no-$defval".
> + */
> +static int parse_opt_passthru(const struct option *opt, const char
> *arg, int unset)

How about adding this to parse-options-cb.c in a separate patch? I guess the description could say something like:

/**
 * Given an option opt, recreate the command-line option, as strbuf. This is useful
 * when a command needs to parse a command-line option in order to pass it to yet
 * another command. This callback can be used in conjunction with the
 * PARSE_OPT_(OPTARG|NOARG|NONEG) options, but not with PARSE_OPT_LASTARG_DEFAULT
 * because it expects `defval` to be the name of the command-line option to
 * reconstruct.
 *
 * The result will be stored in opt->value, which is expected to be a pointer to an
 * strbuf.
 */

Implied by my suggested description, I also propose to put the re-recreated command-line option into a strbuf instead of a char *, to make memory management easier (read: avoid memory leaks).

You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:

int parse_opt_passthru(const struct option *opt, const char *arg, int unset)
{
	struct strbuf *buf = opt->value;

	assert(opt->defval);
	strbuf_reset(buf);
	if (unset) {
		assert(!arg);
		strbuf_addf(buf, "--no-%s", opt->defval);
	}
	else {
		strbuf_addf(buf, "--%s", opt->defval);
		if (arg)
			strbuf_addf(buf, "=%s", arg);
	}

	return 0;
}

>  static struct option pull_options[] = {
> +	/* Shared options */
> +	OPT__VERBOSITY(&opt_verbosity),
> +	{ OPTION_CALLBACK, 0, "progress", &opt_progress, NULL,
> +	  N_("force progress reporting"),
> +	  PARSE_OPT_NOARG, parse_opt_passthru},

I *think* there is a '(intptr_t) "progress"' missing...

>  /**
> + * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
> + */
> +static void argv_push_verbosity(struct argv_array *arr)
> +{
> +	int verbosity;
> +
> +	for (verbosity = opt_verbosity; verbosity > 0; verbosity--)
> +		argv_array_push(arr, "-v");
> +
> +	for (verbosity = opt_verbosity; verbosity < 0; verbosity++)
> +		argv_array_push(arr, "-q");
> +}

Hmm... I guess this is *really* nit-picky, but I'd rather use "i" instead of "verbosity" because the second loop is about quietness instead of verbosity ;-)

Ciao,
Dscho

Re: [PATCH 05/14] pull: error on no merge candidates

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 07ad783..8982fdf 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -207,6 +209,130 @@ static void argv_push_force(struct argv_array *arr)
>  		argv_array_push(arr, "-f");
>  }
>  
> +struct known_remote {
> +	struct known_remote *next;
> +	struct remote *remote;
> +};
> +
> +/**
> + * Use this callback with for_each_remote() to get the configured remotes as
> + * a singly linked known_remote list. cb_data must be a pointer to a
> + * struct known_remote*, which must be initialized to NULL. For example,
> + * For example:
> + *
> + *     struct known_remote *list = NULL;
> + *     for_each_remote(add_known_remote, &list);
> + */
> +static int add_known_remote(struct remote *remote, void *cb_data)
> +{
> +	struct known_remote **list = cb_data;
> +	struct known_remote *item;
> +
> +	item = xmalloc(sizeof(*item));
> +	item->remote = remote;
> +	item->next = *list;
> +	*list = item;
> +	return 0;
> +}

My first reaction to this was: let's use an array and `ALLOC_GROW()` to make this look nicer. But then I saw that there is only one user:

> +static void NORETURN die_no_merge_candidates(const char *repo, const
> char **refspecs)
> +{
> [...]
> +	} else if (!curr_branch->merge_nr) {
> +		struct known_remote *remotes = NULL;
> +		const char *remote_name = "<remote>";
> +
> +		for_each_remote(add_known_remote, &remotes);
> +		if (remotes && !remotes->next)
> +			remote_name = remotes->remote->name;
> +
> [...]

How about this instead:

static int get_only_remote(struct remote *remote, void *cb_data)
{
	const char **p = cb_data;

	if (*p)
		return -1;

	*p = remote->name;
	return 0;
}

[...]
		const char *remote_name = NULL;

		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
			remote_name = "<remote>";

Ciao,
Dscho

Re: [PATCH 06/14] pull: support pull.ff config

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 8982fdf..b305a47 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -209,6 +209,28 @@ static void argv_push_force(struct argv_array *arr)
>  		argv_array_push(arr, "-f");
>  }
>  
> +/**
> + * If pull.ff is "true", returns "--ff". If pull.ff is "false", returns
> + * "--no-ff". If pull.ff is "only", returns "--ff-only". Otherwise, returns
> + * NULL.
> + */
> +static const char *config_get_ff(void)
> +{
> +	const char *value;
> +
> +	if (git_config_get_value("pull.ff", &value))
> +		return NULL;
> +	switch (git_config_maybe_bool("pull.ff", value)) {
> +		case 0:
> +			return "--no-ff";
> +		case 1:
> +			return "--ff";
> +	}
> +	if (!strcmp("pull.ff", "only"))

I think you want to test `!strcmp("only", value)` ;-)

Ciao,
Dscho

Re: [PATCH 07/14] pull: check if in unresolved merge state

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:
> Since, commit d38a30d (Be more user-friendly when refusing to do

Let's remove this comma ;-) The rest of the patch looks fine to my eyes.

Ciao,
Dscho

Re: [PATCH 08/14] pull: fast-forward working tree if head is updated

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:
> Since b10ac50 (Fix pulling into the same branch., 2005-08-25), git-pull,
> upon detecting that git-fetch updated the current head, will
> fast-forward the working tree to the updated head commit.

This is just a quick note about something I noticed while reviewing: I was surprised that this code path would only trigger when `orig_head` is different from the null SHA-1, because in my mind, it is safe to update when we are on an unborn branch. However, I verified that the shell script does the same thing, so your transcription is faithful.

Ciao,
Dscho

Re: [PATCH 00/14] Make git-pull a builtin

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> This series rewrites git-pull.sh into a C builtin, thus improving its
> performance and portability. It is part of my GSoC project to rewrite git-pull
> and git-am into builtins[2].

Earlier you were worried about 'git pull' being used in many tests
for the purpose of testing other parts of the system and not testing
'pull' itself.  For a program as complex as 'git pull', you may want
to take the 'competing implementations' approach.

(1) write an empty cmd_pull() that relaunches "git pull" scripted
    porcelain from the $GIT_EXEC_PATH with given parameters, and
    wire all the necessary bits to git.c.

(2) enhance cmd_pull() a bit by bit, but keep something like this

	if (getenv(GIT_USE_BUILTIN_PULL)) {
        	/* original run_command("git-pull.sh") code */
                exit $?
	}

        ... your "C" version ...

(3) add "GIT_USE_BUILTIN_PULL=Yes; export GIT_USE_BUILTIN_PULL" at
    the beginning of "t55??" test scripts (but not others that rely
    on working pull and that are not interested in catching bugs in
    pull).

(4) once cmd_pull() becomes fully operational, drop (3) and also the
    conditional one you added in (2), and retire the environment
    variable.  Retire the git-pull.sh script to contrib/examples/
    boneyard.

That way, you will always have a reference you can use throughout
the development.

Just a suggestion, not a requirement.

Re: [PATCH 10/14] pull: set reflog message

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:06, Paul Tan wrote:

> diff --git a/builtin/pull.c b/builtin/pull.c
> index ba2ff01..81e31a1 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -212,6 +212,26 @@ static void argv_push_force(struct argv_array *arr)
>  }
>  
>  /**
> + * Sets the GIT_REFLOG_ACTION environment variable to the concatenation of argv
> + */
> +static void set_reflog_message(int argc, const char **argv)
> +{
> +	int i;
> +	struct strbuf msg = STRBUF_INIT;
> +
> +	for (i = 0; i < argc; i++) {
> +		strbuf_addstr(&msg, argv[i]);
> +		strbuf_addch(&msg, ' ');
> +	}
> +
> +	strbuf_rtrim(&msg);

Since this is not performance critical code, I would be slightly in favor of simplifying thusly:

/* Join the argument list, separated by spaces */
for (i = 0; i < argc; i++)
	strbuf_addf(&msg, "%s%s", i ? " " : "", argv[i]);

Just a suggestion,
Dscho

Re: [PATCH 00/14] Make git-pull a builtin

[Johannes Schindelin]

Hi Paul,

I already commented on patches 1-10, and I will look in depth at 11-14 tomorrow.

It is a very pleasant read so far. Thank you.

Ciao,
Dscho

P.S.: I have the feeling about patch 12 that reading `opt_rebase` from the config could be delayed until after we know that the user did not specify `--rebase`, but I'll have to think about that more deeply.

Re: [PATCH 10/14] pull: set reflog message

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

>>  /**
>> + * Sets the GIT_REFLOG_ACTION environment variable to the concatenation of argv
>> + */
>> +static void set_reflog_message(int argc, const char **argv)
>> +{
>> +	int i;
>> +	struct strbuf msg = STRBUF_INIT;
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		strbuf_addstr(&msg, argv[i]);
>> +		strbuf_addch(&msg, ' ');
>> +	}
>> +
>> +	strbuf_rtrim(&msg);
>
> Since this is not performance critical code, I would be slightly in
> favor of simplifying thusly:
>
> /* Join the argument list, separated by spaces */
> for (i = 0; i < argc; i++)
> 	strbuf_addf(&msg, "%s%s", i ? " " : "", argv[i]);

Yeah, either that, or "insert separator only before adding to
something else" pattern, i.e.

	for (i = 0; i < argc; i++) {
        	if (i)
                	addch(&msg, ' ');
		addstr(&msg, argv[i]);
	}

Both are easier to read than "always append SP and trim at the end".
Besides, trimming at the end makes readers wonder if there are cases
where argv[argc-1] ends with whitespaces and the code on purpose
removes them.




                        	

Re: [PATCH 11/14] pull: teach git pull about --rebase

[Stefan Beller]

On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@gmail.com> wrote:

> +enum rebase_type {
> +       REBASE_FALSE,
> +       REBASE_TRUE,
> +       REBASE_PRESERVE
> +};
> +
> +/**
> + * Parses the value of --rebase, branch.*.rebase or pull.rebase. If value is a
> + * false value, returns REBASE_FALSE. If value is a true value, returns
> + * REBASE_TRUE. If value is "preserve", returns REBASE_PRESERVE. Otherwise,
> + * returns -1 to signify an invalid value.
> + */
> +static int parse_config_rebase(const char *value)
> +{
> +       int v = git_config_maybe_bool("pull.rebase", value);
> +       if (v >= 0)
> +               return v;
> +       if (!strcmp(value, "preserve"))
> +               return REBASE_PRESERVE;
> +       return -1;

There is no REBASE_FALSE nor REBASE_TRUE in this function,
how do you make sure it returns the correct value then?

Currently this is probably in sync with what git_config_maybe_bool
returns, but think of a future refactoring/change. Then it will be hard
to have the connection between what the code in git_config_maybe_bool
does, what the code here does and the comment here.

You could get rid of the enum all together, as you're using integer as a
return type anyway. (so you lose type safety, a future change may introduce
a "return 42" which is not part of the enum)

Or you could convert the code to use the enum more strictly and having it
as the return type:

enum rebase_type parse_config_rebase(const char *value) {
    int v = git_config_maybe_bool("pull.rebase", value);
    switch (v) {
        case 0:
            return REBASE_FALSE;
        case 1:
            return REBASE_TRUE;
        default:
             if (!strcmp(value, "preserve"))
                return  REBASE_PRESERVE;
             else {
                die("value pull.rebase must be one of {false, true, preserve}");
                // or if you're more tolerant:
                return REBASE_VALUE_UNINTERPRETABLE;
       }
   }


> +}
> +
> +/**
> + * Callback for --rebase, which parses arg with parse_config_rebase().
> + */
> +static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
> +{
> +       int *value = (int*) opt->value;
> +
> +       if (arg)
> +               *value = parse_config_rebase(arg);
> +       else
> +               *value = unset ? REBASE_FALSE : REBASE_TRUE;
> +       return *value >= 0 ? 0 : -1;

This is repeating the check from above. If you'd go the way of using
stricter enums,
you'd just check for the REBASE_VALUE_UNINTERPRETABLE
value here.

> +}
> +
>  static const char * const pull_usage[] = {
>         N_("git pull [options] [<repo> [<refspec>...]]"),
>         NULL
> @@ -48,7 +84,8 @@ static const char * const pull_usage[] = {
>  static int opt_verbosity;
>  static char *opt_progress;
>
> -/* Options passed to git-merge */
> +/* Options passed to git-merge or git-rebase */
> +static int opt_rebase;
>  static char *opt_diffstat;
>  static char *opt_log;
>  static char *opt_squash;
> @@ -82,8 +119,12 @@ static struct option pull_options[] = {
>           N_("force progress reporting"),
>           PARSE_OPT_NOARG, parse_opt_passthru},
>
> -       /* Options passed to git-merge */
> +       /* Options passed to git-merge or git-rebase */
>         OPT_GROUP(N_("Options related to merging")),
> +       { OPTION_CALLBACK, 'r', "rebase", &opt_rebase,
> +         N_("false|true|preserve"),
> +         N_("incorporate changes by rebasing rather than merging"),
> +         PARSE_OPT_OPTARG, parse_opt_rebase, 0},
>         { OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL,
>           N_("do not show a diffstat at the end of the merge"),
>           PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "no-stat" },
> @@ -506,11 +547,185 @@ static int run_merge(void)
>         return ret;
>  }
>
> +/**
> + * Returns the merge branch for the current branch. Returns NULL if repo is not
> + * a valid remote, HEAD does not point to a branch, repo is not the branch's
> + * configured remote or the branch does not have any configured merge branch.
> + */
> +static char *get_merge_branch_1(const char *repo)
> +{
> +       struct remote *rm;
> +       struct branch *curr_branch;
> +
> +       if (repo && !(rm = remote_get(repo)))
> +               return NULL;
> +       if (!(curr_branch = branch_get("HEAD")))
> +               return NULL;
> +       if (repo && curr_branch->remote != rm)
> +               return NULL;
> +       if (!curr_branch->merge_nr)
> +               return NULL;
> +       return xstrdup(curr_branch->merge[0]->dst);
> +}

This feels a bit tangled because of the repetition of checking
for repo and curr_branch. Also we do not do assignments inside
of conditions (if, while, for), so maybe start with a

    if (!repo)
        return NULL;

and then do the assignments and test on the assigned value.

> +
> +/**
> + * Given a refspec, returns the merge branch. Returns NULL if the refspec src
> + * does not refer to a branch.
> + *
> + * FIXME: It should return the tracking branch. Currently only works with the
> + * default mapping.
> + */
> +static char *get_merge_branch_2(const char *repo, const char *refspec)
> +{
> +       struct refspec *spec;
> +       const char *remote;
> +       char *merge_branch;
> +
> +       spec = parse_fetch_refspec(1, &refspec);
> +       remote = spec->src;
> +       if (!*remote || !strcmp(remote, "HEAD"))
> +               remote = "HEAD";
> +       else if (skip_prefix(remote, "heads/", &remote))
> +               ;
> +       else if (skip_prefix(remote, "refs/heads/", &remote))
> +               ;
> +       else if (starts_with(remote, "refs/") ||
> +               starts_with(remote, "tags/") ||
> +               starts_with(remote, "remotes/"))
> +               remote = "";
> +
> +       if (*remote) {
> +               if (!strcmp(repo, "."))
> +                       merge_branch = mkpathdup("refs/heads/%s", remote);
> +               else
> +                       merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
> +       } else
> +               merge_branch = NULL;
> +
> +       free_refspec(1, spec);
> +       return merge_branch;
> +}
> +
> +/**
> + * Sets fork_point to the point at which the current branch forked from its
> + * remote merge branch. Returns 0 on success, -1 on failure.
> + */
> +static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
> +               const char *repo, const char *refspec)
> +{
> +       int ret;
> +       struct branch *curr_branch;
> +       char *remote_merge_branch;
> +       struct argv_array args = ARGV_ARRAY_INIT;
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       if (!(curr_branch = branch_get("HEAD")))
> +               return -1;
> +
> +       if (refspec)
> +               remote_merge_branch = get_merge_branch_2(repo, refspec);
> +       else
> +               remote_merge_branch = get_merge_branch_1(repo);
> +
> +       if (!remote_merge_branch)
> +               return -1;
> +
> +       argv_array_pushl(&args, "merge-base", "--fork-point",
> +                       remote_merge_branch, curr_branch->name, NULL);
> +       cp.argv = args.argv;
> +       cp.no_stdin = 1;
> +       cp.no_stderr = 1;
> +       cp.git_cmd = 1;
> +
> +       if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
> +               goto cleanup;
> +
> +       if ((ret = get_sha1_hex(sb.buf, fork_point)))
> +               goto cleanup;
> +
> +cleanup:
> +       free(remote_merge_branch);
> +       strbuf_release(&sb);
> +       return ret ? -1 : 0;
> +}
> +
> +/**
> + * Sets merge_base to the octopus merge base of curr_head, merge_head and
> + * fork_point. Returns 0 if a merge base is found, 1 otherwise.
> + */
> +static int get_octopus_merge_base(unsigned char merge_base[GIT_SHA1_HEXSZ],
> +               unsigned char curr_head[GIT_SHA1_RAWSZ],
> +               unsigned char merge_head[GIT_SHA1_RAWSZ],
> +               unsigned char fork_point[GIT_SHA1_RAWSZ])
> +{
> +       struct commit_list *revs = NULL, *result;
> +
> +       commit_list_insert(lookup_commit_reference(curr_head), &revs);
> +       commit_list_insert(lookup_commit_reference(merge_head), &revs);
> +       if (!is_null_sha1(fork_point))
> +               commit_list_insert(lookup_commit_reference(fork_point), &revs);
> +
> +       result = reduce_heads(get_octopus_merge_bases(revs));
> +       free_commit_list(revs);
> +       if (!result)
> +               return 1;
> +
> +       hashcpy(merge_base, result->item->object.sha1);
> +       return 0;
> +}
> +
> +/**
> + * Given the current HEAD SHA1, the merge head returned from git-fetch and the
> + * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
> + * appropriate arguments and returns its exit status.
> + */
> +static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
> +               unsigned char merge_head[GIT_SHA1_RAWSZ],
> +               unsigned char fork_point[GIT_SHA1_RAWSZ])
> +{
> +       int ret;
> +       unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
> +       struct argv_array args = ARGV_ARRAY_INIT;
> +
> +       if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head, fork_point))
> +               if (!is_null_sha1(fork_point) && !hashcmp(oct_merge_base, fork_point))
> +                       hashclr(fork_point);
> +
> +       argv_array_push(&args, "rebase");
> +
> +       /* Shared options */
> +       argv_push_verbosity(&args);
> +
> +       /* Options passed to git-rebase */
> +       if (opt_rebase == REBASE_PRESERVE)
> +               argv_array_push(&args, "--preserve-merges");
> +       if (opt_diffstat)
> +               argv_array_push(&args, opt_diffstat);
> +       argv_push_strategies(&args);
> +       argv_push_strategy_opts(&args);
> +       if (opt_gpg_sign)
> +               argv_array_push(&args, opt_gpg_sign);
> +
> +       argv_array_push(&args, "--onto");
> +       argv_array_push(&args, sha1_to_hex(merge_head));
> +
> +       if (!is_null_sha1(fork_point))
> +               argv_array_push(&args, sha1_to_hex(fork_point));
> +       else
> +               argv_array_push(&args, sha1_to_hex(merge_head));
> +
> +       ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
> +       argv_array_clear(&args);
> +       return ret;
> +}
> +
>  int cmd_pull(int argc, const char **argv, const char *prefix)
>  {
>         const char *repo, **refspecs;
>         struct sha1_array merge_heads = SHA1_ARRAY_INIT;
>         unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
> +       unsigned char rebase_fork_point[GIT_SHA1_RAWSZ];
>
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
> @@ -532,6 +747,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (get_sha1("HEAD", orig_head))
>                 hashclr(orig_head);
>
> +       if (opt_rebase)
> +               if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
> +                       hashclr(rebase_fork_point);
> +
>         if (run_fetch(repo, refspecs))
>                 return 1;
>
> @@ -573,6 +792,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 if (merge_heads.nr > 1)
>                         die(_("Cannot merge multiple branches into empty head."));
>                 return pull_into_void(*merge_heads.sha1, curr_head);
> +       } else if (opt_rebase) {
> +               if (merge_heads.nr > 1)
> +                       die(_("Cannot rebase onto multiple branches."));
> +               return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
>         } else
>                 return run_merge();
>  }
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9414cc1..3798b96 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -212,7 +212,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
>         test "$(git rev-parse third)" = "$(git rev-parse second)"
>  '
>
> -test_expect_failure '--rebase' '
> +test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
>         git commit -m file file &&
> @@ -226,7 +226,7 @@ test_expect_failure '--rebase' '
>         test new = "$(git show HEAD:file2)"
>  '
>
> -test_expect_failure '--rebase fails with multiple branches' '
> +test_expect_success '--rebase fails with multiple branches' '
>         git reset --hard before-rebase &&
>         test_must_fail git pull --rebase . copy master 2>err &&
>         test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
> @@ -310,7 +310,7 @@ test_expect_failure 'pull.rebase=invalid fails' '
>         ! git pull . copy
>  '
>
> -test_expect_failure '--rebase=false create a new merge commit' '
> +test_expect_success '--rebase=false create a new merge commit' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase true &&
>         git pull --rebase=false . copy &&
> @@ -319,7 +319,7 @@ test_expect_failure '--rebase=false create a new merge commit' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase=true rebases and flattens keep-merge' '
> +test_expect_success '--rebase=true rebases and flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase preserve &&
>         git pull --rebase=true . copy &&
> @@ -327,7 +327,7 @@ test_expect_failure '--rebase=true rebases and flattens keep-merge' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase=preserve rebases and merges keep-merge' '
> +test_expect_success '--rebase=preserve rebases and merges keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase true &&
>         git pull --rebase=preserve . copy &&
> @@ -340,7 +340,7 @@ test_expect_success '--rebase=invalid fails' '
>         ! git pull --rebase=invalid . copy
>  '
>
> -test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
> +test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase preserve &&
>         git pull --rebase . copy &&
> @@ -348,7 +348,7 @@ test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-m
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase with rebased upstream' '
> +test_expect_success '--rebase with rebased upstream' '
>
>         git remote add -f me . &&
>         git checkout copy &&
> @@ -366,7 +366,7 @@ test_expect_failure '--rebase with rebased upstream' '
>
>  '
>
> -test_expect_failure '--rebase with rebased default upstream' '
> +test_expect_success '--rebase with rebased default upstream' '
>
>         git update-ref refs/remotes/me/copy copy-orig &&
>         git checkout --track -b to-rebase2 me/copy &&
> @@ -377,7 +377,7 @@ test_expect_failure '--rebase with rebased default upstream' '
>
>  '
>
> -test_expect_failure 'rebased upstream + fetch + pull --rebase' '
> +test_expect_success 'rebased upstream + fetch + pull --rebase' '
>
>         git update-ref refs/remotes/me/copy copy-orig &&
>         git reset --hard to-rebase-orig &&
> @@ -409,7 +409,7 @@ test_expect_failure 'pull --rebase dies early with dirty working directory' '
>
>  '
>
> -test_expect_failure 'pull --rebase works on branch yet to be born' '
> +test_expect_success 'pull --rebase works on branch yet to be born' '
>         git rev-parse master >expect &&
>         mkdir empty_repo &&
>         (cd empty_repo &&
> @@ -456,14 +456,14 @@ test_expect_success 'setup for detecting upstreamed changes' '
>         )
>  '
>
> -test_expect_failure 'git pull --rebase detects upstreamed changes' '
> +test_expect_success 'git pull --rebase detects upstreamed changes' '
>         (cd dst &&
>          git pull --rebase &&
>          test -z "$(git ls-files -u)"
>         )
>  '
>
> -test_expect_failure 'setup for avoiding reapplying old patches' '
> +test_expect_success 'setup for avoiding reapplying old patches' '
>         (cd dst &&
>          test_might_fail git rebase --abort &&
>          git reset --hard origin/master
> @@ -485,7 +485,7 @@ test_expect_failure 'setup for avoiding reapplying old patches' '
>         )
>  '
>
> -test_expect_failure 'git pull --rebase does not reapply old patches' '
> +test_expect_success 'git pull --rebase does not reapply old patches' '
>         (cd dst &&
>          test_must_fail git pull --rebase &&
>          test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 4176e11..56e7377 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -19,7 +19,7 @@ test_expect_success 'git pull -q' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull -q --rebase' '
> +test_expect_success 'git pull -q --rebase' '
>         mkdir clonedqrb &&
>         (cd clonedqrb && git init &&
>         git pull -q --rebase "../parent" >out 2>err &&
> @@ -38,7 +38,7 @@ test_expect_success 'git pull' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull --rebase' '
> +test_expect_success 'git pull --rebase' '
>         mkdir clonedrb &&
>         (cd clonedrb && git init &&
>         git pull --rebase "../parent" >out 2>err &&
> @@ -54,7 +54,7 @@ test_expect_success 'git pull -v' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull -v --rebase' '
> +test_expect_success 'git pull -v --rebase' '
>         mkdir clonedvrb &&
>         (cd clonedvrb && git init &&
>         git pull -v --rebase "../parent" >out 2>err &&
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 1ffd837..dda3929 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -11,8 +11,6 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
>
>  . ./test-lib.sh
>
> -skip_all='skipping submodule update tests, requires git pull --rebase'
> -test_done
>
>  compare_head()
>  {
> --
> 2.1.4
>

Re: [PATCH 12/14] pull: configure --rebase via branch.<name>.rebase or pull.rebase

[Stefan Beller]

On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Since cd67e4d (Teach 'git pull' about --rebase, 2007-11-28),
> fetch+rebase could be set by default by defining the config variable
> branch.<name>.rebase. This setting can be overriden on the command line
> by --rebase and --no-rebase.
>
> Since 6b37dff (pull: introduce a pull.rebase option to enable --rebase,
> 2011-11-06), git-pull --rebase can also be configured via the
> pull.rebase configuration option.
>
> Re-implement support for these two configuration settings by introducing
> config_get_rebase() which is called before parse_options() to set the
> default value of opt_rebase.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  builtin/pull.c  | 35 +++++++++++++++++++++++++++++++++++
>  t/t5520-pull.sh | 12 ++++++------
>  2 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f18a21c..a0958a7 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -294,6 +294,39 @@ static const char *config_get_ff(void)
>         die(_("Invalid value for pull.ff: %s"), value);
>  }
>
> +/**
> + * Returns the default configured value for --rebase. It first looks for the
> + * value of "branch.$curr_branch.rebase", where $curr_branch is the current
> + * branch, and if HEAD is detached or the configuration key does not exist,
> + * looks for the value of "pull.rebase". If both configuration keys do not
> + * exist, returns REBASE_FALSE.
> + */
> +static int config_get_rebase(void)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       struct branch *curr_branch = branch_get("HEAD");
> +       const char *key, *str;
> +       int ret = -1, value;
> +
> +       if (curr_branch) {
> +               strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
> +               key = sb.buf;
> +               ret = git_config_get_value(sb.buf, &str);
> +       }
> +       if (ret) {
> +               key = "pull.rebase";
> +               ret = git_config_get_value(key, &str);
> +       }
> +       if (ret) {
> +               strbuf_release(&sb);
> +               return REBASE_FALSE;
> +       }
> +       if ((value = parse_config_rebase(str)) < 0)

Assignment inside an if (....).
I just looked up the Documentation/CodingGuidelines
which state it as:
 - We try to avoid assignments inside "if ()" conditions.
so my hint from my previous mail (to also apply it to for/while)
is wrong.

Initially the flow confused me a little, so I tried to understand why,
and I guess it's because of the return value used both as a return value
and as conditional.

Maybe you could move the check of the value out of this function
and here you only focus on getting the order right?
Something like (completely untested code, beware):

enum rebase_type config_get_rebase(void)
{
    struct strbuf sb = STRBUF_INIT;
    enum rebase_type value;

    struct branch *curr_branch = branch_get("HEAD");

    if (curr_branch) {
        strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
        if (parse_config_rebase_or_die_on_unparsable(sb.buf, &value))
            return value;
    }

    if (parse_config_rebase_or_die_on_unparsable("pull.rebase", &value))
        return value;

    return REBASE_FALSE;
}

This implementation leaks the strbuf though.


> +               die(_("Invalid value for %s: %s"), key, str);
> +       strbuf_release(&sb);
> +       return value;
> +}
> +
>  struct known_remote {
>         struct known_remote *next;
>         struct remote *remote;
> @@ -730,6 +763,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
>
> +       opt_rebase = config_get_rebase();
> +
>         argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>         parse_repo_refspecs(argc, argv, &repo, &refspecs);
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 3798b96..17254ee 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -234,7 +234,7 @@ test_expect_success '--rebase fails with multiple branches' '
>         test modified = "$(git show HEAD:file)"
>  '
>
> -test_expect_failure 'pull.rebase' '
> +test_expect_success 'pull.rebase' '
>         git reset --hard before-rebase &&
>         test_config pull.rebase true &&
>         git pull . copy &&
> @@ -242,7 +242,7 @@ test_expect_failure 'pull.rebase' '
>         test new = "$(git show HEAD:file2)"
>  '
>
> -test_expect_failure 'branch.to-rebase.rebase' '
> +test_expect_success 'branch.to-rebase.rebase' '
>         git reset --hard before-rebase &&
>         test_config branch.to-rebase.rebase true &&
>         git pull . copy &&
> @@ -280,7 +280,7 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure 'pull.rebase=true flattens keep-merge' '
> +test_expect_success 'pull.rebase=true flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase true &&
>         git pull . copy &&
> @@ -288,7 +288,7 @@ test_expect_failure 'pull.rebase=true flattens keep-merge' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
> +test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase 1 &&
>         git pull . copy &&
> @@ -296,7 +296,7 @@ test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
> +test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase preserve &&
>         git pull . copy &&
> @@ -304,7 +304,7 @@ test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
>         test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
>  '
>
> -test_expect_failure 'pull.rebase=invalid fails' '
> +test_expect_success 'pull.rebase=invalid fails' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase invalid &&
>         ! git pull . copy
> --
> 2.1.4
>

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Stefan Beller]

On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Tweak the error messages printed by die_no_merge_candidates() to take
> into account that we may be "rebasing against" rather than "merging
> with".
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  builtin/pull.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index c8d673d..15b65a0 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -474,10 +474,12 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>         const char *remote = curr_branch ? curr_branch->remote_name : NULL;
>
>         if (*refspecs) {
> -               fprintf(stderr,
> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
> -                       "Generally this means that you provided a wildcard refspec which had no\n"
> -                       "matches on the remote end.\n"));
> +               if (opt_rebase)
> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);

Is there a reason you switch to fputs, instead of fprintf?
$grep -I -r fputs|wc -l
123
$ grep -I -r fprintf|wc -l
689

fputs seems to be used already, though I never came across these parts
of the code
myself, so I wondered if we had fputs in the code base already.

My next guess would have been personal preference, though there seem to be
actual reasons[1] why to use fputs.

Maybe mention the switch in the commit message (As the formatting is
unnecessary,
switch to the lighter fputs function)?

[1] http://stackoverflow.com/questions/5690979/fputs-vs-fprintf


> +               else
> +                       fputs(_("There are no candidates for merging among the refs that you just fetched."), stderr);
> +               fputs(_("Generally this means that you provided a wildcard refspec which had no\n"
> +                       "matches on the remote end."), stderr);
>         } else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
>                 fprintf(stderr,
>                         _("You asked to pull from the remote '%s', but did not specify\n"
> @@ -485,12 +487,15 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>                         "for your current branch, you must specify a branch on the command line.\n"),
>                         repo);
>         } else if (!curr_branch) {
> -               fprintf(stderr,
> -                       _("You are not currently on a branch. Please specify which\n"
> -                       "branch you want to merge with. See git-pull(1) for details.\n"
> -                       "\n"
> -                       "    git pull <remote> <branch>\n"
> -                       "\n"));
> +               fputs(_("You are not currently on a branch."), stderr);
> +               if (opt_rebase)
> +                       fputs(_("Please specify which branch you want to rebase against."), stderr);
> +               else
> +                       fputs(_("Please specify which branch you want to merge with."), stderr);
> +               fputs(_("See git-pull(1) for details."), stderr);
> +               fputs("", stderr);
> +               fputs("    git pull <remote> <branch>", stderr);
> +               fputs("", stderr);
>         } else if (!curr_branch->merge_nr) {
>                 struct known_remote *remotes = NULL;
>                 const char *remote_name = "<remote>";
> @@ -499,14 +504,17 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>                 if (remotes && !remotes->next)
>                         remote_name = remotes->remote->name;
>
> +               fputs(_("There is no tracking information for the current branch."), stderr);
> +               if (opt_rebase)
> +                       fputs(_("Please specify which branch you want to rebase against."), stderr);
> +               else
> +                       fputs(_("Please specify which branch you want to merge with."), stderr);
> +               fputs(_("See git-pull(1) for details."), stderr);
> +               fputs("", stderr);
> +               fputs("    git pull <remote> <branch>", stderr);
> +               fputs("", stderr);
>                 fprintf(stderr,
> -                       _("There is no tracking information for the current branch.\n"
> -                       "Please specify which branch you want to merge with.\n"
> -                       "See git-pull(1) for details.\n"
> -                       "\n"
> -                       "    git pull <remote> <branch>\n"
> -                       "\n"
> -                       "If you wish to set tracking information for this branch you can do so with:\n"
> +                       _("If you wish to set tracking information for this branch you can do so with:\n"
>                         "\n"
>                         "    git branch --set-upstream-to=%s/<branch> %s\n"
>                         "\n"),
> --
> 2.1.4
>

Re: [PATCH 11/14] pull: teach git pull about --rebase

[Johannes Schindelin]

Hi Paul,

I would like to add more suggestions to Stefan's comments, please find them below.

On 2015-05-18 17:06, Paul Tan wrote:
> @@ -506,11 +547,185 @@ static int run_merge(void)
>  	return ret;
>  }
>  
> +/**
> + * Returns the merge branch for the current branch. Returns NULL if repo is not
> + * a valid remote, HEAD does not point to a branch, repo is not the branch's
> + * configured remote or the branch does not have any configured merge branch.
> + */
> +static char *get_merge_branch_1(const char *repo)

A better name might be `get_upstream_branch(const char *repo)`, in line with the function in `sha1_name.c` that implements the `@{upstream}` functionality.

> +{
> +	struct remote *rm;
> +	struct branch *curr_branch;
> +
> +	if (repo && !(rm = remote_get(repo)))
> +		return NULL;
> +	if (!(curr_branch = branch_get("HEAD")))
> +		return NULL;
> +	if (repo && curr_branch->remote != rm)
> +		return NULL;
> +	if (!curr_branch->merge_nr)
> +		return NULL;
> +	return xstrdup(curr_branch->merge[0]->dst);
> +}
> +
> +/**
> + * Given a refspec, returns the merge branch. Returns NULL if the refspec src
> + * does not refer to a branch.
> + *
> + * FIXME: It should return the tracking branch. Currently only works with the
> + * default mapping.
> + */
> +static char *get_merge_branch_2(const char *repo, const char *refspec)
> +{
> +	struct refspec *spec;
> +	const char *remote;
> +	char *merge_branch;
> +
> +	spec = parse_fetch_refspec(1, &refspec);
> +	remote = spec->src;
> +	if (!*remote || !strcmp(remote, "HEAD"))
> +		remote = "HEAD";
> +	else if (skip_prefix(remote, "heads/", &remote))
> +		;
> +	else if (skip_prefix(remote, "refs/heads/", &remote))
> +		;
> +	else if (starts_with(remote, "refs/") ||
> +		starts_with(remote, "tags/") ||
> +		starts_with(remote, "remotes/"))
> +		remote = "";
> +
> +	if (*remote) {
> +		if (!strcmp(repo, "."))
> +			merge_branch = mkpathdup("refs/heads/%s", remote);
> +		else
> +			merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
> +	} else
> +		merge_branch = NULL;
> +
> +	free_refspec(1, spec);
> +	return merge_branch;
> +}

I have to admit that it took me a substantial amount of time to deduce from the code what `get_merge_branch_2()` really does (judging from the description, I thought that it would be essentially the same as `get_merge_branch_1()` without the hard-coded "HEAD"). If the comment above the function would have said something like:

/**
 * Given a refspec, returns the name of the local tracking ref.
 */

I would have had an easier time. Also, I wonder if something like this would do the job:

	spec = parse_fetch_refspec(1, &refspec);
	if (spec->dst)
		return spec->dst;
	if (!(remote = get_remote(remote_name)))
		return NULL;
	if (remote_find_tracking(remote, spec))
		return spec->dst;
	return NULL;

(I guess we'd have to `xstrdup()` the return values because the return value of `get_merge_branch_1()` needs to be `free()`d, but maybe we can avoid so much `malloc()/free()`ing.) Again, the function name should probably be changed to something clearer, maybe to `get_tracking_branch()`.

One thing that is not clear at all to me is whether

	git pull --rebase origin master next

would error out as expected, or simply rebase to `origin/master`.

> +/**
> + * Sets fork_point to the point at which the current branch forked from its
> + * remote merge branch. Returns 0 on success, -1 on failure.
> + */
> +static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
> +		const char *repo, const char *refspec)
> +{
> +	int ret;
> +	struct branch *curr_branch;
> +	char *remote_merge_branch;
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (!(curr_branch = branch_get("HEAD")))
> +		return -1;
> +
> +	if (refspec)
> +		remote_merge_branch = get_merge_branch_2(repo, refspec);
> +	else
> +		remote_merge_branch = get_merge_branch_1(repo);
> +
> +	if (!remote_merge_branch)
> +		return -1;

We should probably `return error(_"No tracking branch found for %s@, refspec ? refspec : "HEAD");` so that the user has a chance to understand that there has been a problem and how to solve it.

> +	argv_array_pushl(&args, "merge-base", "--fork-point",
> +			remote_merge_branch, curr_branch->name, NULL);
> +	cp.argv = args.argv;

Let's just use `cp.args` directly...

> +	cp.no_stdin = 1;
> +	cp.no_stderr = 1;
> +	cp.git_cmd = 1;
> +
> +	if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
> +		goto cleanup;
> +
> +	if ((ret = get_sha1_hex(sb.buf, fork_point)))
> +		goto cleanup;
> +
> +cleanup:
> +	free(remote_merge_branch);
> +	strbuf_release(&sb);
> +	return ret ? -1 : 0;
> +}
> +
> +[...]
> +/**
> + * Given the current HEAD SHA1, the merge head returned from git-fetch and the
> + * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
> + * appropriate arguments and returns its exit status.
> + */
> +static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
> +		unsigned char merge_head[GIT_SHA1_RAWSZ],
> +		unsigned char fork_point[GIT_SHA1_RAWSZ])
> +{
> +	int ret;
> +	unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +
> +	if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head,
> fork_point))

It might be my mail program only that mangled the diff here. But it could also be that this line is a little long (by my count, it is 81 columns wide).

Otherwise, this looks really good to me.

Ciao,
Dscho

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Johannes Schindelin]

Hi,

On 2015-05-19 02:12, Stefan Beller wrote:
> On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> Tweak the error messages printed by die_no_merge_candidates() to take
>> into account that we may be "rebasing against" rather than "merging
>> with".
>>
>> Signed-off-by: Paul Tan <pyokagan@gmail.com>
>> ---
>>  builtin/pull.c | 42 +++++++++++++++++++++++++-----------------
>>  1 file changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index c8d673d..15b65a0 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -474,10 +474,12 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>>         const char *remote = curr_branch ? curr_branch->remote_name : NULL;
>>
>>         if (*refspecs) {
>> -               fprintf(stderr,
>> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
>> -                       "Generally this means that you provided a wildcard refspec which had no\n"
>> -                       "matches on the remote end.\n"));
>> +               if (opt_rebase)
>> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
> 
> Is there a reason you switch to fputs, instead of fprintf?
> $grep -I -r fputs|wc -l
> 123
> $ grep -I -r fprintf|wc -l
> 689
> 
> fputs seems to be used already, though I never came across these parts
> of the code
> myself, so I wondered if we had fputs in the code base already.

My guess was that the new-line I expected and did not see in the `fputs()` call was the reason. But according to http://pubs.opengroup.org/onlinepubs/009695399/functions/fputs.html:

    The puts() function appends a <newline> while fputs() does not.

This comment concludes my review on this round of this patch series. It required quite some time to review, so it must have taken quite a bit more to actually write it: impressive work!

Ciao,
Dscho

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

>>> -               fprintf(stderr,
>>> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
>>> -                       "Generally this means that you provided a wildcard refspec which had no\n"
>>> -                       "matches on the remote end.\n"));
>>> +               if (opt_rebase)
>>> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
>> 
>     The puts() function appends a <newline> while fputs() does not.

Yup, so this update makes the command spew unterminated lines, which
not something intended...

Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

[Paul Tan]

Hi,

On Tue, May 19, 2015 at 1:41 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> On 2015-05-18 17:05, Paul Tan wrote:
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 0b771b9..a4d9c92 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -11,16 +11,64 @@
>>  #include "argv-array.h"
>>  #include "run-command.h"
>>
>> +/**
>> + * Given an option opt, where opt->value points to a char* and opt->defval is a
>> + * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is
>> + * NULL, then opt->value is set to "--$defval". If unset is true, then
>> + * opt->value is set to "--no-$defval".
>> + */
>> +static int parse_opt_passthru(const struct option *opt, const char
>> *arg, int unset)
>
> How about adding this to parse-options-cb.c in a separate patch? I guess the description could say something like:

Yeah, I was also thinking if the callback could be generalized as well.

Specifically, I wonder if we should re-construct just the
last-provided option (the current solution), or all the options (e.g.
something like OPT_STRING_LIST). I'm currently working on the rewrite
for git-am.sh as well, and it turns out it makes heavy use of the
latter, so we probably should support both.

> /**
>  * Given an option opt, recreate the command-line option, as strbuf. This is useful
>  * when a command needs to parse a command-line option in order to pass it to yet
>  * another command. This callback can be used in conjunction with the
>  * PARSE_OPT_(OPTARG|NOARG|NONEG) options, but not with PARSE_OPT_LASTARG_DEFAULT
>  * because it expects `defval` to be the name of the command-line option to
>  * reconstruct.
>  *
>  * The result will be stored in opt->value, which is expected to be a pointer to an
>  * strbuf.
>  */
>

Heh, this docstring reads much better than mine.

> Implied by my suggested description, I also propose to put the re-recreated command-line option into a strbuf instead of a char *, to make memory management easier (read: avoid memory leaks).

Unfortunately, the usage of strbuf means that we lose the ability to
know if an option was not provided at all (the value is NULL). This is
important as some of these options are used to override the default
configured behavior.

> You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:

Hmm, would there be a situation where arg is NULL when `unset != 0`?
At first glance in the source code, it won't unless
PARSE_OPT_LASTARG_DEFAULT is set, I guess. Anyway, there's no harm in
being careful, though I think it's more likely that an invalid pointer
is passed in through "arg", which we can't detect :-(.

> int parse_opt_passthru(const struct option *opt, const char *arg, int unset)
> {
>         struct strbuf *buf = opt->value;
>
>         assert(opt->defval);
>         strbuf_reset(buf);
>         if (unset) {
>                 assert(!arg);
>                 strbuf_addf(buf, "--no-%s", opt->defval);
>         }
>         else {
>                 strbuf_addf(buf, "--%s", opt->defval);
>                 if (arg)
>                         strbuf_addf(buf, "=%s", arg);
>         }
>
>         return 0;
> }
>
>>  static struct option pull_options[] = {
>> +     /* Shared options */
>> +     OPT__VERBOSITY(&opt_verbosity),
>> +     { OPTION_CALLBACK, 0, "progress", &opt_progress, NULL,
>> +       N_("force progress reporting"),
>> +       PARSE_OPT_NOARG, parse_opt_passthru},
>
> I *think* there is a '(intptr_t) "progress"' missing...

Ugh. I think this shows that relying on "defval" is too error-prone.
Anyway, it is only required for the options -n (which maps to
--no-stat), and --summary (which maps to --stat), so we should
probably make it such that if defval is not provided, we fallback to
using long_name or short_name.

>>  /**
>> + * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
>> + */
>> +static void argv_push_verbosity(struct argv_array *arr)
>> +{
>> +     int verbosity;
>> +
>> +     for (verbosity = opt_verbosity; verbosity > 0; verbosity--)
>> +             argv_array_push(arr, "-v");
>> +
>> +     for (verbosity = opt_verbosity; verbosity < 0; verbosity++)
>> +             argv_array_push(arr, "-q");
>> +}
>
> Hmm... I guess this is *really* nit-picky, but I'd rather use "i" instead of "verbosity" because the second loop is about quietness instead of verbosity ;-)

Well, my thinking was that a negative verbosity *is* quietness ;-).

Thanks,
Paul

Re: [PATCH 06/14] pull: support pull.ff config

[Paul Tan]

Hi,

On Tue, May 19, 2015 at 3:02 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Hi Paul,
>
> On 2015-05-18 17:06, Paul Tan wrote:
>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 8982fdf..b305a47 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -209,6 +209,28 @@ static void argv_push_force(struct argv_array *arr)
>>               argv_array_push(arr, "-f");
>>  }
>>
>> +/**
>> + * If pull.ff is "true", returns "--ff". If pull.ff is "false", returns
>> + * "--no-ff". If pull.ff is "only", returns "--ff-only". Otherwise, returns
>> + * NULL.
>> + */
>> +static const char *config_get_ff(void)
>> +{
>> +     const char *value;
>> +
>> +     if (git_config_get_value("pull.ff", &value))
>> +             return NULL;
>> +     switch (git_config_maybe_bool("pull.ff", value)) {
>> +             case 0:
>> +                     return "--no-ff";
>> +             case 1:
>> +                     return "--ff";
>> +     }
>> +     if (!strcmp("pull.ff", "only"))
>
> I think you want to test `!strcmp("only", value)` ;-)

Ugh >.<

Fixed, thanks for catching.

Regards,
Paul

Re: [PATCH 10/14] pull: set reflog message

[Paul Tan]

On Tue, May 19, 2015 at 5:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Yeah, either that, or "insert separator only before adding to
> something else" pattern, i.e.
>
>         for (i = 0; i < argc; i++) {
>                 if (i)
>                         addch(&msg, ' ');
>                 addstr(&msg, argv[i]);
>         }
>
> Both are easier to read than "always append SP and trim at the end".
> Besides, trimming at the end makes readers wonder if there are cases
> where argv[argc-1] ends with whitespaces and the code on purpose
> removes them.

Yeah, this looks better and is more correct.

Thanks,
Paul

Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

[Johannes Schindelin]

Hi Paul,

On 2015-05-21 11:48, Paul Tan wrote:
> 
> On Tue, May 19, 2015 at 1:41 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> On 2015-05-18 17:05, Paul Tan wrote:
>>> diff --git a/builtin/pull.c b/builtin/pull.c
>>> index 0b771b9..a4d9c92 100644
>>> --- a/builtin/pull.c
>>> +++ b/builtin/pull.c
>>> @@ -11,16 +11,64 @@
>>>  #include "argv-array.h"
>>>  #include "run-command.h"
>>>
>>> +/**
>>> + * Given an option opt, where opt->value points to a char* and opt->defval is a
>>> + * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is
>>> + * NULL, then opt->value is set to "--$defval". If unset is true, then
>>> + * opt->value is set to "--no-$defval".
>>> + */
>>> +static int parse_opt_passthru(const struct option *opt, const char
>>> *arg, int unset)
>>
>> Implied by my suggested description, I also propose to put the re-recreated command-line option into a strbuf instead of a char *, to make memory management easier (read: avoid memory leaks).
> 
> Unfortunately, the usage of strbuf means that we lose the ability to
> know if an option was not provided at all (the value is NULL). This is
> important as some of these options are used to override the default
> configured behavior.

Would this not be implied by the strbuf's len being 0?

>> You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:
> 
> Hmm, would there be a situation where arg is NULL when `unset != 0`?

I forgot to say that my suggestion was purely meant as defensive coding. Yes, `arg` *should* be `NULL` when `unset != 0`. Should ;-)

By the way, just to make sure: My comments and suggestions are no demands; you should feel very free to reject them when you feel that your code is better without the suggested changes. I am just trying to be helpful.

Ciao,
Dscho

Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

[Paul Tan]

Hi Johannes,

On Thu, May 21, 2015 at 11:59 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Hi Paul,
>
> On 2015-05-21 11:48, Paul Tan wrote:
>> Unfortunately, the usage of strbuf means that we lose the ability to
>> know if an option was not provided at all (the value is NULL). This is
>> important as some of these options are used to override the default
>> configured behavior.
>
> Would this not be implied by the strbuf's len being 0?

You're right >.< I think I need more coffee as well ;-)

It would mean adding STRBUF_INIT's to all of the option variables, but
I don't think it is a problem. strbufs it is, then.

>>> You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:
>>
>> Hmm, would there be a situation where arg is NULL when `unset != 0`?
>
> I forgot to say that my suggestion was purely meant as defensive coding. Yes, `arg` *should* be `NULL` when `unset != 0`. Should ;-)
>
> By the way, just to make sure: My comments and suggestions are no demands; you should feel very free to reject them when you feel that your code is better without the suggested changes. I am just trying to be helpful.

Thanks, your comments have been really helpful. I really do appreciate it :).

Regards,
Paul

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Paul Tan]

On Wed, May 20, 2015 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>>>> -               fprintf(stderr,
>>>> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
>>>> -                       "Generally this means that you provided a wildcard refspec which had no\n"
>>>> -                       "matches on the remote end.\n"));
>>>> +               if (opt_rebase)
>>>> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
>>>
>>     The puts() function appends a <newline> while fputs() does not.
>
> Yup, so this update makes the command spew unterminated lines, which
> not something intended...

Ugh >< Will put the "\n" back.

And yes, I used fputs() because it seems wasteful to use fprintf()
which will scan the string looking for any '%' formatting units, when
we know there aren't.

I will also update 05/14 to use fputs() as well where appropriate.

Thanks,
Paul

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Johannes Schindelin]

Hi Paul,

On 2015-05-22 15:48, Paul Tan wrote:
> On Wed, May 20, 2015 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>>>> -               fprintf(stderr,
>>>>> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
>>>>> -                       "Generally this means that you provided a wildcard refspec which had no\n"
>>>>> -                       "matches on the remote end.\n"));
>>>>> +               if (opt_rebase)
>>>>> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
>>>>
>>>     The puts() function appends a <newline> while fputs() does not.
>>
>> Yup, so this update makes the command spew unterminated lines, which
>> not something intended...
> 
> Ugh >< Will put the "\n" back.
> 
> And yes, I used fputs() because it seems wasteful to use fprintf()
> which will scan the string looking for any '%' formatting units, when
> we know there aren't.
> 
> I will also update 05/14 to use fputs() as well where appropriate.

I believe the common thinking is that consistency beats speed in error messages, so it would be easier to read and maintain the code if all of those error messages were just using `fprintf(stderr, ...);`. It's not as if we spit out hundreds of thousands of error messages per second where that `%s` parsing would hurt ;-)

Ciao,
Dscho

Re: [PATCH 14/14] pull --rebase: error on no merge candidate cases

[Stefan Beller]

On Fri, May 22, 2015 at 7:14 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Hi Paul,
>
> On 2015-05-22 15:48, Paul Tan wrote:
>> On Wed, May 20, 2015 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>
>>>>>> -               fprintf(stderr,
>>>>>> -                       _("There are no candidates for merging among the refs that you just fetched.\n"
>>>>>> -                       "Generally this means that you provided a wildcard refspec which had no\n"
>>>>>> -                       "matches on the remote end.\n"));
>>>>>> +               if (opt_rebase)
>>>>>> +                       fputs(_("There is no candidate for rebasing against among the refs that you just fetched."), stderr);
>>>>>
>>>>     The puts() function appends a <newline> while fputs() does not.
>>>
>>> Yup, so this update makes the command spew unterminated lines, which
>>> not something intended...
>>
>> Ugh >< Will put the "\n" back.
>>
>> And yes, I used fputs() because it seems wasteful to use fprintf()
>> which will scan the string looking for any '%' formatting units, when
>> we know there aren't.
>>
>> I will also update 05/14 to use fputs() as well where appropriate.
>
> I believe the common thinking is that consistency beats speed in error messages, so it would be easier to read and maintain the code if all of those error messages were just using `fprintf(stderr, ...);`. It's not as if we spit out hundreds of thousands of error messages per second where that `%s` parsing would hurt ;-)
>
> Ciao,
> Dscho

As soon as we spit out one error message,
the speed game is over anyway. (IO is slow,
and I believe in the error case correctness
is the most critical thing to get right, so no
need to pay attention to performance).
Though I don't mind having a fputs/fprintf
mixture. I just happened to work on parts
of code without any fputs before, that's why
I brought up this discussion.

Actually I think I'd prefer fputs when there is no %.

Re: [PATCH 00/14] Make git-pull a builtin

[Paul Tan]

Hi,

On Tue, May 19, 2015 at 3:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> This series rewrites git-pull.sh into a C builtin, thus improving its
>> performance and portability. It is part of my GSoC project to rewrite git-pull
>> and git-am into builtins[2].
>
> Earlier you were worried about 'git pull' being used in many tests
> for the purpose of testing other parts of the system and not testing
> 'pull' itself.  For a program as complex as 'git pull', you may want
> to take the 'competing implementations' approach.
>
> (1) write an empty cmd_pull() that relaunches "git pull" scripted
>     porcelain from the $GIT_EXEC_PATH with given parameters, and
>     wire all the necessary bits to git.c.
>
> (2) enhance cmd_pull() a bit by bit, but keep something like this
>
>         if (getenv(GIT_USE_BUILTIN_PULL)) {
>                 /* original run_command("git-pull.sh") code */
>                 exit $?
>         }
>
>         ... your "C" version ...
>
> (3) add "GIT_USE_BUILTIN_PULL=Yes; export GIT_USE_BUILTIN_PULL" at
>     the beginning of "t55??" test scripts (but not others that rely
>     on working pull and that are not interested in catching bugs in
>     pull).
>
> (4) once cmd_pull() becomes fully operational, drop (3) and also the
>     conditional one you added in (2), and retire the environment
>     variable.  Retire the git-pull.sh script to contrib/examples/
>     boneyard.
>
> That way, you will always have a reference you can use throughout
> the development.
>
> Just a suggestion, not a requirement.

Okay, I'm trying this out in the next re-roll. I do agree that this
patch series should not touch anything in t/ at all.

One problem(?) is that putting builtins/pull.o in the BUILTIN_OBJS and
leaving git-pull.sh in SCRIPT_SH in the Makefile will generate 2
targets to ./git-pull (they will clobber each other). For GNU Make,
the last defined target will win, so in this case it just happens that
git-pull.sh will win because the build targets for the shell scripts
are defined after the build targets for the builtins, so this works in
our favor I guess.

Regards,
Paul

Re: [PATCH 00/14] Make git-pull a builtin

[Paul Tan]

On Sat, May 30, 2015 at 3:29 PM, Paul Tan <pyokagan@gmail.com> wrote:
> Hi,
> Okay, I'm trying this out in the next re-roll. I do agree that this
> patch series should not touch anything in t/ at all.
>
> One problem(?) is that putting builtins/pull.o in the BUILTIN_OBJS and
> leaving git-pull.sh in SCRIPT_SH in the Makefile will generate 2
> targets to ./git-pull (they will clobber each other). For GNU Make,
> the last defined target will win, so in this case it just happens that
> git-pull.sh will win because the build targets for the shell scripts
> are defined after the build targets for the builtins, so this works in
> our favor I guess.
>
> Regards,
> Paul

Just to add on, I just discovered that test-lib.sh unsets all GIT_*
environment variables "for repeatability", so the name
"GIT_USE_BUILTIN_PULL" cannot be used. I'm tempted to just add a
underscore just before the name ("_GIT_USE_BUILTIN_PULL") to work
around this, since it's just a temporary thing.

Thanks,
Paul

Re: [PATCH 11/14] pull: teach git pull about --rebase

[Paul Tan]

Hi Johannes,

On Tue, May 19, 2015 at 9:04 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> On 2015-05-18 17:06, Paul Tan wrote:
>> +/**
>> + * Given a refspec, returns the merge branch. Returns NULL if the refspec src
>> + * does not refer to a branch.
>> + *
>> + * FIXME: It should return the tracking branch. Currently only works with the
>> + * default mapping.
>> + */
>> +static char *get_merge_branch_2(const char *repo, const char *refspec)
>> +{
>> +     struct refspec *spec;
>> +     const char *remote;
>> +     char *merge_branch;
>> +
>> +     spec = parse_fetch_refspec(1, &refspec);
>> +     remote = spec->src;
>> +     if (!*remote || !strcmp(remote, "HEAD"))
>> +             remote = "HEAD";
>> +     else if (skip_prefix(remote, "heads/", &remote))
>> +             ;
>> +     else if (skip_prefix(remote, "refs/heads/", &remote))
>> +             ;
>> +     else if (starts_with(remote, "refs/") ||
>> +             starts_with(remote, "tags/") ||
>> +             starts_with(remote, "remotes/"))
>> +             remote = "";
>> +
>> +     if (*remote) {
>> +             if (!strcmp(repo, "."))
>> +                     merge_branch = mkpathdup("refs/heads/%s", remote);
>> +             else
>> +                     merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
>> +     } else
>> +             merge_branch = NULL;
>> +
>> +     free_refspec(1, spec);
>> +     return merge_branch;
>> +}
>
> I have to admit that it took me a substantial amount of time to deduce from the code what `get_merge_branch_2()` really does (judging from the description, I thought that it would be essentially the same as `get_merge_branch_1()` without the hard-coded "HEAD"). If the comment above the function would have said something like:
>
> /**
>  * Given a refspec, returns the name of the local tracking ref.
>  */
>
> I would have had an easier time. Also, I wonder if something like this would do the job:

Yeah whoops, this came from a confusion over the difference over
"merge branch" and "remote tracking branch". A merge branch would be a
remote tracking branch, but a remote tracking branch is not
necessarily a merge branch.

>         spec = parse_fetch_refspec(1, &refspec);
>         if (spec->dst)
>                 return spec->dst;

Hmm, I notice that get_remote_merge_branch() only looks at the src
part of the refspec. However, I guess it is true that if the dst part
is provided, the user may be wishing for that to be interpreted as the
"remote tracking branch", so we should be looking at it to calculate
the fork point.

>         if (!(remote = get_remote(remote_name)))
>                 return NULL;
>         if (remote_find_tracking(remote, spec))
>                 return spec->dst;

... and if the dst part of the refspec is not provided, we fall back
to see if there is any remote tracking branch in the repo for the src
part of the ref, which matches the intention of
get_remote_merge_branch() I think, while being better because
remote_find_tracking() takes into account the actual configured fetch
refspecs for the remote.

However, we also need to consider if the user provided a wildcard
refspec, as it will not make sense in this case. From my reading,
remote_find_tracking(), which calls query_refspecs(), would just match
the src part literally, so I guess we should explicitly detect and
error out in this case.

>         return NULL;
>
> (I guess we'd have to `xstrdup()` the return values because the return value of `get_merge_branch_1()` needs to be `free()`d, but maybe we can avoid so much `malloc()/free()`ing.) Again, the function name should probably be changed to something clearer, maybe to `get_tracking_branch()`.

Yeah, it should be changed to get_tracking_branch(), since it is
different from get_merge_branch().

> One thing that is not clear at all to me is whether
>
>         git pull --rebase origin master next
>
> would error out as expected, or simply rebase to `origin/master`.

In git-pull.sh, for the rebase fork point calculation it just used the
first refspec. However, when running git-rebase it checked to see if
there was only one merge base, which is replicated here:

    @@ -573,6 +792,10 @@ int cmd_pull(int argc, const char **argv,
const char *prefix)
                    if (merge_heads.nr > 1)
                            die(_("Cannot merge multiple branches into
empty head."));
                    return pull_into_void(*merge_heads.sha1, curr_head);
    +       } else if (opt_rebase) {
    +               if (merge_heads.nr > 1)
    +                       die(_("Cannot rebase onto multiple branches."));
    +               return run_rebase(curr_head, *merge_heads.sha1,
rebase_fork_point);
            } else
                    return run_merge();
     }

Since this is just a 1:1 rewrite I just tried to keep as close to the
original as possible. However, thinking about it, since we *are* just
using the first refspec for fork point calculation, I do agree that we
should probably give an warning() here as well if the user provided
more than one refspec, like "Cannot calculate rebase fork point as you
provided more than one refspec. git-pull will not be able to handle a
rebased upstream". I do not think it is fatal enough that we should
error() or die(), as e.g. the first refspec may be a wildcard refspec
that matches nothing, and the second refspec that matches one merge
head for rebasing. This is pretty contrived though, but still
technically legitimate. I dunno.

>> +/**
>> + * Sets fork_point to the point at which the current branch forked from its
>> + * remote merge branch. Returns 0 on success, -1 on failure.
>> + */
>> +static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
>> +             const char *repo, const char *refspec)
>> +{
>> +     int ret;
>> +     struct branch *curr_branch;
>> +     char *remote_merge_branch;
>> +     struct argv_array args = ARGV_ARRAY_INIT;
>> +     struct child_process cp = CHILD_PROCESS_INIT;
>> +     struct strbuf sb = STRBUF_INIT;
>> +
>> +     if (!(curr_branch = branch_get("HEAD")))
>> +             return -1;
>> +
>> +     if (refspec)
>> +             remote_merge_branch = get_merge_branch_2(repo, refspec);
>> +     else
>> +             remote_merge_branch = get_merge_branch_1(repo);
>> +
>> +     if (!remote_merge_branch)
>> +             return -1;
>
> We should probably `return error(_"No tracking branch found for %s@, refspec ? refspec : "HEAD");` so that the user has a chance to understand that there has been a problem and how to solve it.

Just like the above, I don't think this is serious enough to be
considered an error() though. Sure, this means that we cannot properly
handle the case where the upstream has been rebased, but this is not
always the case. We could probably have a warning() here, but I think
the message should be improved to tell the user what exactly she is
losing out on. e.g. "No tracking branch found for %s. git-pull will
not be able to handle a rebased upstream."

>> +     argv_array_pushl(&args, "merge-base", "--fork-point",
>> +                     remote_merge_branch, curr_branch->name, NULL);
>> +     cp.argv = args.argv;
>
> Let's just use `cp.args` directly...

Yeah, whoops.

>> +     cp.no_stdin = 1;
>> +     cp.no_stderr = 1;
>> +     cp.git_cmd = 1;
>> +
>> +     if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
>> +             goto cleanup;
>> +
>> +     if ((ret = get_sha1_hex(sb.buf, fork_point)))
>> +             goto cleanup;
>> +
>> +cleanup:
>> +     free(remote_merge_branch);
>> +     strbuf_release(&sb);
>> +     return ret ? -1 : 0;
>> +}
>> +
>> +[...]
>> +/**
>> + * Given the current HEAD SHA1, the merge head returned from git-fetch and the
>> + * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
>> + * appropriate arguments and returns its exit status.
>> + */
>> +static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
>> +             unsigned char merge_head[GIT_SHA1_RAWSZ],
>> +             unsigned char fork_point[GIT_SHA1_RAWSZ])
>> +{
>> +     int ret;
>> +     unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
>> +     struct argv_array args = ARGV_ARRAY_INIT;
>> +
>> +     if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head,
>> fork_point))
>
> It might be my mail program only that mangled the diff here. But it could also be that this line is a little long (by my count, it is 81 columns wide).

Yeah, it is, but I felt that breaking the line would reduce readability.

Thanks, these were some really hard questions that you raised ;-).

Regards,
Paul

Re: [PATCH 11/14] pull: teach git pull about --rebase

[Paul Tan]

On Sun, May 31, 2015 at 4:18 PM, Paul Tan <pyokagan@gmail.com> wrote:
> On Tue, May 19, 2015 at 9:04 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> Also, I wonder if something like this would do the job:
>>         spec = parse_fetch_refspec(1, &refspec);
>>         if (spec->dst)
>>                 return spec->dst;
>
> Hmm, I notice that get_remote_merge_branch() only looks at the src
> part of the refspec. However, I guess it is true that if the dst part
> is provided, the user may be wishing for that to be interpreted as the
> "remote tracking branch", so we should be looking at it to calculate
> the fork point.
>
>>         if (!(remote = get_remote(remote_name)))
>>                 return NULL;
>>         if (remote_find_tracking(remote, spec))
>>                 return spec->dst;
>
> ... and if the dst part of the refspec is not provided, we fall back
> to see if there is any remote tracking branch in the repo for the src
> part of the ref, which matches the intention of
> get_remote_merge_branch() I think, while being better because
> remote_find_tracking() takes into account the actual configured fetch
> refspecs for the remote.
>
> However, we also need to consider if the user provided a wildcard
> refspec, as it will not make sense in this case. From my reading,
> remote_find_tracking(), which calls query_refspecs(), would just match
> the src part literally, so I guess we should explicitly detect and
> error out in this case.

With all that said, after thinking about it I feel that this patch
series should focus solely on rewriting git-pull.sh 1:1. While I do
agree with the above suggested improvements, I think they should be
implemented as separated patch(es) on top of this series since we
would be technically changing git-pull's behavior, even if we are
improving it.

As such, the issue that I think should be focused on for this patch
is: does get_merge_branch_1() and get_merge_branch_2() in this patch
implement the same behavior as get_remote_merge_branch() in
git-parse.remote.sh? If it does, then its purpose is fulfilled.

So, I'll keep the overall logic of get_merge_branch_2() the same for
the next re-roll. (Other than renaming the function and fixing code
style issues). Once this series is okay, I'll look into doing a
separate patch on top that changes the function to use
remote_find_tracking() so that we fix the assumption that the default
fetch mapping is used.

The other possibility is that we fix this in git-parse-remote.sh, but
I'm personally getting a bit tired from having to re-implement the
same thing in shell script and C. Furthermore, the only script using
get_remote_merge_branch() is git-pull.sh.

> [...]
> Since this is just a 1:1 rewrite I just tried to keep as close to the
> original as possible. However, thinking about it, since we *are* just
> using the first refspec for fork point calculation, I do agree that we
> should probably give an warning() here as well if the user provided
> more than one refspec, like "Cannot calculate rebase fork point as you
> provided more than one refspec. git-pull will not be able to handle a
> rebased upstream". I do not think it is fatal enough that we should
> error() or die(), as e.g. the first refspec may be a wildcard refspec
> that matches nothing, and the second refspec that matches one merge
> head for rebasing. This is pretty contrived though, but still
> technically legitimate. I dunno.
>[...]
>> We should probably `return error(_"No tracking branch found for %s@, refspec ? refspec : "HEAD");` so that the user has a chance to understand that there has been a problem and how to solve it.
>
> Just like the above, I don't think this is serious enough to be
> considered an error() though. Sure, this means that we cannot properly
> handle the case where the upstream has been rebased, but this is not
> always the case. We could probably have a warning() here, but I think
> the message should be improved to tell the user what exactly she is
> losing out on. e.g. "No tracking branch found for %s. git-pull will
> not be able to handle a rebased upstream."

Likewise, I won't introduce the error()s or warning()s for now.

Other than that, all other code style related issues have been/will be
fixed. Thanks for the reviews.

Regards,
Paul