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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Dscho,

Johannes Schindelin wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Dereck Stolee wrote:

>>>> +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.

[...]
> 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.

[...]
>>>> +* 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?

Confused,
Jonathan