From 3d5fc24daefbdf56bc36a491aed0b7990fa0c62f Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Wed, 21 Jul 2021 01:42:19 +0000 Subject: pull: abort if --ff-only is given and fast-forwarding is impossible The warning about pulling without specifying how to reconcile divergent branches says that after setting pull.rebase to true, --ff-only can still be passed on the command line to require a fast-forward. Make that actually work. Signed-off-by: Alex Henrie [en: updated tests; note 3 fixes and 1 new failure] Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index 3e13f81084..d979660482 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix) can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); - if (rebase_unspecified && !opt_ff && !can_ff) { - if (opt_verbosity >= 0) - show_advice_pull_non_ff(); + if (!can_ff) { + if (opt_ff) { + if (!strcmp(opt_ff, "--ff-only")) + die_ff_impossible(); + } else { + if (rebase_unspecified && opt_verbosity >= 0) + show_advice_pull_non_ff(); + } } if (opt_rebase) { -- cgit v1.2.3 From e4dc25ed49d94c93b1d3f63efe17f32ca682cab7 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 22 Jul 2021 05:04:46 +0000 Subject: pull: since --ff-only overrides, handle it first There are both merge and rebase branches in the logic, and previously both had to handle fast-forwarding. Merge handled that implicitly (because git merge handles it directly), while in rebase it was explicit. Given that the --ff-only flag is meant to override any --rebase or --no-rebase, make the code reflect that by handling --ff-only before the merge-vs-rebase logic. It turns out that this also fixes a bug for submodules. Previously, when --ff-only was given, the code would run `merge --ff-only` on the main module, and then run `submodule update --recursive --rebase` on the submodules. With this change, we still run `merge --ff-only` on the main module, but now run `submodule update --recursive --checkout` on the submodules. I believe this better reflects the intent of --ff-only to have it apply to both the main module and the submodules. (Sidenote: It is somewhat interesting that all merges pass `--checkout` to submodule update, even when `--no-ff` is specified, meaning that it will only do fast-forward merges for submodules. This was discussed in commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule changes only)", 2017-06-23). The same limitations apply now as then, so we are not trying to fix this at this time.) Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index d979660482..1f45202037 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1046,15 +1046,15 @@ int cmd_pull(int argc, const char **argv, const char *prefix) can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); - if (!can_ff) { - if (opt_ff) { - if (!strcmp(opt_ff, "--ff-only")) - die_ff_impossible(); - } else { - if (rebase_unspecified && opt_verbosity >= 0) - show_advice_pull_non_ff(); - } + /* ff-only takes precedence over rebase */ + if (opt_ff && !strcmp(opt_ff, "--ff-only")) { + if (!can_ff) + die_ff_impossible(); + opt_rebase = REBASE_FALSE; } + /* If no action specified and we can't fast forward, then warn. */ + if (!opt_ff && rebase_unspecified && !can_ff) + show_advice_pull_non_ff(); if (opt_rebase) { int ret = 0; -- cgit v1.2.3 From adc27d6a9374d012b091bc348c20f5bfdbee52d1 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 22 Jul 2021 05:04:47 +0000 Subject: pull: make --rebase and --no-rebase override pull.ff=only Fix the last few precedence tests failing in t7601 by now implementing the logic to have --[no-]rebase override a pull.ff=only config setting. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index 1f45202037..9bf0325529 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -966,8 +966,22 @@ int cmd_pull(int argc, const char **argv, const char *prefix) parse_repo_refspecs(argc, argv, &repo, &refspecs); - if (!opt_ff) + if (!opt_ff) { opt_ff = xstrdup_or_null(config_get_ff()); + /* + * A subtle point: opt_ff was set on the line above via + * reading from config. opt_rebase, in contrast, is set + * before this point via command line options. The setting + * of opt_rebase via reading from config (using + * config_get_rebase()) does not happen until later. We + * are relying on the next if-condition happening before + * the config_get_rebase() call so that an explicit + * "--rebase" can override a config setting of + * pull.ff=only. + */ + if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) + opt_ff = "--ff"; + } if (opt_rebase < 0) opt_rebase = config_get_rebase(&rebase_unspecified); -- cgit v1.2.3 From 031e2f7ae195069d00d21cde906fce5b0318dbdd Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 22 Jul 2021 05:04:48 +0000 Subject: pull: abort by default when fast-forwarding is not possible We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now. Initial-patch-by: Alex Henrie Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index 9bf0325529..4514a1478e 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -927,9 +927,9 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ static void show_advice_pull_non_ff(void) { - advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged. You can squelch this message by running one of the following\n" - "commands sometime before your next pull:\n" + advise(_("You have divergent branches and need to specify how to reconcile them.\n" + "You can do so by running one of the following commands sometime before\n" + "your next pull:\n" "\n" " git config pull.rebase false # merge (the default strategy)\n" " git config pull.rebase true # rebase\n" @@ -1067,8 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix) opt_rebase = REBASE_FALSE; } /* If no action specified and we can't fast forward, then warn. */ - if (!opt_ff && rebase_unspecified && !can_ff) + if (!opt_ff && rebase_unspecified && !can_ff) { show_advice_pull_non_ff(); + die(_("Need to specify how to reconcile divergent branches.")); + } if (opt_rebase) { int ret = 0; -- cgit v1.2.3 From 359ff6938990a438b99e95fe36b6b359f3eb9811 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 22 Jul 2021 05:04:49 +0000 Subject: pull: update docs & code for option compatibility with rebasing git-pull.txt includes merge-options.txt, which is written assuming merges will happen. git-pull has allowed rebases for many years; update the documentation to reflect that. While at it, pass any `--signoff` flag through to the rebase backend too so that we don't have to document it as merge-specific. Rebase has supported the --signoff flag for years now as well. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index 4514a1478e..2f1d1f4037 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -893,6 +893,8 @@ static int run_rebase(const struct object_id *newbase, strvec_pushv(&args, opt_strategy_opts.v); if (opt_gpg_sign) strvec_push(&args, opt_gpg_sign); + if (opt_signoff) + strvec_push(&args, opt_signoff); if (opt_autostash == 0) strvec_push(&args, "--no-autostash"); else if (opt_autostash == 1) -- cgit v1.2.3 From 6f843a3355ee590dfe09eb90679051e75fadf675 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 22 Jul 2021 05:04:50 +0000 Subject: pull: fix handling of multiple heads With multiple heads, we should not allow rebasing or fast-forwarding. Make sure any fast-forward request calls out specifically the fact that multiple branches are in play. Also, since we cannot fast-forward to multiple branches, fix our computation of can_ff. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/pull.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'builtin/pull.c') diff --git a/builtin/pull.c b/builtin/pull.c index 2f1d1f4037..b311ea6b9d 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -913,12 +913,18 @@ static int run_rebase(const struct object_id *newbase, return ret; } -static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head) +static int get_can_ff(struct object_id *orig_head, + struct oid_array *merge_heads) { int ret; struct commit_list *list = NULL; struct commit *merge_head, *head; + struct object_id *orig_merge_head; + if (merge_heads->nr > 1) + return 0; + + orig_merge_head = &merge_heads->oid[0]; head = lookup_commit_reference(the_repository, orig_head); commit_list_insert(head, &list); merge_head = lookup_commit_reference(the_repository, orig_merge_head); @@ -1057,10 +1063,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix) die(_("Cannot merge multiple branches into empty head.")); return pull_into_void(merge_heads.oid, &curr_head); } - if (opt_rebase && merge_heads.nr > 1) - die(_("Cannot rebase onto multiple branches.")); + if (merge_heads.nr > 1) { + if (opt_rebase) + die(_("Cannot rebase onto multiple branches.")); + if (opt_ff && !strcmp(opt_ff, "--ff-only")) + die(_("Cannot fast-forward to multiple branches.")); + } - can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); + can_ff = get_can_ff(&orig_head, &merge_heads); /* ff-only takes precedence over rebase */ if (opt_ff && !strcmp(opt_ff, "--ff-only")) { -- cgit v1.2.3