[PATCH i-g-t v2 1/2] lib/igt_chamelium: check drmModeGetConnector() return in wait_for_connected_state()

[Luca Coelho]

On Fri, 2024-06-07 at 16:38 +0200, Kamil Konieczny wrote:
> Hi Luca,
> On 2024-05-30 at 00:13:24 +0300, Luca Coelho wrote:
> > On Wed, 2024-05-29 at 18:06 +0200, Kamil Konieczny wrote:
> > > Hi Luca,
> > 
> > Hi Kamil,
> > 
> > > On 2024-05-28 at 10:58:54 +0300, Luca Coelho wrote:
> > > 
> > > change now looks ok but why did you ignored my suggestion
> > > about short subject?
> > 
> > Oh, sorry, but I didn't _ignore_ your suggestion.  I replied to it,
> > expressed my opinion and explained why I didn't think it needed to be
> > changed.
> > 
> > 
> > > Please see CONTRIBUTING.md about test
> > > description:
> > > The description should contain the spirit of the test (what is
> > > the general idea behind the test) and *not* the letter (C to English
> > > translation of the test).
> > 
> > This is not a new test, so I think this doesn't really apply.  This is
> > a very specific fix in a very specific place of an existing test and I
> > think getting a general idea of what was wrong in the code and why it
> > needs to be changed is the right thing to describe here.
> > 
> > This text you quoted is about what should be in igt_describe() and not
> > in the commit message, right?
> > 
> > 
> > 
> > > The same principle apply to your subject description of change,
> > > it should be short, not 'translation from C code of a change'.
> > > It should also be rather short, up to 75 chars (or maybe even 65)
> > > to keep it all in mailing list discussion.
> > > You coould also look into:
> > > https://patchwork.freedesktop.org/series/134188/
> > > 
> > > So imho here, instead of:
> > > lib/igt_chamelium: check drmModeGetConnector() return in wait_for_connected_state()
> > 
> > This describes what was fixed and where.
> > 
> 
> You can read it in patch itself after 'diff --git a/...'

Of course, the code change will show it, but it's good to know what
it's about with a quick glance.


> > > imho better:
> > > lib/igt_chamelium: Fix NULL dereferencing bug in wait_for_connected_state()
> > 
> > This describes that something was fixed and where, without specifying
> > what was fixed.
> > 
> > 
> > > or even better:
> > > lib/igt_chamelium: Fix bug in wait_for_connected_state()
> > 
> > This is even more generic and there could be several bugfixes in this
> > function which would end up with the same commit subject...
> > 
> > 
> > > or maybe:
> > > lib/igt_chamelium: check drmModeGetConnector() return value before use
> > 
> > Conversely,this describes what was fixed but not where.
> > 
> > 
> > > You could propose other subject if you find a better description.
> > 
> > I generally follow the kernel's coding style.  One important part of
> > the subject line, IMHO, is the uniqueness and identifiability of it. 
> > Like explained here[1]:
> > 
> > "[...]the ``summary phrase`` of your email becomes a
> > globally-unique identifier for that patch.  It propagates all the way
> > into the ``git`` changelog.  The ``summary phrase`` may later be used
> > in
> > developer discussions which refer to the patch.  People will want to
> > google for the ``summary phrase`` to read discussion regarding that
> > patch.  It will also be the only thing that people may quickly see
> > when, two or three months later, they are going through perhaps
> > thousands of patches using tools such as ``gitk`` or ``git log
> > --oneline``."
> > 
> > I admit that the subject is a bit long, but it actually contains only 5
> > relevant words.  The function names are rather long and, with about 20
> > characters of boilerplate ("lib/igt_chamelium: ") and 47 chars only in
> > function names, it's literally impossible to keep the subject line
> > inside the 70-75 recommended length while still keeping it descriptive
> > and unique...
> > 
> 
> What will you write if you will make change in a few functions?

It's obviously case-by-case.  And it should briefly describe what
changed.  If it's something in many functions, there should still be a
common theme for the change in all functions.  For instance:

foo: change bar in all affected functions


> Well, I agree that 70-75 is only recomendation, not strict rule.

Everyone should always strive to fit it (or at least make sure it's as
concise as possible), but if some symbols you want to mention there are
very long, trying to keep it under 70-75 is moot.

--
Cheers,
Luca.