{"thread":{"id":"38831","subject":"[PATCH/RFC/GSOC] make git-pull a builtin","startedAt":"2015-03-17T13:57:42Z","lastAt":"2015-03-24T15:58:06Z","messageCount":15,"participants":["Paul Tan","Stephen Robin","Johannes Schindelin","Matthieu Moy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257852","messageId":"1426600662-32276-1-git-send-email-pyokagan@gmail.com","threadId":"38831","inReplyTo":null,"subject":"[PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-17T13:57:42Z","receivedAt":"2015-03-17T13:57:42Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"The git-pull command is currently implemented by the shell script\ngit-pull.sh. However, git-pull.sh suffers from some problems:\n\n* Inconsistent command line interface: since git-pull.sh does not use\n git's internal parse-options API, it's command line interface is\n inconsistent with other git builtins. For example, the option --st\n should throw an ambigious option error as it could mean --stat or\n --strategy, but git-pull.sh is hardcoded to assume --st resolves to\n --strategy. It also does not list the command line options in its\n usage help.\n\n* Inefficient: shell scripts can only access git functionality through\n spawning git processes, which has a relatively high overhead on\n certain operating systems such as Windows. Furthermore, the repeated\n starting up of git means that commonly-used files such config files,\n the index file and often accessed objects/packfiles are repeatedly\n read, which can cause poor performance on filesystems or operating\n systems with slow IO operations.\n\n* Makes it difficult to port git: git-pull.sh requires a posix\n conformant shell and utilities such as sed and tr. This makes it\n harder to port git to non-posix environments.\n\nBy making git-pull a builtin, it has access to git's internal API and\nthus solves the above problems:\n\n* Usage of parse-options API means greater consistency of the\n command-line interface with other git commands.\n\n* No spawning of external git processes: internal API and builtins are\n called directly. This also means that they use the same underlying\n object/config cache, which reduces needless IO/parsing operations.\n\n* Low-level control over code execution and thus greater efficiency. For\n example commit SHA1s now only need to be parsed from a string once\n (when resolving branch refs), while as a shell script they would have\n to be stored as strings and parsed over and over again.\n\nFurthermore, this reduces git's dependence on posix shells and\nutilities, which would be very helpful in porting git to non-posix\nenvironments such as Windows.\n\nSigned-off-by: Paul Tan \n---\n\nHi, I would like to share this very rough prototype with everyone. All\nthe relevant tests (t5520-t5572) pass.\n\nI did a quick benchmark on my Linux and Windows systems, which have\nnearly the same hardware, by running 'git pull' 100 times on a repo and\nits clone with 1 commit. On Linux, there is negligible performance\ndifference, but on Windows the runtime fell from 8m 25s to 1m 3s. The\ntimings should not be affected by any msys2 emulation as the git\nprocesses were spawned from a native executable. I started this as a\njust-for-fun exercise to learn about the git internal API, but seeing as\nthe results are very encouraging, I'm tempted to take on this project\nfor GSoC.\n\nOf course, it would be easy to make any shell script a builtin by using\nthe run_command* functions, but I feel that that would not take\nadvantage of the potential benefits in using git's internal API\ndirectly. So, I set myself a few requirements for the ideal end result:\n\n* zero spawning of processes so that the internal object, index and\nconfig cache can be taken advantage of. Some operating systems (like\nWindows) also have a relatively large overhead when spawning\nprocesses[1].\n\n* avoid needless parsing/string conversions by directly accessing the C\ndata structures. Massaging of strings cause too much code churn, so\nfunctions that take the richer C data structures as input should be\npreferred over functions that work by parsing string arguments (e.g.\nprefer using the run_diff_files() function instead of calling\ncmd_diff_files or spawning a git diff-files process)\n\n* use the internal API as much as possible: share code between the\nbuiltins (e.g. builtin fmt-merge-msg, exposed in fmt-merge-msg.h)\nin order to reduce code complexity. For example, the fork_point() code\nin this patch should be shared with the merge-base builtin.\n\n[1] http://stackoverflow.com/questions/10710912\n\nHowever, there were some problems encountered when trying to fulfill the\nrequirements above:\n\n* Many builtins only provide their main entry point to access their\nfunctionality -- this means that argument options are parsed, put back\ntogether, and then re-parsed again, which is quite inefficient and can\ncreate a lot of code complexity. A better solution would be to expose\nthe functionality of these builtins as a separate function without\nargument parsing, similar to how fmt_merge_msg is implemented.\n\n* Some buitins do not expect to be called by their cmd_* functions\nthrough code paths other than git.c. For example, fetch.c actually\nfrees the strings it pulls from argv. This patch works around these\nfunctions by not attempting to free memory passed to these functions,\nbut a more comprehensive solution would be to investigate and fix\nthese functions. (Or expose their functionality in as a separate function)\n\n* Finally, there is the possibility that in the process of conversion\nbugs or incompatible behavioral changes may be introduced which are\nnot caught by the test suite. Ideally, I think the solution is to\nimprove the test suite and make it as comprehensive as possible, but\nwriting a comprehensive test suite may be too time consuming. A\nsimpler, but less perfect strategy might be to just convert the shell\nscripts directly statement by statement to C, using the run_command*()\nfunctions as Duy Nguyen[2] suggested, before changing the code to use\nthe internal API.\n\n[2] http://thread.gmane.org/gmane.comp.version-control.git/265575/focus=265602\n\nThis patch is the result of trying to fulfill the above requirements,\nand temporarily working around the above problems until I can find their\nsource/come up with a patch. (There may also be some bugs in the\ngit-pull.sh code related to not taking into account the fact that \"$@\"\ncan contain arguments for git-fetch. However, I have not found a way to\ntrigger them yet in practice.) I would like some feedback on whether the\nabove requirements are sane, and whether there may be disadvantages that\nI'm missing out on. As stated above, there are big performance wins for\nWindows, but I think the biggest win of all is to reduce git's\ndependence on a scripting language, improving portability and reducing\ncode duplication between the languages, thus making git a better\nuniversal cross-platform solution for version control :)\n\nRegards,\nPaul\n\n Makefile | 2 +-\n builtin.h | 1 +\n builtin/pull.c | 938 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n git.c | 1 +\n 4 files changed, 941 insertions(+), 1 deletion(-)\n create mode 100644 builtin/pull.c\n\ndiff --git a/Makefile b/Makefile\nindex 44f1dd1..50a6a16 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -470,7 +470,6 @@ SCRIPT_SH += git-merge-octopus.sh\n SCRIPT_SH += git-merge-one-file.sh\n SCRIPT_SH += git-merge-resolve.sh\n SCRIPT_SH += git-mergetool.sh\n-SCRIPT_SH += git-pull.sh\n SCRIPT_SH += git-quiltimport.sh\n SCRIPT_SH += git-rebase.sh\n SCRIPT_SH += git-remote-testgit.sh\n@@ -872,6 +871,7 @@ BUILTIN_OBJS += builtin/pack-refs.o\n BUILTIN_OBJS += builtin/patch-id.o\n BUILTIN_OBJS += builtin/prune-packed.o\n BUILTIN_OBJS += builtin/prune.o\n+BUILTIN_OBJS += builtin/pull.o\n BUILTIN_OBJS += builtin/push.o\n BUILTIN_OBJS += builtin/read-tree.o\n BUILTIN_OBJS += builtin/receive-pack.o\ndiff --git a/builtin.h b/builtin.h\nindex b87df70..ea3c834 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -98,6 +98,7 @@ extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);\n extern int cmd_patch_id(int argc, const char **argv, const char *prefix);\n extern int cmd_prune(int argc, const char **argv, const char *prefix);\n extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);\n+extern int cmd_pull(int argc, const char **argv, const char *prefix);\n extern int cmd_push(int argc, const char **argv, const char *prefix);\n extern int cmd_read_tree(int argc, const char **argv, const char *prefix);\n extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nnew file mode 100644\nindex 0000000..73fa87e\n--- /dev/null\n+++ b/builtin/pull.c\n@@ -0,0 +1,938 @@\n+/*\n+ * Builtin \"git pull\"\n+ *\n+ * Based on git-pull.sh by Junio C Hamano\n+ *\n+ * Fetch one or more remote refs and merge it/them into the current HEAD.\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"string-list.h\"\n+#include \"cache.h\"\n+#include \"refs.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"submodule.h\"\n+#include \"commit.h\"\n+#include \"dir.h\"\n+#include \"remote.h\"\n+#include \"argv-array.h\"\n+#include \"sha1-array.h\"\n+#include \"run-command.h\"\n+#include \"lockfile.h\"\n+#include \"fmt-merge-msg.h\"\n+\n+static const char * const builtin_pull_usage[] = {\n+\tN_(\"git pull [-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [] ...\"),\n+\tNULL\n+};\n+\n+static const char *opt_prefix;\n+static int opt_verbosity = 0;\n+static int opt_progress = -1;\n+static int opt_diffstat = -1;\n+/* NOTE: git-pull.sh only supports --log and --no-log, as opposed to what\n+ * man git-pull says. */\n+static int opt_shortlog_len;\n+static int opt_commit = -1;\n+static int opt_edit = -1;\n+static int opt_squash = -1;\n+static int opt_ff;\n+static struct string_list opt_merge_strategies = STRING_LIST_INIT_NODUP;\n+static struct string_list opt_merge_strategy_opts = STRING_LIST_INIT_NODUP;\n+static int opt_rebase;\n+static int opt_recurse_submodules;\n+static int opt_verify_signatures = -1;\n+static const char *opt_gpg_sign;\n+static int opt_dry_run;\n+\n+/* Global vars since they are used often */\n+static char *head_name;\n+static const char *head_name_short;\n+static unsigned char head_sha1[20];\n+static int head_flags;\n+\n+enum rebase_type {\n+\tREBASE_FALSE = 0,\n+\tREBASE_TRUE = 1,\n+\tREBASE_PRESERVE = 2\n+};\n+\n+/**\n+ * Parse rebase config/option value and return corresponding int\n+ */\n+static int parse_rebase(const char *arg)\n+{\n+\tif (!strcmp(arg, \"true\"))\n+\t\treturn REBASE_TRUE;\n+\telse if (!strcmp(arg, \"false\"))\n+\t\treturn REBASE_FALSE;\n+\telse if (!strcmp(arg, \"preserve\"))\n+\t\treturn REBASE_PRESERVE;\n+\telse\n+\t\treturn -1; /* Invalid value */\n+}\n+\n+/**\n+ * Returns default rebase option value\n+ */\n+static int rebase_config_default(void)\n+{\n+\tstruct strbuf name = STRBUF_INIT;\n+\tconst char *value = NULL;\n+\tint boolval;\n+\n+\tstrbuf_addf(&name, \"branch.%s.rebase\", head_name_short);\n+\tif (git_config_get_value(name.buf, &value))\n+\t\tgit_config_get_value(\"pull.rebase\", &value);\n+\tstrbuf_release(&name);\n+\tif (!value)\n+\t\treturn REBASE_FALSE;\n+\tboolval = git_config_maybe_bool(\"pull.rebase\", value);\n+\tif (boolval >= 0)\n+\t\treturn boolval ? REBASE_TRUE : REBASE_FALSE;\n+\telse if (value && !strcmp(value, \"preserve\"))\n+\t\treturn REBASE_PRESERVE;\n+\tdie(_(\"invalid value for branch.%s.rebase \\\"%s\\\"\"), head_name_short, value);\n+}\n+\n+static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (arg)\n+\t\t*(int *)opt->value = parse_rebase(arg);\n+\telse\n+\t\t*(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;\n+\treturn (*(int *)opt->value) >= 0 ? 0 : -1;\n+}\n+\n+enum ff_type {\n+\tFF_NO = 0,\n+\tFF_ALLOW = 1,\n+\tFF_ONLY = 2\n+};\n+\n+/**\n+ * Returns default --ff value\n+ */\n+static int ff_config_default(void)\n+{\n+\tconst char *value = NULL;\n+\tint boolval;\n+\n+\tgit_config_get_value(\"pull.ff\", &value);\n+\tboolval = git_config_maybe_bool(\"pull.ff\", value);\n+\tif (boolval >= 0)\n+\t\treturn boolval ? FF_ALLOW : FF_NO;\n+\telse if (value && !strcmp(value, \"only\"))\n+\t\treturn FF_ONLY;\n+\treturn FF_ALLOW; /* don't barf on values from future versions of git */\n+}\n+\n+enum recurse_submodules_type {\n+\tRS_NO = 0,\n+\tRS_YES = 1,\n+\tRS_ON_DEMAND = 2\n+};\n+\n+static int parse_opt_recurse_submodules(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (!arg)\n+\t\t*(int *)opt->value = unset ? RS_NO : RS_YES;\n+\telse if (!strcmp(arg, \"no\"))\n+\t\t*(int *)opt->value = RS_NO;\n+\telse if (!strcmp(arg, \"yes\"))\n+\t\t*(int *)opt->value = RS_YES;\n+\telse if (!strcmp(arg, \"on-demand\"))\n+\t\t*(int *)opt->value = RS_ON_DEMAND;\n+\telse\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static struct option builtin_pull_options[] = {\n+\tOPT__VERBOSITY(&opt_verbosity),\n+\tOPT_BOOL(0, \"progress\", &opt_progress,\n+\t\tN_(\"force progress reporting\")),\n+\t{ OPTION_SET_INT, 'n', \"no-stat\", &opt_diffstat, NULL,\n+\t N_(\"\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n+\t{ OPTION_SET_INT, 0, \"stat\", &opt_diffstat, NULL,\n+\t N_(\"\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},\n+\t{ OPTION_SET_INT, 0, \"summary\", &opt_diffstat, NULL,\n+\t N_(\"\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},\n+\t{ OPTION_SET_INT, 0, \"no-summary\", &opt_diffstat, NULL,\n+\t N_(\"\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n+\t/* NOTE: git-pull.sh only support --log and --no-log */\n+\t{ OPTION_INTEGER, 0, \"log\", &opt_shortlog_len, N_(\"n\"),\n+\t N_(\"populate log with at most entries from shortlog\"),\n+\t PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN},\n+\tOPT_BOOL(0, \"commit\", &opt_commit,\n+\t\tN_(\"perform a commit if the merge succeeds (default)\")),\n+\tOPT_BOOL('e', \"edit\", &opt_edit,\n+\t\tN_(\"edit message before committing\")),\n+\tOPT_BOOL(0, \"squash\", &opt_squash,\n+\t\tN_(\"create a single commit instead of doing a merge\")),\n+\tOPT_BOOL(0, \"ff\", &opt_ff,\n+\t\tN_(\"allow fast-forward\")),\n+\t{ OPTION_SET_INT, 0, \"ff-only\", &opt_ff, NULL,\n+\t N_(\"abort if fast-forward is not possible\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY},\n+\tOPT_STRING_LIST('s', \"strategy\", &opt_merge_strategy_opts,\n+\t\tN_(\"strategy\"),\n+\t\tN_(\"merge strategy to use\")),\n+\tOPT_STRING_LIST('X', \"strategy-option\", &opt_merge_strategy_opts,\n+\t\tN_(\"option=value\"),\n+\t\tN_(\"option for selected merge strategy\")),\n+\t{ OPTION_CALLBACK, 'r', \"rebase\", &opt_rebase, N_(\"false|true|preserve\"),\n+\t N_(\"rebase\"),\n+\t PARSE_OPT_OPTARG, parse_opt_rebase, 0},\n+\tOPT_CALLBACK(0, \"recurse-submodules\", &opt_recurse_submodules, N_(\"yes|on-demand|no\"),\n+\t\tN_(\"New commits of all populated submodules should be fetched too\"),\n+\t\tparse_opt_recurse_submodules),\n+\tOPT_BOOL(0, \"verify-signatures\", &opt_verify_signatures,\n+\t\tN_(\"verify GPG signatures of commits being merged\")),\n+\t{ OPTION_STRING, 0, \"gpg-sign\", &opt_gpg_sign, N_(\"keyid\"),\n+\t\tN_(\"GPG-sign the resulting commit\"),\n+\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG, NULL, (intptr_t) \"\"},\n+\tOPT_BOOL(0, \"dry-run\", &opt_dry_run,\n+\t\tN_(\"dry run\")),\n+\tOPT_END()\n+};\n+\n+/**\n+ * Prints unmerged (conflicting) files before error()-ing with an appropriate\n+ * message depending on the config value of advice.resolveConflict\n+ */\n+static int error_conflict(void) {\n+\tstruct rev_info rev;\n+\tint resolve_conflict = 1;\n+\tinit_revisions(&rev, opt_prefix);\n+\tgit_config(git_diff_basic_config, NULL);\n+\trev.abbrev = 0;\n+\trev.diff = 1;\n+\tDIFF_OPT_SET(&rev.diffopt, IGNORE_SUBMODULES);\n+\tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n+\trev.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;\n+\tdiff_setup_done(&rev.diffopt);\n+\tadd_head_to_pending(&rev);\n+\tif (run_diff_index(&rev, 1))\n+\t\treturn 1;\n+\tgit_config_get_bool(\"advice.resolveConflict\", &resolve_conflict);\n+\tif (resolve_conflict)\n+\t\treturn error(_(\"Pull is not possible because you have unmerged files.\\n\"\n+\t\t\t\"Please, fix them up in the work tree, and then use 'git add/rm '\\n\"\n+\t\t\t\"as appropriate to mark resolution and make a commit.\"));\n+\telse\n+\t\treturn error(_(\"Pull is not possible because you have unmerged files.\"));\n+}\n+\n+/**\n+ * Errors with an appropriate message depending on the config value of\n+ * advice.resolve_conflict\n+ */\n+static int error_merge(void)\n+{\n+\tint resolve_conflict = 1;\n+\tgit_config_get_bool(\"advice.resolveConflict\", &resolve_conflict);\n+\tif (resolve_conflict)\n+\t\treturn error(_(\"You have not concluded your merge (MERGE_HEAD exists).\\n\"\n+\t\t\t\"Please, commit your changes before you can merge.\"));\n+\telse\n+\t\treturn error(_(\"You have not concluded your merge (MERGE_HEAD exists).\"));\n+}\n+\n+/**\n+ * Returns 0 if there are no unstaged changes.\n+ */\n+static int has_unstaged_changes(void)\n+{\n+\t/* diff-files --quiet --ignore-submodules */\n+\tstruct rev_info rev_info;\n+\tint result;\n+\tinit_revisions(&rev_info, opt_prefix);\n+\tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n+\tDIFF_OPT_SET(&rev_info.diffopt, QUICK); /* --quiet */\n+\tdiff_setup_done(&rev_info.diffopt);\n+\tresult = run_diff_files(&rev_info, 0);\n+\treturn diff_result_code(&rev_info.diffopt, result);\n+}\n+\n+/**\n+ * Returns 0 if there are no uncommitted changes.\n+ */\n+static int has_uncommitted_changes(void)\n+{\n+\tstruct rev_info rev_info;\n+\tint result;\n+\tinit_revisions(&rev_info, opt_prefix);\n+\tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n+\tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n+\tadd_head_to_pending(&rev_info);\n+\tdiff_setup_done(&rev_info.diffopt);\n+\tresult = run_diff_index(&rev_info, 1);\n+\treturn diff_result_code(&rev_info.diffopt, result);\n+}\n+\n+/**\n+ * Refresh index cache and attempt to write it.\n+ */\n+static void refresh_and_write_cache(void)\n+{\n+\tstatic struct lock_file lock_file;\n+\tint newfd = hold_locked_index(&lock_file, 0);\n+\trefresh_cache(REFRESH_REALLY | REFRESH_IGNORE_SUBMODULES);\n+\tif (newfd < 0)\n+\t\treturn;\n+\tif (active_cache_changed)\n+\t\twrite_locked_index(&the_index, &lock_file, COMMIT_LOCK);\n+\trollback_lock_file(&lock_file);\n+}\n+\n+\n+static int require_clean_work_tree(const char *action, const char *dowhat)\n+{\n+\tint ret = 0;\n+\n+\trefresh_and_write_cache();\n+\tif (has_unstaged_changes())\n+\t\tret = error(_(\"Cannot %s: You have unstaged changes.\"), action);\n+\tif (has_uncommitted_changes()) {\n+\t\tif (ret)\n+\t\t\terror(_(\"Additionally, your index contains uncommitted changes.\"));\n+\t\telse\n+\t\t\tret = error(_(\"Cannot %s: Your index contains uncommitted changes.\"), action);\n+\t}\n+\tif (ret && dowhat)\n+\t\terror(\"%s\", dowhat);\n+\treturn ret;\n+}\n+\n+/**\n+ * Returns remote for branch\n+ */\n+static const char *config_get_branch_remote(const char *branch)\n+{\n+\tstruct strbuf key = STRBUF_INIT;\n+\tconst char *remote = NULL;\n+\tif (!branch)\n+\t\tbranch = head_name_short;\n+\tstrbuf_addf(&key, \"branch.%s.remote\", branch);\n+\tgit_config_get_value(key.buf, &remote);\n+\tstrbuf_release(&key);\n+\treturn remote;\n+}\n+\n+/**\n+ * Returns upstream branch name for `branch`.\n+ */\n+static const char *config_get_branch_upstream(const char *branch)\n+{\n+\tstruct strbuf key = STRBUF_INIT;\n+\tconst char *upstream = NULL;\n+\tif (!branch)\n+\t\tbranch = head_name_short;\n+\tstrbuf_addf(&key, \"branch.%s.merge\", branch);\n+\tgit_config_get_value(key.buf, &upstream);\n+\tstrbuf_release(&key);\n+\treturn upstream;\n+}\n+\n+static char *get_remote_merge_branch(int argc, const char *argv[])\n+{\n+\tif (argc <= 1) {\n+\t\tconst char *repo = argc ? argv[0] : \"\";\n+\t\tconst char *default_remote = config_get_branch_remote(NULL);\n+\t\tif (!default_remote)\n+\t\t\tdefault_remote = \"origin\";\n+\t\tif (repo[0] == '\\0')\n+\t\t\trepo = default_remote;\n+\t\tif (!strcmp(repo, default_remote)) {\n+\t\t\tstruct branch *branch = branch_get(NULL);\n+\n+\t\t\tif (!branch)\n+\t\t\t\treturn NULL;\n+\t\t\treturn xstrdup(branch->merge[0]->dst);\n+\t\t} else\n+\t\t\treturn NULL;\n+\t} else {\n+\t\tconst char *repo = argv[0];\n+\t\tstruct refspec *ref = parse_fetch_refspec(1, &argv[1]);\n+\t\tconst char *remote;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t/* FIXME: It should return the tracking branch\n+\t\t * Currently only works with the default mapping */\n+\t\tif (ref->src[0] == '\\0' || !strcmp(ref->src, \"HEAD\"))\n+\t\t\tremote = \"HEAD\";\n+\t\telse if (skip_prefix(ref->src, \"heads/\", &remote))\n+\t\t\t;\n+\t\telse if (skip_prefix(ref->src, \"refs/heads/\", &remote))\n+\t\t\t;\n+\t\telse if (starts_with(ref->src, \"refs/\") ||\n+\t\t\tstarts_with(ref->src, \"tags/\") ||\n+\t\t\tstarts_with(ref->src, \"remotes/\"))\n+\t\t\tremote = \"\";\n+\t\telse\n+\t\t\tremote = ref->src;\n+\t\tif (remote[0]) {\n+\t\t\tif (!strcmp(repo, \".\"))\n+\t\t\t\tstrbuf_addf(&out, \"refs/heads/%s\", remote);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(&out, \"refs/remotes/%s/%s\", repo, remote);\n+\t\t\treturn out.buf;\n+\t\t}\n+\t\treturn NULL;\n+\t}\n+}\n+\n+/* FIXME: The following code is copied (and slightly modified) from\n+ * merge-base.c. Ideally, the functionality should be shared */\n+\n+struct rev_collect {\n+\tstruct commit **commit;\n+\tint nr;\n+\tint alloc;\n+\tunsigned int initial : 1;\n+};\n+\n+static void add_one_commit(unsigned char *sha1, struct rev_collect *revs)\n+{\n+\tstruct commit *commit;\n+\n+\tif (is_null_sha1(sha1))\n+\t\treturn;\n+\n+\tcommit = lookup_commit(sha1);\n+\tif (!commit ||\n+\t (commit->object.flags & TMP_MARK) ||\n+\t parse_commit(commit))\n+\t\treturn;\n+\n+\tALLOC_GROW(revs->commit, revs->nr + 1, revs->alloc);\n+\trevs->commit[revs->nr++] = commit;\n+\tcommit->object.flags |= TMP_MARK;\n+}\n+\n+static int collect_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t\t const char *ident, unsigned long timestamp,\n+\t\t\t\t int tz, const char *message, void *cbdata)\n+{\n+\tstruct rev_collect *revs = cbdata;\n+\n+\tif (revs->initial) {\n+\t\trevs->initial = 0;\n+\t\tadd_one_commit(osha1, revs);\n+\t}\n+\tadd_one_commit(nsha1, revs);\n+\treturn 0;\n+}\n+\n+/**\n+ * Returns in out_sha1 the point at which a branch leading to commitname forked\n+ * from another branch ref. Returns 0 if a fork point was found, 1 if it was\n+ * not found.\n+ */\n+static int fork_point(const char *ref, const char *commitname,\n+\t\tunsigned char out_sha1[20])\n+{\n+\tunsigned char sha1[20];\n+\tchar *refname;\n+\tstruct rev_collect revs = {};\n+\tstruct commit *derived;\n+\tstruct commit_list *bases;\n+\tint i, ret = 0;\n+\n+\tif (!ref)\n+\t\treturn error(\"ref is NULL\");\n+\n+\tswitch(dwim_ref(ref, strlen(ref), sha1, &refname)) {\n+\tcase 0:\n+\t\treturn error(\"No such ref: '%s'\", ref);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\treturn error(\"Ambiguous refname: '%s'\", ref);\n+\t}\n+\n+\tif (!commitname)\n+\t\tcommitname = \"HEAD\";\n+\n+\tif (get_sha1(commitname, sha1))\n+\t\treturn error(\"Not a valid object name: '%s'\", commitname);\n+\n+\tderived = lookup_commit_reference(sha1);\n+\n+\trevs.initial = 1;\n+\n+\tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n+\n+\tfor (i = 0; i < revs.nr; i++)\n+\t\trevs.commit[i]->object.flags &= ~TMP_MARK;\n+\n+\tbases = get_merge_bases_many(derived, revs.nr, revs.commit);\n+\t/*\n+\t * There should be one and only one merge base, when we found\n+\t * a common ancestor among reflog entries.\n+\t */\n+\tif (!bases || bases->next) {\n+\t\tret = 1;\n+\t\tgoto cleanup_return;\n+\t}\n+\n+\t/* And the found one must be one of the reflog entries */\n+\tfor (i = 0; i < revs.nr; i++)\n+\t\tif (&bases->item->object == &revs.commit[i]->object)\n+\t\t\tbreak; /* found */\n+\n+\tif (revs.nr <= i) {\n+\t\tret = 1;\n+\t\tgoto cleanup_return;\n+\t}\n+\n+\tif (out_sha1)\n+\t\thashcpy(out_sha1, bases->item->object.sha1);\n+\n+cleanup_return:\n+\tfree_commit_list(bases);\n+\treturn ret;\n+}\n+\n+/**\n+ * Appends FETCH_HEAD's merge heads into arr. Returns number of merge heads,\n+ * or -1 on error.\n+ */\n+static int sha1_array_append_fetch_merge_heads(struct sha1_array *arr)\n+{\n+\tint num_heads = 0;\n+\tchar *filename = git_path(\"FETCH_HEAD\");\n+\tFILE *fp = fopen(filename, \"r\");\n+\tif (!fp) {\n+\t\terror(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n+\t\treturn -1;\n+\t}\n+\tfor (;;) {\n+\t\tunsigned char sha1[20];\n+\t\tchar sha1_text[40];\n+\t\tchar text[128];\n+\t\tint c, ret;\n+\n+\t\tret = fscanf(fp, \"%40c %127[^\\n]\", sha1_text, text);\n+\n+\t\tif (ret == EOF)\n+\t\t\tbreak;\n+\t\telse if (ret != 2) {\n+\t\t\terror(_(\"error parsing %s\"), filename);\n+\t\t\tfclose(fp);\n+\t\t\treturn -1;\n+\t\t}\n+\t\t/* Read until the next newline */\n+\t\tfor (;;) {\n+\t\t\tc = fgetc(fp);\n+\t\t\tif (c == '\\n' || c == EOF)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\t/* Skip over merge heads that are marked not-for-merge */\n+\t\tif (strstr(text, \"not-for-merge\"))\n+\t\t\tcontinue;\n+\t\t/* Parse SHA1 */\n+\t\tif (get_sha1_hex(sha1_text, sha1)) {\n+\t\t\terror(_(\"failed to parse SHA1 from %s\"), filename);\n+\t\t\tfclose(fp);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tsha1_array_append(arr, sha1);\n+\t\tnum_heads ++;\n+\t}\n+\tfclose(fp);\n+\treturn num_heads;\n+}\n+\n+static int get_octopus_merge_base(const struct sha1_array *rev_sha1s,\n+\t\tunsigned char merge_base_sha1[20])\n+{\n+\tstruct commit_list *revs = NULL, *result;\n+\tint i;\n+\tfor (i = rev_sha1s->nr - 1; i >= 0; i--)\n+\t\tcommit_list_insert(lookup_commit_reference(rev_sha1s->sha1[i]), &revs);\n+\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tfree_commit_list(revs);\n+\tif (!result)\n+\t\treturn 1;\n+\tif (merge_base_sha1)\n+\t\thashcpy(merge_base_sha1, result->item->object.sha1);\n+\treturn 0;\n+}\n+\n+static void argv_array_push_verbosity(struct argv_array *arr, int verbosity)\n+{\n+\twhile (verbosity) {\n+\t\tif (verbosity < 0) {\n+\t\t\targv_array_push(arr, \"-q\");\n+\t\t\tverbosity++;\n+\t\t} else if (verbosity > 0) {\n+\t\t\targv_array_push(arr, \"-v\");\n+\t\t\tverbosity--;\n+\t\t}\n+\t}\n+}\n+\n+static void argv_array_push_strategies(struct argv_array *arr,\n+\t\tconst struct string_list *strategies)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, strategies)\n+\t\targv_array_pushl(arr, \"-s\", item->string, NULL);\n+}\n+\n+\n+static void argv_array_push_merge_args(struct argv_array *arr,\n+\t\tconst struct string_list *merge_args)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, merge_args)\n+\t\targv_array_pushl(arr, \"-X\", item->string, NULL);\n+}\n+\n+\n+static void argv_array_push_rs(struct argv_array *arr, int recurse_submodules)\n+{\n+\tswitch (recurse_submodules) {\n+\tcase RS_NO:\n+\t\targv_array_push(arr, \"--no-recurse-submodules\");\n+\t\tbreak;\n+\tcase RS_YES:\n+\t\targv_array_push(arr, \"--recurse-submodules\");\n+\t\tbreak;\n+\tcase RS_ON_DEMAND:\n+\t\targv_array_push(arr, \"--recurse-submodules=on-demand\");\n+\t\tbreak;\n+\t}\n+}\n+\n+static void argv_array_push_ff(struct argv_array *arr, int ff)\n+{\n+\tswitch(ff) {\n+\tcase FF_NO:\n+\t\targv_array_push(arr, \"--no-ff\");\n+\t\tbreak;\n+\tcase FF_ALLOW:\n+\t\targv_array_push(arr, \"--ff\");\n+\t\tbreak;\n+\tcase FF_ONLY:\n+\t\targv_array_push(arr, \"--ff-only\");\n+\t\tbreak;\n+\t}\n+}\n+\n+static void argv_array_push_gpg_sign(struct argv_array *arr,\n+\t\tconst char *gpg_sign)\n+{\n+\tstruct strbuf arg = STRBUF_INIT;\n+\tif (!gpg_sign)\n+\t\treturn;\n+\tstrbuf_addf(&arg, \"-S%s\", gpg_sign);\n+\targv_array_push(arr, arg.buf);\n+\tstrbuf_release(&arg);\n+}\n+\n+static void argv_array_push_shortlog_len(struct argv_array *arr,\n+\t\tint shortlog_len)\n+{\n+\tstruct strbuf arg = STRBUF_INIT;\n+\tstrbuf_addf(&arg, \"--log=%d\", shortlog_len);\n+\targv_array_push(arr, arg.buf);\n+\tstrbuf_release(&arg);\n+}\n+\n+static int error_on_no_merge_candidates(int argc, const char *argv[])\n+{\n+\tconst char *op_type, *op_prep, *upstream, *remote;\n+\n+\top_type = opt_rebase ? \"rebase\" : \"merge\";\n+\top_prep = opt_rebase ? \"against\" : \"with\";\n+\n+\tupstream = config_get_branch_upstream(NULL);\n+\tif (upstream)\n+\t\tskip_prefix(upstream, \"refs/heads/\", &upstream);\n+\tremote = config_get_branch_remote(NULL);\n+\tif (!remote)\n+\t\tremote = \"origin\";\n+\tif (argc > 1) {\n+\t\tif (opt_rebase)\n+\t\t\tfprintf(stderr, \"There is no candidate for rebasing against \");\n+\t\telse\n+\t\t\tfprintf(stderr, \"There are no candidates for merging \");\n+\t\tfputs(\"among the refs that you just fetched.\\n\"\n+\t\t\t\"Generally this means that you provided a wildcard refspec which had no\\n\"\n+\t\t\t\"matches on the remote end.\\n\", stderr);\n+\t} else if (argc > 0 && strcmp(argv[0], remote))\n+\t\tfprintf(stderr, _(\"You asked to pull from the remote '%s', but did not specify\\n\"\n+\t\t\t\"a branch. Because this is not the default configured remote\\n\"\n+\t\t\t\"for your current branch, you must specify a branch on the command line.\\n\"), argv[0]);\n+\telse if (is_null_sha1(head_sha1) || !upstream)\n+\t\tfprintf(stderr, _(\"There is no tracking information for the current branch.\\n\"\n+\t\t\t\"Please specify which branch you want to %s %s\\n\"\n+\t\t\t\"See git-pull(1) for details\\n\\n\"\n+\t\t\t\"\\tgit pull \\n\\n\"\n+\t\t\t\"If you wish to set tracking information for this branch you can do so with:\\n\\n\"\n+\t\t\t\"\\tgit branch --set-upstream-to=%s/ %s\\n\"),\n+\t\t\top_type, op_prep, remote, head_name_short);\n+\telse\n+\t\tfprintf(stderr, _(\"Your configuration specified to %s %s the ref '%s'\\n\"\n+\t\t\t\"from the remote, but no such ref was fetched.\\n\"),\n+\t\t\top_type, op_prep, upstream);\n+\treturn 1;\n+}\n+\n+int cmd_pull(int argc, const char *argv[], const char *prefix)\n+{\n+\tstruct argv_array cmd_args = ARGV_ARRAY_INIT;\n+\tunsigned char oldremoteref[20] = {0};\n+\tunsigned char orig_head_sha1[20];\n+\tstruct sha1_array merge_heads = SHA1_ARRAY_INIT;\n+\tstruct strbuf merge_name = STRBUF_INIT;\n+\tint i, ret = 0, reflog_msg_set = 0;\n+\n+\topt_prefix = prefix;\n+\n+\tgit_config(git_diff_basic_config, NULL);\n+\n+\tif (read_cache() < 0)\n+\t\treturn 1;\n+\n+\tif (unmerged_cache())\n+\t\treturn error_conflict();\n+\n+\tif (file_exists(git_path(\"MERGE_HEAD\")))\n+\t\treturn error_merge();\n+\n+\thead_name = resolve_refdup(\"HEAD\", 0, head_sha1, &head_flags);\n+\tif (!head_name)\n+\t\treturn error(_(\"could not resolve ref HEAD\"));\n+\tif (!skip_prefix(head_name, \"refs/heads/\", &head_name_short))\n+\t\thead_name_short = head_name;\n+\n+\t/* Set reflog message */\n+\tif (!getenv(\"GIT_REFLOG_ACTION\")) {\n+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tstrbuf_addf(&msg, \"pull \");\n+\t\tfor (i = 1; i < argc; i++)\n+\t\t\tstrbuf_add(&msg, argv[i], strlen(argv[i]));\n+\t\tmsg.buf[msg.len - 1] = '\\0'; /* Remove trailing space */\n+\t\tif (!setenv(\"GIT_REFLOG_ACTION\", msg.buf, 1))\n+\t\t\treflog_msg_set = 1;\n+\t\tstrbuf_release(&msg);\n+\t}\n+\n+\tgit_config(fmt_merge_msg_config, NULL);\n+\n+\topt_shortlog_len = merge_log_config;\n+\topt_ff = ff_config_default();\n+\topt_rebase = rebase_config_default();\n+\n+\targc = parse_options(argc, argv, prefix, builtin_pull_options,\n+\t\t\tbuiltin_pull_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (opt_rebase) {\n+\t\tchar *remote_ref;\n+\n+\t\tif (is_null_sha1(head_sha1)) {\n+\t\t\t/* If on unborn branch, index must be unborn as well */\n+\t\t\tif (!is_cache_unborn())\n+\t\t\t\tdie(_(\"updating an unborn branch with changes added to the index\"));\n+\t\t} else {\n+\t\t\tif ((ret = require_clean_work_tree(_(\"pull with rebase\"), _(\"Please commit or stash them.\"))))\n+\t\t\t\tgoto cleanup;\n+\t\t}\n+\t\tremote_ref = get_remote_merge_branch(argc, argv);\n+\t\tif (remote_ref) {\n+\t\t\tif (fork_point(remote_ref, head_name, oldremoteref))\n+\t\t\t\thashclr(oldremoteref);\n+\t\t\tfree(remote_ref);\n+\t\t}\n+\t}\n+\n+\t/* Before running git-fetch, store original head SHA1 */\n+\thashcpy(orig_head_sha1, head_sha1);\n+\n+\t/* Run git-fetch */\n+\targv_array_push(&cmd_args, \"fetch\");\n+\targv_array_push_verbosity(&cmd_args, opt_verbosity);\n+\tif (opt_progress >= 0)\n+\t\targv_array_push(&cmd_args, opt_progress ? \"--progress\" : \"--no-progress\");\n+\targv_array_push(&cmd_args, opt_dry_run ? \"--dry-run\" : \"--no-dry-run\");\n+\targv_array_push_rs(&cmd_args, opt_recurse_submodules);\n+\targv_array_push(&cmd_args, \"--update-head-ok\");\n+\tfor (i = 0; i < argc; i++)\n+\t\targv_array_push(&cmd_args, argv[i]);\n+\tif (cmd_fetch(cmd_args.argc, cmd_args.argv, opt_prefix)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\t/* FIXME: cmd_fetch frees strings in argv */\n+\targv_array_init(&cmd_args);\n+\n+\tif (opt_dry_run)\n+\t\tgoto cleanup;\n+\n+\t/* Get current head SHA1 */\n+\thead_name = resolve_refdup(\"HEAD\", 0, head_sha1, &head_flags);\n+\n+\tif (!is_null_sha1(orig_head_sha1) && hashcmp(head_sha1, orig_head_sha1)) {\n+\t\t/* The fetch involved updating the current branch.\n+\t\t * The working tree and the index file is still based on\n+\t\t * orig_head_sha1 commit, but we are merging into head_sha1.\n+\t\t * First update the working tree to match head_sha1\n+\t\t */\n+\t\twarning(_(\"fetch updated the current branch head\"));\n+\t\twarning(_(\"fast-forwarding your working tree from commit %s\"),\n+\t\t\t\tsha1_to_hex(orig_head_sha1));\n+\t\trefresh_and_write_cache();\n+\t\t/* TODO: Need to catch die() and write the user-friendly\n+\t\t * recovery message */\n+\t\targv_array_pushl(&cmd_args, \"read-tree\", \"-u\", \"-m\", NULL);\n+\t\targv_array_push(&cmd_args, sha1_to_hex(orig_head_sha1));\n+\t\targv_array_push(&cmd_args, sha1_to_hex(head_sha1));\n+\t\tif ((ret = cmd_read_tree(cmd_args.argc, cmd_args.argv, opt_prefix)))\n+\t\t\tgoto cleanup;\n+\t\targv_array_clear(&cmd_args);\n+\t}\n+\n+\t/* Read merge heads from FETCH_HEAD */\n+\tif (sha1_array_append_fetch_merge_heads(&merge_heads) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (merge_heads.nr == 0) {\n+\t\tret = error_on_no_merge_candidates(argc, argv);\n+\t\tgoto cleanup;\n+\t} else if (merge_heads.nr > 1) {\n+\t\tif (is_null_sha1(orig_head_sha1)) {\n+\t\t\tret = error(_(\"Cannot merge multiple branches into empty head\"));\n+\t\t\tgoto cleanup;\n+\t\t} else if (opt_rebase) {\n+\t\t\tret = error(_(\"Cannot rebase into multiple branches\"));\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n+\t/* Pulling into unborn branch: a shorthand for branching off\n+\t * FETCH_HEAD, for lazy typers */\n+\tif (is_null_sha1(orig_head_sha1)) {\n+\t\t/* Two-way merge: we claim the index is based on an empty tree,\n+\t\t * and try to fast-forward to HEAD. This ensures we will not\n+\t\t * lose index/worktree changes that the user already made on\n+\t\t * the unborn branch.\n+\t\t */\n+\t\targv_array_pushl(&cmd_args, \"read-tree\", \"-u\", \"-m\",\n+\t\t\t\tEMPTY_TREE_SHA1_HEX,\n+\t\t\t\tsha1_to_hex(merge_heads.sha1[0]), NULL);\n+\t\tif ((ret = cmd_read_tree(cmd_args.argc, cmd_args.argv, opt_prefix)))\n+\t\t\tgoto cleanup;\n+\t\t/* FIXME: read_tree */\n+\t\targv_array_init(&cmd_args); /* argv_array_clear(&cmd_args) */\n+\t\targv_array_pushl(&cmd_args, \"update-ref\", \"-m\", \"initial pull\",\n+\t\t\t\t\"HEAD\", NULL);\n+\t\targv_array_push(&cmd_args, sha1_to_hex(merge_heads.sha1[0]));\n+\t\targv_array_push(&cmd_args, sha1_to_hex(head_sha1));\n+\t\tret = cmd_update_ref(cmd_args.argc, cmd_args.argv, opt_prefix);\n+\t\tgoto cleanup;\n+\t}\n+\tif (opt_rebase) {\n+\t\tstruct sha1_array octopus_merge_heads = SHA1_ARRAY_INIT;\n+\t\tunsigned char octopus_merge_base[20];\n+\n+\t\tsha1_array_append(&octopus_merge_heads, head_sha1);\n+\t\tsha1_array_append(&octopus_merge_heads, merge_heads.sha1[0]);\n+\t\tif (!is_null_sha1(oldremoteref))\n+\t\t\tsha1_array_append(&octopus_merge_heads, oldremoteref);\n+\t\tif (get_octopus_merge_base(&octopus_merge_heads, octopus_merge_base)) {\n+\t\t\tret = error(_(\"could not get octopus merge base\"));\n+\t\t\tsha1_array_clear(&octopus_merge_heads);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tsha1_array_clear(&octopus_merge_heads);\n+\t\tif (!hashcmp(octopus_merge_base, oldremoteref))\n+\t\t\thashclr(oldremoteref);\n+\t}\n+\t/* fmt-merge-msg */\n+\t{\n+\t\tstruct fmt_merge_msg_opts opts = {0};\n+\t\tFILE *fp;\n+\t\tchar *filename = git_path(\"FETCH_HEAD\");\n+\t\tstruct strbuf input = STRBUF_INIT;\n+\t\topts.add_title = 1;\n+\t\topts.credit_people = 1;\n+\t\topts.shortlog_len = opt_shortlog_len;\n+\t\tfp = fopen(filename, \"r\");\n+\t\tif (!fp) {\n+\t\t\tret = error(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (strbuf_read(&input, fileno(fp), 0) < 0) {\n+\t\t\tret = error(_(\"could not read %s: %s\\n\"), filename, strerror(errno));\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tfclose(fp);\n+\t\tif ((ret = fmt_merge_msg(&input, &merge_name, &opts)))\n+\t\t\tgoto cleanup;\n+\t}\n+\n+\tif (opt_rebase) {\n+\t\targv_array_push(&cmd_args, \"rebase\");\n+\t\tif (opt_diffstat >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_diffstat ? \"--stat\" : \"--no-stat\");\n+\t\targv_array_push_strategies(&cmd_args, &opt_merge_strategies);\n+\t\targv_array_push_merge_args(&cmd_args, &opt_merge_strategy_opts);\n+\t\tif (opt_rebase == REBASE_PRESERVE)\n+\t\t\targv_array_push(&cmd_args, \"--preserve-merges\");\n+\t\targv_array_push_verbosity(&cmd_args, opt_verbosity);\n+\t\targv_array_push_gpg_sign(&cmd_args, opt_gpg_sign);\n+\t\targv_array_push(&cmd_args, \"--onto\");\n+\t\targv_array_push(&cmd_args, sha1_to_hex(merge_heads.sha1[0]));\n+\t\targv_array_push(&cmd_args, sha1_to_hex(is_null_sha1(oldremoteref) ? merge_heads.sha1[0] : oldremoteref));\n+\t\tif (run_command_v_opt(cmd_args.argv, RUN_GIT_CMD)) {\n+\t\t\tret = 1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\targv_array_clear(&cmd_args);\n+\t} else {\n+\t\targv_array_push(&cmd_args, \"merge\");\n+\t\tif (opt_diffstat >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_diffstat ? \"--stat\" : \"--no-stat\");\n+\t\tif (opt_commit >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_commit ? \"--commit\" : \"--no-commit\");\n+\t\tif (opt_verify_signatures >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_verify_signatures ? \"--verify-signatures\" : \"--no-verify-signatures\");\n+\t\tif (opt_edit >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_edit ? \"--edit\" : \"--no-edit\");\n+\t\tif (opt_squash >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_squash ? \"--squash\" : \"--no-squash\");\n+\t\targv_array_push_ff(&cmd_args, opt_ff);\n+\t\targv_array_push_shortlog_len(&cmd_args, opt_shortlog_len);\n+\t\targv_array_push_strategies(&cmd_args, &opt_merge_strategies);\n+\t\targv_array_push_merge_args(&cmd_args, &opt_merge_strategy_opts);\n+\t\targv_array_push_verbosity(&cmd_args, opt_verbosity);\n+\t\tif (opt_progress >= 0)\n+\t\t\targv_array_push(&cmd_args, opt_progress ? \"--progress\" : \"--no-progress\");\n+\t\targv_array_push_gpg_sign(&cmd_args, opt_gpg_sign);\n+\t\targv_array_pushl(&cmd_args, merge_name.buf, \"HEAD\", NULL);\n+\t\tfor (i = 0; i < merge_heads.nr; i++)\n+\t\t\targv_array_push(&cmd_args, sha1_to_hex(merge_heads.sha1[i]));\n+\t\tif ((ret = cmd_merge(cmd_args.argc, cmd_args.argv, opt_prefix)))\n+\t\t\tgoto cleanup;\n+\t\t/* FIXME: merge */\n+\t\targv_array_init(&cmd_args); /* argv_array_clear(&cmd_args); */\n+\t}\n+cleanup:\n+\t/* argv_array_clear(&cmd_args); */\n+\tfree(head_name);\n+\tstring_list_clear(&opt_merge_strategies, 0);\n+\tstring_list_clear(&opt_merge_strategy_opts, 0);\n+\tif (reflog_msg_set)\n+\t\tunsetenv(\"GIT_REFLOG_ACTION\");\n+\treturn ret;\n+}\ndiff --git a/git.c b/git.c\nindex 8c7ee9c..c322b07 100644\n--- a/git.c\n+++ b/git.c\n@@ -443,6 +443,7 @@ static struct cmd_struct commands[] = {\n \t{ \"pickaxe\", cmd_blame, RUN_SETUP },\n \t{ \"prune\", cmd_prune, RUN_SETUP },\n \t{ \"prune-packed\", cmd_prune_packed, RUN_SETUP },\n+\t{ \"pull\", cmd_pull, RUN_SETUP | NEED_WORK_TREE},\n \t{ \"push\", cmd_push, RUN_SETUP },\n \t{ \"read-tree\", cmd_read_tree, RUN_SETUP },\n \t{ \"receive-pack\", cmd_receive_pack },\n-- \n2.1.4\n"},{"id":"257920","messageId":"000701d06156$dd3145e0$9793d1a0$@gmail.com","threadId":"38831","inReplyTo":"1426600662-32276-1-git-send-email-pyokagan@gmail.com","subject":"RE: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Stephen Robin","fromEmail":"stephen.robin@gmail.com","sentAt":null,"receivedAt":"2015-03-18T08:35:09Z","isPatch":true,"sender":{"key":"stephen.robin@gmail.com","avatar":null},"body":"\n> Paul Tan writes:\n> I would like to share this very rough prototype with everyone. \n...\n> I started this as a just-for-fun exercise to learn about the git internal\nAPI\n\nI started to rewrite git-pull for similar reasons a couple of months ago,\nbut I haven't had time to complete it. It looks like my version has less\nwork remaining than yours, would you like me to share it?\n\n> Finally, there is the possibility that in the process of conversion bugs\nor incompatible behavioral changes may be introduced which are not caught by\nthe test suite. Ideally, I think the solution is to improve the test suite\nand make it as comprehensive as possible, but writing a comprehensive test\nsuite may be too time consuming. \n\nThis is why I haven't had time to finish and submit my patch. While the c\ncode is pretty much complete, I felt the test suite needed some extension\nbefore I could be confident the new code is correct.\n"},{"id":"257921","messageId":"4cae16222697894b19856d12b062f68e@www.dscho.org","threadId":"38831","inReplyTo":"1426600662-32276-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-03-18T09:00:40Z","receivedAt":"2015-03-18T09:00:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nthank you for this very detailed mail. It was a real pleasure to read this well-researched document.\n\nIn the following, I will pick out only parts from the mail, in the interest of both of our time. Please assume that I agree with everything that I do not quote below (and even the with quoted parts I agree mostly ;-)).\n\nOn 2015-03-17 14:57, Paul Tan wrote:\n\n> on Windows the runtime fell from 8m 25s to 1m 3s.\n\nThis is *exactly* the type of benefit that makes this project so important! Nice one.\n\n> A simpler, but less perfect strategy might be to just convert the shell\n> scripts directly statement by statement to C, using the run_command*()\n> functions as Duy Nguyen[2] suggested, before changing the code to use\n> the internal API.\n\nYeah, the idea is to have a straight-forward strategy to convert the scripts in as efficient manner as possible. It also makes reviewing easier if the first step is an almost one-to-one translation to `run_command*()`-based builtins.\n\nPlus, it is rewarding to have concise steps that can be completed in a timely manner.\n\n\n> +/* NOTE: git-pull.sh only supports --log and --no-log, as opposed to what\n> + * man git-pull says. */\n> +static int opt_shortlog_len;\n\nThis comment might want to live in the commit message instead... It is prone to get stale in the code while it will always be correct (and easier to spot) in the commit message.\n\n> +/**\n> + * Returns default rebase option value\n> + */\n> +static int rebase_config_default(void)\n> +{\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\tconst char *value = NULL;\n> +\tint boolval;\n> +\n> +\tstrbuf_addf(&name, \"branch.%s.rebase\", head_name_short);\n> +\tif (git_config_get_value(name.buf, &value))\n> +\t\tgit_config_get_value(\"pull.rebase\", &value);\n> +\tstrbuf_release(&name);\n> +\tif (!value)\n> +\t\treturn REBASE_FALSE;\n> +\tboolval = git_config_maybe_bool(\"pull.rebase\", value);\n> +\tif (boolval >= 0)\n> +\t\treturn boolval ? REBASE_TRUE : REBASE_FALSE;\n> +\telse if (value && !strcmp(value, \"preserve\"))\n> +\t\treturn REBASE_PRESERVE;\n> +\tdie(_(\"invalid value for branch.%s.rebase \\\"%s\\\"\"), head_name_short, value);\n> +}\n\nPersonally, I would use a couple of empty lines for enhanced structure. Conceptually, there are four parts: the variable declarations, querying the config, parsing the value, and `die()`ing in case of error. There is already an empty line between the first two parts, and I would imagine that readability would be improved further by having an empty line after the `strbuf_release()` and the `return REBASE_PRESERVE` statement, respectively.\n\n> +static int parse_opt_rebase(const struct option *opt, const char\n> *arg, int unset)\n> +{\n> +\tif (arg)\n> +\t\t*(int *)opt->value = parse_rebase(arg);\n> +\telse\n> +\t\t*(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;\n> +\treturn (*(int *)opt->value) >= 0 ? 0 : -1;\n> +}\n\nIn this function (and also in other places below), there is this pattern that a `struct option` pointer is passed to the function, but then only `*(int *)opt->value` is written to. Therefore, I would suggest to change the signature of the function and pass `(int *)opt->value` as function parameter.\n\n> +static int has_unstaged_changes(void)\n\nYeah, this function, as well as the ones below it, look as if they are so common that they *should* be already somewhere in libgit.a. But I did not find them, either...\n\nOf course it *would* be nice to identify places where essentially the same code is needed, and refactor accordingly. But I think that is outside the scope of this project.\n\nThe rest looks pretty good (and you realize that my comments above are really more nit picks than anything else).\n\nThe FIXMEs should be relatively easy to address. It would be my pleasure to work with you.\n\nCiao,\nJohannes\n"},{"id":"257922","messageId":"457620dde2d2e403a564889367f43955@www.dscho.org","threadId":"38831","inReplyTo":"000701d06156$dd3145e0$9793d1a0$@gmail.com","subject":"RE: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-03-18T09:24:29Z","receivedAt":"2015-03-18T09:24:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stephen,\n\nOn 2015-03-18 09:38, Stephen Robin wrote:\n>> Paul Tan writes:\n>> I would like to share this very rough prototype with everyone.\n> ...\n>> I started this as a just-for-fun exercise to learn about the git internal\n> API\n> \n> I started to rewrite git-pull for similar reasons a couple of months ago,\n> but I haven't had time to complete it. It looks like my version has less\n> work remaining than yours, would you like me to share it?\n\nHmm. It always results in an unnecessary round trip if you ask people to ask you to share it.\n\nAt this point, because Paul already made his work public, I would say that we should continue with his version. Please understand this as an encouragement for the future to share your code pro-actively, just like Git's own source code has been shared with you without the need for you to request it explicitly.\n\nFor the record, Duy also already shared his C code to implement `git pull` in C publicly. I think that Paul decided to start again in order to understand every detail of the process of converting a shell script into a builtin; If that was the rationale, I would agree that it is a wise one.\n\nCiao,\nJohannes\n"},{"id":"257966","messageId":"vpqwq2eyyzl.fsf@anie.imag.fr","threadId":"38831","inReplyTo":"1426600662-32276-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-18T17:52:30Z","receivedAt":"2015-03-18T17:52:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nFirst of all, thanks a lot for working on this. I'm rather impressed to\nsee a working proof of concept so soon! And impressed by the quality for\na \"first draft\".\n\nA few minor remaks below after a very quick look.\n\nPaul Tan writes:\n\n> Ideally, I think the solution is to\n> improve the test suite and make it as comprehensive as possible, but\n> writing a comprehensive test suite may be too time consuming.\n\ntime-consuming, but also very beneficial since the code would end up\nbeing better tested. For sure, a rewrite is a good way to break stuff,\nbut anything untested can also be broken by mistake rather easily at any\ntime.\n\nI'd suggest doing a bit of manual mutation testing: take your C code,\ncomment-out a few lines of code, see if the tests still pass, and if\nthey do, add a failing test that passes again once you uncomment the\ncode.\n\n> diff --git a/Makefile b/Makefile\n> index 44f1dd1..50a6a16 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -470,7 +470,6 @@ SCRIPT_SH += git-merge-octopus.sh\n> SCRIPT_SH += git-merge-one-file.sh\n> SCRIPT_SH += git-merge-resolve.sh\n> SCRIPT_SH += git-mergetool.sh\n> -SCRIPT_SH += git-pull.sh\n\nWhen converting a script into a builtin, we usually move the old script\nto contrib/examples/.\n\n> +static const char * const builtin_pull_usage[] = {\n> +\tN_(\"git pull [-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff|--ff-only] [--[no-]rebase|--rebase=preserve] [-s strategy]... [] ...\"),\n\nI know we have many instances of very long lines for usage string, but\nit would be better IMHO to wrap it both in the code and in the output of\n\"git pull -h\".\n\n> +/* NOTE: git-pull.sh only supports --log and --no-log, as opposed to what\n> + * man git-pull says. */\n\nWe usually write multi-line comments\n\n/*\n * like\n * this\n */\n\n> +/* Global vars since they are used often */\n\nBeing use often does not count as an excuse for being global IMHO.\nHaving global variables means you share the same instance in several\nfunctions, and you have to be careful with things like\n\nvoid g()\n{\n\tglob = bar;\n}\n\nvoid f()\n{\n\tglob = foo;\n\tg();\n\tbar = glob;\n}\n\nAs a reader, I'd rather not have to be careful about this to keep my\nneurons free for other things.\n\n> +static char *head_name;\n\nActually, this one is used only in one function, so \"often\" is not even\ntrue ;-).\n\n> +static struct option builtin_pull_options[] = {\n\nYou may also declare this as local in cmd_pull().\n\n> +/**\n> + * Returns remote for branch\n\nHere and elsewhere: use imperative (return, not return_s_). The comment\nasks the function to return a value.\n\n> +\tstrbuf_addf(&key, \"branch.%s.remote\", branch);\n> +\tgit_config_get_value(key.buf, &remote);\n> +\tstrbuf_release(&key);\n\nThis config API is beautiful :-).\n\n(Before last year's GSoC, you'd have needed ~10 lines of code to do the\nsame thing)\n\n> +\t\treturn error(\"Ambiguous refname: '%s'\", ref);\n\nHere and elsewhere: don't forget to mark strings for translation.\n\n> +/**\n> + * Appends FETCH_HEAD's merge heads into arr. Returns number of merge heads,\n> + * or -1 on error.\n> + */\n> +static int sha1_array_append_fetch_merge_heads(struct sha1_array *arr)\n> +{\n> +\tint num_heads = 0;\n> +\tchar *filename = git_path(\"FETCH_HEAD\");\n> +\tFILE *fp = fopen(filename, \"r\");\n\nI guess this is one instance where we could avoid writting (fetch) and\nthen parsing (here) if we had a better internal API.\n\nBut that can come after, of course.\n\n> +}\n> +\n> +\n> +static void argv_array_push_merge_args(struct argv_array *arr,\n\nBikeshed: you sometimes have two blank lines between functions,\nsometimes one. Not sure it's intended.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257994","messageId":"xmqqpp86kkn9.fsf@gitster.dls.corp.google.com","threadId":"38831","inReplyTo":"1426600662-32276-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-18T22:26:02Z","receivedAt":"2015-03-18T22:26:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> +/* Global vars since they are used often */\n> +static char *head_name;\n> +static const char *head_name_short;\n> +static unsigned char head_sha1[20];\n> +static int head_flags;\n> +\n> +enum rebase_type {\n> +\tREBASE_FALSE = 0,\n> +\tREBASE_TRUE = 1,\n> +\tREBASE_PRESERVE = 2\n> +};\n> +\n> +/**\n> + * Parse rebase config/option value and return corresponding int\n> + */\n> +static int parse_rebase(const char *arg)\n> +{\n> +\tif (!strcmp(arg, \"true\"))\n> +\t\treturn REBASE_TRUE;\n> +\telse if (!strcmp(arg, \"false\"))\n> +\t\treturn REBASE_FALSE;\n> +\telse if (!strcmp(arg, \"preserve\"))\n> +\t\treturn REBASE_PRESERVE;\n> +\telse\n> +\t\treturn -1; /* Invalid value */\n> +}\n\nEven though the original does not use bool-or-string-config, we\nwould want to do the same by doing something like\n\n\tcase (config_maybe_bool()) {\n\tcase 0:\n \treturn REBASE_FALSE;\n\tcase 1:\n \treturn REBASE_TRUE;\n\tdefault:\n \tif (!strcmp(arg, \"preserve\"))\n \treturn REBASE_PRESERVE;\n\t\treturn -1;\n\t}\n\nand then use that in rebase_config_default().\n\n> +\n> +/**\n> + * Returns default rebase option value\n> + */\n> +static int rebase_config_default(void)\n> +{\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\tconst char *value = NULL;\n> +\tint boolval;\n> +\n> +\tstrbuf_addf(&name, \"branch.%s.rebase\", head_name_short);\n> +\tif (git_config_get_value(name.buf, &value))\n> +\t\tgit_config_get_value(\"pull.rebase\", &value);\n\nWhat happens when neither is defined?\n\n> +\tstrbuf_release(&name);\n> +\tif (!value)\n> +\t\treturn REBASE_FALSE;\n\nHmph, are you sure about this? Isn't this \"[pull] rebase\" that does\nnot have \"= value\", in which case pull.rebase is \"true\"?\n\nYou cannot use NULL as the sentinel value to tell that you did not\nfind either branch.*.rebase nor pull.rebase (in which case you want\nto default to 'false'). Either of them can be spelled as an\nequal-less true, which you will see as value==NULL, and you want to\ntake that as 'true'.\n\n\tconst char *value = \"false\";\n ...\n if (get_value(..., &value))\n \tget_value(..., &value));\n strbuf_release(&name);\n if (!value)\n \treturn REBASE_TRUE;\n\treturn parse_rebase(value);\n\nor something along that line, perhaps?\n\n> +\tboolval = git_config_maybe_bool(\"pull.rebase\", value);\n> +\tif (boolval >= 0)\n> +\t\treturn boolval ? REBASE_TRUE : REBASE_FALSE;\n> +\telse if (value && !strcmp(value, \"preserve\"))\n> +\t\treturn REBASE_PRESERVE;\n\nIs value something you need to free before returning from this\nfunction?\n\n> +static int parse_opt_recurse_submodules(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tif (!arg)\n> +\t\t*(int *)opt->value = unset ? RS_NO : RS_YES;\n> +\telse if (!strcmp(arg, \"no\"))\n> +\t\t*(int *)opt->value = RS_NO;\n> +\telse if (!strcmp(arg, \"yes\"))\n> +\t\t*(int *)opt->value = RS_YES;\n> +\telse if (!strcmp(arg, \"on-demand\"))\n> +\t\t*(int *)opt->value = RS_ON_DEMAND;\n> +\telse\n> +\t\treturn -1;\n> +\treturn 0;\n\nI suspect that maybe-bool-or-string comment applies equally here for\nthe UI consistency.\n\nI'll stop here for now. Thanks.\n"},{"id":"258243","messageId":"CACRoPnQp-3RGAPmoo1aM2KUA-phi138J2o82FwP9annM+xvOXA@mail.gmail.com","threadId":"38831","inReplyTo":"vpqwq2eyyzl.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T13:23:21Z","receivedAt":"2015-03-21T13:23:21Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nThanks for the review, though I would like to work on the proposal now\nbefore the deadline passes :)\n\nOn Thu, Mar 19, 2015 at 1:52 AM, Matthieu Moy\n wrote:\n> Paul Tan writes:\n>\n>> Ideally, I think the solution is to\n>> improve the test suite and make it as comprehensive as possible, but\n>> writing a comprehensive test suite may be too time consuming.\n>\n> time-consuming, but also very beneficial since the code would end up\n> being better tested. For sure, a rewrite is a good way to break stuff,\n> but anything untested can also be broken by mistake rather easily at any\n> time.\n>\n> I'd suggest doing a bit of manual mutation testing: take your C code,\n> comment-out a few lines of code, see if the tests still pass, and if\n> they do, add a failing test that passes again once you uncomment the\n> code.\n\nMaybe code coverage tools could help here so we only need to focus on\nthe code paths that are untested by the test suite. At the minimum,\nall of the non-trivial code paths in both the shell script and the\nconverted builtin must be covered by tests. This should help to\neliminate most sources of breakages. Anything further than that would\nrequire an experienced understanding of all the possible important\ninputs to be tested, which I personally feel would make the project\nquite tedious.\n\nI see git already has gcov support. For shell scripts, maybe kcov[1]\ncould be used. With some slight code changes, I managed to generate a\nreport for the git-pull tests[2] which should at least provide a good\nstarting point for how the tests can be improved.\n\n[1] http://simonkagstrom.github.io/kcov/\n[2] \n\nRegards,\nPaul\n"},{"id":"258244","messageId":"CACRoPnThLStsBo3XH7AjZ5W+8G1G3M-tPr=fKGvduykykDoQCQ@mail.gmail.com","threadId":"38831","inReplyTo":"xmqqpp86kkn9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T13:40:18Z","receivedAt":"2015-03-21T13:40:18Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Thu, Mar 19, 2015 at 6:26 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> +/* Global vars since they are used often */\n>> +static char *head_name;\n>> +static const char *head_name_short;\n>> +static unsigned char head_sha1[20];\n>> +static int head_flags;\n>> +\n>> +enum rebase_type {\n>> + REBASE_FALSE = 0,\n>> + REBASE_TRUE = 1,\n>> + REBASE_PRESERVE = 2\n>> +};\n>> +\n>> +/**\n>> + * Parse rebase config/option value and return corresponding int\n>> + */\n>> +static int parse_rebase(const char *arg)\n>> +{\n>> + if (!strcmp(arg, \"true\"))\n>> + return REBASE_TRUE;\n>> + else if (!strcmp(arg, \"false\"))\n>> + return REBASE_FALSE;\n>> + else if (!strcmp(arg, \"preserve\"))\n>> + return REBASE_PRESERVE;\n>> + else\n>> + return -1; /* Invalid value */\n>> +}\n>\n> Even though the original does not use bool-or-string-config, we\n> would want to do the same by doing something like\n>\n> case (config_maybe_bool()) {\n> case 0:\n> return REBASE_FALSE;\n> case 1:\n> return REBASE_TRUE;\n> default:\n> if (!strcmp(arg, \"preserve\"))\n> return REBASE_PRESERVE;\n> return -1;\n> }\n>\n> and then use that in rebase_config_default().\n\nIf you mean letting \"yes\", \"on\", \"no\", \"off\" be accepted on the\ncommand line as well, then yes I guess it will be a good idea.\n\n>\n>> +\n>> +/**\n>> + * Returns default rebase option value\n>> + */\n>> +static int rebase_config_default(void)\n>> +{\n>> + struct strbuf name = STRBUF_INIT;\n>> + const char *value = NULL;\n>> + int boolval;\n>> +\n>> + strbuf_addf(&name, \"branch.%s.rebase\", head_name_short);\n>> + if (git_config_get_value(name.buf, &value))\n>> + git_config_get_value(\"pull.rebase\", &value);\n>\n> What happens when neither is defined?\n>\n>> + strbuf_release(&name);\n>> + if (!value)\n>> + return REBASE_FALSE;\n>\n> Hmph, are you sure about this? Isn't this \"[pull] rebase\" that does\n> not have \"= value\", in which case pull.rebase is \"true\"?\n>\n> You cannot use NULL as the sentinel value to tell that you did not\n> find either branch.*.rebase nor pull.rebase (in which case you want\n> to default to 'false'). Either of them can be spelled as an\n> equal-less true, which you will see as value==NULL, and you want to\n> take that as 'true'.\n>\n> const char *value = \"false\";\n> ...\n> if (get_value(..., &value))\n> get_value(..., &value));\n> strbuf_release(&name);\n> if (!value)\n> return REBASE_TRUE;\n> return parse_rebase(value);\n>\n> or something along that line, perhaps?\n\nWhoops, didn't take into account the possibility that the config value\ncould be NULL. Thanks.\n\n>\n>> + boolval = git_config_maybe_bool(\"pull.rebase\", value);\n>> + if (boolval >= 0)\n>> + return boolval ? REBASE_TRUE : REBASE_FALSE;\n>> + else if (value && !strcmp(value, \"preserve\"))\n>> + return REBASE_PRESERVE;\n>\n> Is value something you need to free before returning from this\n> function?\n\n>From my reading if config.c, the memory of value comes from the\nconfig_set the_config_set (the config cache), so there is no need to\nfree it.\n\n>\n>> +static int parse_opt_recurse_submodules(const struct option *opt, const char *arg, int unset)\n>> +{\n>> + if (!arg)\n>> + *(int *)opt->value = unset ? RS_NO : RS_YES;\n>> + else if (!strcmp(arg, \"no\"))\n>> + *(int *)opt->value = RS_NO;\n>> + else if (!strcmp(arg, \"yes\"))\n>> + *(int *)opt->value = RS_YES;\n>> + else if (!strcmp(arg, \"on-demand\"))\n>> + *(int *)opt->value = RS_ON_DEMAND;\n>> + else\n>> + return -1;\n>> + return 0;\n>\n> I suspect that maybe-bool-or-string comment applies equally here for\n> the UI consistency.\n\nYup, I'll keep that in mind.\n\n> I'll stop here for now. Thanks.\n\nThanks.\n"},{"id":"258245","messageId":"CACRoPnQ4iJMn7+eujOXUN6dks2HMQYRw+nYfM1QvOD+k7BCtEQ@mail.gmail.com","threadId":"38831","inReplyTo":"4cae16222697894b19856d12b062f68e@www.dscho.org","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-21T14:00:58Z","receivedAt":"2015-03-21T14:00:58Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nThanks for your suggestions, I agree with most of them :).\n\nOn Wed, Mar 18, 2015 at 5:00 PM, Johannes Schindelin\n wrote:\n>> +static int parse_opt_rebase(const struct option *opt, const char\n>> *arg, int unset)\n>> +{\n>> + if (arg)\n>> + *(int *)opt->value = parse_rebase(arg);\n>> + else\n>> + *(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;\n>> + return (*(int *)opt->value) >= 0 ? 0 : -1;\n>> +}\n>\n> In this function (and also in other places below), there is this pattern that a `struct option` pointer is passed to the function, but then only `*(int *)opt->value` is written to. Therefore, I would suggest to change the signature of the function and pass `(int *)opt->value` as function parameter.\n\nIt's used as a callback for the argument parser though, so the\ncallback signature is required.\n\n>\n>> +static int has_unstaged_changes(void)\n>\n> Yeah, this function, as well as the ones below it, look as if they are so common that they *should* be already somewhere in libgit.a. But I did not find them, either...\n>\n> Of course it *would* be nice to identify places where essentially the same code is needed, and refactor accordingly. But I think that is outside the scope of this project.\n\nActually, I think that identifying the places where code can be\ntrivially shared (without requiring major refactoring) should be part\nof the project, otherwise lots of code may be duplicated and cause\ncode bloat. The obvious one would be fork_point() in this patch, which\nis copied from the merge-base builtin and not accessible because it\nhas static linkage. The project should, at the very least, allow the\nfunction to be shared between git-pull and git-merge-base, as well as\nto modify the function so that it can fail without die()-ing.\n\nRegards,\nPaul\n"},{"id":"258248","messageId":"0c67273f0d1b8af3391db66617b24de5@www.dscho.org","threadId":"38831","inReplyTo":"CACRoPnQ4iJMn7+eujOXUN6dks2HMQYRw+nYfM1QvOD+k7BCtEQ@mail.gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-03-21T17:27:31Z","receivedAt":"2015-03-21T17:27:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-03-21 15:00, Paul Tan wrote:\n\n> Thanks for your suggestions, I agree with most of them :).\n> \n> On Wed, Mar 18, 2015 at 5:00 PM, Johannes Schindelin\n> wrote:\n>>> +static int parse_opt_rebase(const struct option *opt, const char\n>>> *arg, int unset)\n>>> +{\n>>> + if (arg)\n>>> + *(int *)opt->value = parse_rebase(arg);\n>>> + else\n>>> + *(int *)opt->value = unset ? REBASE_FALSE : REBASE_TRUE;\n>>> + return (*(int *)opt->value) >= 0 ? 0 : -1;\n>>> +}\n>>\n>> In this function (and also in other places below), there is this pattern that a `struct option` pointer is passed to the function, but then only `*(int *)opt->value` is written to. Therefore, I would suggest to change the signature of the function and pass `(int *)opt->value` as function parameter.\n> \n> It's used as a callback for the argument parser though, so the\n> callback signature is required.\n\nGood point ;-)\n\nFor readability, I would then suggest to declare `int *result = (int *)opt->value;` on the top of the function and then use `*result = ...` later.\n\n>>> +static int has_unstaged_changes(void)\n>>\n>> Yeah, this function, as well as the ones below it, look as if they are so common that they *should* be already somewhere in libgit.a. But I did not find them, either...\n>>\n>> Of course it *would* be nice to identify places where essentially the same code is needed, and refactor accordingly. But I think that is outside the scope of this project.\n> \n> Actually, I think that identifying the places where code can be\n> trivially shared (without requiring major refactoring) should be part\n> of the project, otherwise lots of code may be duplicated and cause\n> code bloat. The obvious one would be fork_point() in this patch, which\n> is copied from the merge-base builtin and not accessible because it\n> has static linkage. The project should, at the very least, allow the\n> function to be shared between git-pull and git-merge-base, as well as\n> to modify the function so that it can fail without die()-ing.\n\nYes, I agree wholeheartedly. But I like your choice to strike a balance for the sake of a proof-of-concept.\n\nCiao,\nJohannes\n"},{"id":"258249","messageId":"4388b6e4005c2872aa7f5f83024f021d@www.dscho.org","threadId":"38831","inReplyTo":"CACRoPnQp-3RGAPmoo1aM2KUA-phi138J2o82FwP9annM+xvOXA@mail.gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-03-21T17:35:23Z","receivedAt":"2015-03-21T17:35:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-03-21 14:23, Paul Tan wrote:\n\n> Thanks for the review, though I would like to work on the proposal now\n> before the deadline passes :)\n\nThat makes sense.\n\n> On Thu, Mar 19, 2015 at 1:52 AM, Matthieu Moy\n> wrote:\n>> Paul Tan writes:\n>>\n>>> Ideally, I think the solution is to\n>>> improve the test suite and make it as comprehensive as possible, but\n>>> writing a comprehensive test suite may be too time consuming.\n>>\n>> time-consuming, but also very beneficial since the code would end up\n>> being better tested. For sure, a rewrite is a good way to break stuff,\n>> but anything untested can also be broken by mistake rather easily at any\n>> time.\n>>\n>> I'd suggest doing a bit of manual mutation testing: take your C code,\n>> comment-out a few lines of code, see if the tests still pass, and if\n>> they do, add a failing test that passes again once you uncomment the\n>> code.\n> \n> Maybe code coverage tools could help here so we only need to focus on\n> the code paths that are untested by the test suite. At the minimum,\n> all of the non-trivial code paths in both the shell script and the\n> converted builtin must be covered by tests. This should help to\n> eliminate most sources of breakages. Anything further than that would\n> require an experienced understanding of all the possible important\n> inputs to be tested, which I personally feel would make the project\n> quite tedious.\n> \n> I see git already has gcov support. For shell scripts, maybe kcov[1]\n> could be used. With some slight code changes, I managed to generate a\n> report for the git-pull tests[2] which should at least provide a good\n> starting point for how the tests can be improved.\n\nWhile it is often a tempting idea to make test suites as thorough as possible, there lies a true danger herein. True war story: in one of the projects I was involved in, the test suite grew to a size that one complete run lasted two weeks. Yes, that is fourteen days. Needless to say: this test suite was run rarely. How useful is a test suite that is run rarely? More useful than a non-existent one, to be sure, but it is still more of a burden than a boon.\n\nNow, on Windows the test suite takes almost three hours to run. This really, really slows down development.\n\nSo while we are not yet at the \"too large to be useful state\", I would caution against trying to get there.\n\nInstead, I would really like to focus on the *usage*. Calling `git grep \"git pull\" t/` should give you an idea what usage of `git pull` is already tested. It should be pretty easy to come up with a list of *common* use cases, and if any of them are not covered, adding tests for them is simple and straight-forward, too.\n\nCiao,\nJohannes\n"},{"id":"258295","messageId":"CACRoPnT04p-6o2u984a21RvHkk6CqpZWRyafg=T+WAPOD3hiTg@mail.gmail.com","threadId":"38831","inReplyTo":"4388b6e4005c2872aa7f5f83024f021d@www.dscho.org","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-22T17:39:48Z","receivedAt":"2015-03-22T17:39:48Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Sun, Mar 22, 2015 at 1:35 AM, Johannes Schindelin\n wrote:\n>> Maybe code coverage tools could help here so we only need to focus on\n>> the code paths that are untested by the test suite. At the minimum,\n>> all of the non-trivial code paths in both the shell script and the\n>> converted builtin must be covered by tests. This should help to\n>> eliminate most sources of breakages. Anything further than that would\n>> require an experienced understanding of all the possible important\n>> inputs to be tested, which I personally feel would make the project\n>> quite tedious.\n>>\n>> I see git already has gcov support. For shell scripts, maybe kcov[1]\n>> could be used. With some slight code changes, I managed to generate a\n>> report for the git-pull tests[2] which should at least provide a good\n>> starting point for how the tests can be improved.\n>\n> While it is often a tempting idea to make test suites as thorough as possible, there lies a true danger herein. True war story: in one of the projects I was involved in, the test suite grew to a size that one complete run lasted two weeks. Yes, that is fourteen days. Needless to say: this test suite was run rarely. How useful is a test suite that is run rarely? More useful than a non-existent one, to be sure, but it is still more of a burden than a boon.\n>\n> Now, on Windows the test suite takes almost three hours to run. This really, really slows down development.\n>\n> So while we are not yet at the \"too large to be useful state\", I would caution against trying to get there.\n>\n> Instead, I would really like to focus on the *usage*. Calling `git grep \"git pull\" t/` should give you an idea what usage of `git pull` is already tested. It should be pretty easy to come up with a list of *common* use cases, and if any of them are not covered, adding tests for them is simple and straight-forward, too.\n\nThe code coverage tools can help here as well. The kcov output clearly\nshows which options of git-pull are currently not being tested. But\nyes, I agree that the test suite shouldn't be relied too much on\ncompared to code inspection and review.\n\nOn another important topic, though, along with git-pull.sh, I'm\nlooking for another script to convert in parallel with git-pull.sh so\nthat there will be no blocks due to patch review. Generally, I think\nrewriting scripts that are called frequently by users, or spawn a lot\nof processes due to loops, would be most desirable because the runtime\ngains would be much higher. A quick review of the scripts shows that\ngit-am.sh, git-rebase--interactive.sh and git-quiltimport.sh have\npretty heavy loops with lots of process spawning that grows with\ninput.\n\nI'm currently leaning with git-am because not only is it a frequently\nused command, git-rebase--am.sh (for non-interactive rebase) calls it\nas well. In fact, quick tests show that it takes up 98% of\ngit-rebase's execution time on Windows, so if git-am's performance\nimproves it would be a huge win on many fronts. git-am's code also\nseems to be manageable for a 3-month project.\n\nAnyway, I would like to know if you (or anyone else) have any scripts in mind.\n\n(I also think that just 2 scripts would be enough to fill the 3\nmonths, but that might be me just being too conservative)\n\nRegards,\nPaul\n"},{"id":"258350","messageId":"4f9cfb776b8a1418ba1b3bc07b10f577@www.dscho.org","threadId":"38831","inReplyTo":"CACRoPnT04p-6o2u984a21RvHkk6CqpZWRyafg=T+WAPOD3hiTg@mail.gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-03-23T09:07:39Z","receivedAt":"2015-03-23T09:07:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-03-22 18:39, Paul Tan wrote:\n\n> The code coverage tools can help here as well. The kcov output clearly\n> shows which options of git-pull are currently not being tested. But\n> yes, I agree that the test suite shouldn't be relied too much on\n> compared to code inspection and review.\n\nFully agree.\n\n> On another important topic, though, along with git-pull.sh, I'm\n> looking for another script to convert in parallel with git-pull.sh so\n> that there will be no blocks due to patch review. Generally, I think\n> rewriting scripts that are called frequently by users, or spawn a lot\n> of processes due to loops, would be most desirable because the runtime\n> gains would be much higher. A quick review of the scripts shows that\n> git-am.sh, git-rebase--interactive.sh and git-quiltimport.sh have\n> pretty heavy loops with lots of process spawning that grows with\n> input.\n> \n> I'm currently leaning with git-am because not only is it a frequently\n> used command, git-rebase--am.sh (for non-interactive rebase) calls it\n> as well. In fact, quick tests show that it takes up 98% of\n> git-rebase's execution time on Windows, so if git-am's performance\n> improves it would be a huge win on many fronts. git-am's code also\n> seems to be manageable for a 3-month project.\n\nYeah, `git am` is definitely a good pick.\n\nThanks!\nJohannes\n"},{"id":"258355","messageId":"CACsJy8AP7Q9tNNDuQ2aLpCWpBwuD39qPSfip54pmDP58roztEw@mail.gmail.com","threadId":"38831","inReplyTo":"CACRoPnQp-3RGAPmoo1aM2KUA-phi138J2o82FwP9annM+xvOXA@mail.gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-23T10:18:10Z","receivedAt":"2015-03-23T10:18:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 21, 2015 at 8:23 PM, Paul Tan wrote:\n> I see git already has gcov support. For shell scripts, maybe kcov[1]\n> could be used. With some slight code changes, I managed to generate a\n> report for the git-pull tests[2] which should at least provide a good\n> starting point for how the tests can be improved.\n\nCould you share these changes? I'm just wondering if we can add kcov\nsupport to the test suite.\n\n> [1] http://simonkagstrom.github.io/kcov/\n> [2] \n\nAt least we can see [3] that not many options in git-pull is executed.\nCool reports.\n\n[3] https://83dc3588620c5a97164396ec753a2fa480f6a7b0-www.googledrive.com/host/0B4O2AiYulllpfmJlTW4xT050OVVicnNWWS02dm52aTJ2TFIwQ2QwdWh0VHotSkU4eUNNWjg/git-pull/git-pull.360f32c2.html\n\n-- \nDuy\n"},{"id":"258414","messageId":"CACRoPnQZwVq2ssqKYsK1vKu3kVCcc0wqkf2-2soEb1vP34Td2g@mail.gmail.com","threadId":"38831","inReplyTo":"CACsJy8AP7Q9tNNDuQ2aLpCWpBwuD39qPSfip54pmDP58roztEw@mail.gmail.com","subject":"Re: [PATCH/RFC/GSOC] make git-pull a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-03-24T15:58:06Z","receivedAt":"2015-03-24T15:58:06Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Mon, Mar 23, 2015 at 6:18 PM, Duy Nguyen wrote:\n> Could you share these changes? I'm just wondering if we can add kcov\n> support to the test suite.\n\nIn this case it's more of an embarrassing hack, as I just needed a way\nto make git run \"kcov outdir git-pull.sh\" whenever git pull is called\nsince kcov will not instrument any spawned subprocesses. I modified\nexecv_dashed_external() in git.c to prepend kcov to argv (diff\nprobably munged by gmail):\n\ndiff --git a/git.c b/git.c\nindex 8c7ee9c..0f8e7d4 100644\n--- a/git.c\n+++ b/git.c\n@@ -536,6 +536,8 @@ static void execv_dashed_external(const char **argv)\n struct strbuf cmd = STRBUF_INIT;\n const char *tmp;\n int status;\n+ struct argv_array args = ARGV_ARRAY_INIT;\n+ int i;\n\n if (use_pager == -1)\n use_pager = check_pager_config(argv[0]);\n@@ -551,6 +553,11 @@ static void execv_dashed_external(const char **argv)\n */\n tmp = argv[0];\n argv[0] = cmd.buf;\n+ argv_array_push(&args, \"kcov\");\n+ argv_array_push(&args, \"/home/pyokagan/pyk/git/kcov\");\n+ argv_array_push(&args, cmd.buf);\n+ for (i = 1; argv[i]; i++)\n+ argv_array_push(&args, argv[i]);\n\n trace_argv_printf(argv, \"trace: exec:\");\n\n@@ -558,7 +565,7 @@ static void execv_dashed_external(const char **argv)\n * if we fail because the command is not found, it is\n * OK to return. Otherwise, we just pass along the status code.\n */\n- status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE |\nRUN_CLEAN_ON_EXIT);\n+ status = run_command_v_opt(args.argv, RUN_SILENT_EXEC_FAILURE\n| RUN_CLEAN_ON_EXIT);\n if (status >= 0 || errno != ENOENT)\n exit(status);\n\nI'm guessing a real solution is to follow what the test suite does for\nthe --valgrind option, though I haven't looked into it in detail.\n"}]}