It's the worst of two world if both reviewers and newcomers are upset during the process.
This is a two-side misunderstanding.
If we can at least partially fix this by some kind of rules or hints in the documentation (https://golang.org/doc/contribute.html), it could in theory help to both sides. Reviewer could reference the (potentially violated) rule, the contributor could choose what not to send based on those rules so the problem never arises.

I think it's good if we write down some of the common knowledge that some (most?) of the Go reviewers apply for these CLs. For example:

• Many small, trivial CLs should generally be merged into one
• It's fine for a single CL to cover many unrelated packages if the changes are very similar
• The advantage of a change should outweigh the git noise (e.g. losing git blame, adding to git log)

I'm not sure if doc/contribute.html is a good place for this - we don't want that page to get too big, as we send new contributors there. Perhaps we could add this to the wiki somewhere.

Also /cc @rasky @andybons @dmitshur, for their involvement on the contribution guide and new contributors in general.

The problem is that there are some (unwritten?) rules that make such small cleanup-like changes less likely to be merged. My proposal is to describe these rules and hopefully with some rationale behind it. It would be fair and honest to everyone.

If you're unsure of whether to pursue a cleanup change like the one you described and attempted, then emailing golang-dev@ will give you an answer before you send out a single CL.

I agree with @mvdan to start with the three points he described on the wiki somewhere.

We appreciate any attempt at contribution, but golang.org/cl/141957 is just churn. Blindly standardizing on something so minuscule that doesn't actively help or hurt readability of the code shouldn't be pursued. The costs (code reviews, lost git blame, etc.) far outweigh the benefits. There are differing scenarios where one would be justified in using one form over another.

The following snippet is from the image/gif package:

dst[3*i+0] = r
dst[3*i+1] = g
dst[3*i+2] = b

We made a stylistic decision to have the first line align with the others, when we all knew it could be simplified as dst[3*i] = r. It doesn't matter, though, the compiler outputs the same result (and if it didn't, then we fix the compiler). What would be the benefit to a CL changing it now?

Regarding golint, there are many places within the Go source that don't abide by its standards. That's OK. The documentation for golint is very clear that the tool is meant as a guide and to not treat the output as a gold standard.

So to reiterate, if you're unsure, just ask! We're here to answer any questions and provide justification for our reasoning.

I think it is impossible to have accurate documentation of what kind of CL is acceptable and what it is not.

If anything, it’s probably easier to just point to “help wanted” tag on GitHub as good first issues (some projects do also have a “good first issue” as well).

I think it is impossible to have accurate documentation of what kind of CL is acceptable and what it is not.

I'm fine with no changes to the document. Just wanted to raise my concerns.

Personally, I think a notice about "chorn" CLs and a definition of "chorn" is a good starting point.

I believe that s[:] => s changes also fall into the same category?
There are also changes like !(x == y) => x != y. It also doesn't fix anything and doesn't bring new features or performance improvements. Hard to draw a line here. Personally, I think all of these do make code a little bit simpler and it's a bad practice to make unrelated changes when you work on something complicated (read: important), so spare hands can be useful here. They're also trivial to review, at least from my point of view. Since these changes don't require any context at all, they look like a perfect candidates for a first (or second) CL. This was my initial way of thinking. It's apparent now that it's not a popular one here.

There are also typo fixes. They're also like a chorn in some sense, since the reader still can get an idea of the intention. Just like x = x + 1 can be interpreted as x++.

The problem with s[:] (where s is a slice or a string) in particular is that some people, while reading Go code, think that it has any effect. It doesn't and is optimized away by the compiler.

So to reiterate, if you're unsure, just ask! We're here to answer any questions and provide justification for our reasoning.

This doesn't scale well. And even if I would ask, we can't be sure about someone who never really been involved into anything around Go. It would be exclusive-oriented tactic, not inclusive one.

There are subjective elements to this. I don't think we can write down a rigorous classification.

I would say that changes that make the code clearer are always OK. Typically replacing s[:] with s will make the code clearer. Whether replacing x = x + 1 with x++ makes the code clearer really depends on the code. Generally it is not clearer. Both forms are equally clear.

If a change does not make the code clearer, it should bring some other benefit. Simply changing the code from one form to another without increasing clarity is churn. Churn is bad. It can not help improve the code. It can only keep it the same or introduce a bug. It increases git history and makes git blame less useful. Churn should be avoided.

I'd say it is the same as in many commercial environments: we should not make style only changes. Style has no value in itself, it is merely aesthetic. We should fix bugs or implement new features, then we can also improve the style, while we are at it.

@beoran, you're right, of course, there is no double about concentrating on these things.

But we're talking about contributors guide (which mostly targets new contributors) and it doesn't describe these values and this is a thing I'm trying to draw here. Implicit rules are not obvious (at least not for everyone). There are also different definitions of "contributors-friendly project".

I do like to get involved into any project by sending one or two very minimal, mostly stylistic or typographical (most likely harmless things, so I don't break a build with my first PR) and only then try to grasp something complicated. It's a good way for me to filter out projects with maintainers that have very opinionated attitude, or projects that ignore new contributions (PRs stay open for years), for example. There are other people that follow this practice as well.

And to give some context if it's missing: these problems arise when someone tries to organize Go contributors workshop like event. This is why I did create this issue, since I misunderstood almost everything about it. Even if I made my own conclusions, I would be glad if others can avoid some pitfalls. For example, we can state that sending a CL that merely applies some style linter suggestions is not (generally) a good idea and it can be rejected.

I see what you say, but in general, for all open source projects, I would advise against making such a "testing the waters" contribution. It may be ignored because it might be judged as not having any real value to the project. I would try to find a small bug or feature you are willing to work on and contribute that instead.

As for making it more clear for new contributors, it may be a good idea to mention what a good first contribution could be, and what not.

Note that my proposed addition to the wiki is not a strict set of rules or a way to classify what a "good CL" is. The idea is to give some general advice and guidelines on how to write CLs so that they are more likely to be useful and merged. Different projects use git and code review in different ways, so it's understandable if they're surprised by the Go team's standards.

In this specific case, I think that the three items I described above would have avoided the confusion that Iskander and his workshop attendees had. The new contributors would have either sent better CLs (in the eyes of maintainers), or looked for other changes to contribute.

We already have https://github.com/golang/go/wiki/CodeReviewComments, so perhaps we could add CodeSubmissionTips or GerritGuidelines.

Sure, a new page CodeSubmissionTips sounds like a good idea.