Re: [PATCH v4] documentation: add tutorial for first contribution

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2019, 02:45 UTC
Message-ID: <xmqq5zqlk6k5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190506222844.261788-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> Sorry for not looking at this sooner. 
>
> Firstly, I'm not sure if this file should be named without the ".txt",
> like SubmittingPatches.

SubmittingPatches has historical baggage but this does not, so its source can be left as .txt (alternatively we could have added .txt to SubmittingPatches and left a symlink to keep the historical name, without introducing "copy X to produce X.txt" rule).

cf. http://public-inbox.org/git/xmqqbm15kxi0.fsf@gitster-ct.c.googlers.com/

Show 11 quoted lines

>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index 26a2342bea..fddc3c3c95 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -74,6 +74,7 @@ API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technica
>>  SP_ARTICLES += $(API_DOCS)
>>  
>>  TECH_DOCS += SubmittingPatches
>> +TECH_DOCS += MyFirstContribution
>
> Any reason not to keep this alphabetized?

I do not think the order matters to $(MAKE), and I do not know if the order matters to humans---sane ordering is done to futureproof when we know we will have many more, but I do not know if we will.

So I find it a borderline Meh, but let's not waste your finding, as sorting them in alpha order would be the sensible default.

Show 14 quoted lines

>> +=== Pull the Git codebase
>> +
>> +Git is mirrored in a number of locations. https://git-scm.com/downloads
>> +suggests one of the best places to clone from is GitHub.
>> +
>> +----
>> +$ git clone https://github.com/git/git git
>> +----
>
> I would rename the header to "Clone the Git repository" instead, since
> "pull" has a specific meaning. Also, I think that "one of the best
> places" is unnecessary (I would just say "Clone the Git repository from
> one of its many mirrors, e.g.:"), but perhaps you want to leave it in
> there to maintain the informal tone.

I am guilty of the verbosity there---just did not want to give an impression that that one is the single authoritative copy (the k.org one is probably the one, if only that it is the one pushed to first when a new development happens). I personally feel that your rephrasing is fine, too, and do not have strong preference between the two.

Show 11 quoted lines

>> +We'll also need to add the extern declaration of psuh; open up `builtin.h`,
>> +find the declaration for `cmd_push`, and add a new line for `psuh` immediately
>> +before it, in order to keep the declarations sorted:
>> +
>> +----
>> +extern int cmd_psuh(int argc, const char **argv, const char *prefix);
>> +----
>
> I was going to say to not include the "extern", but I see that builtin.h
> has them already, so it's probably better to leave it there for
> consistency.

Yup, this was discussed before. If we can have the "NEEDSWORK" in the asciidoc source that is not rendered in the end result, it may be worth leaving a note to say when we remove them from builtin.h we need to update this example, or something like that.

Show 5 quoted lines

>> +----
>> +$ git send-email --to=target@example.com
>> +----
>
> Hmm...don't you need to specify a directory?

Even better would be the directory/*.patch glob pattern, as we'll show how to emit format-patch output for v2 into the same directory in a later step. Just giving the directory and letting readdir() collect would work for v1 but not for later iterations.

Show 8 quoted lines

>> +Your Message-Id is `<foo.12345.author@example.com>`. This example will be used
>> +below as well; make sure to replace it with the correct Message-Id for your
>> +**previous cover letter** - that is, if you're sending v2, use the Message-Id
>> +from v1; if you're sending v3, use the Message-Id from v2.
>
> I think it's better to describe the message ID as without the angle
> brackets. Reading the RFC (https://tools.ietf.org/html/rfc2392), the
> message-id doesn't have them.

See earlier review(s). Your reading is probably wrong, as that directly contradicts my earlier suggestion based on the same RFC ;-)

Show 7 quoted lines

>> +----
>> +$ git send-email --to=target@example.com
>> +		 --in-reply-to=<foo.12345.author@example.com>
>> +----
>
> The angle brackets can be omitted. Also, directory (or glob expression
> in this case)?

Yeah, a glob would be appropriate.

Show 17 quoted lines

>> +=== Bonus Chapter: One-Patch Changes
>
> This is not truly a bonus - the mailing list prefers this if the patch
> set contains only one patch.
>
>> +In some cases, your very small change may consist of only one patch. When that
>> +happens, you only need to send one email. Your commit message should already be
>> +meaningful and explain the at a high level the purpose (what is happening and
>> +why) of your patch, but if you need to supply even more context, you can do so
>> +below the `---` in your patch. Take the example below, generated with
>> +`git format-patch` on a single commit:
>
> It's not clear to me how `git format-patch` can generate the extra
> paragraph below. The user would either have to include "---" in the
> commit message (in which case there would be an extra "---" below the
> extra paragraph, which is perfectly safe) or edit the email *after*
> `git-format-patch` has generated the email.

Yes, I think the editions after v2 of this series has consistently encouraged users to edit format-patch output in a separate editor session (be it 0/n cover letter, or 1/1 single patch) before sending it out, discouraging use of --compose in send-email or driving format-patch from within a send-email session.

And the following example just shows how the finished result of such an editing session would look like.

Show 6 quoted lines

> The other meta-concern is maintaining the informal tone when we update
> this document (for example, when we add features like range-diff which
> can be used when sending v2 - well, somebody can add information about
> that to this document once it has been merged); but I don't think that
> is a concern in practice (either we keep the tone or there is a slight
> tone mismatch, and I don't think that either is a big deal).

I am OK as long as those who care about maintaining coherent tone pay attention to changes proposed to this document in the future ;-)

Thanks for comments. Let's finish this topic and merge it down soonish.