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

From: Alex Henrie <alexhenrie24@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: Elijah Newren <newren@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] pull: abort by default if fast-forwarding is impossible
Date: Sun, 27 Jun 2021 13:54:16 -0600	[thread overview]
Message-ID: <CAMMLpeQaY0z_baVQspJhAX0wanaJwneFoM5rG=xwFUmLEE-QaQ@mail.gmail.com> (raw)
In-Reply-To: <60d8ab7fe0761_ba5a2208b@natae.notmuch>

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