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

[Luca Coelho]

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.


> 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...

But this is a different project and I'm not a maintainer, so I can do
whatever you think is right here.  Just tell me which option you prefer
and I'll resubmit my patch with it.


[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n652

--
Cheers,
Luca.