[parted-devel] [PATCH] next branch

[Joel Granados]

On Tue, Jun 02, 2009 at 07:33:54AM +0200, Jim Meyering wrote:
> Joel Granados wrote:
> > On Tue, May 26, 2009 at 03:16:35PM +0200, Jim Meyering wrote:
> >> Joel Granados wrote:
> >> >> Regarding the logs,
> >> >> when you add a new function, just saying "New function."
> >> >> is enough, because there has to be a comment in the
> >> >> code already saying what the function does.
> >> >
> >> > This is true. But I have to insist on a small description, instead of
> >> > just saying "New function" as the diff that accompanies the commit might
> >> > not be as readable or as obvious.  With that said, I'm OK with that
> >> > change.
> >>
> >> The name is there already.
> >> I hope that's enough for you, in general
> >>
> >> It's better not to describe it at all in the commit log).
> >> Otherwise, you risk someone reading and relying on that abbreviated
> >> (or maybe inaccurate) description.  In addition, with any duplication
> >> at all, you'd want to keep them in sync, but the git log is not
> >> modifiable.
> >
> > True, the git log is not modifyable but we should not be allowing
> > inaccurate descriptions anyway.  If the description is inacurate, the
> > patch should be rejected.
> 
> Sometimes I write a patch and keep it on an unofficial branch for months.
> While it's on that branch it may evolve.  It would be annoying (and easy
> to forget) to have to adjust comments-in-log to reflect already adjusted
> comments-in-code every time that happens.

Well, this addresses the process more than the end result.  What I'm
trying to say is that if you forget that the commit comment is
not synced with the actual commit, the patch should not be accepted
anyway

> 
> > You are also correct when you assume that the description might
> > become inaccurate when someone changes the function in the future.
> > But I would still argue that a small description is needed when the
> 
> Description specific to each added function belongs in the code.
> Why duplicate that in the log?  

Context while viewing the log.  And while there might be a little
duplication involved.  I think its worth it in some particular cases.
For example:

libparted/label/dos.c (msdos_align_to_cylinder) : I would not put any
comments on this one as I think the function pretty much explains what
it will do.

libparted/label/dos.c (msdos_simple_primary_align) : Here I would put a
little comment, (if not clear in the primary commit message) as to why
its called "simple".  Something like: "Alignment that is not dependent
on cylinder size." (if that was the reason)

And in the code the msdos_simple_primary_align function will thoroughly
explain why we don't need to align to cylinder sizes and other stuff
that the developer deems necessary.

> The log is the right place for high level
> "why make this change?" discussion.  For example, if this patch is mostly

I think a small function comment can help answer this question.  When
the main commit message falls short.

> preparation for a subsequent change, there's no point in writing that
> in the code.  However it's essential to say it in the log.  Similarly,
> when one commit reverts part of another, the log is the place to write
> about it.  Reference the affected SHA1, too.  However, in this case,
> it's good also to add comments in the code, so that future developers
> have a clue if/when they consider reintroducing the bug.

Agreed.

> 
> > function name leaves some obvious unanswered questions.  These, of
> > course, are situations that must be evaluated per commit.
> >
> >>
> >> Also, it's best to avoid putting too much per-function
> >> description in the log, because that could lull the author
> >> into thinking s/he has already written the required
> >> documentation, without realizing that it was solely
> >> for the log, and not yet made into the mandatory
> >> function-describing comment.
> >
> > With this comment I have an opinion and a question:
> >
> > Question:  One rights documentation of a function when the function is
> > not obvious and when the function is part of the library.  right?
> > More specifically, What are the policies for documenting a function?
> 
> IMHO, nearly every function requires some minimal comment
> describing its functionality, the role of each argument,
> and the return value.  That comment goes just before the
> function definition.  Of course, if it does something
> involved or non-obvious, more in-functions comments are
> appropriate.

Agreed, for in code function comments.

> 
> > Opinion:  I think that there are various resons for not documenting
> > a function.  Placing a short "doubt resolving" comment in the commit, is
> 
> There are very few functions for which a minimal comment would not help.
> Please write them unconditionally, at least for parted.
> 
> > not one of the main ones.  I would argue that not knowing the policy
> > would be a major reason (from the patch creator and from the patch
> > reviewer).  Also, laziness, lack of time also pop into my mind as
> > reasons that would come before being lulled by the commit message.  I
> > would further argue that with the correct knowledge of when to document
> > a function, the reviewer would easily detect when a patch needs to add
> > documentation.
> 
> If I don't write a comment it's because I forget to or ran out of time.
> If I commit a new function with no description, please call me on it.
> I find that the discipline of writing a good function-describing comment
> often clarifies in my mind what the functionality should be, highlights
> an inadequate parameter name, or suggests some other interface change.

Ack

> 
> Regarding *requiring* comments, when it is not possible to cajole
> a contributor into providing -- or even just outlining -- a test case,
> I wonder if it's worth asking for comments.  Perhaps better for the
> maintainers to write them.

Unfortunately, True.

> 
> Of course, if contribution guidelines were to state the goal,
> it would be easier to request the additional few lines per function.


In summary, and to clarify my stand on this issue:
I agree that there might be "cons" regarding individual function
comments in the commit log.
 a. Make the developer think that the function doc is already written
 b. The commit comment is not in sync with the commit.
 c. The specific commit function comment repeats stuff that is said in the main
    commit message.
 d. The commit function comment becomes extensive and gives too much info.

But, if the commit function comments don't fit in (a-d), and they are
sound, and they actually add some context when reading the log, I would
accept the patch.
I also realize that the patch that originated this discussion might be
suffering from a or b or c or d.  And to that I say, I'll be more
careful.

On a side Note:  /me goes and checks all the commit messages to see if
he is actually complying with this discussion :).

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.