Incorrect diff-parseopt conversion?

Incorrect diff-parseopt conversion?

[Ramsay Jones]

Hi Duy,

I am in the middle of rebasing a long running branch onto
current master (v2.22.0-rc1) and noticed something odd with
commit af2f368091 ("diff-parseopt: convert --output-*",
2019-02-21).

As part of the branch I am rebasing, I have defined a new
OPT_LL_CALLBACK() macro[1], which I had intended to apply to
the 'output' option to diff. However, commit af2f368091
defines that option thus:

+               { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
+                 N_("Output to a specific file"),
+                 PARSE_OPT_NONEG, NULL, 0, diff_opt_output },

Note that the 'option type' is given as OPTION_CALLBACK, not
as OPTION_LOWLEVEL_CALLBACK. Is this intended?

ATB,
Ramsay Jones

[1] Yes, the reason my branch is long running is because
we keep changing the same files! We have both defined new
OPT_() macros, some with the same name ... ;-)

Re: Incorrect diff-parseopt conversion?

[Duy Nguyen]

On Wed, May 22, 2019 at 2:56 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Hi Duy,
>
> I am in the middle of rebasing a long running branch onto
> current master (v2.22.0-rc1) and noticed something odd with
> commit af2f368091 ("diff-parseopt: convert --output-*",
> 2019-02-21).
>
> As part of the branch I am rebasing, I have defined a new
> OPT_LL_CALLBACK() macro[1], which I had intended to apply to
> the 'output' option to diff. However, commit af2f368091
> defines that option thus:
>
> +               { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
> +                 N_("Output to a specific file"),
> +                 PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
>
> Note that the 'option type' is given as OPTION_CALLBACK, not
> as OPTION_LOWLEVEL_CALLBACK. Is this intended?

Yeah I think this is correct (phew!).

OPTION_CALLBACK can take two different callback types, the "lowlevel"
one allows more access to parseopt context data while still does
common parsing for us. OPTION_LOWLEVEL_CALLBACK, on the other hand,
calls ll_callback directly and leaves it to the callback to handle
everything. We want OPTION_CALLBACK here so we will not have to do all
the work, we just need extra info (the prefix to be specific).

>
> ATB,
> Ramsay Jones
>
> [1] Yes, the reason my branch is long running is because
> we keep changing the same files! We have both defined new
> OPT_() macros, some with the same name ... ;-)
>
>


-- 
Duy

Re: Incorrect diff-parseopt conversion?

[Ramsay Jones]

On 22/05/2019 01:11, Duy Nguyen wrote:
> On Wed, May 22, 2019 at 2:56 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>> Hi Duy,
>>
>> I am in the middle of rebasing a long running branch onto
>> current master (v2.22.0-rc1) and noticed something odd with
>> commit af2f368091 ("diff-parseopt: convert --output-*",
>> 2019-02-21).
>>
>> As part of the branch I am rebasing, I have defined a new
>> OPT_LL_CALLBACK() macro[1], which I had intended to apply to
>> the 'output' option to diff. However, commit af2f368091
>> defines that option thus:
>>
>> +               { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
>> +                 N_("Output to a specific file"),
>> +                 PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
>>
>> Note that the 'option type' is given as OPTION_CALLBACK, not
>> as OPTION_LOWLEVEL_CALLBACK. Is this intended?
> 
> Yeah I think this is correct (phew!).

OK, I just had a look at the code in parse-options.c.
Hmm, somewhat ugly! :-D

Thanks.

ATB,
Ramsay Jones

Re: Incorrect diff-parseopt conversion?

[Duy Nguyen]

On Wed, May 22, 2019 at 7:35 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
>
> On 22/05/2019 01:11, Duy Nguyen wrote:
> > On Wed, May 22, 2019 at 2:56 AM Ramsay Jones
> > <ramsay@ramsayjones.plus.com> wrote:
> >>
> >> Hi Duy,
> >>
> >> I am in the middle of rebasing a long running branch onto
> >> current master (v2.22.0-rc1) and noticed something odd with
> >> commit af2f368091 ("diff-parseopt: convert --output-*",
> >> 2019-02-21).
> >>
> >> As part of the branch I am rebasing, I have defined a new
> >> OPT_LL_CALLBACK() macro[1], which I had intended to apply to
> >> the 'output' option to diff. However, commit af2f368091
> >> defines that option thus:
> >>
> >> +               { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
> >> +                 N_("Output to a specific file"),
> >> +                 PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
> >>
> >> Note that the 'option type' is given as OPTION_CALLBACK, not
> >> as OPTION_LOWLEVEL_CALLBACK. Is this intended?
> >
> > Yeah I think this is correct (phew!).
>
> OK, I just had a look at the code in parse-options.c.
> Hmm, somewhat ugly! :-D

Yeah it's a bit hackish. The problem is parse-options (and also config
parser) does not allow passing user data to the callback. Changing it
would affect all callsites at once. It's just too scary for me to do
it and I chose an uglier way but with smaller impact.
-- 
Duy

Re: Incorrect diff-parseopt conversion?

[Ævar Arnfjörð Bjarmason]

On Fri, May 24 2019, Duy Nguyen wrote:

> On Wed, May 22, 2019 at 7:35 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>
>>
>> On 22/05/2019 01:11, Duy Nguyen wrote:
>> > On Wed, May 22, 2019 at 2:56 AM Ramsay Jones
>> > <ramsay@ramsayjones.plus.com> wrote:
>> >>
>> >> Hi Duy,
>> >>
>> >> I am in the middle of rebasing a long running branch onto
>> >> current master (v2.22.0-rc1) and noticed something odd with
>> >> commit af2f368091 ("diff-parseopt: convert --output-*",
>> >> 2019-02-21).
>> >>
>> >> As part of the branch I am rebasing, I have defined a new
>> >> OPT_LL_CALLBACK() macro[1], which I had intended to apply to
>> >> the 'output' option to diff. However, commit af2f368091
>> >> defines that option thus:
>> >>
>> >> +               { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
>> >> +                 N_("Output to a specific file"),
>> >> +                 PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
>> >>
>> >> Note that the 'option type' is given as OPTION_CALLBACK, not
>> >> as OPTION_LOWLEVEL_CALLBACK. Is this intended?
>> >
>> > Yeah I think this is correct (phew!).
>>
>> OK, I just had a look at the code in parse-options.c.
>> Hmm, somewhat ugly! :-D
>
> Yeah it's a bit hackish. The problem is parse-options (and also config
> parser) does not allow passing user data to the callback. Changing it
> would affect all callsites at once. It's just too scary for me to do
> it and I chose an uglier way but with smaller impact.

As an aside because I've sometimes wanted to pass such arbitrary data to
the callback.

Do you mean it's painful to change "struct option" because of all the
macros around it?

Instead of all callsites, wouldn't that just be a patch simliar to
bf3ff338a2 ("parse-options: stop abusing 'callback' for lowlevel
callbacks", 2019-01-27)?

I.e. a special callback that'll be called to return a void* to arbitrary
data, and then we'll call the callback with that.

Or, just passing it around in a global variable, since we know the flow
of when we setup the data, and when parse_options() is called.

Re: Incorrect diff-parseopt conversion?

[Duy Nguyen]

On Fri, May 24, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> >> OK, I just had a look at the code in parse-options.c.
> >> Hmm, somewhat ugly! :-D
> >
> > Yeah it's a bit hackish. The problem is parse-options (and also config
> > parser) does not allow passing user data to the callback. Changing it
> > would affect all callsites at once. It's just too scary for me to do
> > it and I chose an uglier way but with smaller impact.
>
> As an aside because I've sometimes wanted to pass such arbitrary data to
> the callback.
>
> Do you mean it's painful to change "struct option" because of all the
> macros around it?

No, it's updating all the callback functions to take a new parameter.

> Instead of all callsites, wouldn't that just be a patch simliar to
> bf3ff338a2 ("parse-options: stop abusing 'callback' for lowlevel
> callbacks", 2019-01-27)?
>
> I.e. a special callback that'll be called to return a void* to arbitrary
> data, and then we'll call the callback with that.

That allows more gradual conversion, but we'll add a bunch new OPT_
macros. Not exactly nice.

If we do this, I'd rather go with converting all callback functions at
once. Though we probably want to avoid going through the same pain
again, by passing a single struct instead, e.g.

int some_callback(const struct option *opt, const struct optval *val);

"arg" and "unset" are in this "val", callback data is in it too. And
it may help reduce those -Wunused-paramters that Jeff's trying to
kill.

> Or, just passing it around in a global variable, since we know the flow
> of when we setup the data, and when parse_options() is called.

This might work fine with parse_options() at top level (e.g. cmd_xx
functions) but apply.c, diff.c and other should not really depend on
it. And we could almost avoid that with parse_opt_ctx_t.
-- 
Duy