[PATCH] pull: abort by default if fast-forwarding is impossible

[PATCH] pull: abort by default if fast-forwarding is impossible

[Alex Henrie]

The behavior of warning but merging anyway is difficult to explain to
users because it sends mixed signals. End the confusion by firmly
requiring the user to specify whether they want a merge or a rebase.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/git-pull.txt | 14 +++++++++-----
 builtin/pull.c             | 32 +++++++++++++++-----------------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c01..0fb185811e 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -16,13 +16,17 @@ DESCRIPTION
 -----------
 
 Incorporates changes from a remote repository into the current
-branch.  In its default mode, `git pull` is shorthand for
-`git fetch` followed by `git merge FETCH_HEAD`.
+branch.  `git pull` is shorthand for `git fetch` followed by
+`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.
 
 More precisely, 'git pull' runs 'git fetch' with the given
-parameters and calls 'git merge' to merge the retrieved branch
-heads into the current branch.
-With `--rebase`, it runs 'git rebase' instead of 'git merge'.
+parameters and then, by default, attempts to fast-forward the
+current branch to the remote branch head.  If fast-forwarding
+is not possible because the local and remote branches have
+diverged, the `--rebase` option causes 'git rebase' to be
+called to rebase the local branch upon the remote branch, and
+the `--no-rebase` option causes 'git merge' to be called to
+merge the retrieved branch heads into the current branch.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
diff --git a/builtin/pull.c b/builtin/pull.c
index e8927fc2ff..21eaebc463 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 	return ret;
 }
 
-static void show_advice_pull_non_ff(void)
+static void die_pull_non_ff(void)
 {
-	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
-		 "discouraged. You can squelch this message by running one of the following\n"
-		 "commands sometime before your next pull:\n"
-		 "\n"
-		 "  git config pull.rebase false  # merge (the default strategy)\n"
-		 "  git config pull.rebase true   # rebase\n"
-		 "  git config pull.ff only       # fast-forward only\n"
-		 "\n"
-		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
-		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-		 "or --ff-only on the command line to override the configured default per\n"
-		 "invocation.\n"));
+	die(_("Pulling without specifying how to reconcile divergent branches is not\n"
+	      "possible. 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\n"
+	      "  git config pull.rebase true   # rebase\n"
+	      "  git config pull.ff only       # fast-forward only\n"
+	      "\n"
+	      "You can replace \"git config\" with \"git config --global\" to set a default\n"
+	      "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
+	      "or --ff-only on the command line to override the configured default per\n"
+	      "invocation.\n"));
 }
 
 int cmd_pull(int argc, const char **argv, const char *prefix)
@@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
-	}
+	if (rebase_unspecified && !opt_ff && !can_ff)
+		die_pull_non_ff();
 
 	if (opt_rebase) {
 		int ret = 0;
-- 
2.32.0.95.g4a22da5c5a

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Elijah Newren]

On Sat, Jun 26, 2021 at 5:10 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> The behavior of warning but merging anyway is difficult to explain to
> users because it sends mixed signals. End the confusion by firmly
> requiring the user to specify whether they want a merge or a rebase.

I would use some of Junio's wording from
https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/:

Given how annoying the "loudly warn but still go ahead" behaviour for
git pull's default behavior when fast-forwarding is not possible
(which was also displayed even when fast-forwarding was possible until
c525de335e ("pull: display default warning only when non-ff",
2020-12-12)), and how easy it is for existing users to have squelched
the annoyance by choosing between rebase and merge already, turn the
warning into an error.  As an added bonus, the previous behavior of
git pull was difficult to explain, and making this change helps us
simplify it.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  Documentation/git-pull.txt | 14 +++++++++-----
>  builtin/pull.c             | 32 +++++++++++++++-----------------
>  2 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 5c3fb67c01..0fb185811e 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -16,13 +16,17 @@ DESCRIPTION
>  -----------
>
>  Incorporates changes from a remote repository into the current
> -branch.  In its default mode, `git pull` is shorthand for
> -`git fetch` followed by `git merge FETCH_HEAD`.
> +branch.  `git pull` is shorthand for `git fetch` followed by

I'd replace "shorthand for" with "shorthand for either"

> +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.
>
>  More precisely, 'git pull' runs 'git fetch' with the given
> -parameters and calls 'git merge' to merge the retrieved branch
> -heads into the current branch.
> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.
> +parameters and then, by default, attempts to fast-forward the
> +current branch to the remote branch head.  If fast-forwarding
> +is not possible because the local and remote branches have

Good to here.

> +diverged, the `--rebase` option causes 'git rebase' to be...
> +called to rebase the local branch upon the remote branch, and
> +the `--no-rebase` option causes 'git merge' to be called to
> +merge the retrieved branch heads into the current branch.

This is okay, but can we just simplify it to:

....diverged, then the user must specify whether to rebase or merge
(see the --rebase and --no-rebase flags).



While at it, we may also want to change the documentation for the
--no-rebase flag.  Instead of

--no-rebase::
    Override earlier --rebase.

which can be confusing since --rebase takes so many different choices, instead:

--no-rebase::
    This is shorthand for --rebase=false


>  <repository> should be the name of a remote repository as
>  passed to linkgit:git-fetch[1].  <refspec> can name an
> diff --git a/builtin/pull.c b/builtin/pull.c
> index e8927fc2ff..21eaebc463 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
>         return ret;
>  }
>
> -static void show_advice_pull_non_ff(void)
> +static void die_pull_non_ff(void)
>  {
> -       advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> -                "discouraged. You can squelch this message by running one of the following\n"
> -                "commands sometime before your next pull:\n"
> -                "\n"
> -                "  git config pull.rebase false  # merge (the default strategy)\n"
> -                "  git config pull.rebase true   # rebase\n"
> -                "  git config pull.ff only       # fast-forward only\n"
> -                "\n"
> -                "You can replace \"git config\" with \"git config --global\" to set a default\n"
> -                "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -                "or --ff-only on the command line to override the configured default per\n"
> -                "invocation.\n"));
> +       die(_("Pulling without specifying how to reconcile divergent branches is not\n"
> +             "possible. 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\n"
> +             "  git config pull.rebase true   # rebase\n"
> +             "  git config pull.ff only       # fast-forward only\n"
> +             "\n"
> +             "You can replace \"git config\" with \"git config --global\" to set a default\n"
> +             "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> +             "or --ff-only on the command line to override the configured default per\n"
> +             "invocation.\n"));
>  }
>
>  int cmd_pull(int argc, const char **argv, const char *prefix)
> @@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>
> -       if (rebase_unspecified && !opt_ff && !can_ff) {
> -               if (opt_verbosity >= 0)
> -                       show_advice_pull_non_ff();
> -       }
> +       if (rebase_unspecified && !opt_ff && !can_ff)
> +               die_pull_non_ff();
>
>         if (opt_rebase) {
>                 int ret = 0;
> --
> 2.32.0.95.g4a22da5c5a

The code changes look good, but you'll need to add several test
changes as well, or this code change will cause test failures.  You'll
need to go through the relevant `git pull` invocations in the
testsuite and add the --no-rebase flag.

Thanks for working on this.

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Felipe Contreras]

Elijah Newren wrote:

> The code changes look good, but you'll need to add several test
> changes as well, or this code change will cause test failures.

Wouldn't you consider sending a patch without running 'make test'
"cavalier"?

> Thanks for working on this.

Such a completely different tone for a "cavalier" patch depending 100%
on the person who sent it. Weird.

-- 
Felipe Contreras

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Elijah Newren]

On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
>
> > The code changes look good, but you'll need to add several test
> > changes as well, or this code change will cause test failures.
>
> Wouldn't you consider sending a patch without running 'make test'
> "cavalier"?
>
> > Thanks for working on this.
>
> Such a completely different tone for a "cavalier" patch depending 100%
> on the person who sent it. Weird.

First of all, attacking a random bystander like Alex is rather
uncalled for.  As far as we know, Alex is trying to help out and made
a mistake or oversight, but will likely correct the problem if kindly
pointed out.  This email is egregiously far from constructive
criticism for Alex.

Second, why do you grossly mischaracterize folks (not just me) and try
so hard to insinuate unfairness?  I don't get it.  Do you really not
understand?

Let me attempt to explain...



Not running the tests is a problem, sure.  I pointed it out.  It could
have been a mistake or oversight on Alex's part.  Now, if Alex were to
state it was not accidental and then repeatedly defend submitting
patches without running tests, I would absolutely start calling out
both his submission and his professed level of reasonable due
diligence.  He hasn't done that though.  People deserve a chance to
respond and correct things first.

We gave you that same opportunity.  It was pointed out to you rather
politely that you had neglected to act on a report that a patch caused
a segfault[1]; in fact, you re-submitted the old patch as-is and
suggested it might be a good default[2].  You had a chance to point
out that what you had done was just a mistake or oversight.  Instead,
you confirmed that you saw the report and just didn't bother acting on
it[3].  And then you defended your misbehavior[4].  Over[5] and
over[6] and over[7] again.  And you also defended your related
patches[8] instead of double checking or trying to correct them --
even when prompted to double check[9] -- despite those additional
patches having severe problems of their own[10].

So, yes, I used terms like negligent, reckless, careless, and cavalier
in my critiques with you, but I very carefully always used those terms
to describe either the patches you submitted or the amount of due
diligence that you repeatedly defended.  I never applied those terms
to you.  And those terms didn't apply to you, as you could have chosen
to change and use a different level of due diligence.  So, your
accusations of personal attacks, harassment, and ad hominem arguments
that you made against me earlier[11], and your insinuations of
unfairness both then and now, are all false.

This, of course, isn't your first time grossly mischaracterizing
events and trying to claim unfairness.  Let me give you a second
example of why there was no unfairness in case the first is hard for
you to understand.  Let's use [12] as another example, and explain the
lead-up to that time: I politely gave you a chance to correct a
misleading claim[13], which you then instead admitted to being no
mistake[14], and didn't bother to correct.  The original misleading
claim was a minor issue.  If it had been corrected (even if it had
been intentional), it could have been no big deal.  The reason I drew
significant attention to it[15] was due to your response.  I pointed
out quite clearly when I drew attention to it, that it was your
*response* to the error that was problematic, and even reiterated it
in a follow-up.  I do not see why you ignored that, mischaracterized
my objections as being entirely about the initial error, hand picked
an example that didn't even do what you said it did, all to lead up to
your "Could it be that this has absolutely nothing to do with the
action, and everything to do with the person doing the action?"  The
answer is quite simply "Of course not."  I would also call out other
people if they stated that misleading claims of theirs were
intentionally submitted and did not try to correct them.  I just
haven't ever seen anyone else do that.  (Let alone follow it up again
a short time later quite similarly[16], except that at least you
corrected it.)

Unfortunately, these aren't isolated incidents of claims of
unfairness, and it's not just me you hurl these accusations against.
In fact, in other cases with other people you go much further
overboard.  For example, you recently used the phrase "petty personal
animus"[17] with Junio.  Perhaps you don't see the problem, so let me
state it in a way you might recognize:
     Could it be that you always use these perjorative phrases in
questions just to provide plausible deniability that you are directly
accusing folks of severe personal failings?
Just as I shouldn't ask the above question (I only did so to try to
highlight how your questions come across), you shouldn't be asking
yours.  It's inappropriate.  Putting such negative words in a question
format might make the phrases less objectionable, but the questions
are still way out of line.  If you react negatively to my example
sentence above, hopefully that helps you see why that is
inappropriate.  Please correct this pattern.

And all of this hardly even touches on your interactions with
reviewers, which sadly is following the same arc as in 2013 and
2014[18].

In short, please stop:
  * Please stop attacking random bystanders like Alex (or other folks[19])
  * Please stop defending shockingly inadequate levels of due
diligence, and adopt a higher one.
  * Please stop accusing others of bias, unfairness, and other
shortcomings.  Learn to recognize why your behavior often results in
others changing theirs.
  * And please find ways to stop burning out reviewers, especially
since they are the rate-limiting resource in determining git's overall
velocity


Please know that I really do think you are talented.  I tried really
hard to help you change and improve because I thought you could have
been a great addition to the community.  I'm nowhere near as talented
as Michael in expressing it (again, [18]), but I really wish you had
chosen to change.  I agree with his sentiments back then.


[1] https://lore.kernel.org/git/YMYnVWSEgxvKRU9j@coredump.intra.peff.net/
[2] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/
[3] https://lore.kernel.org/git/60c647c1d9b5c_41f452089@natae.notmuch/
[4] https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/
[5] https://lore.kernel.org/git/60c86ff87d598_e6332085b@natae.notmuch/
[6] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/
[7] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/

[8] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/
[9] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/
[10] https://lore.kernel.org/git/CABPp-BEXtJtkkh9Diuo4e1K1ci=vggGxkLRDfkpOH12LM8TCfA@mail.gmail.com/

[11] https://lore.kernel.org/git/60cb5a02f1b31_1259c2086f@natae.notmuch/
[12] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/
[13] https://lore.kernel.org/git/CABPp-BGstXDbzxpySw7q_jn22HD05MsrZeHNv+kXFHOFS2_WCQ@mail.gmail.com/
[14] https://lore.kernel.org/git/60c887f678c88_e63320846@natae.notmuch/
[15] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/
[16] https://lore.kernel.org/git/CABPp-BF1noWhiJadHzjJmnGo8hdZj6Fk7XnZ=u6BVVSGfHE7og@mail.gmail.com/

[17] https://lore.kernel.org/git/60d289c84fadf_312208dc@natae.notmuch/

[18] https://lore.kernel.org/git/53709788.2050201@alum.mit.edu/

[19] "if you eyes have trouble seeing", from
https://lore.kernel.org/git/60839422353fc_10cb9208c7@natae.notmuch/
       https://lore.kernel.org/git/87fszfafkh.fsf@angela.anarc.at/

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Felipe Contreras]

Elijah Newren wrote:
> On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Elijah Newren wrote:
> >
> > > The code changes look good, but you'll need to add several test
> > > changes as well, or this code change will cause test failures.
> >
> > Wouldn't you consider sending a patch without running 'make test'
> > "cavalier"?
> >
> > > Thanks for working on this.
> >
> > Such a completely different tone for a "cavalier" patch depending 100%
> > on the person who sent it. Weird.
> 
> First of all, attacking a random bystander like Alex is rather
> uncalled for.

I did not attack Alex, I simply pointed out the fact that his patch
breaks the test suite, which you did too.

I asked *you* if you didn't consider this "cavalier". Your reaction
seems inconsistent. This question has nothing to do with Alex.

I don't throw personal attacks, nor do I chastise contributors for
attempting to improve the project. That's your department.

I focus on the ball, not the player.

-- 
Felipe Contreras

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Alex Henrie]

On Sat, Jun 26, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote:
>
> Thanks for working on this.

Thanks for all the feedback. I will incorporate your suggestions about
the commit message and documentation into the next revision, whenever
that is.

On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Also, a bunch of tests are broken after this change:
>
>   t4013-diff-various.sh
>   t5521-pull-options.sh
>   t5524-pull-msg.sh
>   t5520-pull.sh
>   t5553-set-upstream.sh
>   t5604-clone-reference.sh
>   t6409-merge-subtree.sh
>   t6402-merge-rename.sh
>   t6417-merge-ours-theirs.sh
>   t7601-merge-pull-config.sh
>   t7603-merge-reduce-heads.sh
>
> If you didn't mean this patch to be applied then perhaps add the RFC
> prefix.

I actually did run `make test` before sending the patch, but when so
many seemingly unrelated tests failed, I foolishly assumed that they
were pre-existing failures. I should have run the tests on master for
comparison, sorry. Or at least put "RFC" in the subject instead of
"PATCH" as you suggest. I sincerely apologize for my lack of due
diligence and I know that I need to do better at self-reviewing
patches before sending them.

> Alex Henrie wrote:
>
> > +           "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"));
>
> Can I?
>
>   git -c pull.rebase=true pull --ff-only
>
> `--ff-only` doesn't seem to be overriding the configuration.

Ahh, so /that's/ what you meant by "3. Fix all the wrong behavior with
--ff, --no-ff, and -ff-only". That does seem like something worth
trying to fix before making the switch.

On Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
>
> > The code changes look good, but you'll need to add several test
> > changes as well, or this code change will cause test failures.
>
> Wouldn't you consider sending a patch without running 'make test'
> "cavalier"?
>
> > Thanks for working on this.
>
> Such a completely different tone for a "cavalier" patch depending 100%
> on the person who sent it. Weird.

I'm trying to make things better, as I'm sure you are as well, and I
know that I make a lot of mistakes and need the help of more
experienced contributors like you. So please be nice, even if others
are mean to you, because regardless of whether these comments were
directed at me, this kind of comment just makes me feel like giving
up.

On Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> I don't throw personal attacks, nor do I chastise contributors for
> attempting to improve the project. That's your department.

"That's your department" is a personal attack. How about we all go
spend some time enjoying the weather, and then get back to working on
Git problems later this week.

-Alex

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie wrote:

> On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Also, a bunch of tests are broken after this change:
> >
> >   t4013-diff-various.sh
> >   t5521-pull-options.sh
> >   t5524-pull-msg.sh
> >   t5520-pull.sh
> >   t5553-set-upstream.sh
> >   t5604-clone-reference.sh
> >   t6409-merge-subtree.sh
> >   t6402-merge-rename.sh
> >   t6417-merge-ours-theirs.sh
> >   t7601-merge-pull-config.sh
> >   t7603-merge-reduce-heads.sh
> >
> > If you didn't mean this patch to be applied then perhaps add the RFC
> > prefix.
> 
> I actually did run `make test` before sending the patch, but when so
> many seemingly unrelated tests failed, I foolishly assumed that they
> were pre-existing failures. I should have run the tests on master for
> comparison, sorry. Or at least put "RFC" in the subject instead of
> "PATCH" as you suggest. I sincerely apologize for my lack of due
> diligence and I know that I need to do better at self-reviewing
> patches before sending them.

I personally don't see any need for apologies, we all make mistakes,
just keep it in mind for the future.

Personally I prefer to run prove instead, because the output is less
verbose, and there's a nice summary at the end:

  prove t[0-9][0-9][0-9][0-9]-*.sh

> > Alex Henrie wrote:
> >
> > > +           "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"));
> >
> > Can I?
> >
> >   git -c pull.rebase=true pull --ff-only
> >
> > `--ff-only` doesn't seem to be overriding the configuration.
> 
> Ahh, so /that's/ what you meant by "3. Fix all the wrong behavior with
> --ff, --no-ff, and -ff-only". That does seem like something worth
> trying to fix before making the switch.

That is just one of the inconsistencies, there's many others.

Consider all these:

	git pull
	git pull --ff-only
	git -c pull.ff=only pull
	git -c pull.ff=only pull --ff-only

Do you see any good reason why the error message should be different for
any of these?

How about these?

	git pull
	git pull --ff

Why does --ff make the second command work?

Even worse when you start considering more combinations, like
"--no-ff --rebase" which is obviously nonsense, but it depends on the
configuration.

I proposed a solution for some of these inconsistencies with --ff*, but
was ignored [1].

> On Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Elijah Newren wrote:
> >
> > > The code changes look good, but you'll need to add several test
> > > changes as well, or this code change will cause test failures.
> >
> > Wouldn't you consider sending a patch without running 'make test'
> > "cavalier"?
> >
> > > Thanks for working on this.
> >
> > Such a completely different tone for a "cavalier" patch depending 100%
> > on the person who sent it. Weird.
> 
> I'm trying to make things better, as I'm sure you are as well, and I
> know that I make a lot of mistakes and need the help of more
> experienced contributors like you. So please be nice, even if others
> are mean to you, because regardless of whether these comments were
> directed at me, this kind of comment just makes me feel like giving
> up.

My mail was directed towards Elijah, not you.

I do appreciate all contributions, especially if they were done pro
bono.

> On Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > I don't throw personal attacks, nor do I chastise contributors for
> > attempting to improve the project. That's your department.
> 
> "That's your department" is a personal attack. How about we all go
> spend some time enjoying the weather, and then get back to working on
> Git problems later this week.

Stating facts about what a person did is not a personal attack, it's
just stating facts. Elijah did attacked me personally, that's a fact [2]:

  If I were in charge, at this point I would drop all of Felipe's
  patches on the floor (not just the ones from these threads), and not
  accept any further ones.  I am not in charge, though, and you have
  more patience than me.  But I will not be reviewing or responding to
  Felipe's patches further.  Please do not trust future submissions from
  him that have an "Acked-by" or "Reviewed-by" from me, including
  resubmissions of past patches.

And he did so after I made a "mistake" (in his view) that was much much
less serious than the one you did.

I am merely pointing out the inconsistency, that's all.

This has absolutely nothing to do with you, again... I appreciate you
used some of your free time to try to improve git pull.

Whatever beef Elijah has with me is an entirely different subject, that
I suggest you leave on the table. I don't think anything constructive
can come from trying to defend what he did.

Let's just focus on the patches.

Cheers.

[1] https://lore.kernel.org/git/5fdd154264baf_130e182082b@natae.notmuch/
[2] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/

-- 
Felipe Contreras

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 27 2021, Felipe Contreras wrote:

> Alex Henrie wrote:
>
>> On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>> >
>> > Also, a bunch of tests are broken after this change:
>> >
>> >   t4013-diff-various.sh
>> >   t5521-pull-options.sh
>> >   t5524-pull-msg.sh
>> >   t5520-pull.sh
>> >   t5553-set-upstream.sh
>> >   t5604-clone-reference.sh
>> >   t6409-merge-subtree.sh
>> >   t6402-merge-rename.sh
>> >   t6417-merge-ours-theirs.sh
>> >   t7601-merge-pull-config.sh
>> >   t7603-merge-reduce-heads.sh
>> >
>> > If you didn't mean this patch to be applied then perhaps add the RFC
>> > prefix.
>> 
>> I actually did run `make test` before sending the patch, but when so
>> many seemingly unrelated tests failed, I foolishly assumed that they
>> were pre-existing failures. I should have run the tests on master for
>> comparison, sorry. Or at least put "RFC" in the subject instead of
>> "PATCH" as you suggest. I sincerely apologize for my lack of due
>> diligence and I know that I need to do better at self-reviewing
>> patches before sending them.
> I personally don't see any need for apologies, we all make mistakes,
> just keep it in mind for the future.

Yes, for someone joining the project it's not obvious what the status of
the tests is. No problem.

Alex: To elaborate, our exists tests should pass, and should pass on
every commit (both as a matter of fact and future coding practice). We
also have CI, so if you e.g. have a fork of git/git and push to your
fork you'll find that CI is run for you on several platforms.

See below for a one-liner to possibly speed up the testing for you.

> Personally I prefer to run prove instead, because the output is less
> verbose, and there's a nice summary at the end:
>
>   prove t[0-9][0-9][0-9][0-9]-*.sh

I also like "prove" better (well, I added the support for it, so
...). It's generally better to use e.g.:

    make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="--jobs $(nproc)"

Since we do some basic checking via the Makefile that effectively form a
part of our tests.

FWIW for your one-liner it can be just:

    prove t[0-9]*.sh

Alex: You might also find that if you specify --root as the path to a
ramdisk the tests are much faster, e.g. on my Linux boxes I set
--root=/run/user/`id -u`/git.

Re: [PATCH] pull: abort by default if fast-forwarding is impossible

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Jun 27 2021, Felipe Contreras wrote:

> > Personally I prefer to run prove instead, because the output is less
> > verbose, and there's a nice summary at the end:
> >
> >   prove t[0-9][0-9][0-9][0-9]-*.sh
> 
> I also like "prove" better (well, I added the support for it, so
> ...). It's generally better to use e.g.:
> 
>     make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="--jobs $(nproc)"

Or just add "-j$(nproc)" to ~/.proverc so you don't need to specify
GIT_PROVE_OPTS every time.

> Since we do some basic checking via the Makefile that effectively form a
> part of our tests.
> 
> FWIW for your one-liner it can be just:
> 
>     prove t[0-9]*.sh
> 
> Alex: You might also find that if you specify --root as the path to a
> ramdisk the tests are much faster, e.g. on my Linux boxes I set
> --root=/run/user/`id -u`/git.

Or TEST_OUTPUT_DIRECTORY=/tmp/git.

-- 
Felipe Contreras

RE: [PATCH] pull: abort by default if fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie wrote:
> The behavior of warning but merging anyway is difficult to explain to
> users because it sends mixed signals. End the confusion by firmly
> requiring the user to specify whether they want a merge or a rebase.

When were users warned about this change? Generally before making such
big UI changes to core commands there needs to be a deprecation period
before pulling the trigger.

Second, supposing this was a proposal to add such warning, we would need
a configuration so the user can decide to retain the old behavior, or
move to the new one. What is that configuration?

How can a user choose to enable the new behavior in v2.32 (or any other
version)?

Also, a bunch of tests are broken after this change:

  t4013-diff-various.sh
  t5521-pull-options.sh
  t5524-pull-msg.sh
  t5520-pull.sh
  t5553-set-upstream.sh
  t5604-clone-reference.sh
  t6409-merge-subtree.sh
  t6402-merge-rename.sh
  t6417-merge-ours-theirs.sh
  t7601-merge-pull-config.sh
  t7603-merge-reduce-heads.sh

If you didn't mean this patch to be applied then perhaps add the RFC
prefix.

> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -16,13 +16,17 @@ DESCRIPTION
>  -----------
>  
>  Incorporates changes from a remote repository into the current
> -branch.  In its default mode, `git pull` is shorthand for
> -`git fetch` followed by `git merge FETCH_HEAD`.
> +branch.  `git pull` is shorthand for `git fetch` followed by
> +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.

Is it?

If I have a pull.rebase=merges is `git pull` the same as doing `git
rebase FETCH_HEAD`?

>  More precisely, 'git pull' runs 'git fetch' with the given
> -parameters and calls 'git merge' to merge the retrieved branch
> -heads into the current branch.
> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.
> +parameters and then, by default, attempts to fast-forward the
> +current branch to the remote branch head.

OK.

> +If fast-forwarding
> +is not possible because the local and remote branches have
> +diverged, the `--rebase` option causes 'git rebase' to be
> +called to rebase the local branch upon the remote branch,

Does --rebase only work when a fast-forward isn't possible.

> +and
> +the `--no-rebase` option causes 'git merge' to be called to
> +merge the retrieved branch heads into the current branch.

Isn't merge called when a fast-forward is possible?

> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
>  	return ret;
>  }
>  
> -static void show_advice_pull_non_ff(void)
> +static void die_pull_non_ff(void)
>  {
> -	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> -		 "discouraged. You can squelch this message by running one of the following\n"
> -		 "commands sometime before your next pull:\n"
> -		 "\n"
> -		 "  git config pull.rebase false  # merge (the default strategy)\n"
> -		 "  git config pull.rebase true   # rebase\n"
> -		 "  git config pull.ff only       # fast-forward only\n"
> -		 "\n"
> -		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
> -		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -		 "or --ff-only on the command line to override the configured default per\n"
> -		 "invocation.\n"));
> +	die(_("Pulling without specifying how to reconcile divergent branches is not\n"
> +	      "possible. You can squelch this message by running one of the following\n"
> +	      "commands sometime before your next pull:\n"

Is squelching this message really what we are aming for?

> +	      "  git config pull.rebase false  # merge\n"
> +	      "  git config pull.rebase true   # rebase\n"
> +	      "  git config pull.ff only       # fast-forward only\n"

`git config pull.ff only` will get rid of this messge, but the pull
would still fail, only that it will be a different message:

  fatal: Not possible to fast-forward, aborting.

Doesn't seem very user-friendly.

> +	      "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"));

Can I?

  git -c pull.rebase=true pull --ff-only

`--ff-only` doesn't seem to be overriding the configuration.


In addition to all the issues I've already pointed out, the message
seems rather big for a die().

I think this is close to the end-goal we should be aiming for, but
there's a lot that is missing before we should even consider flipping
the switch.

Cheers.

-- 
Felipe Contreras