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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 27, 2021, 21:29 UTC
Message-ID: <60d8edbb3803f_70e20886@natae.notmuch>
In-Reply-To: <CAMMLpeQaY0z_baVQspJhAX0wanaJwneFoM5rG=xwFUmLEE-QaQ@mail.gmail.com>

Alex Henrie wrote:

Show 27 quoted lines

> 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

Show 16 quoted lines

> > 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].

Show 22 quoted lines

> 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.

Show 9 quoted lines

> 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