Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error

[jonathan chang <ttjtftx@gmail.com>]

On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 03/10, Jonathan Chang wrote:
> > Hi,
> >
> > Thanks for the reviews.
> >
> > Here are the changes in the second version:
> >       - bug fixes
> >       - add preparatory patch
> >       - seperate different files to different patch
> >       - change to use test_line_count in a seperate patch
> >
> > Also I found that there is no such function as test_char_count,
> > is it worthwile to add such function? Here are some stat:
> >
> > `git grep 'test_line_count' | wc -l` = 626
> > `git grep 'wc -l' | wc -l` = 294
> > `git grep 'wc -c' | wc -l` = 68
>
> I do think it would be helpful to introduce that helper, especially if
> it is useful in this patch series.  There seem to be enough other
> places where it can be useful to make it worth adding the helper.

Thanks for the feedback.

> > -- >8 --
> >
> > This is a preparatory step prior to removing the pipes after git
> > commands, which discards git's exit code and may mask a crash.
>
> The commit message should also describe why we need this preparatory
> step. Maybe something like:
>
>       To reduce the noise in when refactoring this pipeline in a
>       subsequent commit fix the indentation.  This has been wrong
>       since the refactoring done in 1b5b2b641a ("t0000: modernise
>       style", 2012-03-02), but carries no meaning.
>
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> >
>
> Out of curiosity, how did you create the patch?  'git format-patch'
> would add a '---' line followed by the diffstat, where you would
> usually put the commentary that you put before the scissors line.  It
> seems like 'git am' still handles this fine, which I didn't know, just
> something I noticed because I'm used to the other format.

I believe I used git for that. But I cannot think of why it happened nor
reproduce it.

> Since this patch series is now 5 patches, that commentary should go
> into a cover letter (see the --cover-letter option in format-patch),
> so the reviewers can read that first, and read the patches with that
> in mind, focusing on the patch only, and not additional commentary
> that applies to the whole series when reading the patch.

I wasn't aware of this option. I tried to produce the format in others
cover letter using 'git diff' and options like '--stat', '--summary', with no
success. I consulted Documentation/SubmittingPatches, where I got the
idea of cover letter, but it doesn't mention the option '--cover-letter' and
the idea of cover letter is even confused with '--notes'[1].

I just reread some of the GSoC related mails in the mailing list and
found one[2] that introduced the usage of 'cover-letter', '--range-diff' and
'--interdiff'. As a newbie, I personally think it would be helpful to include
theses options along with others mentioned in SubmittingPatches.

[1]: Documentation/SubmittingPatches:

> You often want to add additional explanation about the patch,
> other than the commit message itself. Place such "cover letter"
> material between the three-dash line and the diffstat. For
> patches requiring multiple iterations of review and discussion,
> an explanation of changes between each iteration can be kept in
> Git-notes and inserted automatically following the three-dash
> line via `git format-patch --notes`.

[2]: https://public-inbox.org/git/CAPig+cSsAufCnHPJfjQd8A778UNAsXEst1m+ekQ7T83=2mMUnw@mail.gmail.com/