[PATCH v2 00/19] Make git-pull a builtin

[PATCH v2 00/19] Make git-pull a builtin

[Paul Tan]

This is a re-roll of [v1]. Thanks Johannes, Stefan and Junio for the reviews
last round.

Previous versions:

[v1] http://thread.gmane.org/gmane.comp.version-control.git/269258

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 -- they need to
spawn a lot of processes, introduce a lot of dependencies and cannot take
advantage of git's 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[1].

[1] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1


Paul Tan (19):
  parse-options-cb: implement parse_opt_pass_strbuf()
  parse-options-cb: implement parse_opt_pass_argv_array()
  argv-array: implement argv_array_pushv()
  pull: implement skeletal builtin pull
  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
  pull: remove redirection to git-pull.sh

 Documentation/technical/api-argv-array.txt  |   3 +
 Makefile                                    |   2 +-
 advice.c                                    |   8 +
 advice.h                                    |   1 +
 argv-array.c                                |   6 +
 argv-array.h                                |   1 +
 builtin.h                                   |   1 +
 builtin/pull.c                              | 888 ++++++++++++++++++++++++++++
 git-pull.sh => contrib/examples/git-pull.sh |   0
 git.c                                       |   1 +
 parse-options-cb.c                          |  61 ++
 parse-options.h                             |   2 +
 12 files changed, 973 insertions(+), 1 deletion(-)
 create mode 100644 builtin/pull.c
 rename git-pull.sh => contrib/examples/git-pull.sh (100%)

-- 
2.1.4

[PATCH v2 01/19] parse-options-cb: implement parse_opt_pass_strbuf()

[Paul Tan]

Certain git commands, such as git-pull, are simply wrappers around other
git commands like git-fetch, git-merge and git-rebase. As such, these
wrapper commands will typically need to "pass through" command-line
options of the commands they wrap.

Implement the parse_opt_pass_strbuf() parse-options callback, which will
reconstruct the command-line option into an strbuf, such that it can be
passed to another git command.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * Previously implemented as a static function in builtin/pull.c. It now
      uses an strbuf instead of returning newly-allocated strings, to make
      memory management easier.
    
    * We don't use defval anymore. Instead, we use long_name and short_name.

 parse-options-cb.c | 29 +++++++++++++++++++++++++++++
 parse-options.h    |  1 +
 2 files changed, 30 insertions(+)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index be8c413..5b1dbcf 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -134,3 +134,32 @@ int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
 {
 	return 0;
 }
+
+/**
+ * For an option opt, recreates the command-line option in opt->value which
+ * must be an strbuf. This is useful when we need to pass the command-line
+ * option to another command.
+ */
+int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
+{
+	struct strbuf *sb = opt->value;
+
+	strbuf_reset(sb);
+
+	if (opt->long_name) {
+		strbuf_addstr(sb, unset ? "--no-" : "--");
+		strbuf_addstr(sb, opt->long_name);
+		if (arg) {
+			strbuf_addch(sb, '=');
+			strbuf_addstr(sb, arg);
+		}
+	} else if (opt->short_name && !unset) {
+		strbuf_addch(sb, '-');
+		strbuf_addch(sb, opt->short_name);
+		if (arg)
+			strbuf_addstr(sb, arg);
+	} else
+		return -1;
+
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index c71e9da..1d21398 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -224,6 +224,7 @@ extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
 extern int parse_opt_string_list(const struct option *, const char *, int);
 extern int parse_opt_noop_cb(const struct option *, const char *, int);
+extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
-- 
2.1.4

[PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

[Paul Tan]

Certain git commands, such as git-pull, are simply wrappers around other
git commands like git-fetch, git-merge and git-rebase. As such, these
wrapper commands will typically need to "pass through" command-line
options of the commands they wrap.

Implement the parse_opt_pass_argv_array() parse-options callback, which
will reconstruct all the provided command-line options into an
argv_array, such that it can be passed to another git command. This is
useful for passing command-line options that can be specified multiple
times.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * This function is a requirement for the rewrite of git-am to handle
      passing git-apply's options to git-apply. Since it would be
      implemented anyway I thought it would be good if git-pull could take
      advantage of it as well to handle --strategy and --strategy-option.

 parse-options-cb.c | 32 ++++++++++++++++++++++++++++++++
 parse-options.h    |  1 +
 2 files changed, 33 insertions(+)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 5b1dbcf..7330506 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "color.h"
 #include "string-list.h"
+#include "argv-array.h"
 
 /*----- some often used options -----*/
 
@@ -163,3 +164,34 @@ int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
 
 	return 0;
 }
+
+/**
+ * For an option opt, recreate the command-line option, appending it to
+ * opt->value which must be a argv_array. This is useful when we need to pass
+ * the command-line option, which can be specified multiple times, to another
+ * command.
+ */
+int parse_opt_pass_argv_array(const struct option *opt, const char *arg, int unset)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct argv_array *opt_value = opt->value;
+
+	if (opt->long_name) {
+		strbuf_addstr(&sb, unset ? "--no-" : "--");
+		strbuf_addstr(&sb, opt->long_name);
+		if (arg) {
+			strbuf_addch(&sb, '=');
+			strbuf_addstr(&sb, arg);
+		}
+	} else if (opt->short_name && !unset) {
+		strbuf_addch(&sb, '-');
+		strbuf_addch(&sb, opt->short_name);
+		if (arg)
+			strbuf_addstr(&sb, arg);
+	} else
+		return -1;
+
+	argv_array_push(opt_value, sb.buf);
+	strbuf_release(&sb);
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index 1d21398..b663f87 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -225,6 +225,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);
 extern int parse_opt_string_list(const struct option *, const char *, int);
 extern int parse_opt_noop_cb(const struct option *, const char *, int);
 extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
+extern int parse_opt_pass_argv_array(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
-- 
2.1.4

[PATCH v2 03/19] argv-array: implement argv_array_pushv()

[Paul Tan]

When we have a null-terminated array, it would be useful to convert it
or append it to an argv_array for further manipulation.

Implement argv_array_pushv() which will push a null-terminated array of
strings on to an argv_array.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Documentation/technical/api-argv-array.txt | 3 +++
 argv-array.c                               | 6 ++++++
 argv-array.h                               | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/technical/api-argv-array.txt b/Documentation/technical/api-argv-array.txt
index 1a79781..8076172 100644
--- a/Documentation/technical/api-argv-array.txt
+++ b/Documentation/technical/api-argv-array.txt
@@ -46,6 +46,9 @@ Functions
 	Format a string and push it onto the end of the array. This is a
 	convenience wrapper combining `strbuf_addf` and `argv_array_push`.
 
+`argv_array_pushv`::
+	Push a null-terminated array of strings onto the end of the array.
+
 `argv_array_pop`::
 	Remove the final element from the array. If there are no
 	elements in the array, do nothing.
diff --git a/argv-array.c b/argv-array.c
index 256741d..eaed477 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -49,6 +49,12 @@ void argv_array_pushl(struct argv_array *array, ...)
 	va_end(ap);
 }
 
+void argv_array_pushv(struct argv_array *array, const char **argv)
+{
+	for (; *argv; argv++)
+		argv_array_push(array, *argv);
+}
+
 void argv_array_pop(struct argv_array *array)
 {
 	if (!array->argc)
diff --git a/argv-array.h b/argv-array.h
index c65e6e8..a2fa0aa 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -17,6 +17,7 @@ __attribute__((format (printf,2,3)))
 void argv_array_pushf(struct argv_array *, const char *fmt, ...);
 LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
+void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
 
-- 
2.1.4

[PATCH v2 04/19] pull: implement skeletal builtin pull

[Paul Tan]

For the purpose of rewriting git-pull.sh into a C builtin, implement a
skeletal builtin/pull.c that redirects to $GIT_EXEC_PATH/git-pull.sh if
the environment variable _GIT_USE_BUILTIN_PULL is not defined. This
allows us to fall back on the functional git-pull.sh when running the
test suite for tests that depend on a working git-pull implementation.

This redirection should be removed when all the features of git-pull.sh
have been re-implemented in builtin/pull.c.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile       |  1 +
 builtin.h      |  1 +
 builtin/pull.c | 33 +++++++++++++++++++++++++++++++++
 git.c          |  1 +
 4 files changed, 36 insertions(+)
 create mode 100644 builtin/pull.c

diff --git a/Makefile b/Makefile
index 54ec511..2057a9d 100644
--- a/Makefile
+++ b/Makefile
@@ -877,6 +877,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..f8b79a2
--- /dev/null
+++ b/builtin/pull.c
@@ -0,0 +1,33 @@
+/*
+ * 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 "exec_cmd.h"
+
+static const char * const pull_usage[] = {
+	NULL
+};
+
+static struct option pull_options[] = {
+	OPT_END()
+};
+
+int cmd_pull(int argc, const char **argv, const char *prefix)
+{
+	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
+		const char *path = mkpath("%s/git-pull", git_exec_path());
+
+		if (sane_execvp(path, (char**) argv) < 0)
+			die_errno("could not exec %s", path);
+	}
+
+	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
+
+	return 0;
+}
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 },
-- 
2.1.4

[PATCH v2 05/19] pull: implement fetch + merge

[Paul Tan]

Implement the fetch + merge functionality of git-pull, by first running
git-fetch with the repo and refspecs provided on the command line, then
running git-merge on FETCH_HEAD to merge the fetched refs into the
current branch.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index f8b79a2..0ca23a3 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -9,8 +9,10 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 
 static const char * const pull_usage[] = {
+	N_("git pull [options] [<repository> [<refspec>...]]"),
 	NULL
 };
 
@@ -18,8 +20,60 @@ 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;
+
 	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
 		const char *path = mkpath("%s/git-pull", git_exec_path());
 
@@ -29,5 +83,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
-	return 0;
+	parse_repo_refspecs(argc, argv, &repo, &refspecs);
+
+	if (run_fetch(repo, refspecs))
+		return 1;
+
+	return run_merge();
 }
-- 
2.1.4

[PATCH v2 06/19] 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>
---

Notes:
    v2
    
    * Use parse_opt_pass_strbuf().

 builtin/pull.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 0ca23a3..c9c2cc0 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -16,11 +16,35 @@ static const char * const pull_usage[] = {
 	NULL
 };
 
+/* Shared options */
+static int opt_verbosity;
+static struct strbuf opt_progress = STRBUF_INIT;
+
 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_pass_strbuf},
+
 	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.
@@ -46,6 +70,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.len)
+		argv_array_push(&args, opt_progress.buf);
+
 	if (repo)
 		argv_array_push(&args, repo);
 	while (*refspecs)
@@ -64,6 +94,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.len)
+		argv_array_push(&args, opt_progress.buf);
+
 	argv_array_push(&args, "FETCH_HEAD");
 	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
 	argv_array_clear(&args);
-- 
2.1.4

[PATCH v2 07/19] 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)

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * Use opt_parse_pass_strbuf(), opt_parse_pass_argv_array() and
      argv_array_pushv()

 builtin/pull.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index c9c2cc0..5f08634 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -20,6 +20,18 @@ static const char * const pull_usage[] = {
 static int opt_verbosity;
 static struct strbuf opt_progress = STRBUF_INIT;
 
+/* Options passed to git-merge */
+static struct strbuf opt_diffstat = STRBUF_INIT;
+static struct strbuf opt_log = STRBUF_INIT;
+static struct strbuf opt_squash = STRBUF_INIT;
+static struct strbuf opt_commit = STRBUF_INIT;
+static struct strbuf opt_edit = STRBUF_INIT;
+static struct strbuf opt_ff = STRBUF_INIT;
+static struct strbuf opt_verify_signatures = STRBUF_INIT;
+static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
+static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
+static struct strbuf opt_gpg_sign = STRBUF_INIT;
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -27,6 +39,49 @@ static struct option pull_options[] = {
 	  N_("force progress reporting"),
 	  PARSE_OPT_NOARG, parse_opt_pass_strbuf},
 
+	/* 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_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "stat", &opt_diffstat, NULL,
+	  N_("show a diffstat at the end of the merge"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "summary", &opt_diffstat, NULL,
+	  N_("(synonym to --stat)"),
+	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, parse_opt_pass_strbuf },
+	{ 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_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "squash", &opt_squash, NULL,
+	  N_("create a single commit instead of doing a merge"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "commit", &opt_commit, NULL,
+	  N_("perform a commit if the merge succeeds (default)"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "edit", &opt_edit, NULL,
+	  N_("edit message before committing"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "ff", &opt_ff, NULL,
+	  N_("allow fast-forward"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "ff-only", &opt_ff, NULL,
+	  N_("abort if fast-forward is not possible"),
+	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_pass_strbuf },
+	{ 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_pass_strbuf },
+	{ OPTION_CALLBACK, 's', "strategy", &opt_strategies, N_("strategy"),
+	  N_("merge strategy to use"),
+	  0, parse_opt_pass_argv_array },
+	{ OPTION_CALLBACK, 'X', "strategy-option", &opt_strategy_opts,
+	  N_("option=value"),
+	  N_("option for selected merge strategy"),
+	  0, parse_opt_pass_argv_array },
+	{ OPTION_CALLBACK, 'S', "gpg-sign", &opt_gpg_sign, N_("key-id"),
+	  N_("GPG sign commit"),
+	  PARSE_OPT_OPTARG, parse_opt_pass_strbuf },
+
 	OPT_END()
 };
 
@@ -100,6 +155,26 @@ static int run_merge(void)
 	if (opt_progress.len)
 		argv_array_push(&args, opt_progress.buf);
 
+	/* Options passed to git-merge */
+	if (opt_diffstat.len)
+		argv_array_push(&args, opt_diffstat.buf);
+	if (opt_log.len)
+		argv_array_push(&args, opt_log.buf);
+	if (opt_squash.len)
+		argv_array_push(&args, opt_squash.buf);
+	if (opt_commit.len)
+		argv_array_push(&args, opt_commit.buf);
+	if (opt_edit.len)
+		argv_array_push(&args, opt_edit.buf);
+	if (opt_ff.len)
+		argv_array_push(&args, opt_ff.buf);
+	if (opt_verify_signatures.len)
+		argv_array_push(&args, opt_verify_signatures.buf);
+	argv_array_pushv(&args, opt_strategies.argv);
+	argv_array_pushv(&args, opt_strategy_opts.argv);
+	if (opt_gpg_sign.len)
+		argv_array_push(&args, opt_gpg_sign.buf);
+
 	argv_array_push(&args, "FETCH_HEAD");
 	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
 	argv_array_clear(&args);
-- 
2.1.4

[PATCH v2 08/19] pull: pass git-fetch's options to git-fetch

[Paul Tan]

Since eb2a8d9 (pull: handle git-fetch's options as well, 2015-06-02),
git-pull knows about and handles git-fetch's options, passing them to
git-fetch. Re-implement this behavior.

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

Notes:
    v2
    
    * Use parse_opt_parse_strbuf()

 builtin/pull.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5f08634..0b66b43 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -32,6 +32,21 @@ static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
 static struct strbuf opt_gpg_sign = STRBUF_INIT;
 
+/* Options passed to git-fetch */
+static struct strbuf opt_all = STRBUF_INIT;
+static struct strbuf opt_append = STRBUF_INIT;
+static struct strbuf opt_upload_pack = STRBUF_INIT;
+static int opt_force;
+static struct strbuf opt_tags = STRBUF_INIT;
+static struct strbuf opt_prune = STRBUF_INIT;
+static struct strbuf opt_recurse_submodules = STRBUF_INIT;
+static int opt_dry_run;
+static struct strbuf opt_keep = STRBUF_INIT;
+static struct strbuf opt_depth = STRBUF_INIT;
+static struct strbuf opt_unshallow = STRBUF_INIT;
+static struct strbuf opt_update_shallow = STRBUF_INIT;
+static struct strbuf opt_refmap = STRBUF_INIT;
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -82,6 +97,46 @@ static struct option pull_options[] = {
 	  N_("GPG sign commit"),
 	  PARSE_OPT_OPTARG, parse_opt_pass_strbuf },
 
+	/* 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_pass_strbuf },
+	{ OPTION_CALLBACK, 'a', "append", &opt_append, 0,
+	  N_("append to .git/FETCH_HEAD instead of overwriting"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "upload-pack", &opt_upload_pack, N_("path"),
+	  N_("path to upload pack on remote end"),
+	  0, parse_opt_pass_strbuf },
+	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_pass_strbuf },
+	{ OPTION_CALLBACK, 'p', "prune", &opt_prune, 0,
+	  N_("prune remote-tracking branches no longer on remote"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "recurse-submodules", &opt_recurse_submodules,
+	  N_("on-demand"),
+	  N_("control recursive fetching of submodules"),
+	  PARSE_OPT_OPTARG, parse_opt_pass_strbuf },
+	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_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "depth", &opt_depth, N_("depth"),
+	  N_("deepen history of shallow clone"),
+	  0, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "unshallow", &opt_unshallow, 0,
+	  N_("convert to a complete repository"),
+	  PARSE_OPT_NONEG | PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "update-shallow", &opt_update_shallow, 0,
+	  N_("accept refs that update .git/shallow"),
+	  PARSE_OPT_NOARG, parse_opt_pass_strbuf },
+	{ OPTION_CALLBACK, 0, "refmap", &opt_refmap, N_("refmap"),
+	  N_("specify fetch refmap"),
+	  PARSE_OPT_NONEG, parse_opt_pass_strbuf },
+
 	OPT_END()
 };
 
@@ -100,6 +155,16 @@ static void argv_push_verbosity(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.
@@ -131,6 +196,33 @@ static int run_fetch(const char *repo, const char **refspecs)
 	if (opt_progress.len)
 		argv_array_push(&args, opt_progress.buf);
 
+	/* Options passed to git-fetch */
+	if (opt_all.len)
+		argv_array_push(&args, opt_all.buf);
+	if (opt_append.len)
+		argv_array_push(&args, opt_append.buf);
+	if (opt_upload_pack.len)
+		argv_array_push(&args, opt_upload_pack.buf);
+	argv_push_force(&args);
+	if (opt_tags.len)
+		argv_array_push(&args, opt_tags.buf);
+	if (opt_prune.len)
+		argv_array_push(&args, opt_prune.buf);
+	if (opt_recurse_submodules.len)
+		argv_array_push(&args, opt_recurse_submodules.buf);
+	if (opt_dry_run)
+		argv_array_push(&args, "--dry-run");
+	if (opt_keep.len)
+		argv_array_push(&args, opt_keep.buf);
+	if (opt_depth.len)
+		argv_array_push(&args, opt_depth.buf);
+	if (opt_unshallow.len)
+		argv_array_push(&args, opt_unshallow.buf);
+	if (opt_update_shallow.len)
+		argv_array_push(&args, opt_update_shallow.buf);
+	if (opt_refmap.len)
+		argv_array_push(&args, opt_refmap.buf);
+
 	if (repo)
 		argv_array_push(&args, repo);
 	while (*refspecs)
@@ -199,5 +291,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();
 }
-- 
2.1.4

[PATCH v2 09/19] 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.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * Switched to using fprintf_ln() which will append the trailing newline
      at the end for us.
    
    * The die_no_merge_candidates() code has been reorganized a bit to
      support the later patch which will tweak the messages to be aware of
      git-pull --rebase.

 builtin/pull.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 0b66b43..42a061d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -10,6 +10,8 @@
 #include "parse-options.h"
 #include "exec_cmd.h"
 #include "run-command.h"
+#include "sha1-array.h"
+#include "remote.h"
 
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repository> [<refspec>...]]"),
@@ -165,6 +167,111 @@ static void argv_push_force(struct argv_array *arr)
 }
 
 /**
+ * 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);
+}
+
+/**
+ * Used by die_no_merge_candidates() as a for_each_remote() callback to
+ * retrieve the name of the remote if the repository only has one remote.
+ */
+static int get_only_remote(struct remote *remote, void *cb_data)
+{
+	const char **remote_name = cb_data;
+
+	if (*remote_name)
+		return -1;
+
+	*remote_name = remote->name;
+	return 0;
+}
+
+/**
+ * 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_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
+		fprintf_ln(stderr, _("Generally this means that you provided a wildcard refspec which had no\n"
+					"matches on the remote end."));
+	} else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
+		fprintf_ln(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."),
+			repo);
+	} else if (!curr_branch) {
+		fprintf_ln(stderr, _("You are not currently on a branch."));
+		fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
+		fprintf_ln(stderr, _("See git-pull(1) for details."));
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, "    git pull <remote> <branch>");
+		fprintf(stderr, "\n");
+	} else if (!curr_branch->merge_nr) {
+		const char *remote_name = NULL;
+
+		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
+			remote_name = "<remote>";
+
+		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
+		fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
+		fprintf_ln(stderr, _("See git-pull(1) for details."));
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, "    git pull <remote> <branch>");
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, _("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"),
+				remote_name, curr_branch->name);
+	} else
+		fprintf_ln(stderr, _("Your configuration specifies to merge with the ref '%s'\n"
+			"from the remote, but no such ref was fetched."),
+			*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`
  * is not provided in argv, it is set to NULL.
@@ -276,6 +383,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;
 
 	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
 		const char *path = mkpath("%s/git-pull", git_exec_path());
@@ -294,5 +402,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();
 }
-- 
2.1.4

[PATCH v2 10/19] 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>
---

Notes:
    v2
    
    * Yup, I did mean strcmp(value, "only")

 builtin/pull.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 42a061d..1c1883d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -167,6 +167,32 @@ static void argv_push_force(struct argv_array *arr)
 }
 
 /**
+ * If pull.ff is "true", sets sb to "--ff". If pull.ff is "false", sets sb to
+ * "--no-ff". If pull.ff is "only", sets sb to "--ff-only". If pull.ff is
+ * set to an invalid value, die with an error.
+ */
+static void config_get_ff(struct strbuf *sb)
+{
+	const char *value;
+
+	if (git_config_get_value("pull.ff", &value))
+		return;
+	switch (git_config_maybe_bool("pull.ff", value)) {
+		case 0:
+			strbuf_addstr(sb, "--no-ff");
+			return;
+		case 1:
+			strbuf_addstr(sb, "--ff");
+			return;
+	}
+	if (!strcmp(value, "only")) {
+		strbuf_addstr(sb, "--ff-only");
+		return;
+	}
+	die(_("Invalid value for pull.ff: %s"), value);
+}
+
+/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
@@ -396,6 +422,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
+	if (!opt_ff.len)
+		config_get_ff(&opt_ff);
+
 	if (run_fetch(repo, refspecs))
 		return 1;
 
-- 
2.1.4

[PATCH v2 11/19] pull: check if in unresolved merge state

[Paul Tan]

Since 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 +++++++++
 3 files changed, 18 insertions(+)

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 1c1883d..703fc1d 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"
 
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repository> [<refspec>...]]"),
@@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	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.len)
 		config_get_ff(&opt_ff);
 
-- 
2.1.4

[PATCH v2 12/19] 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 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 703fc1d..8db0db2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -411,6 +411,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];
 
 	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
 		const char *path = mkpath("%s/git-pull", git_exec_path());
@@ -434,12 +435,41 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!opt_ff.len)
 		config_get_ff(&opt_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)
-- 
2.1.4

[PATCH v2 13/19] 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 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 8db0db2..f0d4710 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"
 
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repository> [<refspec>...]]"),
@@ -367,6 +368,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)
@@ -475,5 +497,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();
 }
-- 
2.1.4

[PATCH v2 14/19] 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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * Don't use strbuf_rtrim().

 builtin/pull.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index f0d4710..c44cc90 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -169,6 +169,25 @@ 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++) {
+		if (i)
+			strbuf_addch(&msg, ' ');
+		strbuf_addstr(&msg, argv[i]);
+	}
+
+	setenv("GIT_REFLOG_ACTION", msg.buf, 0);
+
+	strbuf_release(&msg);
+}
+
+/*
  * If pull.ff is "true", sets sb to "--ff". If pull.ff is "false", sets sb to
  * "--no-ff". If pull.ff is "only", sets sb to "--ff-only". If pull.ff is
  * set to an invalid value, die with an error.
@@ -442,6 +461,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die_errno("could not exec %s", path);
 	}
 
+	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);
-- 
2.1.4

[PATCH v2 15/19] 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().

Helped-by: Stefan Beller <sbeller@google.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * enum rebase_type gained a new value REBASE_INVALID = -1 to represent
      invalid values passed to --rebase. We can't immediately die() on
      invalid values because we need parse_options() to show the git-pull
      usage as well.
    
    * parse_config_rebase() now returns an enum rebase_type to make it clear
      what kind of value it returns.
    
    * parse_config_rebase() now does not depend on git_config_maybe_bool()
      returning 1 for true and 0 for false. If it returns 0, it is
      REBASE_FALSE. If it returns any true value >=0, it is interpreted as
      REBASE_TRUE.
    
    * get_merge_branch_1() has been renamed to get_upstream_branch() to be
      consistent with the similar get_upstream_branch() in sha1_name.c.
    
    * get_merge_branch_2() has been renamed to get_tracking_branch() to
      indicate what it is doing: deriving the remote tracking branch from
      the refspec.
    
    * Various small variable re-names and docstring tweaks. Hopefully
      everything reads better now.

 builtin/pull.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 237 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index c44cc90..7645937 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -15,6 +15,45 @@
 #include "dir.h"
 #include "refs.h"
 
+enum rebase_type {
+	REBASE_INVALID = -1,
+	REBASE_FALSE = 0,
+	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 enum rebase_type parse_config_rebase(const char *value)
+{
+	int v = git_config_maybe_bool("pull.rebase", value);
+	if (!v)
+		return REBASE_FALSE;
+	else if (v >= 0)
+		return REBASE_TRUE;
+	else if (!strcmp(value, "preserve"))
+		return REBASE_PRESERVE;
+	return REBASE_INVALID;
+}
+
+/**
+ * Callback for --rebase, which parses arg with parse_config_rebase().
+ */
+static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
+{
+	enum rebase_type *value = opt->value;
+
+	if (arg)
+		*value = parse_config_rebase(arg);
+	else
+		*value = unset ? REBASE_FALSE : REBASE_TRUE;
+	return *value == REBASE_INVALID ? -1 : 0;
+}
+
 static const char * const pull_usage[] = {
 	N_("git pull [options] [<repository> [<refspec>...]]"),
 	NULL
@@ -24,7 +63,8 @@ static const char * const pull_usage[] = {
 static int opt_verbosity;
 static struct strbuf opt_progress = STRBUF_INIT;
 
-/* Options passed to git-merge */
+/* Options passed to git-merge or git-rebase */
+static enum rebase_type opt_rebase;
 static struct strbuf opt_diffstat = STRBUF_INIT;
 static struct strbuf opt_log = STRBUF_INIT;
 static struct strbuf opt_squash = STRBUF_INIT;
@@ -58,8 +98,12 @@ static struct option pull_options[] = {
 	  N_("force progress reporting"),
 	  PARSE_OPT_NOARG, parse_opt_pass_strbuf},
 
-	/* 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 },
 	{ 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_pass_strbuf },
@@ -448,11 +492,194 @@ static int run_merge(void)
 	return ret;
 }
 
+/**
+ * Returns remote's upstream branch for the current branch. If remote is NULL,
+ * the current branch's configured default remote is used. Returns NULL if
+ * `remote` does not name a valid remote, HEAD does not point to a branch,
+ * remote is not the branch's configured remote or the branch does not have any
+ * configured upstream branch.
+ */
+static char *get_upstream_branch(const char *remote)
+{
+	struct remote *rm;
+	struct branch *curr_branch;
+
+	rm = remote_get(remote);
+	if (!rm)
+		return NULL;
+
+	curr_branch = branch_get("HEAD");
+	if (!curr_branch)
+		return NULL;
+
+	if (curr_branch->remote != rm)
+		return NULL;
+
+	if (!curr_branch->merge_nr)
+		return NULL;
+
+	return xstrdup(curr_branch->merge[0]->dst);
+}
+
+/**
+ * Derives the remote tracking branch from the remote and refspec.
+ *
+ * FIXME: The current implementation assumes the default mapping of
+ * refs/heads/<branch_name> to refs/remotes/<remote_name>/<branch_name>.
+ */
+static char *get_tracking_branch(const char *remote, const char *refspec)
+{
+	struct refspec *spec;
+	const char *spec_src;
+	char *merge_branch;
+
+	spec = parse_fetch_refspec(1, &refspec);
+	spec_src = spec->src;
+	if (!*spec_src || !strcmp(spec_src, "HEAD"))
+		spec_src = "HEAD";
+	else if (skip_prefix(spec_src, "heads/", &spec_src))
+		;
+	else if (skip_prefix(spec_src, "refs/heads/", &spec_src))
+		;
+	else if (starts_with(spec_src, "refs/") ||
+		starts_with(spec_src, "tags/") ||
+		starts_with(spec_src, "remotes/"))
+		spec_src = "";
+
+	if (*spec_src) {
+		if (!strcmp(remote, "."))
+			merge_branch = mkpathdup("refs/heads/%s", spec_src);
+		else
+			merge_branch = mkpathdup("refs/remotes/%s/%s", remote, spec_src);
+	} else
+		merge_branch = NULL;
+
+	free_refspec(1, spec);
+	return merge_branch;
+}
+
+/**
+ * Given the repo and refspecs, sets fork_point to the point at which the
+ * current branch forked from its remote tracking 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_branch;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	curr_branch = branch_get("HEAD");
+	if (!curr_branch)
+		return -1;
+
+	if (refspec)
+		remote_branch = get_tracking_branch(repo, refspec);
+	else
+		remote_branch = get_upstream_branch(repo);
+
+	if (!remote_branch)
+		return -1;
+
+	argv_array_pushl(&cp.args, "merge-base", "--fork-point",
+			remote_branch, curr_branch->name, NULL);
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.git_cmd = 1;
+
+	ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ);
+	if (ret)
+		goto cleanup;
+
+	ret = get_sha1_hex(sb.buf, fork_point);
+	if (ret)
+		goto cleanup;
+
+cleanup:
+	free(remote_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.len)
+		argv_array_push(&args, opt_diffstat.buf);
+	argv_array_pushv(&args, opt_strategies.argv);
+	argv_array_pushv(&args, opt_strategy_opts.argv);
+	if (opt_gpg_sign.len)
+		argv_array_push(&args, opt_gpg_sign.buf);
+
+	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_USE_BUILTIN_PULL")) {
 		const char *path = mkpath("%s/git-pull", git_exec_path());
@@ -482,6 +709,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;
 
@@ -523,6 +754,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();
 }
-- 
2.1.4

[PATCH v2 16/19] 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.

Helped-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

Notes:
    v2
    
    * Previously, config_get_rebase() attempted to do too many things. It:
    
      1. Checked if there was a configuration for
         branch.$curr_branch.rebase, and if not, then pull.rebase
    
      2. Parsed the configuration value and died if the value is invalid.
    
      These 2 functions have been split into config_get_rebase_default() and
      config_get_rebase() respectively.

 builtin/pull.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 7645937..9759720 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -258,6 +258,52 @@ static void config_get_ff(struct strbuf *sb)
 }
 
 /**
+ * Sets `value` to the REBASE_* value for the configuration setting `key`.
+ * Returns 0 is `key` exists, -1 if it does not. Dies if the value of `key` is
+ * invalid.
+ */
+static int config_get_rebase(const char *key, enum rebase_type *value)
+{
+	const char *str_value;
+
+	if (git_config_get_value(key, &str_value))
+		return -1;
+	*value = parse_config_rebase(str_value);
+	if (*value == REBASE_INVALID)
+		die(_("Invalid value for %s: %s"), key, str_value);
+	return 0;
+}
+
+/**
+ * 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 enum rebase_type config_get_rebase_default(void)
+{
+	struct branch *curr_branch = branch_get("HEAD");
+	enum rebase_type rebase_type;
+
+	if (curr_branch) {
+		int ret;
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
+		ret = config_get_rebase(sb.buf, &rebase_type);
+		strbuf_release(&sb);
+		if (!ret)
+			return rebase_type;
+	}
+
+	if (!config_get_rebase("pull.rebase", &rebase_type))
+		return rebase_type;
+
+	return REBASE_FALSE;
+}
+
+/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
@@ -691,6 +737,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_default();
+
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
-- 
2.1.4

[PATCH v2 17/19] 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 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9759720..f5d437a 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"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -304,6 +306,73 @@ static enum rebase_type config_get_rebase_default(void)
 }
 
 /**
+ * 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);
+}
+
+/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
@@ -757,9 +826,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;
-- 
2.1.4

[PATCH v2 18/19] 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>
---

Notes:
    v2
    
    * Decided to use fprintf_ln() for the sake of code consistency, and for
      the added trailing newline.

 builtin/pull.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index f5d437a..4e1ab5b 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -439,7 +439,10 @@ 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_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
+		if (opt_rebase)
+			fprintf_ln(stderr, _("There is no candidate for rebasing against among the refs that you just fetched."));
+		else
+			fprintf_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
 		fprintf_ln(stderr, _("Generally this means that you provided a wildcard refspec which had no\n"
 					"matches on the remote end."));
 	} else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
@@ -449,7 +452,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			repo);
 	} else if (!curr_branch) {
 		fprintf_ln(stderr, _("You are not currently on a branch."));
-		fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
+		if (opt_rebase)
+			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
+		else
+			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
 		fprintf_ln(stderr, _("See git-pull(1) for details."));
 		fprintf(stderr, "\n");
 		fprintf_ln(stderr, "    git pull <remote> <branch>");
@@ -461,7 +467,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			remote_name = "<remote>";
 
 		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
-		fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
+		if (opt_rebase)
+			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
+		else
+			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
 		fprintf_ln(stderr, _("See git-pull(1) for details."));
 		fprintf(stderr, "\n");
 		fprintf_ln(stderr, "    git pull <remote> <branch>");
-- 
2.1.4

[PATCH v2 19/19] pull: remove redirection to git-pull.sh

[Paul Tan]

At the beginning of the rewrite of git-pull.sh to C, we introduced a
redirection to git-pull.sh if the environment variable
_GIT_USE_BUILTIN_PULL was not defined in order to not break test scripts
that relied on a functional git-pull.

Now that all of git-pull's functionality has been re-implemented in
builtin/pull.c, remove this redirection, and retire the old git-pull.sh
into contrib/examples/.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile                                    | 1 -
 builtin/pull.c                              | 7 -------
 git-pull.sh => contrib/examples/git-pull.sh | 0
 3 files changed, 8 deletions(-)
 rename git-pull.sh => contrib/examples/git-pull.sh (100%)

diff --git a/Makefile b/Makefile
index 2057a9d..67cef1c 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
diff --git a/builtin/pull.c b/builtin/pull.c
index 4e1ab5b..dad49cf 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -805,13 +805,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
 	unsigned char rebase_fork_point[GIT_SHA1_RAWSZ];
 
-	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
-		const char *path = mkpath("%s/git-pull", git_exec_path());
-
-		if (sane_execvp(path, (char**) argv) < 0)
-			die_errno("could not exec %s", path);
-	}
-
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
diff --git a/git-pull.sh b/contrib/examples/git-pull.sh
similarity index 100%
rename from git-pull.sh
rename to contrib/examples/git-pull.sh
-- 
2.1.4

Re: [PATCH v2 17/19] pull --rebase: exit early when the working directory is dirty

[Kevin Daudt]

On Wed, Jun 03, 2015 at 02:49:01PM +0800, Paul Tan wrote:
> Re-implement the behavior introduced by f9189cf (pull --rebase: exit
> early when the working directory is dirty, 2008-05-21).

When the option rebase.autoStash is set, it should not be necessary to
die in this case. See also this[1] patch I'm working on.

[1]: http://article.gmane.org/gmane.comp.version-control.git/270612

Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

[Stefan Beller]

On Tue, Jun 2, 2015 at 11:48 PM, Paul Tan <pyokagan@gmail.com> wrote:
> Certain git commands, such as git-pull, are simply wrappers around other
> git commands like git-fetch, git-merge and git-rebase. As such, these
> wrapper commands will typically need to "pass through" command-line
> options of the commands they wrap.
>
> Implement the parse_opt_pass_argv_array() parse-options callback, which
> will reconstruct all the provided command-line options into an
> argv_array, such that it can be passed to another git command. This is
> useful for passing command-line options that can be specified multiple
> times.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     v2
>
>     * This function is a requirement for the rewrite of git-am to handle
>       passing git-apply's options to git-apply. Since it would be
>       implemented anyway I thought it would be good if git-pull could take
>       advantage of it as well to handle --strategy and --strategy-option.
>
>  parse-options-cb.c | 32 ++++++++++++++++++++++++++++++++
>  parse-options.h    |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 5b1dbcf..7330506 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -4,6 +4,7 @@
>  #include "commit.h"
>  #include "color.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>
>  /*----- some often used options -----*/
>
> @@ -163,3 +164,34 @@ int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
>
>         return 0;
>  }
> +
> +/**
> + * For an option opt, recreate the command-line option, appending it to
> + * opt->value which must be a argv_array. This is useful when we need to pass
> + * the command-line option, which can be specified multiple times, to another
> + * command.
> + */
> +int parse_opt_pass_argv_array(const struct option *opt, const char *arg, int unset)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       struct argv_array *opt_value = opt->value;
> +
> +       if (opt->long_name) {
> +               strbuf_addstr(&sb, unset ? "--no-" : "--");
> +               strbuf_addstr(&sb, opt->long_name);
> +               if (arg) {
> +                       strbuf_addch(&sb, '=');
> +                       strbuf_addstr(&sb, arg);
> +               }
> +       } else if (opt->short_name && !unset) {
> +               strbuf_addch(&sb, '-');
> +               strbuf_addch(&sb, opt->short_name);
> +               if (arg)
> +                       strbuf_addstr(&sb, arg);
> +       } else
> +               return -1;
> +
> +       argv_array_push(opt_value, sb.buf);
> +       strbuf_release(&sb);
> +       return 0;
> +}
> diff --git a/parse-options.h b/parse-options.h
> index 1d21398..b663f87 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -225,6 +225,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);
>  extern int parse_opt_noop_cb(const struct option *, const char *, int);
>  extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
> +extern int parse_opt_pass_argv_array(const struct option *, const char *, int);
>
>  #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
>  #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
> --
> 2.1.4
>

This patch looks very similar to the first one, so I wonder if the
combination of these
could be written as:

static int parse_opt_pass_strbuf_internal(const struct option *opt,
const char *arg, int unset, struct strbuf *sb)
{
       if (opt->long_name) {
               strbuf_addstr(sb, unset ? "--no-" : "--");
               strbuf_addstr(sb, opt->long_name);
               if (arg) {
                       strbuf_addch(sb, '=');
                       strbuf_addstr(sb, arg);
               }
       } else if (opt->short_name && !unset) {
               strbuf_addch(sb, '-');
               strbuf_addch(sb, opt->short_name);
               if (arg)
                       strbuf_addstr(sb, arg);
       } else
               return -1;
      return 0;
}

/**
 * For an option opt, recreates the command-line option in opt->value which
 * must be an strbuf. This is useful when we need to pass the command-line
 * option to another command.
 */
int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
{
       struct strbuf *sb = opt->value;
       strbuf_reset(sb);
       return parse_opt_pass_strbuf_internal(...);
}

/**
 * For an option opt, recreate the command-line option, appending it to
 * opt->value which must be a argv_array. This is useful when we need to pass
 * the command-line option, which can be specified multiple times, to another
 * command.
 */
int parse_opt_pass_argv_array(const struct option *opt, const char
*arg, int unset)
{
       struct strbuf sb = STRBUF_INIT;
       struct argv_array *opt_value = opt->value;

       int ret = parse_opt_pass_strbuf_internal(...)

       argv_array_push(opt_value, sb.buf);
       strbuf_release(&sb);
       return 0;
}

Re: [PATCH v2 08/19] pull: pass git-fetch's options to git-fetch

[Stefan Beller]

On Tue, Jun 2, 2015 at 11:48 PM, Paul Tan <pyokagan@gmail.com> wrote:
> Since eb2a8d9 (pull: handle git-fetch's options as well, 2015-06-02),
> git-pull knows about and handles git-fetch's options, passing them to
> git-fetch. Re-implement this behavior.
>
> 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>
> ---
>
> Notes:
>     v2
>
>     * Use parse_opt_parse_strbuf()
>
>  builtin/pull.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 5f08634..0b66b43 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -32,6 +32,21 @@ static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
>  static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
>  static struct strbuf opt_gpg_sign = STRBUF_INIT;
>
> +/* Options passed to git-fetch */
> +static struct strbuf opt_all = STRBUF_INIT;
> +static struct strbuf opt_append = STRBUF_INIT;
> +static struct strbuf opt_upload_pack = STRBUF_INIT;
> +static int opt_force;
> +static struct strbuf opt_tags = STRBUF_INIT;
> +static struct strbuf opt_prune = STRBUF_INIT;
> +static struct strbuf opt_recurse_submodules = STRBUF_INIT;
> +static int opt_dry_run;
> +static struct strbuf opt_keep = STRBUF_INIT;
> +static struct strbuf opt_depth = STRBUF_INIT;
> +static struct strbuf opt_unshallow = STRBUF_INIT;
> +static struct strbuf opt_update_shallow = STRBUF_INIT;
> +static struct strbuf opt_refmap = STRBUF_INIT;
> +
>  static struct option pull_options[] = {
>         /* Shared options */
>         OPT__VERBOSITY(&opt_verbosity),
> @@ -82,6 +97,46 @@ static struct option pull_options[] = {
>           N_("GPG sign commit"),
>           PARSE_OPT_OPTARG, parse_opt_pass_strbuf },
>
> +       /* 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_pass_strbuf },
> +       { OPTION_CALLBACK, 'a', "append", &opt_append, 0,
> +         N_("append to .git/FETCH_HEAD instead of overwriting"),
> +         PARSE_OPT_NOARG, parse_opt_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "upload-pack", &opt_upload_pack, N_("path"),
> +         N_("path to upload pack on remote end"),
> +         0, parse_opt_pass_strbuf },
> +       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_pass_strbuf },
> +       { OPTION_CALLBACK, 'p', "prune", &opt_prune, 0,
> +         N_("prune remote-tracking branches no longer on remote"),
> +         PARSE_OPT_NOARG, parse_opt_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "recurse-submodules", &opt_recurse_submodules,
> +         N_("on-demand"),
> +         N_("control recursive fetching of submodules"),
> +         PARSE_OPT_OPTARG, parse_opt_pass_strbuf },
> +       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_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "depth", &opt_depth, N_("depth"),
> +         N_("deepen history of shallow clone"),
> +         0, parse_opt_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "unshallow", &opt_unshallow, 0,
> +         N_("convert to a complete repository"),
> +         PARSE_OPT_NONEG | PARSE_OPT_NOARG, parse_opt_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "update-shallow", &opt_update_shallow, 0,
> +         N_("accept refs that update .git/shallow"),
> +         PARSE_OPT_NOARG, parse_opt_pass_strbuf },
> +       { OPTION_CALLBACK, 0, "refmap", &opt_refmap, N_("refmap"),
> +         N_("specify fetch refmap"),
> +         PARSE_OPT_NONEG, parse_opt_pass_strbuf },
> +
>         OPT_END()
>  };
>
> @@ -100,6 +155,16 @@ static void argv_push_verbosity(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)

This made me chuckle despite the formatting,
as we referred to it as the limes operator in school for fun

    #define limes while
    limes (n --> 0) { // n goes towards zero
        ...
    }

A quick
    grep -r "--" -- *.c |grep while
shows we actually use this quite a lot, though the "> 0" is
omitted quite often.

> +               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.
> @@ -131,6 +196,33 @@ static int run_fetch(const char *repo, const char **refspecs)
>         if (opt_progress.len)
>                 argv_array_push(&args, opt_progress.buf);
>
> +       /* Options passed to git-fetch */
> +       if (opt_all.len)
> +               argv_array_push(&args, opt_all.buf);
> +       if (opt_append.len)
> +               argv_array_push(&args, opt_append.buf);
> +       if (opt_upload_pack.len)
> +               argv_array_push(&args, opt_upload_pack.buf);
> +       argv_push_force(&args);
> +       if (opt_tags.len)
> +               argv_array_push(&args, opt_tags.buf);
> +       if (opt_prune.len)
> +               argv_array_push(&args, opt_prune.buf);
> +       if (opt_recurse_submodules.len)
> +               argv_array_push(&args, opt_recurse_submodules.buf);
> +       if (opt_dry_run)
> +               argv_array_push(&args, "--dry-run");
> +       if (opt_keep.len)
> +               argv_array_push(&args, opt_keep.buf);
> +       if (opt_depth.len)
> +               argv_array_push(&args, opt_depth.buf);
> +       if (opt_unshallow.len)
> +               argv_array_push(&args, opt_unshallow.buf);
> +       if (opt_update_shallow.len)
> +               argv_array_push(&args, opt_update_shallow.buf);
> +       if (opt_refmap.len)
> +               argv_array_push(&args, opt_refmap.buf);
> +
>         if (repo)
>                 argv_array_push(&args, repo);
>         while (*refspecs)
> @@ -199,5 +291,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();
>  }
> --
> 2.1.4
>

Re: [PATCH v2 18/19] pull --rebase: error on no merge candidate cases

[Stefan Beller]

On Tue, Jun 2, 2015 at 11:49 PM, 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>
> ---
>
> Notes:
>     v2
>
>     * Decided to use fprintf_ln() for the sake of code consistency, and for
>       the added trailing newline.
>
>  builtin/pull.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f5d437a..4e1ab5b 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -439,7 +439,10 @@ 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_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
> +               if (opt_rebase)
> +                       fprintf_ln(stderr, _("There is no candidate for rebasing against among the refs that you just fetched."));
> +               else
> +                       fprintf_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
>                 fprintf_ln(stderr, _("Generally this means that you provided a wildcard refspec which had no\n"
>                                         "matches on the remote end."));
>         } else if (repo && curr_branch && (!remote || strcmp(repo, remote))) {
> @@ -449,7 +452,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>                         repo);
>         } else if (!curr_branch) {
>                 fprintf_ln(stderr, _("You are not currently on a branch."));
> -               fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
> +               if (opt_rebase)
> +                       fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +               else
> +                       fprintf_ln(stderr, _("Please specify which branch you want to merge with."));


Now that you're using fprintf you could make use of its formatting
capabilities, but then it occurred to me
it's translated strings, so it's most likely better to not make it
concise but rather easier for the translators
by having each sentence written out in full.

>                 fprintf_ln(stderr, _("See git-pull(1) for details."));
>                 fprintf(stderr, "\n");
>                 fprintf_ln(stderr, "    git pull <remote> <branch>");
> @@ -461,7 +467,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>                         remote_name = "<remote>";
>
>                 fprintf_ln(stderr, _("There is no tracking information for the current branch."));
> -               fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
> +               if (opt_rebase)
> +                       fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +               else
> +                       fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
>                 fprintf_ln(stderr, _("See git-pull(1) for details."));
>                 fprintf(stderr, "\n");
>                 fprintf_ln(stderr, "    git pull <remote> <branch>");
> --
> 2.1.4
>

Re: [PATCH v2 19/19] pull: remove redirection to git-pull.sh

[Stefan Beller]

On Tue, Jun 2, 2015 at 11:49 PM, Paul Tan <pyokagan@gmail.com> wrote:
> At the beginning of the rewrite of git-pull.sh to C, we introduced a
> redirection to git-pull.sh if the environment variable
> _GIT_USE_BUILTIN_PULL was not defined in order to not break test scripts
> that relied on a functional git-pull.
>
> Now that all of git-pull's functionality has been re-implemented in
> builtin/pull.c, remove this redirection, and retire the old git-pull.sh
> into contrib/examples/.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>

The whole series was a pleasant read.

Thanks,
Stefan

> ---
>  Makefile                                    | 1 -
>  builtin/pull.c                              | 7 -------
>  git-pull.sh => contrib/examples/git-pull.sh | 0
>  3 files changed, 8 deletions(-)
>  rename git-pull.sh => contrib/examples/git-pull.sh (100%)
>
> diff --git a/Makefile b/Makefile
> index 2057a9d..67cef1c 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
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 4e1ab5b..dad49cf 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -805,13 +805,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
>         unsigned char rebase_fork_point[GIT_SHA1_RAWSZ];
>
> -       if (!getenv("_GIT_USE_BUILTIN_PULL")) {
> -               const char *path = mkpath("%s/git-pull", git_exec_path());
> -
> -               if (sane_execvp(path, (char**) argv) < 0)
> -                       die_errno("could not exec %s", path);
> -       }
> -
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
>
> diff --git a/git-pull.sh b/contrib/examples/git-pull.sh
> similarity index 100%
> rename from git-pull.sh
> rename to contrib/examples/git-pull.sh
> --
> 2.1.4
>

Re: [PATCH v2 01/19] parse-options-cb: implement parse_opt_pass_strbuf()

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index be8c413..5b1dbcf 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -134,3 +134,32 @@ int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
>  {
>  	return 0;
>  }
> +
> +/**
> + * For an option opt, recreates the command-line option in opt->value which
> + * must be an strbuf. This is useful when we need to pass the command-line
> + * option to another command.
> + */

This is to be used to create a single argument, not an entire
command line to be executed via run_command() or a shell, right?
It wasn't very clear from the above command.

If the same option is given more than once, the earlier ones are
discarded and the last one wins.  That may be OK for your expected
callers, and I do not think this needs to have an option to instead
accumulate them, but it needs to be made clear in the comment and
the API documentation for future developers who want to use the
parse-options API.

> +int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
> +{
> +	struct strbuf *sb = opt->value;
> +
> +	strbuf_reset(sb);
> +
> +	if (opt->long_name) {
> +		strbuf_addstr(sb, unset ? "--no-" : "--");
> +		strbuf_addstr(sb, opt->long_name);
> +		if (arg) {
> +			strbuf_addch(sb, '=');
> +			strbuf_addstr(sb, arg);
> +		}
> +	} else if (opt->short_name && !unset) {
> +		strbuf_addch(sb, '-');
> +		strbuf_addch(sb, opt->short_name);
> +		if (arg)
> +			strbuf_addstr(sb, arg);
> +	} else
> +		return -1;
> +
> +	return 0;
> +}
> diff --git a/parse-options.h b/parse-options.h
> index c71e9da..1d21398 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -224,6 +224,7 @@ extern int parse_opt_with_commit(const struct option *, const char *, int);
>  extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);
>  extern int parse_opt_noop_cb(const struct option *, const char *, int);
> +extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
>  
>  #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
>  #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))

Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Certain git commands, such as git-pull, are simply wrappers around other
> git commands like git-fetch, git-merge and git-rebase. As such, these
> wrapper commands will typically need to "pass through" command-line
> options of the commands they wrap.
>
> Implement the parse_opt_pass_argv_array() parse-options callback, which
> will reconstruct all the provided command-line options into an
> argv_array, such that it can be passed to another git command. This is
> useful for passing command-line options that can be specified multiple
> times.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     v2
>     
>     * This function is a requirement for the rewrite of git-am to handle
>       passing git-apply's options to git-apply. Since it would be
>       implemented anyway I thought it would be good if git-pull could take
>       advantage of it as well to handle --strategy and --strategy-option.
>
>  parse-options-cb.c | 32 ++++++++++++++++++++++++++++++++
>  parse-options.h    |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 5b1dbcf..7330506 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -4,6 +4,7 @@
>  #include "commit.h"
>  #include "color.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>  
>  /*----- some often used options -----*/
>  
> @@ -163,3 +164,34 @@ int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
>  
>  	return 0;
>  }
> +
> +/**
> + * For an option opt, recreate the command-line option, appending it to
> + * opt->value which must be a argv_array. This is useful when we need to pass
> + * the command-line option, which can be specified multiple times, to another
> + * command.
> + */

Almost the same comment as 01/19 applies to this comment.

I think it makes good sense to have two variants, one that lets the
last one win and pass only that last one (i.e. 01/19) and the other
that accumulates them into an argv_array (i.e. this one).  But it
feels iffy, given that the "acculate" version essentially creates an
array of (char *), to make "the last one wins, leaving a single
string" to use strbuf.  I'd find it much more understandable if 01/19
took (char **) as opt->value instead of a strbuf.

In any case, these two need to be added as a related pair to the API
documentation.

Thanks.

> +int parse_opt_pass_argv_array(const struct option *opt, const char *arg, int unset)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct argv_array *opt_value = opt->value;
> +
> +	if (opt->long_name) {
> +		strbuf_addstr(&sb, unset ? "--no-" : "--");
> +		strbuf_addstr(&sb, opt->long_name);
> +		if (arg) {
> +			strbuf_addch(&sb, '=');
> +			strbuf_addstr(&sb, arg);
> +		}
> +	} else if (opt->short_name && !unset) {
> +		strbuf_addch(&sb, '-');
> +		strbuf_addch(&sb, opt->short_name);
> +		if (arg)
> +			strbuf_addstr(&sb, arg);
> +	} else
> +		return -1;
> +
> +	argv_array_push(opt_value, sb.buf);
> +	strbuf_release(&sb);
> +	return 0;
> +}
> diff --git a/parse-options.h b/parse-options.h
> index 1d21398..b663f87 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -225,6 +225,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);
>  extern int parse_opt_noop_cb(const struct option *, const char *, int);
>  extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
> +extern int parse_opt_pass_argv_array(const struct option *, const char *, int);
>  
>  #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
>  #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))

Re: [PATCH v2 05/19] pull: implement fetch + merge

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> +/**
> + * 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++);

As you cannot say "git pull refspecs...", the above might be more
clear if you spelled it like this instead:

	if (repo) {
		argv_array_push(&args, repo);
		argv_array_pushv(&args, refspecs);
	} else if (*refspecs) {
        	die("BUG: refspec without repo?");
	}

> +/**
> + * 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;
> +}

No frills yet, which is a good way to start with and show the
overall structure.

Re: [PATCH v2 06/19] pull: pass verbosity, --progress flags to fetch and merge

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

Forgot to rephrase?  parse-opt-passthru() is a good name for "pass
the single string, last one wins", but is implemented in another
patch.

	Use parse_opt_passthru() implemented earlier to pass the
	"--[no-]progress" command line options to git-fetch and
	git-merge.

or something like that.

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 0ca23a3..c9c2cc0 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -16,11 +16,35 @@ static const char * const pull_usage[] = {
>  	NULL
>  };
>  
> +/* Shared options */
> +static int opt_verbosity;
> +static struct strbuf opt_progress = STRBUF_INIT;
> +
>  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_pass_strbuf},
> +
>  	OPT_END()
>  };

Seeing this use case convinces me that the new parse-opt callback
parse-opt-pass-single-string-last-one-wins() in 01/19 should not be
strbuf based interface but should be (const char *) based one.

Other than that the change looks nicely done.  So far, the series
has been a very pleasant read up to this step.

Re: [PATCH v2 09/19] pull: error on no merge candidates

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>  /**
> + * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
> + * into merge_heads.
> + */

Hmph, I vaguely recall doing that in C elsewhere already, even
though I do not remember where offhand...

> +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) {

Missing SP after "while"

> +		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"))

Look for "\tnot-for-merge\t" instead?

The patch overall looks good.  Thanks.

Re: [PATCH v2 10/19] pull: support pull.ff config

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>     
>     * Yup, I did mean strcmp(value, "only")

I may be missing some backstory behind this comment; in the patch, I
instead see !strcmp(value, "only") and I think it makes sense.

> +	if (git_config_get_value("pull.ff", &value))
> +		return;
> +	switch (git_config_maybe_bool("pull.ff", value)) {
> +		case 0:
> +			strbuf_addstr(sb, "--no-ff");
> +			return;

Align switch and case at the same indent level; the body of switch
is overindented.

> +		case 1:
> +			strbuf_addstr(sb, "--ff");
> +			return;
> +	}
> +	if (!strcmp(value, "only")) {
> +		strbuf_addstr(sb, "--ff-only");
> +		return;
> +	}

Re: [PATCH v2 04/19] pull: implement skeletal builtin pull

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> +int cmd_pull(int argc, const char **argv, const char *prefix)
> +{
> +	if (!getenv("_GIT_USE_BUILTIN_PULL")) {
> +		const char *path = mkpath("%s/git-pull", git_exec_path());
> +
> +		if (sane_execvp(path, (char**) argv) < 0)

Style: "(char **)argv".

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> @@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	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.len)
>  		config_get_ff(&opt_ff);

Hmph.

If you are going to do the git_config() call yourself, it might make
more sense to define git_pull_config() callback and parse the pull.ff
yourself, updating the use of the lazy git_config_get_value() API you
introduced in patch 10/19.

The above "might" is stronger than my usual "might"; I am undecided
yet before reading the remainder of the series.

Re: [PATCH v2 13/19] pull: implement pulling into an unborn branch

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>  /**
> + * "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])
> +{

It is not wrong per-se, but is rather unusual (and misleading) to
specify the array-ness and array-size of parameters.  Our codebase
tends to prefer spelling it like so instead:

	static int pull_into_void(unsigned char *merge_head,
			          unsigned char *curr_head)
	{

> +	/*
> +	 * Two-way merge: we claim the index is based on an empty tree,

s/claim/treat/ perhaps?

> +	 * 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)
> @@ -475,5 +497,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();
>  }

Sensible.

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> +enum rebase_type {
> +	REBASE_INVALID = -1,
> +	REBASE_FALSE = 0,
> +	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 enum rebase_type parse_config_rebase(const char *value)
> +{
> +	int v = git_config_maybe_bool("pull.rebase", value);
> +	if (!v)
> +		return REBASE_FALSE;
> +	else if (v >= 0)
> +		return REBASE_TRUE;

It is somewhat misleading to say "v >= 0" when you already use !v to
signal something else.  Perhaps "else if (v > 0)" is better?

> +/**
> + * Returns remote's upstream branch for the current branch. If remote is NULL,
> + * the current branch's configured default remote is used. Returns NULL if
> + * `remote` does not name a valid remote, HEAD does not point to a branch,
> + * remote is not the branch's configured remote or the branch does not have any
> + * configured upstream branch.
> + */
> +static char *get_upstream_branch(const char *remote)
> +{
> +	struct remote *rm;
> +	struct branch *curr_branch;
> +
> +	rm = remote_get(remote);
> +	if (!rm)
> +		return NULL;
> +
> +	curr_branch = branch_get("HEAD");
> +	if (!curr_branch)
> +		return NULL;
> +
> +	if (curr_branch->remote != rm)
> +		return NULL;
> +
> +	if (!curr_branch->merge_nr)
> +		return NULL;
> +
> +	return xstrdup(curr_branch->merge[0]->dst);
> +}

Hmph, it is somewhat surprising that we do not have such a helper
already. Wouldn't we need this logic to implement $branch@{upstream}
syntax?

> +/**
> + * Derives the remote tracking branch from the remote and refspec.
> + *
> + * FIXME: The current implementation assumes the default mapping of
> + * refs/heads/<branch_name> to refs/remotes/<remote_name>/<branch_name>.
> + */
> +static char *get_tracking_branch(const char *remote, const char *refspec)
> +{

This does smell like an incomplete reimplementation of what
get_fetch_map() knows how to do.

> +/**
> + * Given the repo and refspecs, sets fork_point to the point at which the
> + * current branch forked from its remote tracking 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)
> +{
> +...
> +}

This function looks OK (the two get_*_branch() helpers it uses I am
not sure about though).

Same comment on "fork_point[]" parameter's type applies here,
though.  While I do not mind if you used "struct object_id" to
represent these object names, if you are sticking to the traditional
"unsigned char [20]", then these should be "unsigned char *" to be
consistent with others.

> +/**
> + * 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])
> +{
> +...
> +}

OK (and everything after this point looks good).

Re: [PATCH v2 17/19] pull --rebase: exit early when the working directory is dirty

[Kevin Daudt]

On Wed, Jun 03, 2015 at 12:27:16PM +0200, Kevin Daudt wrote:
> On Wed, Jun 03, 2015 at 02:49:01PM +0800, Paul Tan wrote:
> > Re-implement the behavior introduced by f9189cf (pull --rebase: exit
> > early when the working directory is dirty, 2008-05-21).
> 
> When the option rebase.autoStash is set, it should not be necessary to
> die in this case. See also this[1] patch I'm working on.
> 

Sorry, didn't notice it was you who reviewed my patch.

Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

[Paul Tan]

On Wed, Jun 10, 2015 at 7:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Almost the same comment as 01/19 applies to this comment.
>
> I think it makes good sense to have two variants, one that lets the
> last one win and pass only that last one (i.e. 01/19) and the other
> that accumulates them into an argv_array (i.e. this one).  But it
> feels iffy, given that the "acculate" version essentially creates an
> array of (char *), to make "the last one wins, leaving a single
> string" to use strbuf.  I'd find it much more understandable if 01/19
> took (char **) as opt->value instead of a strbuf.

I don't see how it feels iffy. The purpose of using strbufs (and
argv_arrays) is to avoid error-prone manual memory management.

> In any case, these two need to be added as a related pair to the API
> documentation.

Okay, I guess I could also add their macro functions as well.

Thanks,
Paul

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Paul Tan]

On Wed, Jun 10, 2015 at 9:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> +enum rebase_type {
>> +     REBASE_INVALID = -1,
>> +     REBASE_FALSE = 0,
>> +     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 enum rebase_type parse_config_rebase(const char *value)
>> +{
>> +     int v = git_config_maybe_bool("pull.rebase", value);
>> +     if (!v)
>> +             return REBASE_FALSE;
>> +     else if (v >= 0)
>> +             return REBASE_TRUE;
>
> It is somewhat misleading to say "v >= 0" when you already use !v to
> signal something else.  Perhaps "else if (v > 0)" is better?

Ah, right.

>> +/**
>> + * Returns remote's upstream branch for the current branch. If remote is NULL,
>> + * the current branch's configured default remote is used. Returns NULL if
>> + * `remote` does not name a valid remote, HEAD does not point to a branch,
>> + * remote is not the branch's configured remote or the branch does not have any
>> + * configured upstream branch.
>> + */
>> +static char *get_upstream_branch(const char *remote)
>> +{
>> +     struct remote *rm;
>> +     struct branch *curr_branch;
>> +
>> +     rm = remote_get(remote);
>> +     if (!rm)
>> +             return NULL;
>> +
>> +     curr_branch = branch_get("HEAD");
>> +     if (!curr_branch)
>> +             return NULL;
>> +
>> +     if (curr_branch->remote != rm)
>> +             return NULL;
>> +
>> +     if (!curr_branch->merge_nr)
>> +             return NULL;
>> +
>> +     return xstrdup(curr_branch->merge[0]->dst);
>> +}
>
> Hmph, it is somewhat surprising that we do not have such a helper
> already. Wouldn't we need this logic to implement $branch@{upstream}
> syntax?

Right, the @{upstream} syntax is implemented by branch_get_upstream()
in remote.c. It, however, does not check to see if the branch's remote
matches what is provided on the command-line, so we still have to
implement this check ourselves, which means this helper function is
still required.

I guess we could still use branch_get_upstream() in this function though.

>> +/**
>> + * Derives the remote tracking branch from the remote and refspec.
>> + *
>> + * FIXME: The current implementation assumes the default mapping of
>> + * refs/heads/<branch_name> to refs/remotes/<remote_name>/<branch_name>.
>> + */
>> +static char *get_tracking_branch(const char *remote, const char *refspec)
>> +{
>
> This does smell like an incomplete reimplementation of what
> get_fetch_map() knows how to do.

Yeah, this is just a direct rewrite of get_remote_merge_branch() in
git-parse-remote.sh. Johannes pointed out[1] that
remote_find_tracking() in remote.c does the exact same thing without
the assumption of the default fetch refmap. However, this would be a
separate modification on its own, so it may be better to do it in a
separate patch with regression tests. (e.g. what should we do if the
refspec dst is provided?)

[1] http://thread.gmane.org/gmane.comp.version-control.git/269258/focus=269350

>> +/**
>> + * Given the repo and refspecs, sets fork_point to the point at which the
>> + * current branch forked from its remote tracking 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)
>> +{
>> +...
>> +}
>
> This function looks OK (the two get_*_branch() helpers it uses I am
> not sure about though).
>
> Same comment on "fork_point[]" parameter's type applies here,
> though.  While I do not mind if you used "struct object_id" to
> represent these object names, if you are sticking to the traditional
> "unsigned char [20]", then these should be "unsigned char *" to be
> consistent with others.

Okay.

Thanks,
Paul

Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> I don't see how it feels iffy.

That is largely a taste thing.  And a good taste matters.

What is iffy is to use strbuf as an external interface between the
implementation of the parse_opt_pass() API function and its users.

I would expect that no users of the parse_opt_pass() would ever take
advantage of the fact that the value they are using is implemented
as a strbuf, allowing them to use the family of functions in the
strbuf API to further manipulate it.  All users you added wants to
use a plain vanilla string, and only because you used strbuf as the
interface element, they have to say "var.buf" to get to the value
they want to use.

> The purpose of using strbufs (and
> argv_arrays) is to avoid error-prone manual memory management.

It is a good idea to use strbuf as an implementation detail of
parse_opt_pass() function.  After all, safer, easier and efficient
manipulation of strings is why we added the strbuf API to the system
in the first place.

But it is a different matter to expose that as an API element when
the recipient is not going to benefit.

In other words, callers of the API designed with a better taste
would look more like this:

	static const char *opt_diffstat;

	static struct option pull_options[] = {
        	...
                { 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_pass
        	},
		...
	};

	static int run_merge(void)
        {
        	...
                if (opt_diffstat)
                	argv_array_push(&args, opt_diffstat);
		...
	}

That way, they do not have to define strbuf variables and
dereference the value as opt_diffstat.buf.

And the implementation of the API element is not all that different
from what you wrote:

	int parse_opt_pass(const struct options *opt, const char *arg, int unset)
	{
		struct strbuf sb = STRBUF_INIT;
		char **value = opt->value;

		if (*value)
                	free(*value);
		if (opt->long_name) {
                	strbuf_addf(&sb, "--%s%s",
                        	    unset ? "no-" : "",
                                    opt->long_name);
			...
                }
                ...
                *value = strbuf_detach(&sb, NULL);
		return 0;
	}

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
>> @@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>  
>>  	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.len)
>>  		config_get_ff(&opt_ff);
>
> Hmph.
>
> If you are going to do the git_config() call yourself, it might make
> more sense to define git_pull_config() callback and parse the pull.ff
> yourself, updating the use of the lazy git_config_get_value() API you
> introduced in patch 10/19.
>
> The above "might" is stronger than my usual "might"; I am undecided
> yet before reading the remainder of the series.

Let me clarify the above with s/stronger/with much less certainty/;

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>> Hmph, it is somewhat surprising that we do not have such a helper
>> already. Wouldn't we need this logic to implement $branch@{upstream}
>> syntax?
>
> Right, the @{upstream} syntax is implemented by branch_get_upstream()
> in remote.c. It, however, does not check to see if the branch's remote
> matches what is provided on the command-line, so we still have to
> implement this check ourselves, which means this helper function is
> still required.
>
> I guess we could still use branch_get_upstream() in this function though.

It is entirely expected that existing function may not do exactly
what the new caller you introduce might want to do, or may do more
than what it wants.  That is where refactoring of existing code
comes in.

It somewhat feels strange that you have to write more than "shim"
code to glue existing helpers and API functions together to
re-implement what a scripted Porcelain is already doing, though.
It can't be that git-pull.sh implements this logic as shell script,
and it must be asking existing code in Git to do what the callers
you added for this function would want to do, right?  That suggests
that we must have enough logic already in C.

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Paul Tan]

On Wed, Jun 10, 2015 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>> @@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>>
>>>      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.len)
>>>              config_get_ff(&opt_ff);
>>
>> Hmph.
>>
>> If you are going to do the git_config() call yourself, it might make
>> more sense to define git_pull_config() callback and parse the pull.ff
>> yourself, updating the use of the lazy git_config_get_value() API you
>> introduced in patch 10/19.
>>
>> The above "might" is stronger than my usual "might"; I am undecided
>> yet before reading the remainder of the series.
>
> Let me clarify the above with s/stronger/with much less certainty/;

Uh, I have no idea what a "strong might" or a "less certain might" is. :-/

Parsing all the config values with a git_config() callback function is
certainly possible, but I was under the impression that we are moving
towards migrating use of all the git_config() callback loops to the
git_config_get_X() API.

In this case though, we have to use git_config() to initialize the
advice.resolveConflict config setting, but I don't see why it must be
in conflict with the above goal.

Thanks,
Paul

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Paul Tan]

On Wed, Jun 10, 2015 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>>> Hmph, it is somewhat surprising that we do not have such a helper
>>> already. Wouldn't we need this logic to implement $branch@{upstream}
>>> syntax?
>>
>> Right, the @{upstream} syntax is implemented by branch_get_upstream()
>> in remote.c. It, however, does not check to see if the branch's remote
>> matches what is provided on the command-line, so we still have to
>> implement this check ourselves, which means this helper function is
>> still required.
>>
>> I guess we could still use branch_get_upstream() in this function though.
>
> It is entirely expected that existing function may not do exactly
> what the new caller you introduce might want to do, or may do more
> than what it wants.  That is where refactoring of existing code
> comes in.
>
> It somewhat feels strange that you have to write more than "shim"
> code to glue existing helpers and API functions together to
> re-implement what a scripted Porcelain is already doing, though.
> It can't be that git-pull.sh implements this logic as shell script,
> and it must be asking existing code in Git to do what the callers
> you added for this function would want to do, right?

Not git-pull.sh, but get_remote_merge_branch() git-parse-remote.sh.
The shell code that get_upstream_branch() in this patch implements is:

    0|1)
        origin="$1"
        default=$(get_default_remote)
        test -z "$origin" && origin=$default
        curr_branch=$(git symbolic-ref -q HEAD) &&
        [ "$origin" = "$default" ] &&

^ This here is where it checks to see if the branch's configured
remote matches the remote provided on the command line.

        echo $(git for-each-ref --format='%(upstream)' $curr_branch)
        ;;

^ While here it calls git to get the upstream branch, which is
implemented by branch_get_upstream() on the C side.

So yes, we can use branch_get_upstream(), but we still need to
implement some code on top.

Just to add on, the shell code that get_tracking_branch() in this
patch implements is:

    *)
        repo=$1
        shift
        ref=$1
        # FIXME: It should return the tracking branch
        #        Currently only works with the default mapping
        case "$ref" in
        +*)
        ref=$(expr "z$ref" : 'z+\(.*\)')
        ;;
        esac
        expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
        remote=$(expr "z$ref" : 'z\([^:]*\):')
        case "$remote" in
        '' | HEAD ) remote=HEAD ;;
        heads/*) remote=${remote#heads/} ;;
        refs/heads/*) remote=${remote#refs/heads/} ;;
        refs/* | tags/* | remotes/* ) remote=
        esac
        [ -n "$remote" ] && case "$repo" in
        .)
            echo "refs/heads/$remote"
            ;;
        *)
            echo "refs/remotes/$repo/$remote"
            ;;
        esac

so it's more or less a direct translation of the shell script, and we
can be sure it will have the same behavior. I'm definitely in favor of
switching this to use remote_find_tracking(), the question is whether
we want to do it in this patch or in a future patch on top.

Thanks,
Paul

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> so it's more or less a direct translation of the shell script, and we
> can be sure it will have the same behavior. I'm definitely in favor of
> switching this to use remote_find_tracking(), the question is whether
> we want to do it in this patch or in a future patch on top.

Ah, OK.

I then agree that the topic should be a straight-forward literal
translation of the current scripted Porcelain.  Switching
implementation detail to share more code can and should come on top
with more careful consideration, as that step can change the
semantics by mistake.

Thanks.

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> On Wed, Jun 10, 2015 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> If you are going to do the git_config() call yourself, it might make
>>> more sense to define git_pull_config() callback and parse the pull.ff
>>> yourself, updating the use of the lazy git_config_get_value() API you
>>> introduced in patch 10/19.
>>>
>>> The above "might" is stronger than my usual "might"; I am undecided
>>> yet before reading the remainder of the series.
>>
>> Let me clarify the above with s/stronger/with much less certainty/;
>
> Uh, I have no idea what a "strong might" or a "less certain might" is. :-/
>
> Parsing all the config values with a git_config() callback function is
> certainly possible, but I was under the impression that we are moving
> towards migrating use of all the git_config() callback loops to the
> git_config_get_X() API.
>
> In this case though, we have to use git_config() to initialize the
> advice.resolveConflict config setting, but I don't see why it must be
> in conflict with the above goal.

I (or at least some part of me) actually view git_config_get_*() as
"if you are only going to peek a few variables, you do not have to
do the looping yourself" convenience, which leads me (or at least
that part of me) to say "if you are doing the looping anyway, you
may be better off picking up the variables you care about yourself
in that loop".

By calling git_config() before calling any git_config_get_*()
functions, you would be priming the cache layer with the entire
contents of the configuration files anyway, so later calls to
git_config_get_*() will read from that cache, so there is no
performance penalty in mixing the two methods to access
configuration data.  That is why I felt less certain that the
suggestion to stick to one method (instead of mixing the two) is a
good thing to do (hence "less certain 'might'").

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> +/**
> + * Returns remote's upstream branch for the current branch. If remote is NULL,
> + * the current branch's configured default remote is used. Returns NULL if
> + * `remote` does not name a valid remote, HEAD does not point to a branch,
> + * remote is not the branch's configured remote or the branch does not have any
> + * configured upstream branch.
> + */
> +static char *get_upstream_branch(const char *remote)
> +{
> +	struct remote *rm;
> +	struct branch *curr_branch;
> +
> +	rm = remote_get(remote);
> +	if (!rm)
> +		return NULL;
> +
> +	curr_branch = branch_get("HEAD");
> +	if (!curr_branch)
> +		return NULL;
> +
> +	if (curr_branch->remote != rm)
> +		return NULL;
> +
> +	if (!curr_branch->merge_nr)
> +		return NULL;
> +
> +	return xstrdup(curr_branch->merge[0]->dst);
> +}

This part needs to be rebased, as branch->remote no longer exists in
the recent world order.

Re: [PATCH v2 09/19] pull: error on no merge candidates

[Paul Tan]

On Wed, Jun 10, 2015 at 7:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>>  /**
>> + * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
>> + * into merge_heads.
>> + */
>
> Hmph, I vaguely recall doing that in C elsewhere already, even
> though I do not remember where offhand...

Right, handle_fetch_head() in builtin/merge.c. It looks up the commit
IDs into commit objects though, which is not required for git-pull. We
only need the list of hashes.

>> +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) {
>
> Missing SP after "while"

OK

>> +             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"))
>
> Look for "\tnot-for-merge\t" instead?

Right, it's better to be stricter.

Thanks,
Paul

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Paul Tan]

On Thu, Jun 11, 2015 at 1:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I (or at least some part of me) actually view git_config_get_*() as
> "if you are only going to peek a few variables, you do not have to
> do the looping yourself" convenience, which leads me (or at least
> that part of me) to say "if you are doing the looping anyway, you
> may be better off picking up the variables you care about yourself
> in that loop".

Not just a convenience, but I personally find that callback functions
usually leads to code that is harder to understand because of the use
of static/global variables to preserve state and the fact that it is
harder to break it up into smaller functions to reason about. This is
just a matter of taste though, so I don't have strong feelings about
it.

Besides, there is a greater technical reason why we should not use
git_config(): It essentially performs a linear search[1], while the
git_config_get_*() functions do a constant-time lookup. Ideally, we
should remove uses of git_config() for users that have no need to
iterate over all the configuration entries.

[1] Since we do a strcmp() for every supported config setting, for
every config entry.

I should emphasize that the call to git_config(git_default_config,
NULL) is just a workaround to load the advice.* settings. In fact,
git_default_config() only peeks at a few config settings anyway, and
can be re-written to not iterate over all the user's config entries.
As such, I don't see why the builtin/pull.c code should be written to
support the git_config() way of doing things, even if we have to use
the workaround of calling git_config(). It's like saying: we have a
bad solution, now let's make it worse ;-)

> By calling git_config() before calling any git_config_get_*()
> functions, you would be priming the cache layer with the entire
> contents of the configuration files anyway, so later calls to
> git_config_get_*() will read from that cache, so there is no
> performance penalty in mixing the two methods to access
> configuration data.  That is why I felt less certain that the
> suggestion to stick to one method (instead of mixing the two) is a
> good thing to do (hence "less certain 'might'").

Right, although I think that the performance penalty due to using
git_config() is greater, and switching all the git_config_get_*()
calls to use it would just make it worse.

By the way, I got the idea that git development was moving towards
replacing use of git_config() from 5801d3b (builtin/gc.c: replace
`git_config()` with `git_config_get_*()` family, 2014-08-07).

Thanks,
Paul