Re: [PATCH v2 02/14] pull: improve default warning
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 5, 2020, 16:28 UTC
- Message-ID
- <CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com>
- In-Reply-To
- <CAMP44s1=aZL7BMKSjzKJ7qYqg-usScwzRUJmaOhsCGvQ4ieYow@mail.gmail.com>
On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:
Show 12 quoted lines
> > On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote: > > > > Hi Felipe, > > > > On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote: > > > > > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > > > <felipe.contreras@gmail.com> wrote:
[...]
Show 47 quoted lines
> > > > > + "You can squelch this message by running one of the following commands:\n" > > > > > + "\n" > > > > > + " git config pull.rebase false # merge (the default strategy)\n" > > > > > > > > Should this be labelled as the default given the desire to make > > > > --ff-only the default? Perhaps I'm jumping ahead and you plan to > > > > change that later in this series. > > > > > > That's right. > > > > > > In the previous series which does indeed make "pull.mode=ff-only" the > > > default [1], I do change the warning to specify the future default > > > [2], but in that series the warnings is changed to: > > > > > > The pull was not fast-forward, in the future you will have to choose > > > a merge, or a rebase. > > > To squelch this message and maintain the current behavior, use: > > > > > > git config --global pull.mode merge > > > > > > To squelch this message and adopt the new behavior now, use: > > > > > > git config --global push.mode ff-only > > > > > > Falling back to old style for now (merge). > > > Read "git pull --help" for more information. > > > > > > Since that series didn't get any traction, I decided to only implement > > > step 1: fix the current situation. And later on another series would > > > do step 2: introduce "pull.mode=ff-only" and do the preparations to > > > make it the default. > > > > I like this longer plan. > > Yeah. It's a better plan, just more work for me. > > > However, on the shorter scale plan...I think > > the suggestion to use "git pull --no-rebase" makes the current > > situation worse rather than fixing it. > > Well, I already said I partly agree with you: in the --ff-only case > the suggestion should not be brought forward. > > But in the "git pull" default case, *today* it's doing a merge. If > uttering --merge and thus making the current behavior explicit instead > of implicit seems dangerous it's because it is. But not documenting it > doesn't make it any less dangerous.
Sounds like we agree that the future should be ff-only-as-default. I also agree with you that the primary problem is the current default behavior, and I'll agree with you that documenting the current default is okay. However, I disagree that your wording here:
+ "If unsure, run \"git pull --no-rebase\".\n"
does anything of the sort. It does not mention that this is the default behavior the users would get if they provided no flags. More importantly, it makes a recommendation...and one that undercuts the point of the message. It makes it feel like the message shouldn't exist at all in any circumstances. I even suspect that adding that sentence may undercut any efforts towards changing the default to ff-only-as-default. While I'm a big fan of most of what you've done in this series, I will object to its merging for as long as this stays. (I definitely don't have veto power or anything close to it, just stating what my opinion is.)