Go: doc: clearer plain git contributing guidelines

[leighmcculloch]

What:

Change the contributing guidelines document to make the recommended process more familiar and obvious to users of plain git.

What it is not:

This issue is not a request or proposal to replace Gerrit or the review process. This issue is not a request or proposal to remove the git-codereview tool or to make drastic changes to how anyone on the Go team or existing contributors go about contributing.

Why:

There are other open issues proposing a switch to GitHub (#21956), or adding the ability to support pushing via GitHub (#18517). Based on my own experience I think much of the friction that GitHub users are experiencing is caused by the contributing guidelines centering around git-codereview, which abstracts git away from the developer. Abstracting git away from developers is jarring when developers know how to use that tool already. It gives them the experience that they are using something that is entirely different to what they use today, even though it is not.

Context:

I recently submitted my first contribution to Go and was surprised at how straight forward and refreshing Gerrit was in comparison to GitHub. I had delayed contributing because the process described at https://golang.org/doc/contribute.html appeared so different to what I was use to. In practice it wasn't. It only appeared that way because there was a layer of abstraction.

Before contributing the main point of friction for me wasn't the use of Gerrit vs GitHub, but the fact that all of the examples centered around the use of git-codereview, a custom tool for interacting with the Go Gerrit instance. I realize this tool is optional, but it took time to read all of the text and note down which git commands each git-codereview command actually used.

At the end it came down to:

1. Install two hooks
2. git clone
3. git pull -r
4. git checkout -b <branch>
5. git commit
6. git push -u origin HEAD:refs/for/master (patch set 1)
7. git commit --amend
8. git push -u origin HEAD:refs/for/master (patch set 2)

The above is almost identical to the workflows I use at work and on open source on GitHub, and it is instantly familiar to me as a git user.

The contribution guidelines tell a different story:

1. git codereview hooks
2. git clone
3. git sync
4. git change
5. git mail (patch set 1)
6. git change (to amend)
7. git mail (patch set 2)

While the above is technically less commands, it's four new commands that I had never seen before and had to learn about. These commands abstract git away from plain git users, unnecessarily.

How:

Some ideas for how the contributing guidelines document could be made more familiar to plain git users:

• Replace examples using git-codereview with plain git commands where the corresponding git commands are simple and straight forward. e.g. git sync vs git pull -r, git push vs git mail, etc.
• Display plain git commands side-by-side (not one after the other) in the same size, font, weight, etc.
• Make git-codereview the recommended path for those not familiar with git, or power users.
• Make plain git the recommended path for those familiar with git, with a suggestion to check out git-codereview.
• Offer the pre-commit hook at a URL that can be curl'd like how the commit-msg hook is available so that it's simple for developers to install the commit hooks how they would any others for other projects.

I've got a branch with changes to the contributing doc that I made to experiment with these ideas and to confirm if focusing on plain git commands would make it appear more familiar and approachable. I'd love to be involved with improving the guidelines in this way if this is something that the community agrees with.

[cznic]

While the above is technically less commands, it's four new commands that I had never seen before and had to learn about.

Programmers are expected to constantly learn new things.

These commands abstract git away from plain git users, unnecessarily.

Unnecessarily??? Of course not.

[ianlancetaylor]

CC @spf13

[huguesb]

Before doing any changes to the contributing guidelines I think it might be informative to poll existing contributors to see what proportions of them use which codereview commands.

FWIW, I personally only use the mail subcommand because it is shorter to type, and use plain git for everything else.

The contributing guidelines does mention the plain git equivalent of most codereview subcommands but the layout/formatting could use some improvement. Consider for instance:

Once you have the changes queued up, you will want to commit them. In the Go contribution workflow this is done with a git change command, which creates a local branch and commits the changes directly to that local branch.

$ git change <branch>

The name <branch> is an arbitrary one you choose to identify the local branch containing your changes and will not be used elsewhere. This is an offline operation and nothing will be sent to the server yet.

(In Git terms, git change <branch> runs git checkout -b branch, then git branch --set-upstream-to origin/master, then git commit.)

Instead of assuming the use of codereview, it would arguably be better to first describe the operation using plain git concepts, and then mention the change subcommand, along with the plain git equivalent e.g:

Once you have the changes queued up, you will want to commit them. In the Go contribution workflow this is done by committing your changes to a local branch. This is an offline operation and nothing will be sent to the server yet.

For convenience, the codereview tool provides a change subcommand that performs the necessary git operations.

$ git change <branch>                  $ git checkout -b <branch>
                                       $ git --set-upstream-to origin/master
                                       $ git commit

The name <branch> is an arbitrary one you choose to identify the local branch containing your changes and will not be used elsewhere.

[mdempsky]

I think I'm outside the new contributor guide's target audience, but I thought I'd just voice that I've never learned to use git-codereview aside from the commit hooks. I was already comfortable using Git and Gerrit from previous projects before Go switched to using them, so I've just always used plain Git/Gerrit.

Perhaps I'm projecting, but I would expect 1) more potential contributors are already familiar with Git/Gerrit than git-codereview, 2) contributors would be more interested in learning tools that are applicable outside of just the Go project (to be fair, I have no idea how reusable git-codereview is for contributing to other Git projects), and 3) there's more documentation on how to use Git/Gerrit than git-codereview (e.g., StackOverflow).

[davecheney]

I use git codereview exclusively. I consider the gerrit dance to be gerrits
problem and I'm grateful that Russ wrote a tool to hide it.

On Tue, Oct 31, 2017 at 3:30 PM, Matthew Dempsky [e-mail]
wrote:

I think I'm outside the new contributor guide's target audience, but I
thought I'd just voice that I've never learned to use git-codereview aside
from the commit hooks. I was already comfortable using Git and Gerrit from
previous projects, so I've just always done that.

So perhaps I'm projecting, but I would expect 1) more potential
contributors are already familiar with Git/Gerrit than git-codereview, 2)
contributors would be more interested in learning tools that are applicable
outside of just the Go project (to be fair, I have no idea how reusable
git-codereview is for contributing to other Git projects), and 3) there's
more documentation on how to use Git/Gerrit than git-codereview (e.g.,
StackOverflow).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/22484#issuecomment-340656949, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA1zZ3xodMiTQsgtI12dlBtgyuwSaks5sxqJxgaJpZM4QKM9n
.

[spf13]

@mdempsky you might be the only person who could say you were more familiar with Git and Gerrit from previous projects. The vast majority of people coming to the Go project haven't heard of Gerrit before, much less used it.

@leighmcculloch
I've spent a lot of time with our first time contributors and I'd say most don't have the same level of familiarity using Git as you do. Many memorize the handful of commands they need to use and never go past that. Many of those git commands would be unfamiliar, especially installing the hooks. This is the first step and would cause significant friction.

I would support having the instructions still preferring git codereview but expanding the documentation to include the git alternative.
I think @huguesb suggestion is perfect and we should pursue that approach.

If someone wants to prepare that CL and assign it to me as a reviewer that would be awesome.

[leighmcculloch]

I'm up for doing it. The draft change I had made was essentially the same example @huguesb included, with codereview and plain git side-by-side. I'll clean it up and push it.

[huguesb]

@spf13 I think it's rather unlikely that @mdempsky would be the only person familiar with Git and Gerrit from previous projects. I had been using Gerrit at work for a few years before I contributed to Go. Likewise, a significant portion of Go contributors are (ex-)Googlers who have been exposed to Mondrian. That's not to say we shouldn't be trying to make it easier to onboard unfamiliar people, just that we also have to be careful to avoid confusing people already familiar with Git/Gerrit, and fwiw, I found the codereview commands to be more confusing than helpful.

@davecheney I find this really surprising as I've personally used git in a number of contexts where gerrit was not involved and the only significant differences in workflow was using git push origin <branch> as opposed to git push -u origin HEAD:refs/for/master which is arguably a rather minor variation. Out of curiosity, were you familiar with git before contributing to Go? If so, what was your typical workflow like and what are your pain points in the Gerrit workflow?

[davecheney]

On 1 Nov 2017, at 02:32, Hugues [e-mail] wrote:

@spf13 I think it's rather unlikely that @mdempsky would be the only person familiar with Git and Gerrit from previous projects. I had been using Gerrit at work for a few years before I contributed to Go. Likewise, a significant portion of Go contributors are (ex-)Googlers who have been exposed to Mondrian. That's not to say we shouldn't be trying to make it easier to onboard unfamiliar people, just that we also have to be careful to avoid confusing people already familiar with Git/Gerrit, and fwiw, I found the codereview commands to be more confusing than helpful.

@davecheney I find this really surprising as I've personally used git in a number of contexts where gerrit was not involved and the only significant differences in workflow was using git push origin as opposed to git push -u origin HEAD:refs/for/master which is arguably a rather minor variation. Out of curiosity, were you familiar with git before contributing to Go? If so, what was your typical workflow like and what are your pain points in the Gerrit workflow?

I don’t have any pain points, I’ve always used git codereview and I’m grateful that I can just ignore whatever Gerrit does behind the scenes. All I have to remember is to use git mail rather than git push to send a cl. Easy.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[mdempsky]

@spf13

@mdempsky you might be the only person who could say you were more familiar with Git and Gerrit from previous projects.

To be clear, I wasn't claiming that a lot of people are already familiar with Git and Gerrit (though I do think a lot of people are familiar with Git, at least based on how frequently we're asked to switch to GitHub pull requests). I was just saying that more people are likely to be familiar with Git and Gerrit (which are used by some other large projects) than with git-codereview (which seems to be an exclusively Go tool).

[leighmcculloch]

I've got two versions implemented and I can submit a CL for either. In both versions the plain git commands are presented the same as the git-codereview commands rather than being buried in the paragraph text.

Side by side:

One after the other:

I think side by side looks good on a desktop browser, but does require a small amount of new CSS in the godoc tool which will be one more thing that needs to be maintained and may be a little harder to read on screens that are smaller. To resolve the small screen size issue I can make it responsive and one-after-the-other on smaller screens.

I think the one-after-the-other screenshot above is sufficient and lower maintenance.

@spf13 Thoughts?

[gopherbot]

Change https://golang.org/cl/76318 mentions this issue: doc: move plain git examples in contribution guide

[leighmcculloch]

@spf13 CL 76318 ☝️ keep the plain git examples listed after the git-codereview examples, but changes their format so they are in code blocks and less hidden. I think it sufficiently improves the guide to make it more familiar for those familiar with git, without adding maintenance burden to this page which I think would happen if we introduced the side-by-side examples.

I pushed two smaller CLs ahead of this one that contain small changes that also help structure the introduction of using plain git.

[spf13]

@leighmcculloch apologies, I'm seeing these comments after the CLs.

I think the approach side by side with responsive after is best and the extra maintenance of a bit more CSS is ok.

[leighmcculloch]

No worries. I have a branch prepared for side by side, I'll add the responsive behavior to fallback to one after the other and submit them shortly.

Do I need to do anything to ensure the godoc style.css change is merged first and available in the same release?

[jimmyfrasche]

For the side-by-side version, it's not clear at a glance that both are equivalent. Perhaps adding labels like "with codrereview aliases" (ideally linked to that section) and "plain git"?

For accessibility reasons those should be there regardless, even if hidden from visual user agents. Otherwise, a screen reader would just read them in order, making it sound like you have to run all four commands in a row. The other version didn't need this because of the explicitly included "or" between the panes.

[leighmcculloch]

@spf13 Screenshots of the side-by-side falling back to one after the other:

Greater than 500px:

Less than 500px:

If this is 👍 I'll look at adding labels per @jimmyfrasche's suggestion, assuming you agree @spf13.

[spf13]

Yes. I think the labels are critical to make it clear. I think we are
heading in the right direction.

On Wed, Nov 8, 2017 at 2:31 AM Leigh McCulloch [e-mail]
wrote:

@spf13 https://github.com/spf13 Screenshots of the side-by-side falling
back to one after the other:

Greater than 500px:
[image: screen shot 2017-11-07 at 11 25 59 pm]
https://user-images.githubusercontent.com/351529/32536399-382619e0-c413-11e7-93b1-d2975c4d09e4.png

Less than 500px:
[image: screen shot 2017-11-07 at 11 26 12 pm]
https://user-images.githubusercontent.com/351529/32536406-3d17793a-c413-11e7-8af6-a73e915bc380.png

If this is 👍 I'll look at adding labels per @jimmyfrasche
https://github.com/jimmyfrasche's suggestion, assuming you agree @spf13
https://github.com/spf13.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/22484#issuecomment-342733579, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAKlZNQbUD5JRaz5H1DK2K_v23Q3A9x3ks5s0VizgaJpZM4QKM9n
.

[leighmcculloch]

@spf13 Screenshots with labels at the three different responsive layouts that godoc renders in:

Greater than 700px:

Greater than 500px:

Less than 500px:

If this looks good from a presentation point-of-view I'll push CLs to golang/tools and golang/go with the css and html changes for code review.

[jimmyfrasche]

@leighmcculloch can't speak for @spf13 but that ticks my boxes and any specifics can be worked out in the CL.

[gopherbot]

Change https://golang.org/cl/76874 mentions this issue: godoc/static: add css for codereview and plain git side-by-side examples

[leighmcculloch]

I've pushed CLs for this now:

• https://go-review.googlesource.com/c/go/+/76318 – contribute.html
• https://go-review.googlesource.com/c/tools/+/76874 – style.css

[leighmcculloch]

@spf13 I addressed your concerns back in November in the two CL's above.

[rasky]

I refactored big chunks of the contribute guide for clarity, adding also very short overviews of the processes for people just skimming it. For reference, this is what it looks like today:
https://tip.golang.org/doc/contribute.html

I don't think that we should go with this proposal of showing expanded git commands. I'll explain my reasoning:

• If you look at the section "Sending a change via Gerrit", it now provides a very fast overview of the involved process. It only contains two new commands: change and mail. They're explained with simple examples, in the contexts of other standard git commands, so I think they're clear to use and learn. Compared to when this issue was opened, they are 2 instead of 4, and they're more clearly explained in a general, quick overview.
• I disagree that telling people to use git push -u origin HEAD:refs/for/master will make things easier to understand and follow. The syntax HEAD:refs/for/master is not very common among average git users. If people have to copy & paste it, they can just copy & paste the codereview command which is shorter and more clear.
• There's now a Github PR-based process in place. People that don't want to even bother using codereview once can simply open a GitHub PR. It's even suggested now for first time contributors, to lessen the barrier.
• Even If we want to provide a more complete explanation of how codereview works under the hood, I don't think it should go in the standard flow of reading. The contribution guide is not a reference: it reads as a tutorial, and explaining how codereview works under the hood is not something that should be done in the tutorial, in the same context where people are just trying to send a change. If anything, it should go in the new section in the new "Advanced topics" chapter.

All in all, I think this belongs more to the git codereview reference documentation, rather than the contribution guide. Another alternative might be to put it into the wiki, which is more comprehensive and reads more as a reference and less as a tutorial.

[leighmcculloch]

Closing this issue since @rasky pointed out that the direction of contribute.html has changed. Also the number of Gerrit specific commands in the examples has been reduced with the rewrite :claps:. I agree now that GitHub PRs are supported this change is less impactful. Not just for new contributors but for those who prefer plain git, abandoning Gerrit to use a plain git interface via GitHub is much simpler.