From 77a7ec632952ee335cb544fee7c6b3fb1c5ec147 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sat, 12 Dec 2020 10:52:06 -0600 Subject: [PATCH 1/5] pull: refactor fast-forward check We would like to be able to make this check before the decision to rebase is made in a future step. Besides, using a separate helper makes the code easier to follow. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/pull.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index aa56ebcdd0..03e6d53243 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -924,6 +924,20 @@ 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) +{ + int ret; + struct commit_list *list = NULL; + struct commit *merge_head, *head; + + head = lookup_commit_reference(the_repository, orig_head); + commit_list_insert(head, &list); + merge_head = lookup_commit_reference(the_repository, orig_merge_head); + ret = repo_is_descendant_of(the_repository, merge_head, list); + free_commit_list(list); + return ret; +} + int cmd_pull(int argc, const char **argv, const char *prefix) { const char *repo, **refspecs; @@ -1040,22 +1054,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix) submodule_touches_in_range(the_repository, &upstream, &curr_head)) die(_("cannot rebase with locally recorded submodule modifications")); if (!autostash) { - struct commit_list *list = NULL; - struct commit *merge_head, *head; - - head = lookup_commit_reference(the_repository, - &orig_head); - commit_list_insert(head, &list); - merge_head = lookup_commit_reference(the_repository, - &merge_heads.oid[0]); - if (repo_is_descendant_of(the_repository, - merge_head, list)) { + if (get_can_ff(&orig_head, &merge_heads.oid[0])) { /* we can fast-forward this without invoking rebase */ opt_ff = "--ff-only"; ran_ff = 1; ret = run_merge(); } - free_commit_list(list); } if (!ran_ff) ret = run_rebase(&newbase, &upstream); From 278f4be806f93579c64cd9993fdad2eb589f86c6 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sat, 12 Dec 2020 10:52:07 -0600 Subject: [PATCH 2/5] pull: give the advice for choosing rebase/merge much later Eventually we want to be omit the advice when we can fast-forward in which case there is no reason to require the user to choose between rebase or merge. In order to do so, we need to delay giving the advice up to the point where we can check if we can fast-forward or not. Additionally, config_get_rebase() was probably never its true home. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/pull.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 03e6d53243..ff8e3ce137 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,6 +27,8 @@ #include "commit-reach.h" #include "sequencer.h" +static int default_mode; + /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is @@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void) if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); - if (opt_verbosity >= 0 && !opt_ff) { - 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" - "\n" - " git config pull.rebase false # merge (the default strategy)\n" - " git config pull.rebase true # rebase\n" - " git config pull.ff only # fast-forward only\n" - "\n" - "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); - } + default_mode = 1; return REBASE_FALSE; } @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); + if (default_mode && opt_verbosity >= 0 && !opt_ff) { + 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" + "\n" + " git config pull.rebase false # merge (the default strategy)\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n" + "\n" + "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" + "or --ff-only on the command line to override the configured default per\n" + "invocation.\n")); + } + if (opt_rebase) { int ret = 0; int ran_ff = 0; From b044db9172f020768e85835f87bfc5564e310714 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 14 Dec 2020 09:05:41 -0800 Subject: [PATCH 3/5] pull: get rid of unnecessary global variable It is easy enough to do, and gives a more descriptive name to the variable that is scoped in a more focused way. Signed-off-by: Junio C Hamano --- builtin/pull.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index ff8e3ce137..2976b8e5cb 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,8 +27,6 @@ #include "commit-reach.h" #include "sequencer.h" -static int default_mode; - /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is @@ -326,7 +324,7 @@ static const char *config_get_ff(void) * looks for the value of "pull.rebase". If both configuration keys do not * exist, returns REBASE_FALSE. */ -static enum rebase_type config_get_rebase(void) +static enum rebase_type config_get_rebase(int *rebase_unspecified) { struct branch *curr_branch = branch_get("HEAD"); const char *value; @@ -346,7 +344,7 @@ static enum rebase_type config_get_rebase(void) if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); - default_mode = 1; + *rebase_unspecified = 1; return REBASE_FALSE; } @@ -934,6 +932,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) struct object_id orig_head, curr_head; struct object_id rebase_fork_point; int autostash; + int rebase_unspecified = 0; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv); @@ -955,7 +954,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) opt_ff = xstrdup_or_null(config_get_ff()); if (opt_rebase < 0) - opt_rebase = config_get_rebase(); + opt_rebase = config_get_rebase(&rebase_unspecified); if (read_cache_unmerged()) die_resolve_conflict("pull"); @@ -1029,7 +1028,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); - if (default_mode && opt_verbosity >= 0 && !opt_ff) { + if (rebase_unspecified && opt_verbosity >= 0 && !opt_ff) { 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" From 7539fdc6290750c617c0b638c2f3b2f63c389ed3 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 14 Dec 2020 09:08:57 -0800 Subject: [PATCH 4/5] pull: correct condition to trigger non-ff advice Refactor the advise() call that teaches users how they can choose between merge and rebase into a helper function. This revealed that the caller's logic needs to be further clarified to allow future actions (like "erroring out" instead of the current "go ahead and merge anyway") that should happen whether the advice message is squelched out. Signed-off-by: Junio C Hamano --- builtin/pull.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 2976b8e5cb..1b87ea95eb 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ return ret; } +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" + "\n" + " git config pull.rebase false # merge (the default strategy)\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n" + "\n" + "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" + "or --ff-only on the command line to override the configured default per\n" + "invocation.\n")); +} + int cmd_pull(int argc, const char **argv, const char *prefix) { const char *repo, **refspecs; @@ -1028,19 +1044,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); - if (rebase_unspecified && opt_verbosity >= 0 && !opt_ff) { - 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" - "\n" - " git config pull.rebase false # merge (the default strategy)\n" - " git config pull.rebase true # rebase\n" - " git config pull.ff only # fast-forward only\n" - "\n" - "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); + if (rebase_unspecified && !opt_ff) { + if (opt_verbosity >= 0) + show_advice_pull_non_ff(); } if (opt_rebase) { From c525de335e4a3919d49a9126b76d1288f35737be Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sat, 12 Dec 2020 10:52:08 -0600 Subject: [PATCH 5/5] pull: display default warning only when non-ff There's no need to display the annoying warning on every pull... only the ones that are not fast-forward. The current warning tests still pass, but not because of the arguments or the configuration, but because they are all fast-forward. We need to test non-fast-forward situations now. Suggestions-by: Junio C Hamano Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/pull.c | 7 ++-- t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 1b87ea95eb..e8927fc2ff 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -949,6 +949,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) struct object_id rebase_fork_point; int autostash; int rebase_unspecified = 0; + int can_ff; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv); @@ -1044,7 +1045,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); - if (rebase_unspecified && !opt_ff) { + 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(); } @@ -1063,7 +1066,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) submodule_touches_in_range(the_repository, &upstream, &curr_head)) die(_("cannot rebase with locally recorded submodule modifications")); if (!autostash) { - if (get_can_ff(&orig_head, &merge_heads.oid[0])) { + if (can_ff) { /* we can fast-forward this without invoking rebase */ opt_ff = "--ff-only"; ran_ff = 1; diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 6774e9d86f..52e8ccc933 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -29,11 +29,8 @@ test_expect_success 'setup' ' test_expect_success 'pull.rebase not set' ' git reset --hard c0 && - git -c color.advice=always pull . c1 2>err && - test_decode_color decoded && - test_i18ngrep "hint: " decoded && - test_i18ngrep "Pulling without specifying how to reconcile" decoded - + git pull . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and pull.ff=true' ' @@ -87,6 +84,65 @@ test_expect_success 'pull.rebase not set and --ff-only given' ' test_i18ngrep ! "Pulling without specifying how to reconcile" err ' +test_expect_success 'pull.rebase not set (not-fast-forward)' ' + git reset --hard c2 && + git -c color.advice=always pull . c1 2>err && + test_decode_color decoded && + test_i18ngrep "hint: " decoded && + test_i18ngrep "Pulling without specifying how to reconcile" decoded +' + +test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' ' + git reset --hard c2 && + test_config pull.ff true && + git pull . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' ' + git reset --hard c2 && + test_config pull.ff false && + git pull . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' ' + git reset --hard c2 && + test_config pull.ff only && + test_must_fail git pull . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' ' + git reset --hard c2 && + git pull --rebase . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' ' + git reset --hard c2 && + git pull --no-rebase . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' ' + git reset --hard c2 && + git pull --ff . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' ' + git reset --hard c2 && + git pull --no-ff . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' ' + git reset --hard c2 && + test_must_fail git pull --ff-only . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + test_expect_success 'merge c1 with c2' ' git reset --hard c1 && test -f c0.c &&