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

[Kamil Konieczny]

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

> 
> > 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?
Well, I agree that 70-75 is only recomendation, not strict rule.

Regards,
Kamil

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