[flashrom] [PATCH 1/3] 128 bytes write granularity support

[Paul Kocialkowski]

Hi Stefan,

Le jeudi 22 octobre 2015 à 22:05 +0200, Stefan Tauner a écrit :
> On Thu, 22 Oct 2015 08:58:53 +0200
> Paul Kocialkowski <contact at paulk.fr> wrote:
> 
> > Hi Carl-Daniel,
> > 
> > Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a
> > écrit :
> > > On 18.10.2015 19:37, Paul Kocialkowski wrote:
> > > > I noticed that you have reworded the commit message: I usually do
> > > > not > use conjugated verbs in commit headline. Does this conflict
> > > > with >
> > > flashrom's guidelines?
> > > 
> > > No formal guidelines, but "128 bytes write granularity support" is
> > > missing a crucial piece of information: Was this a bugfix, a new
> > > feature
> > > or something you removed?
> > > The reworded commit message has that info.
> > 
> > Well, it does end with "support", which indicates that support for this
> > was introduced, so I don't think it was confusing.
> > 
> > I really see commit headlines as titles, not active phrases and would
> > like to keep my commits phrased this way.
> > 
> > Please let me know during patch review next time you find the commit
> > headline not precise enough.
> 
> Hello Paul,
> 
> I have not replied yet because Carl-Daniel's message described the
> thoughts I had/have when changing commit logs pretty much exactly.
> Your approach on commit logs (seeing the first line as the "subject") is
> IMHO a defensible argument. After all, that's how git calls it as well
> in many places.
> 
> However, using imperative sentences is much more common and IMHO it is
> better. I see every commit as an entity that changes our code base...
> so it always *does* something and often it is interesting what the most
> descriptive verb is for what it is doing (e.g. add, fix, improve,
> refactor, ...) to better understand what the change is about. Just like
> Carl-Daniel explained for the commit in question.

Well, I don't see both points of view as contradictory. When introducing
support for something new, I think we're not losing any information when
formatting the subject line as:
Feature foo support introduction
instead of
Introduce support for feature foo

This goes for all the things that we might want to describe:
* Bug #123 fixup
* Feature bar improvement
* Baz code re-factoring

Most active verbs have (more or less direct) equivalent nouns that can
be used.

I get the argument that we're doing something to the code base, so it
makes sense to use active verbs, but for some reason, I just find that
somewhat ugly. Perhaps my vision of things will change with time, but
that's how I see it for now.

Also, it seems that there is a lot of disparity regarding the tense used
for such verbs : Added support for X/Add support for X/Adds support for
X. And I don't really see any good way to choose between those. And I
hate that sort of lack of consistency, so using non-verbal sentences
felt like an elegant solution.

> I can not be sure that if we someone looks at the commit in three years
> he will remember or even know that 128 byte granularity was introduced
> about the time this commit happened. It is much easier to understand
> a commit if the subject contains a verb that makes the action clear.

Well, let's not forget that commit messages come with longer
explanations that must play a role, more than saying the same as the
subject. I really see the subject as a title to the longer commit
message.

> Also, let's suppose we would have to revert a commit because it was
> really a bad idea to commit it in the first place and we can not fix it
> easily. The normal commit message would be
> «Revert "128 bytes write granularity support"» in your case vs.
> «Revert "Add support for 128 bytes write granularity.» in the
> imperative case. The issue with the subject without a verb is similar
> to the original. In the second case it is completely clear that the
> support was removed again. Without a verb it could be a fix that is
> reverted, an improvement, or even the revert of removing the support...
> it is simply not explicit enough IMHO.

Well, I think that "support" implies "support introduction", but that's
something else. Also, git wraps the original line around quotation
marks, which also adds some clarity.

> Others have written about the subject as well, but I can not find a lot
> of texts explaining the WHY very good... this is one example:
> 
> http://chris.beams.io/posts/git-commit/#imperative

One very good point here is that git uses the imperative when creating a
commit on its own (Revert, Merge, etc). That sounds reasonable enough as
a guideline for me to switch my behaviour.

> Your other patches are affected as well and I intend to rephrase them
> as I see fit. I hope you understand/agree now... if not I'll have
> another argument: the maintainers have to maintain the codebase and are
> the ones who read commit messages the most, hence it should be the
> decision of the maintainers what's written in commit messages because
> they are affected most by them. The submitter can simply dump the code
> and hope it gets merged but apart from seeing some mails with the
> subject during communication it does not affect his work on the project
> later (because there often is none... otherwise he is not merely a
> submitter).

Well, that makes sense, yes, but keep in mind that it's also part of the
author's contribution, so I'd like things to be discussed beforehand,
just like it is somewhat rude to change a patch the way you like and
merge it without discussing it with its author through code review
first.

Let's say that I wouldn't object to having commit subjects rephrased the
way git does it internally.

Thanks for taking the time to explain all this.

Paul Kocialkowski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20151027/5d4f4891/attachment.asc>