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

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

[Felipe Contreras]

The discussion about making fast-forward-only pulls the default is stuck on mud, and there's no
agreement about what we should even be warning our users about.

Even my straightforward patches about improving documentation, and the consistency of the UI with
--merge and other obvious fixes lost traction.

This is not the totality of part I, it's doing one thing, and one thing only: stops warning the
users on every single pull (warns only if they are not-fast-forward).

Everything else is dropped.

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 21b50aff77..5c3fb67c01 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -38,20 +38,6 @@ as set by linkgit:git-branch[1] `--track`.
 Assume the following history exists and the current branch is
 "`master`":
 
-------------
-	  A---B---C master on origin
-	 /
-    D---E master
-------------
-
-Then `git pull` will merge in a fast-forward way up to the new master.
-
-------------
-    D---E---A---B---C master, origin/master
-------------
-
-However, a non-fast-forward case looks very different.
-
 ------------
 	  A---B---C master on origin
 	 /
@@ -60,9 +46,6 @@ However, a non-fast-forward case looks very different.
 	origin/master in your repository
 ------------
 
-By default `git pull` will warn about these situations, however, most likely
-you would want to force a merge, which you can do with `git pull --merge`.
-
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
 until its current commit (`C`) on top of `master` and record the
@@ -148,11 +131,8 @@ It rewrites history, which does not bode well when you
 published that history already.  Do *not* use this option
 unless you have read linkgit:git-rebase[1] carefully.
 
--m::
---merge::
-	Force a merge.
-+
-Previously this was --no-rebase, but that usage has been deprecated.
+--no-rebase::
+	Override earlier --rebase.
 
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/pull.c b/builtin/pull.c
index 118fbdeb62..9a7caf3a3e 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
@@ -74,7 +76,7 @@ static char *opt_progress;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 
 /* Options passed to git-merge or git-rebase */
-static enum rebase_type opt_rebase;
+static enum rebase_type opt_rebase = -1;
 static char *opt_diffstat;
 static char *opt_log;
 static char *opt_signoff;
@@ -126,11 +128,9 @@ static struct option pull_options[] = {
 	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
 	OPT_CALLBACK_F('r', "rebase", &opt_rebase,
-		"(false|true|merges|preserve|interactive)",
-		N_("incorporate changes by rebasing rather than merging"),
-		PARSE_OPT_OPTARG, parse_opt_rebase),
-	OPT_SET_INT('m', "merge", &opt_rebase,
-		N_("incorporate changes by merging"), REBASE_FALSE),
+	  "(false|true|merges|preserve|interactive)",
+	  N_("incorporate changes by rebasing rather than merging"),
+	  PARSE_OPT_OPTARG, parse_opt_rebase),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -346,7 +346,9 @@ static enum rebase_type config_get_rebase(void)
 	if (!git_config_get_value("pull.rebase", &value))
 		return parse_config_rebase("pull.rebase", value, 1);
 
-	return REBASE_DEFAULT;
+	default_mode = 1;
+
+	return REBASE_FALSE;
 }
 
 /**
@@ -441,7 +443,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 	const char *remote = curr_branch ? curr_branch->remote_name : NULL;
 
 	if (*refspecs) {
-		if (opt_rebase >= REBASE_TRUE)
+		if (opt_rebase)
 			fprintf_ln(stderr, _("There is no candidate for rebasing against among the refs that you just fetched."));
 		else
 			fprintf_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
@@ -454,7 +456,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			repo);
 	} else if (!curr_branch) {
 		fprintf_ln(stderr, _("You are not currently on a branch."));
-		if (opt_rebase >= REBASE_TRUE)
+		if (opt_rebase)
 			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
 		else
 			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -469,7 +471,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			remote_name = _("<remote>");
 
 		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
-		if (opt_rebase >= REBASE_TRUE)
+		if (opt_rebase)
 			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
 		else
 			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -931,11 +933,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
+	int autostash;
 	int can_ff;
 
-	opt_ff = xstrdup_or_null(config_get_ff());
-	opt_rebase = config_get_rebase();
-
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
@@ -952,6 +952,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
+	if (!opt_ff)
+		opt_ff = xstrdup_or_null(config_get_ff());
+
+	if (opt_rebase < 0)
+		opt_rebase = config_get_rebase();
+
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
 
@@ -961,8 +967,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	if (opt_rebase >= REBASE_TRUE) {
-		int autostash = config_autostash;
+	autostash = config_autostash;
+	if (opt_rebase) {
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
 
@@ -1021,28 +1027,29 @@ 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 >= REBASE_TRUE && merge_heads.nr > 1)
+	if (opt_rebase && merge_heads.nr > 1)
 		die(_("Cannot rebase onto multiple branches."));
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
-		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
-			"you need to specify if you want a merge, or a rebase.\n"
-			"You can squelch this message by running one of the following commands:\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.\n"
-			"If unsure, run \"git pull --merge\".\n"
-			"Read \"git pull --help\" for more information."));
+	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"
+			 "\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 >= REBASE_TRUE) {
+	if (opt_rebase) {
 		int ret = 0;
+		int ran_ff = 0;
 
 		struct object_id newbase;
 		struct object_id upstream;
@@ -1053,15 +1060,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
-
-		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
-			free(opt_ff);
-			opt_ff = xstrdup_or_null("--ff-only");
-			ret = run_merge();
-		} else {
-			ret = run_rebase(&newbase, &upstream);
+		if (!autostash) {
+			if (can_ff) {
+				/* we can fast-forward this without invoking rebase */
+				opt_ff = "--ff-only";
+				ran_ff = 1;
+				ret = run_merge();
+			}
 		}
+		if (!ran_ff)
+			ret = run_rebase(&newbase, &upstream);
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
diff --git a/rebase.h b/rebase.h
index 34d4acfd74..cc723d4748 100644
--- a/rebase.h
+++ b/rebase.h
@@ -3,8 +3,7 @@
 
 enum rebase_type {
 	REBASE_INVALID = -1,
-	REBASE_DEFAULT = 0,
-	REBASE_FALSE,
+	REBASE_FALSE = 0,
 	REBASE_TRUE,
 	REBASE_PRESERVE,
 	REBASE_MERGES,
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 1a4fe2583a..db1a381cd9 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -11,10 +11,10 @@ test_expect_success 'setup' '
 	 git commit -m one)
 '
 
-test_expect_success 'git pull -q' '
+test_expect_success 'git pull -q --no-rebase' '
 	mkdir clonedq &&
 	(cd clonedq && git init &&
-	git pull -q "../parent" >out 2>err &&
+	git pull -q --no-rebase "../parent" >out 2>err &&
 	test_must_be_empty err &&
 	test_must_be_empty out)
 '
@@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull' '
+test_expect_success 'git pull --no-rebase' '
 	mkdir cloned &&
 	(cd cloned && git init &&
-	git pull "../parent" >out 2>err &&
+	git pull --no-rebase "../parent" >out 2>err &&
 	test -s err &&
 	test_must_be_empty out)
 '
@@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v' '
+test_expect_success 'git pull -v --no-rebase' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
-	git pull -v "../parent" >out 2>err &&
+	git pull -v --no-rebase "../parent" >out 2>err &&
 	test -s err &&
 	test_must_be_empty out)
 '
@@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v -q' '
+test_expect_success 'git pull -v -q --no-rebase' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&
-	git pull -v -q "../parent" >out 2>err &&
+	git pull -v -q --no-rebase "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test_must_be_empty err)
 '
 
-test_expect_success 'git pull -q -v' '
+test_expect_success 'git pull -q -v --no-rebase' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
-	git pull -q -v "../parent" >out 2>err &&
+	git pull -q -v --no-rebase "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test -s err)
 '
 test_expect_success 'git pull --cleanup errors early on invalid argument' '
 	mkdir clonedcleanup &&
 	(cd clonedcleanup && git init &&
-	test_must_fail git pull --cleanup invalid "../parent" >out 2>err &&
+	test_must_fail git pull --no-rebase --cleanup invalid "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test -s err)
 '
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 8a6aae564a..6b4adab8b1 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -33,6 +33,7 @@ test_expect_success 'pull.rebase not set' '
 	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 (fast-forward)' '
@@ -45,7 +46,7 @@ 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_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
@@ -68,16 +69,16 @@ test_expect_success 'pull.rebase not set and --rebase given' '
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
-test_expect_success 'pull.rebase not set and --merge given' '
+test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c2 &&
-	git pull --merge . c1 2>err &&
+	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 c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '


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 | 28 ++++++++++-------
 2 files changed, 53 insertions(+), 36 deletions(-)

-- 
2.29.2

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

[Junio C Hamano]

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

> The discussion about making fast-forward-only pulls the default is
> stuck on mud, and there's no agreement about what we should even
> be warning our users about.

The above perception of yours is mostly due to misunderstanding, I
would have to say.  We are in agreement on what we should be warning
about at least, assuming that you are expressing what you want
clearly in the latest round of responses and I understood them
correctly [*1*].

I do not know if others on the list agree, though.

I do agree that there is no agreement on the behaviour in the
endgame.  In principle, I am in favor of disabling the more
dangerous half of the "git pull" command for those who haven't
configured anything.  But I can understand those who do not want
that behaviour, as the fallout would be quite big.

> Even my straightforward patches about improving documentation, and
> the consistency of the UI with --merge and other obvious fixes
> lost traction.

It may be obvious to you, but may not be to others on the list who
spoke in the thread and who didn't speak but read the discussion.

I did see potential goodness in the documentation update and that
was why I offered polishment on top of your patches in a v3 round,
but seeing the suggestions dismissed without convincing arguments
before v4 was sent out would have discouraged even the most patient
reviewers among us.  If you meant by "lost traction" the lack of
comments on v4, that was my reason for not commenting.

In any case, these three patches in this round looked quite sensible
to me, except for the tests in 3/3, and minor details of 2/3, both
of which I gave a more detailed review and suggestion.

Thanks.


[Footnote]

*1* The only difference between us is whether it is sensible to
allow explicitly ask to see the same behaviour as an unconfigured
user except for the help text---I do not think it is, and I do want
to avoid introducing pull.mode, but I've shown a way or two to get
the behaviour without adding pull.mode in the mix.

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

[Felipe Contreras]

On Fri, Dec 11, 2020 at 5:22 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > The discussion about making fast-forward-only pulls the default is
> > stuck on mud, and there's no agreement about what we should even
> > be warning our users about.
>
> The above perception of yours is mostly due to misunderstanding, I
> would have to say.  We are in agreement on what we should be warning
> about at least, assuming that you are expressing what you want
> clearly in the latest round of responses and I understood them
> correctly [*1*].

I'm not trying to be difficult here, but at every round where you have
stated what it is that I want, it's not actually what I want, and the
last round is no exception, in my option.

Let's assume that I'm not explaining clearly what I want.

In the last round you said you wanted an error, not a warning. That's
not what I want; I'm proposing a warning.

But that's not what I was referring to here.

> I do not know if others on the list agree, though.

This is what I was referring to. Initially there seemed to be some
interest, and suddenly that interest disappeared.

> I do agree that there is no agreement on the behaviour in the
> endgame.

See? I disagree.

I think the endgame is clear. How we get there is where there's no agreement.

> In principle, I am in favor of disabling the more
> dangerous half of the "git pull" command for those who haven't
> configured anything.  But I can understand those who do not want
> that behaviour, as the fallout would be quite big.

And who is that? Did anyone in the list express that they did not want
that behavior?

> > Even my straightforward patches about improving documentation, and
> > the consistency of the UI with --merge and other obvious fixes
> > lost traction.
>
> It may be obvious to you, but may not be to others on the list who
> spoke in the thread and who didn't speak but read the discussion.
>
> I did see potential goodness in the documentation update and that
> was why I offered polishment on top of your patches in a v3 round,
> but seeing the suggestions dismissed without convincing arguments
> before v4 was sent out would have discouraged even the most patient
> reviewers among us.  If you meant by "lost traction" the lack of
> comments on v4, that was my reason for not commenting.

I did not dismiss your suggestions, I replied to your suggestions [1].
You did not reply back.

Moreover, in patch 2 I saw you had some confusion [2], in which you
said you didn't see any value in updating the message without changing
the condition that triggers, to which I replied [3]: "Maybe it will be
clearer when I send all the patches."

That's why I sent v4; not because I thought the review of v3 was done,
but because we were stuck not seeing the evolution of the warning.

In v4 I went through every step of the evolution [4], and I went back
to what I said in v3:

  At this point we can update the warning to mention that we are inside
  a non-fast-forward case. But it's not necessary.

So I did not dismiss the suggestion, I replied to it, and put a pin on it.

You can certainly bring the same suggestion in v4, but I seem to have
convinced Elijah Newren that "fast-forward" can be used as an adverb
perfectly well, and it in fact is, in many places in the documentation
both internal, and external.

> In any case, these three patches in this round looked quite sensible
> to me, except for the tests in 3/3, and minor details of 2/3, both
> of which I gave a more detailed review and suggestion.

Great.

That should improve the situation of most users. And also has the
added benefit that it's 3 less patches I have to carry around on every
round.

Cheers.

[1] https://lore.kernel.org/git/CAMP44s1ZDXzGfEqpTeiG=aGAYK40ebnBLQKAbA7KGtcePGARfw@mail.gmail.com/
[2] https://lore.kernel.org/git/xmqq4kkx9vzx.fsf@gitster.c.googlers.com/
[3] https://lore.kernel.org/git/CAMP44s1aYqzCVvELH8zULaTkOdgLSSAQ0LE8WfgQKLPfU2MHfg@mail.gmail.com/
[4] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com

-- 
Felipe Contreras

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

[Junio C Hamano]

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

> But that's not what I was referring to here.
>
>> I do not know if others on the list agree, though.
>
> This is what I was referring to. Initially there seemed to be some
> interest, and suddenly that interest disappeared.

Perhaps most of them are happy enough with the current behaviour.

Perhaps nobody cares strongly enough to say what they want to see,
as they fear that by speaking up they would be drawn into a
discussion that is needlessly hot and unpleasant.

>> I do agree that there is no agreement on the behaviour in the
>> endgame.
>
> See? I disagree.
>
> I think the endgame is clear. How we get there is where there's no agreement.

What you want as the endgame may be clear to you.

But I do not think there is clear concensus among people on the
list.

>> In principle, I am in favor of disabling the more
>> dangerous half of the "git pull" command for those who haven't
>> configured anything.  But I can understand those who do not want
>> that behaviour, as the fallout would be quite big.
>
> And who is that? Did anyone in the list express that they did not want
> that behavior?

I thought that you at least saw Dscho's reaction to the breakage
caused by "future" patch in response to one of the recent What's
cooking reports.  Doesn't that count "anyone on the list express"?

I am starting to feel myself that "don't do anything if this is not
a fast-forward" may be something that would have been great if we
had it from day one but is no longer a feasible default with
existing users, to be quite honest, so you can count my 20% as
another example.

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

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > But that's not what I was referring to here.
> >
> >> I do not know if others on the list agree, though.
> >
> > This is what I was referring to. Initially there seemed to be some
> > interest, and suddenly that interest disappeared.
> 
> Perhaps most of them are happy enough with the current behaviour.

Or perhaps they are happhy enough the current behavior *for them*, but
not the other users.

> Perhaps nobody cares strongly enough to say what they want to see,
> as they fear that by speaking up they would be drawn into a
> discussion that is needlessly hot and unpleasant.

Or perhaps they got busy.

Or perhaps they got sick.

I don't see how listing hypotheticals is fruitful.

> >> I do agree that there is no agreement on the behaviour in the
> >> endgame.
> >
> > See? I disagree.
> >
> > I think the endgame is clear. How we get there is where there's no agreement.
> 
> What you want as the endgame may be clear to you.
> 
> But I do not think there is clear concensus among people on the
> list.

I do. It has been clear for many years.

> >> In principle, I am in favor of disabling the more
> >> dangerous half of the "git pull" command for those who haven't
> >> configured anything.  But I can understand those who do not want
> >> that behaviour, as the fallout would be quite big.
> >
> > And who is that? Did anyone in the list express that they did not want
> > that behavior?
> 
> I thought that you at least saw Dscho's reaction to the breakage
> caused by "future" patch in response to one of the recent What's
> cooking reports.

Yes, I saw it, but it was a complaint about the breakage of test due to
the merge of a patch that wasn't meant to be merged today.

> Doesn't that count "anyone on the list express"?

No. These are two different statements.

 1. This will break things for *me*
 2. This is a bad thing for *everyone*

These two statements have a different object. Also, the first doesn't
necessarily require rationale, we can take his word for it. But the
second one does.

It's not enough to say "I think this patch is bad for everyone"; you
have to explain *why*. I'd be happy to discuss with him his rationale,
which he hasn't given.

Moreover, isn't Schindelin advocating for a change that will cause
breakage (renaming the master branch).

> I am starting to feel myself that "don't do anything if this is not
> a fast-forward" may be something that would have been great if we
> had it from day one but is no longer a feasible default with
> existing users, to be quite honest, so you can count my 20% as
> another example.

As opposed to virtually everyone in this mailing list that has given an
opinion about the topic in the last 10 years (including you I count 3
out of literally dozens).

I will do some mail archeology and present the results.

Anyway, this is not relevant today, because you were the one that
proposed to go straight to an error.

What I propose for today is to introduce the option
"pull.mode=fast-forward", and improve the warning. Not an error.

That doesn't break the behavior for anyone, including Schindelin.

Cheers.

-- 
Felipe Contreras

[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 0/3] pull: stop warning on every pull

[Felipe Contreras]

On Sat, Dec 12, 2020 at 10:36 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Junio C Hamano wrote:

> > I am starting to feel myself that "don't do anything if this is not
> > a fast-forward" may be something that would have been great if we
> > had it from day one but is no longer a feasible default with
> > existing users, to be quite honest, so you can count my 20% as
> > another example.
>
> As opposed to virtually everyone in this mailing list that has given an
> opinion about the topic in the last 10 years (including you I count 3
> out of literally dozens).

A quick update after reading a great deal of mails.

The only person I counted in 2013 that was against changing the
default is Matthieu Moy, and reading back what he said, even him was
in favor of adding a configuration for the new mode [1]. What he was
against was making it the default, and adding a warning stating it
would be the default.

Fortunately my patches introduce the mode [2] without making it
necessarily the default (we don't even have to change the warning and
say it will be changed in the future [3]).

So *nobody* was against the introduction of such mode.

Also, after reading the input from GitHub trainers, which Jeff
provided [4], perhaps leaving the default as "merge" makes sense. Our
curse of knowledge may bias us into thinking the user knows what a
rebase is. Maybe a permanent advice warning is the way to go, I'm
still mulling it over.

> Anyway, this is not relevant today, because you were the one that
> proposed to go straight to an error.
>
> What I propose for today is to introduce the option
> "pull.mode=fast-forward", and improve the warning. Not an error.
>
> That doesn't break the behavior for anyone, including Schindelin.

This is still true. We don't lose anything by introducing the mode.

We can make the decision to flip the default later (or not).

Cheers.

[1] https://lore.kernel.org/git/vpqbo41lo2v.fsf@anie.imag.fr/
[2] https://lore.kernel.org/git/20201208002648.1370414-18-felipe.contreras@gmail.com/
[3] https://lore.kernel.org/git/20201208002648.1370414-19-felipe.contreras@gmail.com/
[4] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/

-- 
Felipe Contreras