Re: [RFC] Contributing to Git (on Windows)

[Derrick Stolee <stolee@gmail.com>]

I really appreciate the feedback on this document, Jonathan.

On 3/3/2018 1:27 PM, Jonathan Nieder wrote:
> Hi Dscho,
>
> Johannes Schindelin wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>>> Dereck Stolee wrote:

nit: s/Dereck/Derrick/ Is my outgoing email name misspelled, or do you 
have a misspelled contact info for me?

>>>>> +Test Your Changes on Linux
>>>>> +--------------------------
>>>>> +
>>>>> +It can be important to work directly on the [core Git
>>>>> codebase](https://github.com/git/git), +such as a recent commit into
>>>>> the `master` or `next` branch that has not been incorporated +into
>>>>> Git for Windows. Also, it can help to run functional and performance
>>>>> tests on your +code in Linux before submitting patches to the
>>>>> Linux-focused mailing list.
>>>> I'm surprised at this advice.  Does it actually come up?
>> Yes.
>>
>> I personally set up the automated builds on Windows, Linux and macOS for
>> our team, and as a rule we always open an internal Pull Request on our
>> topic branches as we develop them, and you would probably not believe the
>> number of issues we caught before sending the patches to this list. Issues
>> including
> [nice list snipped]
>
> Thanks for explaining.  I still am going to push back on the wording
> here, and here is why:
>
>   1. Approximately 1/2 of the differences you describe apply to Mac as
>      well as Windows.  The advice certainly does not apply on Mac.
>
>      You might object: Mac readers would not be reading this text!  But
>      that is not how humans work: if project documentation (e.g. the
>      CONTRIBUTING.md on GitHub!) says that the project is Linux-focused
>      and if you don't test on Linux then you might as well not bother,
>      then people are going to believe it.
>
>   2. It is not unusual for Linux users to make portability mistakes that
>      are quickly pointed out on list.  If anything, the advice to test on
>      other platforms should apply to contributors on Linux even more.
>      This happens especially often to new contributors, who sometimes use
>      bashisms, etc that get quickly pointed out.
>
>   3. I do not *want* Git to be a Linux-focused project; I want the code
>      to perform well on all popular platforms and for people not to be
>      afraid to make it so.
>
>      If the docs say that all we care about is Linux, then people are
>      likely to be too scared to do the necessary and valuable work of
>      making it work well on Mac, Windows, etc.  The actual mix of
>      contributors doesn't bear it out anyway: a large number of
>      contributors are already on Mac or Windows.
>
> Fortunately this is pretty straightforward to fix in the doc: it could
> say something like "to the multi-platform focused mailing list", for
> example.

I like the "multi-platform" wording, and I'll try to use it as often as 
possible. I'll keep the Linux instructions because it is free to set up 
a Linux environment and testing in Windows and Linux will catch most of 
the cross-platform issues.

>
> [...]
>> To my chagrin, this idea of making most of the boring stuff (and I include
>> formatting in that category, but I am probably special in that respect) as
>> automatable, and as little of an issue for review as possible, leaving
>> most brain cycles to work on the correctness of the patches instead, did
>> not really receive a lot of traction on this mailing list.
> Huh?  I'm confident that this is a pretty widely supported idea within
> the Git project.
>
> I get the feeling you must have misread something or misinterpreted a
> different response.
>
> [...]
>> No, this advice comes straight from my personal observation that the
>> reviewers on the Git mailing list are Linux-centric.
> Hopefully the clarifications and suggestions higher in this message
> help.  If they don't, then I'm nervous about our ability to understand
> each other.
>
> [...]
>> Now, how reasonable do I think it is to ask those contributors to purchase
>> an Apple machine to test their patches on macOS (you cannot just download
>> an .iso nor would it be legal to run a macOS VM on anything but Apple
>> hardware)? You probably guessed my answer: not at all.
> Agreed, this is something that needs to be automated (and not via a
> CONTRIBUTING.md file).  As a stopgap, having a section in the
> contribution instructions about testing using Windows's Linux
> subsystem is a valuable thing, and thanks for that; I never meant to
> imply otherwise.
>
> [...]
>> On Fri, 2 Mar 2018, Junio C Hamano wrote:
>>> In fact, portability issues in a patch originally written for a platform
>>> is rather quickly discovered if the original platform is more minor than
>>> the others, so while it is good advice to test your ware before you make
>>> it public, singling out the portability issues may not add much value.
>>> The fewer number of obvious bugs remaining, the fewer number of
>>> iterations it would take for a series to get in a good shape.
> [...]
>> For you, Junio, however, the task *now* is to put yourself into the shoes
>> of a Computer Science student in their 2nd year who wants to become an
>> Open Source contributor and is a little afraid to talk directly to "the
>> core committers", and quite scared what negative feedback they might
>> receive.
>>
>> "What if they say my code is not good enough?"
> Sure, though there is something implied in what is Junio is saying
> that is useful for such people.
>
> It is patience.  It is the message that if you miss a portability bug,
> we won't be disappointed in you, and it in fact happens all the time
> to the best of contributors.
>
> If there's a straightforward way to convey that in the text, I agree
> with Junio that it's worth conveying.
>
> [...]
>>>>> +When preparing your patch, it is important to put yourself in the
>>>>> shoes of the maintainer.
>>>> ... and in the shoes of other users and developers working with Git down
>>>> the line who will interact with the patch later!
>>>>
>>>> Actually, the maintainer is one of the least important audiences for a
>>>> commit message.  But may 'the maintainer' is a stand-in for 'the
>>>> project' here?
> [...]
>> Is this not missing the point of this sentence? Those "senior reviewers"
>> also put themselves into the shoes of the maintainer, because considering
>> future readers is kind of the typical responsibility of the maintainer.
> Only in the sense that what the project does is to "maintain".
> (Aside: is developing, deploying, supporting users part of
> maintenance?  I don't actually think so!  We can call it that if you
> like, though.).
>
> To put this discussion on a more practical footing: I see new
> contributors confusing the maintainer for the audience for their
> changes all the time, and it hurts them.  They get some useful (and
> some unuseful, sadly) replies from the project and none from the
> maintainer and they end up confused: does this mean that the person
> that matters doesn't even care about my patch?
>
> Clarifying that the audience is *not* the maintainer can help.
>
> I would not be surprised if the Git for Windows project works
> differently: maybe you have the time, inclination, and ability to
> respond to every contributor directly early in the process.  There's
> nothing wrong with that, and it probably helps to make the process
> easier for contributors with this particular confusion.  Having a
> single person that is ready to talk to them and can stand in for what
> the entire project needs can make their life easier.
>
> But that's not sustainable in the Git project.  Larger projects either
> need multiple maintainers or a decrease in scope of the maintainer's
> role.  Junio uses the latter, which seems fine to me.
>
> Even something as simple as s/maintainer/maintainers/ would make this
> text less misleading for people contributing to upstream Git.

I really like the suggestion to swap "maintainer" for "community". It's 
something I've learned by being on the list since I first wrote that 
paragraph about "the maintainer" in October. Back then, I was new to the 
list and my main way of understanding what was going on was by reading 
Junio's "What's Cooking" emails.

> [...]
>>>>> +* Make sure the commits are signed off using `git commit
>>>>> (-s|--signoff)`. This means that you testify to be allowed to
>>>>> contribute your changes, in particular that if you did this on
>>>>> company time, said company approved to releasing your work as Open
>>>>> Source under the GNU Public License v2.
>>>> Can this either point to or quote the relevant legal text from
>>>> Documentation/SubmittingPatches?  It feels dangerous (in the sense of,
>>>> potentially leading to misunderstandings and major future pain) to ask
>>>> people to sign off without them knowing exactly what that means.
>>> When you can point at an existing test in legal context, it is safer
>>> to do so rather than attempting to "rephrase" it yourself (unless
>>> you are a lawyer, of course, in which case you can assess the best
>>> course of action yourself).
>> Please note: Documentation/SubmittingPatches is not a legal text. At least
>> I have not seen any accredited lawyer commenting on the validity of this
>> document, or how much sense it makes.
> This seems to reflect a misunderstanding of how law works, at least in
> the United States.
>
> I don't know how one would define what a legal text means.  I assume
> you mean "a contract", and I certainly don't care to evaluate whether
> some particular text is a contract: I am not a lawyer.
>
> But there are all kinds of other actions or words with legal
> implications:
>
>   - promises
>   - injuries
>   - etc, etc, etc
>
> Basically anything someone ever does has potential legal implications.
>
> The developer's certificate of origin has been reviewed by plenty of
> lawyers.  You can have your own lawyers review it too if you like.
>
> That's all side-stepping what I was saying, which is that it's bad for
> the project for people to be signing off without knowing what it
> means.
>
> [...]
>> Even so, Git for Windows' own wiki
>> (https://github.com/git-for-windows/git/wiki/Good-commits) links to a
>> particular version of Documentation/SubmittingPatches so that it can
>> specifically mark the DCO:
>>
>> https://github.com/git/git/blob/v2.8.1/Documentation/SubmittingPatches#L234-L286
> Great.  That's the sort of thing I'm asking for in this new
> contributors document.  Why in the world is it useful to push back
> against such a request?
>

I have a habit of being too loose in language around lawyer-speak. I 
should not have attempted to summarize what "Signed-off-by:" means and 
will use that helpful link for the description instead.

Thanks,
-Stolee