diff mbox

[FFmpeg-devel] avformat/http: remove duplicate user-agent option

Message ID CADxeRw=WrUsCfBEs_htfTn+tOhVnEARNLGmumjwUAjSv7YwR0A@mail.gmail.com
State Superseded
Headers show

Commit Message

Steven Liu Sept. 15, 2016, 2:04 p.m. UTC 
double user-agent option, same option, remove one.

Signed-off-by: Steven Liu <lingjiujianke@gmail.com>
---
 libavformat/http.c | 1 -
 1 file changed, 1 deletion(-)

-    { "user-agent", "override User-Agent header", OFFSET(user_agent),
AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },
     { "multiple_requests", "use persistent connections",
OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | E },
     { "post_data", "set custom HTTP post data", OFFSET(post_data),
AV_OPT_TYPE_BINARY, .flags = D | E },
     { "mime_type", "export the MIME type", OFFSET(mime_type),
AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, AV_OPT_FLAG_EXPORT |
AV_OPT_FLAG_READONLY },

Comments

Clément Bœsch Sept. 15, 2016, 2:21 p.m. UTC | #1 
On Thu, Sep 15, 2016 at 10:04:48PM +0800, Steven Liu wrote:
> double user-agent option, same option, remove one.
> 
> Signed-off-by: Steven Liu <lingjiujianke@gmail.com>
> ---
>  libavformat/http.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavformat/http.c b/libavformat/http.c
> index adb3d92..f2bfb17 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -131,7 +131,6 @@ static const AVOption options[] = {
>      { "headers", "set custom HTTP headers, can override built in default
> headers", OFFSET(headers), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E
> },
>      { "content_type", "set a specific content type for the POST messages",
> OFFSET(content_type), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E },
>      { "user_agent", "override User-Agent header", OFFSET(user_agent),
> AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },
> -    { "user-agent", "override User-Agent header", OFFSET(user_agent),

one has '_', the other '-'
Carl Eugen Hoyos Sept. 15, 2016, 2:22 p.m. UTC | #2 
2016-09-15 16:04 GMT+02:00 Steven Liu <lingjiujianke@gmail.com>:
> double user-agent option, same option, remove one.

Looking at the documentation, the patch is not ok as-is.

I suggest to deprecate the option now, and to remove it after
the next release.

Carl Eugen
Steven Liu Sept. 15, 2016, 2:22 p.m. UTC | #3 
2016-09-15 22:21 GMT+08:00 Clément Bœsch <u@pkh.me>:

> On Thu, Sep 15, 2016 at 10:04:48PM +0800, Steven Liu wrote:
> > double user-agent option, same option, remove one.
> >
> > Signed-off-by: Steven Liu <lingjiujianke@gmail.com>
> > ---
> >  libavformat/http.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index adb3d92..f2bfb17 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -131,7 +131,6 @@ static const AVOption options[] = {
> >      { "headers", "set custom HTTP headers, can override built in default
> > headers", OFFSET(headers), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D
> | E
> > },
> >      { "content_type", "set a specific content type for the POST
> messages",
> > OFFSET(content_type), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E },
> >      { "user_agent", "override User-Agent header", OFFSET(user_agent),
> > AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },
> > -    { "user-agent", "override User-Agent header", OFFSET(user_agent),
>
> one has '_', the other '-'
>
> --
> Clément B.
>

all right, i ignore the '_' . ignore this patch please.
Steven Liu Sept. 15, 2016, 2:34 p.m. UTC | #4 
2016-09-15 22:22 GMT+08:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:

> 2016-09-15 16:04 GMT+02:00 Steven Liu <lingjiujianke@gmail.com>:
> > double user-agent option, same option, remove one.
>
> Looking at the documentation, the patch is not ok as-is.
>
> I suggest to deprecate the option now, and to remove it after
> the next release.
>
> Carl Eugen
>
>
Carl Eugen's mean is apply this patch after the next release?
Clément Bœsch Sept. 15, 2016, 2:40 p.m. UTC | #5 
On Thu, Sep 15, 2016 at 10:34:37PM +0800, Steven Liu wrote:
> 2016-09-15 22:22 GMT+08:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 
> > 2016-09-15 16:04 GMT+02:00 Steven Liu <lingjiujianke@gmail.com>:
> > > double user-agent option, same option, remove one.
> >
> > Looking at the documentation, the patch is not ok as-is.
> >
> > I suggest to deprecate the option now, and to remove it after
> > the next release.
> >
> > Carl Eugen
> >
> >
> Carl Eugen's mean is apply this patch after the next release?

You need to deprecate the option with at least the classic #ifdefery dance
(git grep FF_API for examples), and ideally by also printing a warning
when using the wrong option.
Carl Eugen Hoyos Sept. 15, 2016, 3:03 p.m. UTC | #6 
2016-09-15 16:40 GMT+02:00 Clément Bœsch <u@pkh.me>:

>> Carl Eugen's mean is apply this patch after the next release?
>
> You need to deprecate the option with at least the classic #ifdefery dance
> (git grep FF_API for examples), and ideally by also printing a warning
> when using the wrong option.

Imo, it is completely sufficient to add a line to the filter documentation
(and print a warning if this is easily possible).

Carl Eugen
Steven Liu Sept. 15, 2016, 3:27 p.m. UTC | #7 
2016-09-15 23:03 GMT+08:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:

> 2016-09-15 16:40 GMT+02:00 Clément Bœsch <u@pkh.me>:
>
> >> Carl Eugen's mean is apply this patch after the next release?
> >
> > You need to deprecate the option with at least the classic #ifdefery
> dance
> > (git grep FF_API for examples), and ideally by also printing a warning
> > when using the wrong option.
>
> Imo, it is completely sufficient to add a line to the filter documentation
> (and print a warning if this is easily possible).
>
> Carl Eugen
>
>
user_agent and user-agent use the same variable 'user_agent', if print a
warning, maybe add a user variable for  user_agent option is a good way?
Clément Bœsch Sept. 15, 2016, 3:37 p.m. UTC | #8 
On Thu, Sep 15, 2016 at 11:27:02PM +0800, Steven Liu wrote:
> 2016-09-15 23:03 GMT+08:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 
> > 2016-09-15 16:40 GMT+02:00 Clément Bœsch <u@pkh.me>:
> >
> > >> Carl Eugen's mean is apply this patch after the next release?
> > >
> > > You need to deprecate the option with at least the classic #ifdefery
> > dance
> > > (git grep FF_API for examples), and ideally by also printing a warning
> > > when using the wrong option.
> >
> > Imo, it is completely sufficient to add a line to the filter documentation
> > (and print a warning if this is easily possible).
> >
> > Carl Eugen
> >
> >
> user_agent and user-agent use the same variable 'user_agent', if print a
> warning, maybe add a user variable for  user_agent option is a good way?

Yes, surrounded by the #ifdefery so it will go away with the option.

Note: use av_strdup() to transfer from one field to the other
Nicolas George Sept. 15, 2016, 3:39 p.m. UTC | #9 
Le decadi 30 fructidor, an CCXXIV, Clement Boesch a écrit :
> Yes, surrounded by the #ifdefery so it will go away with the option.
> 
> Note: use av_strdup() to transfer from one field to the other

Another possibility would be to implement AV_OPT_FLAG_DEPRECATED and let the
options system itself print the warning.

Regards,
Clément Bœsch Sept. 15, 2016, 3:42 p.m. UTC | #10 
On Thu, Sep 15, 2016 at 05:39:25PM +0200, Nicolas George wrote:
> Le decadi 30 fructidor, an CCXXIV, Clement Boesch a écrit :
> > Yes, surrounded by the #ifdefery so it will go away with the option.
> > 
> > Note: use av_strdup() to transfer from one field to the other
> 
> Another possibility would be to implement AV_OPT_FLAG_DEPRECATED and let the
> options system itself print the warning.
> 

That's not a bad idea; you might need to add a field to redirect to
the new option though.
diff mbox

Patch

diff --git a/libavformat/http.c b/libavformat/http.c
index adb3d92..f2bfb17 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -131,7 +131,6 @@  static const AVOption options[] = {
     { "headers", "set custom HTTP headers, can override built in default
headers", OFFSET(headers), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E
},
     { "content_type", "set a specific content type for the POST messages",
OFFSET(content_type), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E },
     { "user_agent", "override User-Agent header", OFFSET(user_agent),
AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },