On Tue, Jan 6, 2015 at 2:37 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> When sending out patch series one of the last things doing is writing
>> the cover letter. The cover letter would be a good place to remind
>> people to check the todo list for sending patches.
>
> I do not quite understand. Wouldn't a check-list be useful _before_
> you start series of things (I am assuming that you meant a list like
> 1. run spell check; 2. run checkpatch; 3. run full test suite;
> 4. format the docs for HTML and manpage)? Time to write cover
> letter (or running format-patch in general) is way too late for
> many of these things.
Actually I do mean these things.
As I have all changes to be sent in branches, each commit having all the
notes, sending them to the mailing list is becoming less of a hassle.
All I need to do is
git format-patch --cover-letter --notes -subject-prefix=PATCHvX
origin/master.. &&
$editor 0000-cover-letter.patch &&
git send-email 00* --to=git@xxxxxxxxxxxxxxx --cc=...
There is *no* postprocessing of other *.patch files apart from the cover letter
involved any more in my workflow. If something needs to change I need to
change it directly in the commit with
git rebase -i origin/master
... and edit/reword the specific patch
By choosing this way of working I am able to send out any branch for review
in a heartbeat. I think the advantage of this approach is to have git
support for
most of the time, i.e. I can use gitk for reviewing my patches
including notes as
well as inspecting the filesystem. Also I try to force myself into
reviewing each
patch twice (with time in between) before sending it out for public review.
The disadvantage is that I need to have the high quality in a branch before
sending it out for review. But I personally find it easier to deal
with git branches
than with patch files of different versions. Branches do not forget
anything once
I edited it in which turns that point into an advantage for me.
>
> There may be a check-list that is still useful after commits to be
> sent are perfect and ready to be formatted.
> "Describe change since
> the last round after three-dash line." would be one of them
> ("Sign-off the patch" is not---without one, the commits would not
> have been perfect yet).
But how do I know if a patch is perfect?
By checking all points of my checklist for each patch. But during the
iterations of reviews, patches come and go, so I need to have
a checklist after I think each patch is perfect on its own.
So what about:
* Are the patches in the correct order? (Implement the feature
before advertising it)
* Do some of the patches still make sense in the context of this
series? (Sometimes the focus of a series changes quite a bit
after input from reviewers, so some patches may be dropped)
> But for such a check-list, wouldn't we want
> remainder not only on the cover but on each individual patch?
>
> Perhaps --add-header="x-reminder: what changed since the last?"
> would be sufficient for your purpose instead?
I am not quite sure if that is my problem any more. Say I have the
following check list:
* Do I follow the coding style?
* indentation by tabs and spaces
* no superfluous braces
* The code itself
* Does it embed into the current logic flow?
* memory leaks?
* Does it compile and test (git rebase --exec=make --exec="make test") ?
* Is a patch small enough?
* Does it do one thing? (move code or add code, not both!)
* Do I have a proper commit message?
* spelling and grammar
* Does the commit message (still) describe the changes of the patch?
* After doing changes, wait at least 12 hours for second self-review
* sending out:
git format-patch --cover-letter --notes --subject-prefix=PATCHvX
Most of it is on a per-patch basis, but it is easier to check for the whole
branch/series in one go after some time when you found some mental
distance to the code you wrote yourself. And for me this is usually
the next day, when I review it again and ask myself: Do I send out or not?
As you can see there may be quite some discussion on what you want
to put into that check list, hence it should be configurable. We could of
course think about pre-populating the check list for new comers.
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html