Re: [PATCH 11/14] pull: teach git pull about --rebase
flat view
From: Stefan Beller <hidden>
Date: 2016-06-15 23:04:49
On Mon, May 18, 2015 at 8:06 AM, Paul Tan [off-list ref] wrote:
quoted hunk ↗ jump to hunk
+enum rebase_type { + REBASE_FALSE, + REBASE_TRUE, + REBASE_PRESERVE +}; + +/** + * Parses the value of --rebase, branch.*.rebase or pull.rebase. If value is a + * false value, returns REBASE_FALSE. If value is a true value, returns + * REBASE_TRUE. If value is "preserve", returns REBASE_PRESERVE. Otherwise, + * returns -1 to signify an invalid value. + */ +static int parse_config_rebase(const char *value) +{ + int v = git_config_maybe_bool("pull.rebase", value); + if (v >= 0) + return v; + if (!strcmp(value, "preserve")) + return REBASE_PRESERVE; + return -1;
There is no REBASE_FALSE nor REBASE_TRUE in this function,
how do you make sure it returns the correct value then?
Currently this is probably in sync with what git_config_maybe_bool
returns, but think of a future refactoring/change. Then it will be hard
to have the connection between what the code in git_config_maybe_bool
does, what the code here does and the comment here.
You could get rid of the enum all together, as you're using integer as a
return type anyway. (so you lose type safety, a future change may introduce
a "return 42" which is not part of the enum)
Or you could convert the code to use the enum more strictly and having it
as the return type:
enum rebase_type parse_config_rebase(const char *value) {
int v = git_config_maybe_bool("pull.rebase", value);
switch (v) {
case 0:
return REBASE_FALSE;
case 1:
return REBASE_TRUE;
default:
if (!strcmp(value, "preserve"))
return REBASE_PRESERVE;
else {
die("value pull.rebase must be one of {false, true, preserve}");
// or if you're more tolerant:
return REBASE_VALUE_UNINTERPRETABLE;
}
}
quoted hunk ↗ jump to hunk
+} + +/** + * Callback for --rebase, which parses arg with parse_config_rebase(). + */ +static int parse_opt_rebase(const struct option *opt, const char *arg, int unset) +{ + int *value = (int*) opt->value; + + if (arg) + *value = parse_config_rebase(arg); + else + *value = unset ? REBASE_FALSE : REBASE_TRUE; + return *value >= 0 ? 0 : -1;
This is repeating the check from above. If you'd go the way of using stricter enums, you'd just check for the REBASE_VALUE_UNINTERPRETABLE value here.
quoted hunk ↗ jump to hunk
+} + static const char * const pull_usage[] = { N_("git pull [options] [<repo> [<refspec>...]]"), NULL@@ -48,7 +84,8 @@ static const char * const pull_usage[] = { static int opt_verbosity; static char *opt_progress; -/* Options passed to git-merge */ +/* Options passed to git-merge or git-rebase */ +static int opt_rebase; static char *opt_diffstat; static char *opt_log; static char *opt_squash;@@ -82,8 +119,12 @@ static struct option pull_options[] = { N_("force progress reporting"), PARSE_OPT_NOARG, parse_opt_passthru}, - /* Options passed to git-merge */ + /* Options passed to git-merge or git-rebase */ OPT_GROUP(N_("Options related to merging")), + { OPTION_CALLBACK, 'r', "rebase", &opt_rebase, + N_("false|true|preserve"), + N_("incorporate changes by rebasing rather than merging"), + PARSE_OPT_OPTARG, parse_opt_rebase, 0}, { OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL, N_("do not show a diffstat at the end of the merge"), PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "no-stat" },@@ -506,11 +547,185 @@ static int run_merge(void) return ret; } +/** + * Returns the merge branch for the current branch. Returns NULL if repo is not + * a valid remote, HEAD does not point to a branch, repo is not the branch's + * configured remote or the branch does not have any configured merge branch. + */ +static char *get_merge_branch_1(const char *repo) +{ + struct remote *rm; + struct branch *curr_branch; + + if (repo && !(rm = remote_get(repo))) + return NULL; + if (!(curr_branch = branch_get("HEAD"))) + return NULL; + if (repo && curr_branch->remote != rm) + return NULL; + if (!curr_branch->merge_nr) + return NULL; + return xstrdup(curr_branch->merge[0]->dst); +}
This feels a bit tangled because of the repetition of checking
for repo and curr_branch. Also we do not do assignments inside
of conditions (if, while, for), so maybe start with a
if (!repo)
return NULL;
and then do the assignments and test on the assigned value.
quoted hunk ↗ jump to hunk
+ +/** + * Given a refspec, returns the merge branch. Returns NULL if the refspec src + * does not refer to a branch. + * + * FIXME: It should return the tracking branch. Currently only works with the + * default mapping. + */ +static char *get_merge_branch_2(const char *repo, const char *refspec) +{ + struct refspec *spec; + const char *remote; + char *merge_branch; + + spec = parse_fetch_refspec(1, &refspec); + remote = spec->src; + if (!*remote || !strcmp(remote, "HEAD")) + remote = "HEAD"; + else if (skip_prefix(remote, "heads/", &remote)) + ; + else if (skip_prefix(remote, "refs/heads/", &remote)) + ; + else if (starts_with(remote, "refs/") || + starts_with(remote, "tags/") || + starts_with(remote, "remotes/")) + remote = ""; + + if (*remote) { + if (!strcmp(repo, ".")) + merge_branch = mkpathdup("refs/heads/%s", remote); + else + merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote); + } else + merge_branch = NULL; + + free_refspec(1, spec); + return merge_branch; +} + +/** + * Sets fork_point to the point at which the current branch forked from its + * remote merge branch. Returns 0 on success, -1 on failure. + */ +static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ], + const char *repo, const char *refspec) +{ + int ret; + struct branch *curr_branch; + char *remote_merge_branch; + struct argv_array args = ARGV_ARRAY_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf sb = STRBUF_INIT; + + if (!(curr_branch = branch_get("HEAD"))) + return -1; + + if (refspec) + remote_merge_branch = get_merge_branch_2(repo, refspec); + else + remote_merge_branch = get_merge_branch_1(repo); + + if (!remote_merge_branch) + return -1; + + argv_array_pushl(&args, "merge-base", "--fork-point", + remote_merge_branch, curr_branch->name, NULL); + cp.argv = args.argv; + cp.no_stdin = 1; + cp.no_stderr = 1; + cp.git_cmd = 1; + + if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ))) + goto cleanup; + + if ((ret = get_sha1_hex(sb.buf, fork_point))) + goto cleanup; + +cleanup: + free(remote_merge_branch); + strbuf_release(&sb); + return ret ? -1 : 0; +} + +/** + * Sets merge_base to the octopus merge base of curr_head, merge_head and + * fork_point. Returns 0 if a merge base is found, 1 otherwise. + */ +static int get_octopus_merge_base(unsigned char merge_base[GIT_SHA1_HEXSZ], + unsigned char curr_head[GIT_SHA1_RAWSZ], + unsigned char merge_head[GIT_SHA1_RAWSZ], + unsigned char fork_point[GIT_SHA1_RAWSZ]) +{ + struct commit_list *revs = NULL, *result; + + commit_list_insert(lookup_commit_reference(curr_head), &revs); + commit_list_insert(lookup_commit_reference(merge_head), &revs); + if (!is_null_sha1(fork_point)) + commit_list_insert(lookup_commit_reference(fork_point), &revs); + + result = reduce_heads(get_octopus_merge_bases(revs)); + free_commit_list(revs); + if (!result) + return 1; + + hashcpy(merge_base, result->item->object.sha1); + return 0; +} + +/** + * Given the current HEAD SHA1, the merge head returned from git-fetch and the + * fork point calculated by get_rebase_fork_point(), runs git-rebase with the + * appropriate arguments and returns its exit status. + */ +static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ], + unsigned char merge_head[GIT_SHA1_RAWSZ], + unsigned char fork_point[GIT_SHA1_RAWSZ]) +{ + int ret; + unsigned char oct_merge_base[GIT_SHA1_RAWSZ]; + struct argv_array args = ARGV_ARRAY_INIT; + + if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head, fork_point)) + if (!is_null_sha1(fork_point) && !hashcmp(oct_merge_base, fork_point)) + hashclr(fork_point); + + argv_array_push(&args, "rebase"); + + /* Shared options */ + argv_push_verbosity(&args); + + /* Options passed to git-rebase */ + if (opt_rebase == REBASE_PRESERVE) + argv_array_push(&args, "--preserve-merges"); + if (opt_diffstat) + argv_array_push(&args, opt_diffstat); + argv_push_strategies(&args); + argv_push_strategy_opts(&args); + if (opt_gpg_sign) + argv_array_push(&args, opt_gpg_sign); + + argv_array_push(&args, "--onto"); + argv_array_push(&args, sha1_to_hex(merge_head)); + + if (!is_null_sha1(fork_point)) + argv_array_push(&args, sha1_to_hex(fork_point)); + else + argv_array_push(&args, sha1_to_hex(merge_head)); + + ret = run_command_v_opt(args.argv, RUN_GIT_CMD); + argv_array_clear(&args); + return ret; +} + int cmd_pull(int argc, const char **argv, const char *prefix) { const char *repo, **refspecs; struct sha1_array merge_heads = SHA1_ARRAY_INIT; unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ]; + unsigned char rebase_fork_point[GIT_SHA1_RAWSZ]; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv);@@ -532,6 +747,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (get_sha1("HEAD", orig_head)) hashclr(orig_head); + if (opt_rebase) + if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs)) + hashclr(rebase_fork_point); + if (run_fetch(repo, refspecs)) return 1;@@ -573,6 +792,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (merge_heads.nr > 1) die(_("Cannot merge multiple branches into empty head.")); return pull_into_void(*merge_heads.sha1, curr_head); + } else if (opt_rebase) { + if (merge_heads.nr > 1) + die(_("Cannot rebase onto multiple branches.")); + return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point); } else return run_merge(); }diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 9414cc1..3798b96 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh@@ -212,7 +212,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' ' test "$(git rev-parse third)" = "$(git rev-parse second)" ' -test_expect_failure '--rebase' ' +test_expect_success '--rebase' ' git branch to-rebase && echo modified again > file && git commit -m file file &&@@ -226,7 +226,7 @@ test_expect_failure '--rebase' ' test new = "$(git show HEAD:file2)" ' -test_expect_failure '--rebase fails with multiple branches' ' +test_expect_success '--rebase fails with multiple branches' ' git reset --hard before-rebase && test_must_fail git pull --rebase . copy master 2>err && test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&@@ -310,7 +310,7 @@ test_expect_failure 'pull.rebase=invalid fails' ' ! git pull . copy ' -test_expect_failure '--rebase=false create a new merge commit' ' +test_expect_success '--rebase=false create a new merge commit' ' git reset --hard before-preserve-rebase && test_config pull.rebase true && git pull --rebase=false . copy &&@@ -319,7 +319,7 @@ test_expect_failure '--rebase=false create a new merge commit' ' test file3 = "$(git show HEAD:file3.t)" ' -test_expect_failure '--rebase=true rebases and flattens keep-merge' ' +test_expect_success '--rebase=true rebases and flattens keep-merge' ' git reset --hard before-preserve-rebase && test_config pull.rebase preserve && git pull --rebase=true . copy &&@@ -327,7 +327,7 @@ test_expect_failure '--rebase=true rebases and flattens keep-merge' ' test file3 = "$(git show HEAD:file3.t)" ' -test_expect_failure '--rebase=preserve rebases and merges keep-merge' ' +test_expect_success '--rebase=preserve rebases and merges keep-merge' ' git reset --hard before-preserve-rebase && test_config pull.rebase true && git pull --rebase=preserve . copy &&@@ -340,7 +340,7 @@ test_expect_success '--rebase=invalid fails' ' ! git pull --rebase=invalid . copy ' -test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-merge' ' +test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' ' git reset --hard before-preserve-rebase && test_config pull.rebase preserve && git pull --rebase . copy &&@@ -348,7 +348,7 @@ test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-m test file3 = "$(git show HEAD:file3.t)" ' -test_expect_failure '--rebase with rebased upstream' ' +test_expect_success '--rebase with rebased upstream' ' git remote add -f me . && git checkout copy &&@@ -366,7 +366,7 @@ test_expect_failure '--rebase with rebased upstream' ' ' -test_expect_failure '--rebase with rebased default upstream' ' +test_expect_success '--rebase with rebased default upstream' ' git update-ref refs/remotes/me/copy copy-orig && git checkout --track -b to-rebase2 me/copy &&@@ -377,7 +377,7 @@ test_expect_failure '--rebase with rebased default upstream' ' ' -test_expect_failure 'rebased upstream + fetch + pull --rebase' ' +test_expect_success 'rebased upstream + fetch + pull --rebase' ' git update-ref refs/remotes/me/copy copy-orig && git reset --hard to-rebase-orig &&@@ -409,7 +409,7 @@ test_expect_failure 'pull --rebase dies early with dirty working directory' ' ' -test_expect_failure 'pull --rebase works on branch yet to be born' ' +test_expect_success 'pull --rebase works on branch yet to be born' ' git rev-parse master >expect && mkdir empty_repo && (cd empty_repo &&@@ -456,14 +456,14 @@ test_expect_success 'setup for detecting upstreamed changes' ' ) ' -test_expect_failure 'git pull --rebase detects upstreamed changes' ' +test_expect_success 'git pull --rebase detects upstreamed changes' ' (cd dst && git pull --rebase && test -z "$(git ls-files -u)" ) ' -test_expect_failure 'setup for avoiding reapplying old patches' ' +test_expect_success 'setup for avoiding reapplying old patches' ' (cd dst && test_might_fail git rebase --abort && git reset --hard origin/master@@ -485,7 +485,7 @@ test_expect_failure 'setup for avoiding reapplying old patches' ' ) ' -test_expect_failure 'git pull --rebase does not reapply old patches' ' +test_expect_success 'git pull --rebase does not reapply old patches' ' (cd dst && test_must_fail git pull --rebase && test 1 = $(find .git/rebase-apply -name "000*" | wc -l)diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 4176e11..56e7377 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh@@ -19,7 +19,7 @@ test_expect_success 'git pull -q' ' test_must_be_empty out) ' -test_expect_failure 'git pull -q --rebase' ' +test_expect_success 'git pull -q --rebase' ' mkdir clonedqrb && (cd clonedqrb && git init && git pull -q --rebase "../parent" >out 2>err &&@@ -38,7 +38,7 @@ test_expect_success 'git pull' ' test_must_be_empty out) ' -test_expect_failure 'git pull --rebase' ' +test_expect_success 'git pull --rebase' ' mkdir clonedrb && (cd clonedrb && git init && git pull --rebase "../parent" >out 2>err &&@@ -54,7 +54,7 @@ test_expect_success 'git pull -v' ' test_must_be_empty out) ' -test_expect_failure 'git pull -v --rebase' ' +test_expect_success 'git pull -v --rebase' ' mkdir clonedvrb && (cd clonedvrb && git init && git pull -v --rebase "../parent" >out 2>err &&diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 1ffd837..dda3929 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh@@ -11,8 +11,6 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD. . ./test-lib.sh -skip_all='skipping submodule update tests, requires git pull --rebase' -test_done compare_head() { --2.1.4