Re: Incorrect diff-parseopt conversion?

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 24, 2019, 10:08 UTC
Message-ID: <87mujcf9l6.fsf@evledraar.gmail.com>
In-Reply-To: <CACsJy8AzY4EO_io_8R41uVDsbCj+_Fn8YOQ8e+JmAC27dFW7UA@mail.gmail.com>

On Fri, May 24 2019, Duy Nguyen wrote:

Show 37 quoted lines

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