Re: [FFmpeg-devel] Revert "avformat/tls_openssl: properly get new BIO index"

Re: [FFmpeg-devel] Revert "avformat/tls_openssl: properly get new BIO index"

[Nicolas George]

Kacper Michajłow (HE12025-07-30):
> > Note that BIO_get_new_index() can only be used 127 times before it
> > returns an error.
> 
> We cannot call it repeatedly, because it will fail eventually.
> 
> To my understanding the index is not needed in our use and we could
> safely use BIO_TYPE_NONE. Documentation states:
> 
> > type can be set to either BIO_TYPE_NONE or via BIO_get_new_index() if
> > a unique type is required for searching (See BIO_find_type(3))
> 
> We don't use any search related functions.

This should never have reached the commit message.

-- 
  Nicolas George
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Kacper Michajlow]

On Wed, 30 Jul 2025 at 19:53, Nicolas George <george@nsup.org> wrote:
>
> Kacper Michajłow (HE12025-07-30):
> > > Note that BIO_get_new_index() can only be used 127 times before it
> > > returns an error.
> >
> > We cannot call it repeatedly, because it will fail eventually.
> >
> > To my understanding the index is not needed in our use and we could
> > safely use BIO_TYPE_NONE. Documentation states:
> >
> > > type can be set to either BIO_TYPE_NONE or via BIO_get_new_index() if
> > > a unique type is required for searching (See BIO_find_type(3))
> >
> > We don't use any search related functions.
>
> This should never have reached the commit message.

Sorry, if the commit description is not formal enough. I'm not native
English speaker, I tend to word things in uncommon ways. I gave
commentary there to excerpts from the OpenSSL documentation.

As for BIO_TYPE_NONE vs BIO_TYPE_SOURCE_SINK, there is no difference
in current or previous versions of OpenSSL, BIO_TYPE_SOURCE_SINK is
not actually used in any meaningful way. I didn't include the 2nd
commit to change to BIO_TYPE_NONE. As the main objective was to fix
regression only and restore the previous status quo is good for now.

- Kacper
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Niklas Haas]

On Wed, 30 Jul 2025 19:52:59 +0200 Nicolas George <george@nsup.org> wrote:
> Kacper Michajłow (HE12025-07-30):
> > > Note that BIO_get_new_index() can only be used 127 times before it
> > > returns an error.
> >
> > We cannot call it repeatedly, because it will fail eventually.
> >
> > To my understanding the index is not needed in our use and we could
> > safely use BIO_TYPE_NONE. Documentation states:
> >
> > > type can be set to either BIO_TYPE_NONE or via BIO_get_new_index() if
> > > a unique type is required for searching (See BIO_find_type(3))
> >
> > We don't use any search related functions.
>
> This should never have reached the commit message.

I disagree. Without this important context, the motivation behind the
change is much less clear.

>
> --
>   Nicolas George
> _______________________________________________
> 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] Revert "avformat/tls_openssl: properly get new BIO index"

[Nicolas George]

Niklas Haas (HE12025-07-30):
> I disagree. Without this important context, the motivation behind the
> change is much less clear.

It is not context, it is a dump of an exchange of mails that led to the
patch. It should have been rewritten, “To my understanding” is not
something that should appear in a commit message.

And it is definitely not a matter of language, it is matter of
attention.

Regards,

-- 
  Nicolas George
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Kacper Michajlow]

On Wed, 30 Jul 2025 at 21:53, Nicolas George <george@nsup.org> wrote:
>
> Niklas Haas (HE12025-07-30):
> > I disagree. Without this important context, the motivation behind the
> > change is much less clear.
>
> It is not context, it is a dump of an exchange of mails that led to the
> patch. It should have been rewritten, “To my understanding” is not
> something that should appear in a commit message.
>
> And it is definitely not a matter of language, it is matter of
> attention.
>
> Regards,

Can we please find something more relevant to be upset about?

I've written this commit description, and I disagree that it's as bad
as you're making it out to be.

It's not a dump of any email exchange. It's commentary to OpenSSL documentation.

That said, I’ve taken your feedback seriously and will continue to
improve going forward.

- Kacper
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Nicolas George]

Kacper Michajlow (HE12025-07-30):
> Can we please find something more relevant to be upset about?

Can you please stop behaving with such arrogance when somebody who has
30 times as much experience as you in the project points you you did
something wrong?

-- 
  Nicolas George
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Niklas Haas]

On Wed, 30 Jul 2025 23:39:18 +0200 Nicolas George <george@nsup.org> wrote:
> Kacper Michajlow (HE12025-07-30):
> > Can we please find something more relevant to be upset about?
>
> Can you please stop behaving with such arrogance when somebody who has
> 30 times as much experience as you in the project points you you did
> something wrong?

What you pointed out is pointess nitpicking, and I could point you to
plenty of other comments by "experienced" FFmpeg developers who have done
the same in the past, presumably without having sat through diatribes.

>
> --
>   Nicolas George
> _______________________________________________
> 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] Revert "avformat/tls_openssl: properly get new BIO index"

[Nicolas George]

Niklas Haas (HE12025-07-30):
> What you pointed out is pointess nitpicking, and I could point you to
> plenty of other comments by "experienced" FFmpeg developers who have done
> the same in the past, presumably without having sat through diatribes.

Where they argued that they were right instead of admitting “my bad”? I
doubt it, but you can try.

-- 
  Nicolas George
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Alexander Strasser via ffmpeg-devel]

On 2025-07-31 00:02 +0200, Nicolas George wrote:
> Niklas Haas (HE12025-07-30):
> > What you pointed out is pointess nitpicking, and I could point you to
> > plenty of other comments by "experienced" FFmpeg developers who have done
> > the same in the past, presumably without having sat through diatribes.
> 
> Where they argued that they were right instead of admitting “my bad”? I
> doubt it, but you can try.

I don't understand what the issue with the commit message is?

Read the message multiple times and it is above average for commit
messages in general and especially for reverts.

1. It spells out why the revert was done
2. It gives reference to where the problem was discovered
3. It cites corresponding relevant parts of the OpenSSL docs inline

Nicolas, can you clear this up? What am I missing?


Best regards,
  Alexander
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Nicolas George]

Alexander Strasser via ffmpeg-devel (HE12025-08-02):
> I don't understand what the issue with the commit message is?

Two issues:

- Form: “>” is not part of our practice for commit message. Look at the
  past year of commit message, the only ones that include “>” lines are
  mistakes.

- Substance: “To my understanding”, either the patch was reviewed, and
  nobody's individual understanding has to be mentioned in the commit
  message, or it was not and it should not have been applied.

It is minor, but there is a reason we historically gave write access
only to experienced contributors: two pairs of eyes are better than one
to fix this kind of minor mistakes and keep our history as clean as
possible.

Regards,

-- 
  Nicolas George
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Alexander Strasser via ffmpeg-devel]

On 2025-08-02 20:08 +0200, Nicolas George wrote:
> Alexander Strasser via ffmpeg-devel (HE12025-08-02):
> > I don't understand what the issue with the commit message is?
> 
> Two issues:

OK, now I understand what you mean.
It wasn't clear to me from your original replies in this thread.


> - Form: “>” is not part of our practice for commit message. Look at the
>   past year of commit message, the only ones that include “>” lines are
>   mistakes.

It is common to use this style of quoting outside of text MUAs as
well. E.g. when replying to things in IRC chat or in Markdown.

I find it acceptable and would encourage people to use it.

Not to paste emails by mistake, of course.
Though it is OK to quote mails on purpose if it helps to
have it included in the commit message. E.g.:

    From the discussion in the ML thread:
    > > This is like this and that.
    >
    > But there is also another case we need to consider.


Note that Kacper introduced the documentation quote like this (`^` for
highlighting inserted by me):

    type definition. As we can read in the documentation:
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
    > Note that BIO_get_new_index() can only be used 127 times before it
    > returns an error.
    
    We cannot call it repeatedly, because it will fail eventually.
    
    To my understanding the index is not needed in our use and we could
    safely use BIO_TYPE_NONE. Documentation states:
                              ^^^^^^^^^^^^^^^^^^^^^
    
    > type can be set to either BIO_TYPE_NONE or via BIO_get_new_index() if
    > a unique type is required for searching (See BIO_find_type(3))

Would you agree that it is indeed a helpful way to use for quoting in
commit messages?


> - Substance: “To my understanding”, either the patch was reviewed, and
>   nobody's individual understanding has to be mentioned in the commit
>   message, or it was not and it should not have been applied.

I find it potentially helpful to include uncertainty in the commit
message. It is helpful for people doing code archaeology.


> It is minor, but there is a reason we historically gave write access
> only to experienced contributors: two pairs of eyes are better than one
> to fix this kind of minor mistakes and keep our history as clean as
> possible.

This was a special case, the commit that changed that code recently
introduced a regression. Therefore reverting earlier is better.

As soon as someone has a better understanding, the code can be changed
again or left as is. No reason to let the regression live longer just
to avoid reverts. These kinds of reverts are healthy as long as they
include the reason why the reverts were done.

I agree with you that we should try to keep our history as clean as
possible and so does Kacper as he already stated.


  Alexander
_______________________________________________
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] Revert "avformat/tls_openssl: properly get new BIO index"

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 808 bytes --]

Hi

On Wed, Jul 30, 2025 at 09:53:26PM +0200, Nicolas George wrote:
> Niklas Haas (HE12025-07-30):
> > I disagree. Without this important context, the motivation behind the
> > change is much less clear.
> 
> It is not context, it is a dump of an exchange of mails that led to the

It seems the commit message contains quotes from openssl documentation
not emails

I did misread this commit the same way thinking its mails

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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