[FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages

[FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages

[softworkz .]

Hello everybody,

I have an organizational question that I cannot quite figure out how to do it right:

Whether and when to add 'Reviewed-by:  xxx' to a commit message?

Specific example: The “execution graph printing” patchset:

Andreas had reviewed the patchset initially. I addressed the mentioned
issues. Is that the point to add 'Reviewed-by:’ already? And what about
the commits without comments? Should I assume them to be reviewed 
as well? And how about later changes, do they invalidate it?

From Stefano, I believe that he didn’t review the last two commits (due
to being outside of the text formatting scope, I suppose). I added him
as “reviewed by” only to those where he said “should be ok” or similar.
Andreas didn’t say anything like that, yet I’m sure that he has carefully
looked over everything.

It might not be a big thing after all, but I don't want to be unjust to 
anybody and I'm unsure how to handle this, because I can imagine that 
someone might either say "Hey, why is he mentioning me, that's not the 
version that I have reviewed!" but also "Why doesn't he mention me, I've
reviewed the whole thing in detail?".
That's the circle by which I got trapped at the moment. 😉 

I'd be glad if somebody could provide me some guidance in this regard.

Thanks a lot,
sw
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of softworkz .
> Sent: Samstag, 3. Mai 2025 11:32
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: [FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages
> 
> Hello everybody,
> 
> I have an organizational question that I cannot quite figure out how to do it
> right:
> 
> Whether and when to add 'Reviewed-by:  xxx' to a commit message?
> 
> Specific example: The “execution graph printing” patchset:
> 
> Andreas had reviewed the patchset initially. I addressed the mentioned
> issues. Is that the point to add 'Reviewed-by:’ already? And what about
> the commits without comments? Should I assume them to be reviewed
> as well? And how about later changes, do they invalidate it?
> 
> From Stefano, I believe that he didn’t review the last two commits (due
> to being outside of the text formatting scope, I suppose). I added him
> as “reviewed by” only to those where he said “should be ok” or similar.
> Andreas didn’t say anything like that, yet I’m sure that he has carefully
> looked over everything.
> 
> It might not be a big thing after all, but I don't want to be unjust to
> anybody and I'm unsure how to handle this, because I can imagine that
> someone might either say "Hey, why is he mentioning me, that's not the
> version that I have reviewed!" but also "Why doesn't he mention me, I've
> reviewed the whole thing in detail?".
> That's the circle by which I got trapped at the moment. 😉
> 
> I'd be glad if somebody could provide me some guidance in this regard.
> 
> Thanks a lot,
> sw
> _______________________________________________

Hello everybody,
 
recently, I've been criticized for this message on IRC as follows:

"softworkz: last month you even had to publicly ask on the mailing list 
about a policy which pretty much everyone understood"


So - what does this mean to express? That it would be an indication of 
incompetence when somebody is asking others for advice on something one
doesn't know?

What _I_ consider as a sign of incompetence is just the opposite: 
Pretending to know everything without knowing, and being afraid of 
asking questions as that might shed a bad light on oneself.

When I have a question on something that I do not know, then I ask that
question, no matter what anybody might think about it.
Because - at the end of the day - I'll be the one who knows the answer
and all those who were just pretending to know it - won't 
(unless public like here).

And anyway, where are we - that asking for advice would be a bad thing?

---

In this regard, I still don't know the answer. It's not about the basic
principle of procedure but goes into some quite specific whereabouts
where I'm not sure how exactly it is supposed to be done.


Thanks a lot for any advice or guidance.
sw

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages

[Marvin Scholz]

On 3 May 2025, at 11:32, softworkz . wrote:

> Hello everybody,
>
> I have an organizational question that I cannot quite figure out how to do it right:
>
> Whether and when to add 'Reviewed-by:  xxx' to a commit message?
>

Personally I try add this when someone reviewed and gave a LGTM
for the patch as then I can be sure they (hopefully) actually
reviewed the whole patch.

I sometimes comment on patches, that does not mean I reviewed
those fully, just that I saw something that I felt worth to add
a remark, I would not like to be added as Reviewed-by for that,
as it would not be true.

> Specific example: The “execution graph printing” patchset:
>
> Andreas had reviewed the patchset initially. I addressed the mentioned
> issues. Is that the point to add 'Reviewed-by:’ already? And what about
> the commits without comments? Should I assume them to be reviewed
> as well? And how about later changes, do they invalidate it?
>
> From Stefano, I believe that he didn’t review the last two commits (due
> to being outside of the text formatting scope, I suppose). I added him
> as “reviewed by” only to those where he said “should be ok” or similar.
> Andreas didn’t say anything like that, yet I’m sure that he has carefully
> looked over everything.
>
> It might not be a big thing after all, but I don't want to be unjust to
> anybody and I'm unsure how to handle this, because I can imagine that
> someone might either say "Hey, why is he mentioning me, that's not the
> version that I have reviewed!" but also "Why doesn't he mention me, I've
> reviewed the whole thing in detail?".
> That's the circle by which I got trapped at the moment. 😉
>
> I'd be glad if somebody could provide me some guidance in this regard.
>
> Thanks a lot,
> sw
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit messages

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Dienstag, 27. Mai 2025 11:28
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] When to add 'Reviewed-by: xxx' in commit
> messages
> 
> 
> 
> On 3 May 2025, at 11:32, softworkz . wrote:
> 
> > Hello everybody,
> >
> > I have an organizational question that I cannot quite figure out how
> to do it right:
> >
> > Whether and when to add 'Reviewed-by:  xxx' to a commit message?
> >
> 
> Personally I try add this when someone reviewed and gave a LGTM
> for the patch as then I can be sure they (hopefully) actually
> reviewed the whole patch.
> 
> I sometimes comment on patches, that does not mean I reviewed
> those fully, just that I saw something that I felt worth to add
> a remark, I would not like to be added as Reviewed-by for that,
> as it would not be true.

Hi Marvin,

thanks a lot for the reply!

On Patchwork, there's the "A/L/R/T" column. AFAIK, the 

A (acked-by), R (reviewed-by) and T (tested-by) columns are detected
from lines in the commit message and L (LGTM) is based on detection
from response messages on the ML.

Side note: LGTM detection only seems to work when it's at the start 
           of a line.

Responses with LGTM have become a bit rare these days, that's what
is making the situation somewhat more unclear to me (and eventually
led me to ask about it).

On the other hand, what you are describing is a very clear and straight-
forward pattern and I will adhere to it in the future:

LGTM => reviewed-by <...> in the next revision

(unless others have additional thoughts to share and consider)

Thank you very much
sw




_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".