[PATCH v5 0/3] pull: stop warning on every pull

[PATCH v5 0/3] pull: stop warning on every pull

[Felipe Contreras]

The future of having a sensible "git pull" default is uncertain (it's
very likely that nothing will change), in the meantime there's no reason
to keep annoying our users unnecessarily.

This patch is doing one thing, and one thing only: stops warning the
users on every single pull (warns only if they are not-fast-forward).

Changes since v4:

 1. Improved some commit messages
 2. In patch 3 current the tests are kept as is, per Junio's request
    (IMO they are no-ops)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b4adab8b1..52e8ccc933 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -28,66 +28,116 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pull.rebase not set' '
-	git reset --hard c2 &&
-	git -c color.advice=always pull . c1 2>err &&
-	test_decode_color <err >decoded &&
-	test_i18ngrep "<YELLOW>hint: " decoded &&
-	test_i18ngrep "Pulling without specifying how to reconcile" decoded
+	git reset --hard c0 &&
+	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' '
+	git reset --hard c0 &&
+	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 (fast-forward)' '
+test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c0 &&
+	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=true' '
+test_expect_success 'pull.rebase not set and pull.ff=only' '
+	git reset --hard c0 &&
+	test_config pull.ff only &&
+	git pull . c1 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --rebase given' '
+	git reset --hard c0 &&
+	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' '
+	git reset --hard c0 &&
+	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' '
+	git reset --hard c0 &&
+	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' '
+	git reset --hard c0 &&
+	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' '
+	git reset --hard c0 &&
+	git pull --ff-only . c1 2>err &&
+	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 <err >decoded &&
+	test_i18ngrep "<YELLOW>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' '
+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' '
+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' '
+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' '
+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' '
+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' '
+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' '
+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


Felipe Contreras (3):
  pull: refactor fast-forward check
  pull: move default warning
  pull: display default warning only when non-ff

 builtin/pull.c               | 61 +++++++++++++++++++--------------
 t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++++---
 2 files changed, 97 insertions(+), 30 deletions(-)

-- 
2.29.2

[PATCH v5 1/3] pull: refactor fast-forward check

[Felipe Contreras]

It makes the code much cleaner.

Plus, we would like to be able to make this check before the decision to
rebase is made in a future step.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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);
-- 
2.29.2

[PATCH v5 2/3] pull: move default warning

[Felipe Contreras]

Eventually we want to be able to display the warning only when
fast-forward merges are not possible.

In order to do so, we need to move the default warning 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.

This requires a temporary variable to check if we are in the
"default mode" (no --rebase or --no-rebase specified). But this variable
can be removed later on with further code reorganization.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
-- 
2.29.2

[PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

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 <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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 ff8e3ce137..9a7caf3a3e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -934,6 +934,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 can_ff;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1029,7 +1030,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 (default_mode && opt_verbosity >= 0 && !opt_ff) {
+	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+
+	if (default_mode && !can_ff && 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"
@@ -1058,7 +1061,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 <err >decoded &&
-	test_i18ngrep "<YELLOW>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 <err >decoded &&
+	test_i18ngrep "<YELLOW>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 &&
-- 
2.29.2

Re: [PATCH v5 0/3] pull: stop warning on every pull

[Felipe Contreras]

On Sat, Dec 12, 2020 at 10:52 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> The future of having a sensible "git pull" default is uncertain (it's
> very likely that nothing will change), in the meantime there's no reason
> to keep annoying our users unnecessarily.
>
> This patch is doing one thing, and one thing only: stops warning the
> users on every single pull (warns only if they are not-fast-forward).
>
> Changes since v4:

Hmm, it's not 4 -> 5, It's 5 -> 6.

-- 
Felipe Contreras

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> The original author of this test cared that pulling c1 with --rebase
> >> into c0 succeeds, and that it does not give the error message.
> >
> > I prefer not to attempt to read minds (plus, the author might have not
> > cared that the pulling succeeds), and anyway; that's not what matters.
> > ...
> > Again, I don't particularly care to mindread what the test authors might
> > have cared about.
> 
> You do not have to read mind.  What is written in the test is clear
> enough: run "git pull --rebase . c1" into c0 and see what happens.

That is precisely my point: it is written in the test, no mindreading
neccessary.

-- 
Felipe Contreras

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> The original author of this test cared that pulling c1 with --rebase
>> into c0 succeeds, and that it does not give the error message.
>
> I prefer not to attempt to read minds (plus, the author might have not
> cared that the pulling succeeds), and anyway; that's not what matters.
> ...
> Again, I don't particularly care to mindread what the test authors might
> have cared about.

You do not have to read mind.  What is written in the test is clear
enough: run "git pull --rebase . c1" into c0 and see what happens.

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >>  test_expect_success 'pull.rebase not set and --rebase given' '
> >> -	git reset --hard c0 &&
> >> +	git reset --hard c2 &&
> >>  	git pull --rebase . c1 2>err &&
> >>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >>  '
> >
> > This used to make sure an attempt to rebase c1 onto c0, which can be
> > fast-forwarded, would work fine, even though it used to give
> > warning.  We should keep testing the same condition.  The
> > expectation of seeing the warning is what must be changed, not the
> > test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no
> > longer making sure that c1 can be rebased onto c0 cleanly.
> 
> Let's try to explain it in a different way.
> 
> The original author of this test cared that pulling c1 with --rebase
> into c0 succeeds, and that it does not give the error message.

I prefer not to attempt to read minds (plus, the author might have not
cared that the pulling succeeds), and anyway; that's not what matters.

What matters is the current situation, and the desired situation.

> We have no right [*1*] to say that scenario (i.e. "pull --rebase" c1
> into c0) no longer matters without a good justification.

But nobody is saying that. What I said is that in *this* particular
test-case that's not the objective of the test. And if you consider
hypothetical secondary objectives of the test, those are being tested
elsewhere.

> And it is not a good justification to say that the current code
> happens to behave identically whether running "pull --rebase" of c1
> into c0 or c2 so it is sufficient to test the operation into c2.  The
> test is *not* about how the current code happens to work.  It is to
> make sure the scenarios test authors care about will keep behaving the
> same way.

Again, I don't particularly care to mindread what the test authors might
have cared about.

It's clear from the tests themselves what they are trying to do: check
if the warning exists, or doesn't.

> Some tests may be expecting that pulling c1 into c0 would issue the
> message, and that the command succeeds, and with the patch 3/3 the
> outcome may become different, i.e. the command succeeds without
> annoying message.

No. All the tests (sans 1) check that the warning is *not* present.

If you do a fast-forward pull, the warning will not be present
(regardless of options), and the test would pass, but for the wrong
reasons.

> That would break the expectation of the original
> test authors, and it is a good thing.  By recording a change to the
> expectation, we can document how the new behaviour works better under
> the same scenario.

No, the expectation has not changed one iota; it's exactly same.

It's the reason for the same output that changed.

If I take the current tests, and I remove the thing that makes them
special (arguments and/or configuration), essentially making them all
"git pull":

@@ -35,52 +35,49 @@ test_expect_success 'pull.rebase not set' '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c0 &&
-	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' '
 	git reset --hard c0 &&
-	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' '
 	git reset --hard c0 &&
-	test_config pull.ff only &&
 	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c0 &&
-	git pull --rebase . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c0 &&
-	git pull --no-rebase . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c0 &&
-	git pull --ff . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c0 &&
-	git pull --no-ff . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c0 &&
-	git pull --ff-only . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
What happens?

ok 3 - pull.rebase not set and pull.ff=true
ok 4 - pull.rebase not set and pull.ff=false
ok 5 - pull.rebase not set and pull.ff=only
ok 6 - pull.rebase not set and --rebase given
ok 7 - pull.rebase not set and --no-rebase given
ok 8 - pull.rebase not set and --ff given
ok 9 - pull.rebase not set and --no-ff given
ok 10 - pull.rebase not set and --ff-only given

They all pass. What are they supposed to be testing now? I don't know.

In my opinion they are no-ops now, but fine; I'll leave them as is.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Junio C Hamano]

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

>>  test_expect_success 'pull.rebase not set and --rebase given' '
>> -	git reset --hard c0 &&
>> +	git reset --hard c2 &&
>>  	git pull --rebase . c1 2>err &&
>>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>>  '
>
> This used to make sure an attempt to rebase c1 onto c0, which can be
> fast-forwarded, would work fine, even though it used to give
> warning.  We should keep testing the same condition.  The
> expectation of seeing the warning is what must be changed, not the
> test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no
> longer making sure that c1 can be rebased onto c0 cleanly.

Let's try to explain it in a different way.

The original author of this test cared that pulling c1 with --rebase
into c0 succeeds, and that it does not give the error message.  We
have no right [*1*] to say that scenario (i.e. "pull --rebase" c1
into c0) no longer matters without a good justification.  And it is
not a good justification to say that the current code happens to
behave identically whether running "pull --rebase" of c1 into c0 or
c2 so it is sufficient to test the operation into c2.  The test is
*not* about how the current code happens to work.  It is to make
sure the scenarios test authors care about will keep behaving the
same way.

Some tests may be expecting that pulling c1 into c0 would issue the
message, and that the command succeeds, and with the patch 3/3 the
outcome may become different, i.e. the command succeeds without
annoying message.  That would break the expectation of the original
test authors, and it is a good thing.  By recording a change to the
expectation, we can document how the new behaviour works better under
the same scenario.


[Footnote]

*1* That does not mean we must not care about other scenarios that
are different from what have been tested with existing tests.  If
there is new behaviour introduced by patch 3/3, it is prudent to
protect it from future breakage by adding a test that pulls c1 into
c2, if that case is not already tested with existing tests.  

I suspect we already make sure a non-ff merge gives the annoying
message while going ahead, so there may be no new additional test
required, though.

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

On Fri, Dec 11, 2020 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> We seem to be losing test coverage by checking how pull.ff=only prevents
> >> the command from working in a non-ff merge.
> >
> > No we don't. Remove the "test_config pull.ff only" and the test fails,
> > as expected.
>
> > ...
> > What do we get?
> >
> > not ok 4 - pull.rebase not set and pull.ff=true
> > not ok 5 - pull.rebase not set and pull.ff=false
> > not ok 6 - pull.rebase not set and pull.ff=only
> > not ok 7 - pull.rebase not set and --rebase given
> > not ok 8 - pull.rebase not set and --no-rebase given
> > not ok 9 - pull.rebase not set and --ff given
> > not ok 10 - pull.rebase not set and --no-ff given
> > not ok 11 - pull.rebase not set and --ff-only given
> >
> > All failures. Exactly as expected.
>
> Assuming only one kind of breakage and try to break exactly that
> thing does not prove much.

It proves we are testing exactly the thing the test is meant to test.

No test is ever perfect.

> I'll keep this short as I am supposed to be off officially.

I appreciate that.

> With pulling without choosing between rebase/merge, the old code
> had one behaviour wrt the message---it always advised, whether
> the pull was ff or not.
>
> The new code has two behaviour wrt this aspect.  It behaves
> differently when the pull is ff or non-ff.  That would double the
> possibility that needs to be tested if we wanted to keep covering
> the original set of conditions *and* cover all new possibilities.
>
> I am saying that you should keep the original ones, and add new ones
> to cover the new cases if that matters.  Otherwise the conditions
> under which the original tests were checking would no longer be
> tested.

I disagree. The old condition is perfectly well tested with this test:

  test_expect_success 'pull.rebase not set' '
    git reset --hard c2 &&
    git -c color.advice=always pull . c1 2>err &&
    test_decode_color <err >decoded &&
    test_i18ngrep "<YELLOW>hint: " decoded &&
    test_i18ngrep "Pulling without specifying how to reconcile" decoded
  '

In other words; we have tests going horizontally, and tests going
vertically. That should cover all the bases.

But fine, if you want a matrix of tests I can do that.

> >  test_expect_success 'pull.rebase not set and --rebase given' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       git pull --rebase . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
>
> This used to make sure an attempt to rebase c1 onto c0, which can be
> fast-forwarded, would work fine,

No it didn't. It may very well have done a merge, or done nothing at all.

The tests that actually check that "git pull --rebase" works both in
the fast-forward and non-fast-forward are in t/t5520-pull.sh.

The goal of this particular test is to check that the warning is not there.

> even though it used to give
> warning.  We should keep testing the same condition.

We are testing what was originally tested: that the warning is not presented.

> The expectation of seeing the warning is what must be changed, not the
> test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no
> longer making sure that c1 can be rebased onto c0 cleanly.

We disagree on what the "test condition" is supposed to be on this test.

But fine, I'll create the matrix.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> We seem to be losing test coverage by checking how pull.ff=only prevents
>> the command from working in a non-ff merge.
>
> No we don't. Remove the "test_config pull.ff only" and the test fails,
> as expected.

> ...
> What do we get?
>
> not ok 4 - pull.rebase not set and pull.ff=true
> not ok 5 - pull.rebase not set and pull.ff=false
> not ok 6 - pull.rebase not set and pull.ff=only
> not ok 7 - pull.rebase not set and --rebase given
> not ok 8 - pull.rebase not set and --no-rebase given
> not ok 9 - pull.rebase not set and --ff given
> not ok 10 - pull.rebase not set and --no-ff given
> not ok 11 - pull.rebase not set and --ff-only given
>
> All failures. Exactly as expected.

Assuming only one kind of breakage and try to break exactly that
thing does not prove much.

I'll keep this short as I am supposed to be off officially.

With pulling without choosing between rebase/merge, the old code
had one behaviour wrt the message---it always advised, whether
the pull was ff or not.

The new code has two behaviour wrt this aspect.  It behaves
differently when the pull is ff or non-ff.  That would double the
possibility that needs to be tested if we wanted to keep covering
the original set of conditions *and* cover all new possibilities.

I am saying that you should keep the original ones, and add new ones
to cover the new cases if that matters.  Otherwise the conditions
under which the original tests were checking would no longer be
tested.

>  test_expect_success 'pull.rebase not set and --rebase given' '
> -	git reset --hard c0 &&
> +	git reset --hard c2 &&
>  	git pull --rebase . c1 2>err &&
>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '

This used to make sure an attempt to rebase c1 onto c0, which can be
fast-forwarded, would work fine, even though it used to give
warning.  We should keep testing the same condition.  The
expectation of seeing the warning is what must be changed, not the
test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no
longer making sure that c1 can be rebased onto c0 cleanly.

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

On Fri, Dec 11, 2020 at 3:22 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > There's no need to display the annoying warning on every pull... only
> > the ones that are not fast-forward.
>
> Yes!  And thanks to the previous two steps, the change to the code
> is quite obvious.  I don't have to give any further comment on the
> part that changes code---it is well done, period.
>
> > This requires the tests to pick another base, so the merge is not
> > fast-forward. And in the cases where --ff-only is specified add
> > test_must_fail (since now they are non-fast-forward).
>
> I am not sure what this means.

It means in order to test the previous warning--which appeared in
fast-forward and non-fast-forward merges, and now appears only on
non-fast-forward merges--we need non-fast-forward merge situations.

> Existing tests pull histories that may or may not be descendants of
> the HEAD.  If we do not change the pair of commits involved in the
> tests, i.e. if we do not apply many s/c0/c2/ replacement I see in
> the patch, some of these tests would change their behaviour with
> respect to their advice output (but not the outcome).

All the tests, actually.

> The ones that
> pulled fast-forward would stop showing the warning, and that is a
> good effect of this change.  We want to see that in the update to
> the tests, no?

Yes.

> The ones that pulled non-fast-forward history would still show the
> warning as they used to, and that is also what we want to see after
> this change.

Currently no test is doing that; all are in a fast-forward situation.

> In other words, the changes the paragraph says that the commit made
> to the tests sound quite backwards.

I'm not sure how it is backwards. All the tests are fast-forward, we
need them not fast-forward, so we pick another base... so the merges
are not fast-forward.

> The actual changes to some of the tests do look sensible, testing
> both sides of the coin.  I've looked at the changes to the tests,
> but cannot convince me that we are not making irrelevant changes
> to the tests, or losing coverage needlessly because of s/c0/c2/
> (i.e. turning tests that used to check fast-forward situations
> into tests that check non-ff situations).

No test is checking fast-forward situations, because the warning
doesn't check fast-forward situations.

> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> > index 6774e9d86f..6b4adab8b1 100755
> > --- a/t/t7601-merge-pull-config.sh
> > +++ b/t/t7601-merge-pull-config.sh
> > @@ -28,7 +28,7 @@ test_expect_success 'setup' '
> >  '
> >
> >  test_expect_success 'pull.rebase not set' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       git -c color.advice=always pull . c1 2>err &&
> >       test_decode_color <err >decoded &&
> >       test_i18ngrep "<YELLOW>hint: " decoded &&
>
> This is not "keeping what the original test does and adjusting the
> expectation, to demonstrate how behaviour changed"; instead, we make
> sure that the message we originally gave and we intend to keep
> giving is shown in non-ff situation by choosing the current commit
> that won't allow a ff merge.  This is OK if we did not lose test
> coverage---as long as we test that we no longer give the message in
> ff situation somewhere else, And that happens later, I think.
>
> > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
> >
> >  '
> >
> > -test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +test_expect_success 'pull.rebase not set (fast-forward)' '
> >       git reset --hard c0 &&
> > +     git pull . c1 2>err &&
> > +     test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +'
>
> This is the new test to check the other side of the coin.  It sees
> how the original test to merge c1 into c0 would behave with the new
> code.  We make sure we do not give the advice because it is
> irrelevant in this situation.
>
> So the above two are good, even though the way this patch updates
> tests is probably a bit more error prone than necessary.

Any suggestions to make it less error prone are welcome.

> Since we have checked how the new code behave for fast-forward with
> this new test, the remainder of the entire test script can be
> modified to test only non-ff situation without losing test coverage?
>
> I am not sure if that is the case.

It is the case. That's what the patch does.

> > +test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +     git reset --hard c2 &&
> >       test_config pull.ff true &&
> >       git pull . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
>
> We are merely allowing fast-forward merges without unnecessary merge
> commits, but we are faced to merge c1 into c2, which is not ff.  The
> command goes ahead and merges anyway, but why shouldn't we be seeing
> the message?  I am puzzled.

Because that's what the current code does; it just checks that any
opt_ff is set, it doesn't check for any specific values.

I tried to fix that since v1, v2, v3, and v4 of the series [1], which
only received comments from Elijah Newren, but that's a separate
patch, and as I mentioned in the cover letter of v5; I've dropped all
those other patches.

> >  test_expect_success 'pull.rebase not set and pull.ff=false' '
> > -     git reset --hard c0 &&
> > +     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' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       test_config pull.ff only &&
> > -     git pull . c1 2>err &&
> > +     test_must_fail git pull . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
>
> This used to test that fast-forwarding the HEAD from c0 to c2 would
> be done successfully without issuing the message.  Shouldn't that
> still be true in the improved "do not complain on fast-forward" code?

No. It doesn't care if it's fast-forward or not; it's just checking
that "pull.ff=only" is set.

Before the code just checks "!opt_ff", now it checks "!opt_ff && !can_ff".

So, in order to do the "pull.ff=only" check, we need a non-fast-forward merge.

> We seem to be losing test coverage by checking how pull.ff=only prevents
> the command from working in a non-ff merge.

No we don't. Remove the "test_config pull.ff only" and the test fails,
as expected.

> I'd stop my review on the tests here, but I generally think s/c0/c2/
> done in this patch is a wrong thing to do.  We are changing the
> condition under which the messages is given (we are narrowing it to
> avoid giving it when it is irrelevant), without changing the final
> outcome (even though we changed the condition to give the message,
> we didn't change what the final outcome of the pull command would
> be), so I'd strongly prefer testing the same set of scenarios and
> update the expectation to an improved reality.

We are testing *exactly* the same test scenarios, we are just forcing
can_ff to be false.

If I remove the precise thing each test-case is supposed to test:

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b4adab8b1..6c3413ddc9 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -44,52 +44,49 @@ test_expect_success 'pull.rebase not set (fast-forward)' '

 test_expect_success 'pull.rebase not set and pull.ff=true' '
  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' '
  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' '
  git reset --hard c2 &&
- test_config pull.ff only &&
- test_must_fail git pull . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

 test_expect_success 'pull.rebase not set and --rebase given' '
  git reset --hard c2 &&
- git pull --rebase . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

 test_expect_success 'pull.rebase not set and --no-rebase given' '
  git reset --hard c2 &&
- git pull --no-rebase . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

 test_expect_success 'pull.rebase not set and --ff given' '
  git reset --hard c2 &&
- git pull --ff . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

 test_expect_success 'pull.rebase not set and --no-ff given' '
  git reset --hard c2 &&
- git pull --no-ff . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

 test_expect_success 'pull.rebase not set and --ff-only given' '
  git reset --hard c2 &&
- test_must_fail git pull --ff-only . c1 2>err &&
+ git pull . c1 2>err &&
  test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '

What do we get?

not ok 4 - pull.rebase not set and pull.ff=true
not ok 5 - pull.rebase not set and pull.ff=false
not ok 6 - pull.rebase not set and pull.ff=only
not ok 7 - pull.rebase not set and --rebase given
not ok 8 - pull.rebase not set and --no-rebase given
not ok 9 - pull.rebase not set and --ff given
not ok 10 - pull.rebase not set and --no-ff given
not ok 11 - pull.rebase not set and --ff-only given

All failures. Exactly as expected.

So what coverage precisely are we losing?

Cheers.

[1] https://lore.kernel.org/git/20201204061623.1170745-13-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: [PATCH v5 3/3] pull: display default warning only when non-ff

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no need to display the annoying warning on every pull... only
> the ones that are not fast-forward.

Yes!  And thanks to the previous two steps, the change to the code
is quite obvious.  I don't have to give any further comment on the
part that changes code---it is well done, period.

> This requires the tests to pick another base, so the merge is not
> fast-forward. And in the cases where --ff-only is specified add
> test_must_fail (since now they are non-fast-forward).

I am not sure what this means.  

Existing tests pull histories that may or may not be descendants of
the HEAD.  If we do not change the pair of commits involved in the
tests, i.e. if we do not apply many s/c0/c2/ replacement I see in
the patch, some of these tests would change their behaviour with
respect to their advice output (but not the outcome).  The ones that
pulled fast-forward would stop showing the warning, and that is a
good effect of this change.  We want to see that in the update to
the tests, no?

The ones that pulled non-fast-forward history would still show the
warning as they used to, and that is also what we want to see after
this change.  Which means we want the tests to keep doing what they
do, while adjusting what we expect out of the tested commands to the
world that is made better with the code change in this patch.  I do
not think we need to add new tests to demonstrate the new behaviour,
as they (i.e. when the test pulls fast-forwardable history, with
various combinations of options and configuration) seem to be pretty
well covered already.

In other words, the changes the paragraph says that the commit made
to the tests sound quite backwards.

The actual changes to some of the tests do look sensible, testing
both sides of the coin.  I've looked at the changes to the tests,
but cannot convince me that we are not making irrelevant changes
to the tests, or losing coverage needlessly because of s/c0/c2/
(i.e. turning tests that used to check fast-forward situations
into tests that check non-ff situations).

> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 6774e9d86f..6b4adab8b1 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -28,7 +28,7 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'pull.rebase not set' '
> -	git reset --hard c0 &&
> +	git reset --hard c2 &&
>  	git -c color.advice=always pull . c1 2>err &&
>  	test_decode_color <err >decoded &&
>  	test_i18ngrep "<YELLOW>hint: " decoded &&

This is not "keeping what the original test does and adjusting the
expectation, to demonstrate how behaviour changed"; instead, we make
sure that the message we originally gave and we intend to keep
giving is shown in non-ff situation by choosing the current commit
that won't allow a ff merge.  This is OK if we did not lose test
coverage---as long as we test that we no longer give the message in
ff situation somewhere else, And that happens later, I think.

> @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
>  
>  '
>  
> -test_expect_success 'pull.rebase not set and pull.ff=true' '
> +test_expect_success 'pull.rebase not set (fast-forward)' '
>  	git reset --hard c0 &&
> +	git pull . c1 2>err &&
> +	test_i18ngrep ! "Pulling without specifying how to reconcile" err
> +'

This is the new test to check the other side of the coin.  It sees
how the original test to merge c1 into c0 would behave with the new
code.  We make sure we do not give the advice because it is
irrelevant in this situation.

So the above two are good, even though the way this patch updates
tests is probably a bit more error prone than necessary.

Since we have checked how the new code behave for fast-forward with
this new test, the remainder of the entire test script can be
modified to test only non-ff situation without losing test coverage?

I am not sure if that is the case.

> +test_expect_success 'pull.rebase not set and pull.ff=true' '
> +	git reset --hard c2 &&
>  	test_config pull.ff true &&
>  	git pull . c1 2>err &&
>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '

We are merely allowing fast-forward merges without unnecessary merge
commits, but we are faced to merge c1 into c2, which is not ff.  The
command goes ahead and merges anyway, but why shouldn't we be seeing
the message?  I am puzzled.

>  test_expect_success 'pull.rebase not set and pull.ff=false' '
> -	git reset --hard c0 &&
> +	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' '
> -	git reset --hard c0 &&
> +	git reset --hard c2 &&
>  	test_config pull.ff only &&
> -	git pull . c1 2>err &&
> +	test_must_fail git pull . c1 2>err &&
>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '

This used to test that fast-forwarding the HEAD from c0 to c2 would
be done successfully without issuing the message.  Shouldn't that
still be true in the improved "do not complain on fast-forward" code?

We seem to be losing test coverage by checking how pull.ff=only prevents
the command from working in a non-ff merge.

I'd stop my review on the tests here, but I generally think s/c0/c2/
done in this patch is a wrong thing to do.  We are changing the
condition under which the messages is given (we are narrowing it to
avoid giving it when it is irrelevant), without changing the final
outcome (even though we changed the condition to give the message,
we didn't change what the final outcome of the pull command would
be), so I'd strongly prefer testing the same set of scenarios and
update the expectation to an improved reality.

>  test_expect_success 'pull.rebase not set and --rebase given' '
> -	git reset --hard c0 &&
> +	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' '
> -	git reset --hard c0 &&
> +	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' '
> -	git reset --hard c0 &&
> +	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' '
> -	git reset --hard c0 &&
> +	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' '
> -	git reset --hard c0 &&
> -	git pull --ff-only . c1 2>err &&
> +	git reset --hard c2 &&
> +	test_must_fail git pull --ff-only . c1 2>err &&
>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '

[PATCH v5 3/3] pull: display default warning only when non-ff

[Felipe Contreras]

There's no need to display the annoying warning on every pull... only
the ones that are not fast-forward.

This requires the tests to pick another base, so the merge is not
fast-forward. And in the cases where --ff-only is specified add
test_must_fail (since now they are non-fast-forward).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c               |  7 +++++--
 t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index ff8e3ce137..9a7caf3a3e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -934,6 +934,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 can_ff;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1029,7 +1030,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 (default_mode && opt_verbosity >= 0 && !opt_ff) {
+	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+
+	if (default_mode && !can_ff && 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"
@@ -1058,7 +1061,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..6b4adab8b1 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -28,7 +28,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pull.rebase not set' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git -c color.advice=always pull . c1 2>err &&
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
@@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
 
 '
 
-test_expect_success 'pull.rebase not set and pull.ff=true' '
+test_expect_success 'pull.rebase not set (fast-forward)' '
 	git reset --hard c0 &&
+	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' '
+	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' '
-	git reset --hard c0 &&
+	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' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	test_config pull.ff only &&
-	git pull . c1 2>err &&
+	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' '
-	git reset --hard c0 &&
+	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' '
-	git reset --hard c0 &&
+	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' '
-	git reset --hard c0 &&
+	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' '
-	git reset --hard c0 &&
+	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' '
-	git reset --hard c0 &&
-	git pull --ff-only . c1 2>err &&
+	git reset --hard c2 &&
+	test_must_fail git pull --ff-only . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
-- 
2.29.2