On Fri, Dec 11, 2020 at 4:02 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > > >> And when we stop in such a manner, it is sensible to give an error > >> message telling them > >> > >> - why we are stopping, > >> > >> - what they can do to move the immediate situation forward > >> (i.e. command line option that lets them choose), and > >> > >> - what they can do to make their choice permanent so that they > >> would never see the command stop when facing a non-ff history > >> (i.e. the configuration variables). > >> > >> Up to this point, I think both of us agree with the above. > > > > I don't agree with the above. > > > > The error I propose is just: > > > > The pull was not fast-forward, please either merge or rebase. > > > > That's it. Nothing more. > > It says "why we are stopping." quite well. It would be a good > message to use as the first part of the three-part message I > mentioned above. The two key parts of the message are: 1. It is an *error* 2. It is *permanent* > > I explained that was the final end goal in my list of steps [1]. I do > > not think any suggestion for commands or configurations belongs in a > > *permanent* error message. > > In the design I have in mind in the message you are responding to, > the users who haven't told their choice to Git would be the only > folks who get all three. What would that error message look like? And do you have any other example of the current user interface where such a condescending long error message is displayed? > You want to let the user express: "I do not want to choose either > rebase or merge. I want 'pull' to fail when it needs to deal with > non-ff history. But I do not need to be told about command line > option and configuration every time." That's right. > I said I don't (I view that disabling half the "git pull" just a > safe fallback behaviour until the user chooses between merge and > rebase), but if we wanted to offer it as a valid choice to users, we > can do so. We just make it possible to squelch the latter two parts > of the three-part message---you leave pull.rebase unconfigured and > squelch the latter two parts of the message, and you got the "stop > me, I do not merge or rebase, but don't even tell me how to further > configure" already. > > I agree the latter two should not be part of *permanent* error > message. And my suggestion did not intend to make them so---it > should have been quite obvious to who read the message you are > responding to through to the end and understood what it said. It doesn't matter (much) if it's temporary or permanent, it's still an *error* message. Currently it's a warning, and people are complaining, even though the pull still works. And you want to make it an error, and *always* fail? Even though the user has not been warned that such a change was coming and how to evade it? I'm against that. I would rather do nothing than intentionally break user experience without warning. Johannes said he didn't mind the warning. I want the warning to remain, just make it a different warning. You want to break his experience without a grace period and suddenly make it an error. > Now, how would we make it possible to squelch the latter two parts? ... This is irrelevant. As long as it's an error I don't care if it's short or long. I'm against turning on an error from one version to the next. > > The reason "pull.mode=ff-only" needs to be introduced is that > > --ff-only doesn't work. Otherwise there's no way the user cannot > > select the "safe default" mode. It has absolutely nothing to do with > > what we present the user with. > > I too initially thought that pull.mode may be needed, but probably I > was wrong. I do think this can be done without pull.mode at all, at > least in two ways, without adding different ways to do the same > thing. > > - When pull.rebase is set to 'no' and pull.ff is set to 'only', > "git pull" that sees a non-ff history should error out safely. > The user is telling that their preference is to merge, but the > difference between merge and rebase does not really matter > because pull.ff=only would mean we forbid merges of non-ff > history anyway. The message you'd get would be "fatal: Not > possible to fast-forward, aborting." though. > > - Or with the advice that hides the latter two points, a user can > unset pull.rebase and set the advice.pullNonFF to false to get > the same behaviour (i.e. disable the more dangerous half of > "pull") with just the "we stopped" error message. So, after your hypothetical patch, there would be no difference between: git -c pull.rebase=no -c pull.ff=only pull and: git -c advice.pullnonff=false pull ? > I think either of these are close enough to what you want, and I > think the latter gives us more flexibility in how we tone down the > message with advice.pullNonFF. You are missing at least two things. I'll wait for your response. Cheers. -- Felipe Contreras
