Re: [PATCH 0/3] Reject non-ff pulls by default

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sep 8, 2013, 08:43 UTC
Message-ID: <CAMP44s3SbaMy7c_5W72J-hMx+xq+4VB5dV7ttCK=TVSbbYcr8A@mail.gmail.com>
In-Reply-To: <20130908075046.GL14019@sigill.intra.peff.net>

On Sun, Sep 8, 2013 at 2:50 AM, Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

> On Sun, Sep 08, 2013 at 02:15:17AM -0500, Felipe Contreras wrote:
>
>> > I think "the guy" may be git itself. For example, here is a possible
>> > session with jc/pull-training-wheel:
>> >
>> >   $ git push
>>
>> Who told him to use 'git push'? Certainly not git.
>
> Any of the hundreds of existing tutorials that teach basic git commands
> like "push"?

You can't use a tutorial out there that just tells you to replace svn commands with git alternatives, go to work and mess up the repository history.

I'm trying to take the point of view of your hypothetical user working on a repository where history is not important, but it seems more and more than this person is just not real. If it's OK to push crappy merges, somebody must have told him that was OK and provided him with the commands.

If it's just some random person that read some random tutorial from 'svn' -> 'git' working on a random repository that happens to accept merges all over the place. Well I think that's a very very exceptional situation.

And this person still wouldn't have a problem finding another tutorial explaining what a merge is.

Show 20 quoted lines

>> >   To ...
>> >    ! [rejected]        master -> master (non-fast-forward)
>> >   error: failed to push some refs to '...'
>> >   hint: Updates were rejected because the tip of your current branch is behind
>> >   hint: its remote counterpart. Integrate the remote changes (e.g.
>> >   hint: 'git pull ...') before pushing again.
>> >   hint: See the 'Note about fast-forwards' in 'git push --help' for details.
>> [...]
>>
>> Why stop there? Post the whole man page already.
>>
>> Moreover, it's overly verbose on all the wrong and irrelevant
>> information. If you are going to waste precious screen state, explain
>> wth a "non fast-forward" is; people can figure out what a merge is,
>> and maybe a rebase, but a "non fast-forward" definitely not.
>
> Note that I was not trying to defend any of the messages, but only
> showing a plausible mechanism by which a user with basic knowledge that
> he wants to push may arrive at the question "what is the difference
> between merge and rebase?".

Yes, and this person would have to read it online, like everything else, because clearly Git documentation would do a bad job at it. That's why the online documentation was needed in the first place.

The first hits of 'git merge vs rebase' are rather useful: http://mislav.uniqpath.com/2013/02/merge-vs-rebase/ http://stackoverflow.com/questions/16336014/git-merge-vs-rebase http://www.derekgourlay.com/archives/428 http://blog.sourcetreeapp.com/2012/08/21/merge-or-rebase/ http://git-scm.com/book/en/Git-Branching-Rebasing

Notice how none of the results point to official documentation, precisely because it's not really useful.

> If you want to suggest revisions for the push message, go ahead. The
> push advice _is_ an attempt to define non-fast-forwards in plain
> language without taking up too much space, but perhaps you can do
> better.

I definitely can, but you would disagree.

But anyway, you are relying on the user having pushed first, what if he is pulling first, or what if he doesn't have write access and is only pulling?

> You could even suggest omitting it entirely, but I'm not sure if
> that is a good idea. It was not added in a vacuum; we lacked that advice
> for many years, and people complained about it quite a bit until it was
> added.

I would have to see the evidence, as I have never seen any complaints about that. The complains are about the UI, and they still remain.

Show 12 quoted lines

>> > The user is pointed at "pull" from "push", and then gets presented with
>> > the "merge or rebase" choice. It may be that the advice you can find by
>> > googling "merge vs rebase" is enough to then help the person along
>> > (and/or we may need to improve the manpages in that respect).
>>
>> Yes, but that's not the use-case we are talking about. You mentioned
>> specifically a "svn-like" worfklow where the guy was told by somebody
>> else to replace the svn commands with git ones.
>
> No, I mentioned an "svn-like" workflow. I didn't say anything about how
> they were told. They might have been told by a co-worker, or read a
> brief tutorial on git, or read something like "Git-SVN Crash Course".

Once again, this doesn't make any sense. People can't just push crap merges to any repository.

>> If we are talking about a guy that is learning git, that's and
>> entirely different case.
>
> That is certainly what I meant to be talking about.

If he is learning Git, then he will be looking for the meaning of a merge and a rebase. The fact that the repository accepts crappy merges wouldn't be relevant.

Show 6 quoted lines

>> The purpose of this change in the code is not to change the user
>> behavior. The choice of merge vs. rebase is entirely up to the user,
>> and we are not changing that.
>
> Right, but by not doing anything by default, you are forcing the user to
> make a decision.

No, it would be a warning first, he wouldn't be *forced* to make a decision, only after the deprecation period is over.

Then yes, if by then he hasn't learned that what he wants is a merge, he would be forced to learn it.

> Right now, we strongly encourage merging by making it
> the default, and you have to learn about rebasing separately. But a
> message that mentions them both as equals is going to lead to extra work
> for the user; they have to figure out which one is most appropriate.

No, they don't need to figure out which is most appropriate, they only need to figure out they have been doing merges all along.

My warning message achieves precisely that:

"The pull was not fast-forward, in the future you would have to choose a merge or a rebase, merging automatically for now. For more information read 'git pull --help'."

The part "merging automatically for now". This teaches the user that 'git pull' is doing a merge, so by the time 'git pull' errors out, he knows he wants a merge, all he needs to figure out is how to do it, and 'git pull --help' would tell him that. Perhaps adding a "(git pull --merge)" to the deprecation warning would help, but I still don't see the need in the final error.

Once again, nobody is forcing anybody to change their workflows.

> My
> concern is that this is non-trivial for new users, and that they may end
> up arbitrarily picking rebase, which is probably not doing them any
> favors if they do not understand it.

Why would they pick a rebase? If git tells them 'git pull' is doing a merge for months, why would they choose to do something different?

> For clueful users, choosing between the two is not hard. But some people
> seem to have trouble understanding the DAG. I don't know how large a
> group that is, and how any pain caused by this change might compare to
> the times it will help.

They don't need to learn what's more appropriate, they can keep doing what they have been doing.

Show 24 quoted lines

>> > If you are interested, I can ask the opinion of some of the GitHub
>> > trainers. They see a lot of new users and have a sense of what kinds of
>> > confusion come up most frequently, what kinds of workflows they tend to
>> > see, etc. Their experience may be biased towards corporate-ish users,
>> > though, because those are the people who pay for training.
>>
>> Ask. I'm sure they will tell you doing merges by mistake with 'git
>> pull' is an issue.
>
> I've sent an email. I'll post the response when I get it.
>
>> >> "Any more babysitting with essay long messages is counter-productive
>> >> to the vast majority of Git users."
>> >
>> > I think that is what we have advice.* for.
>>
>> I don't understand what that means.
>
> It means that some time ago, after many people complained that git did
> not give enough hints, we added many hints. Some people who did not need
> these hints would want to disable them, and we have the "advice.*"
> config options to do so. So we can have a longer message for new users,
> and a shorter one for people who do not want to be bothered with the
> long advice.

I don't see Junio's proposal being affected by this advice thing.

And I have used and contributed to Git for many years, used it since day one, and this is the first time I hear about it. I doubt even a tiny fraction of Git users know about it. Where is the documentation about that?

-- 
Felipe Contreras