{"thread":{"id":"51145","subject":"Incorrect diff-parseopt conversion?","startedAt":"2019-05-21T20:04:25Z","lastAt":"2019-05-25T10:39:10Z","messageCount":6,"participants":["Ramsay Jones","Duy Nguyen","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"376011","messageId":"82f79cd9-5273-b9a4-744d-f5fa95dd1f47@ramsayjones.plus.com","threadId":"51145","inReplyTo":null,"subject":"Incorrect diff-parseopt conversion?","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-05-21T19:56:49Z","receivedAt":"2019-05-21T20:04:25Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Duy,\n\nI am in the middle of rebasing a long running branch onto\ncurrent master (v2.22.0-rc1) and noticed something odd with\ncommit af2f368091 (\"diff-parseopt: convert --output-*\",\n2019-02-21).\n\nAs part of the branch I am rebasing, I have defined a new\nOPT_LL_CALLBACK() macro[1], which I had intended to apply to\nthe 'output' option to diff. However, commit af2f368091\ndefines that option thus:\n\n+ { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n+ N_(\"Output to a specific file\"),\n+ PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n\nNote that the 'option type' is given as OPTION_CALLBACK, not\nas OPTION_LOWLEVEL_CALLBACK. Is this intended?\n\nATB,\nRamsay Jones\n\n[1] Yes, the reason my branch is long running is because\nwe keep changing the same files! We have both defined new\nOPT_() macros, some with the same name ... ;-)\n\n\n"},{"id":"376024","messageId":"CACsJy8BHeVDDnEYXaOAPtEoesTVNCiPGSgnJYjtL6aYSU0-J5A@mail.gmail.com","threadId":"51145","inReplyTo":"82f79cd9-5273-b9a4-744d-f5fa95dd1f47@ramsayjones.plus.com","subject":"Re: Incorrect diff-parseopt conversion?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-22T00:11:27Z","receivedAt":"2019-05-22T00:11:56Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 22, 2019 at 2:56 AM Ramsay Jones\n wrote:\n>\n> Hi Duy,\n>\n> I am in the middle of rebasing a long running branch onto\n> current master (v2.22.0-rc1) and noticed something odd with\n> commit af2f368091 (\"diff-parseopt: convert --output-*\",\n> 2019-02-21).\n>\n> As part of the branch I am rebasing, I have defined a new\n> OPT_LL_CALLBACK() macro[1], which I had intended to apply to\n> the 'output' option to diff. However, commit af2f368091\n> defines that option thus:\n>\n> + { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n> + N_(\"Output to a specific file\"),\n> + PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n>\n> Note that the 'option type' is given as OPTION_CALLBACK, not\n> as OPTION_LOWLEVEL_CALLBACK. Is this intended?\n\nYeah I think this is correct (phew!).\n\nOPTION_CALLBACK can take two different callback types, the \"lowlevel\"\none allows more access to parseopt context data while still does\ncommon parsing for us. OPTION_LOWLEVEL_CALLBACK, on the other hand,\ncalls ll_callback directly and leaves it to the callback to handle\neverything. We want OPTION_CALLBACK here so we will not have to do all\nthe work, we just need extra info (the prefix to be specific).\n\n>\n> ATB,\n> Ramsay Jones\n>\n> [1] Yes, the reason my branch is long running is because\n> we keep changing the same files! We have both defined new\n> OPT_() macros, some with the same name ... ;-)\n>\n>\n\n\n-- \nDuy\n"},{"id":"376033","messageId":"791da98b-4065-e477-92cb-ceac80cc04e6@ramsayjones.plus.com","threadId":"51145","inReplyTo":"CACsJy8BHeVDDnEYXaOAPtEoesTVNCiPGSgnJYjtL6aYSU0-J5A@mail.gmail.com","subject":"Re: Incorrect diff-parseopt conversion?","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-05-22T00:35:39Z","receivedAt":"2019-05-22T00:35:44Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/05/2019 01:11, Duy Nguyen wrote:\n> On Wed, May 22, 2019 at 2:56 AM Ramsay Jones\n> wrote:\n>>\n>> Hi Duy,\n>>\n>> I am in the middle of rebasing a long running branch onto\n>> current master (v2.22.0-rc1) and noticed something odd with\n>> commit af2f368091 (\"diff-parseopt: convert --output-*\",\n>> 2019-02-21).\n>>\n>> As part of the branch I am rebasing, I have defined a new\n>> OPT_LL_CALLBACK() macro[1], which I had intended to apply to\n>> the 'output' option to diff. However, commit af2f368091\n>> defines that option thus:\n>>\n>> + { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n>> + N_(\"Output to a specific file\"),\n>> + PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n>>\n>> Note that the 'option type' is given as OPTION_CALLBACK, not\n>> as OPTION_LOWLEVEL_CALLBACK. Is this intended?\n> \n> Yeah I think this is correct (phew!).\n\nOK, I just had a look at the code in parse-options.c.\nHmm, somewhat ugly! :-D\n\nThanks.\n\nATB,\nRamsay Jones\n"},{"id":"376168","messageId":"CACsJy8AzY4EO_io_8R41uVDsbCj+_Fn8YOQ8e+JmAC27dFW7UA@mail.gmail.com","threadId":"51145","inReplyTo":"791da98b-4065-e477-92cb-ceac80cc04e6@ramsayjones.plus.com","subject":"Re: Incorrect diff-parseopt conversion?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-24T09:40:53Z","receivedAt":"2019-05-24T09:41:21Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 22, 2019 at 7:35 AM Ramsay Jones\n wrote:\n>\n>\n>\n> On 22/05/2019 01:11, Duy Nguyen wrote:\n> > On Wed, May 22, 2019 at 2:56 AM Ramsay Jones\n> > wrote:\n> >>\n> >> Hi Duy,\n> >>\n> >> I am in the middle of rebasing a long running branch onto\n> >> current master (v2.22.0-rc1) and noticed something odd with\n> >> commit af2f368091 (\"diff-parseopt: convert --output-*\",\n> >> 2019-02-21).\n> >>\n> >> As part of the branch I am rebasing, I have defined a new\n> >> OPT_LL_CALLBACK() macro[1], which I had intended to apply to\n> >> the 'output' option to diff. However, commit af2f368091\n> >> defines that option thus:\n> >>\n> >> + { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n> >> + N_(\"Output to a specific file\"),\n> >> + PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n> >>\n> >> Note that the 'option type' is given as OPTION_CALLBACK, not\n> >> as OPTION_LOWLEVEL_CALLBACK. Is this intended?\n> >\n> > Yeah I think this is correct (phew!).\n>\n> OK, I just had a look at the code in parse-options.c.\n> Hmm, somewhat ugly! :-D\n\nYeah it's a bit hackish. The problem is parse-options (and also config\nparser) does not allow passing user data to the callback. Changing it\nwould affect all callsites at once. It's just too scary for me to do\nit and I chose an uglier way but with smaller impact.\n-- \nDuy\n"},{"id":"376173","messageId":"87mujcf9l6.fsf@evledraar.gmail.com","threadId":"51145","inReplyTo":"CACsJy8AzY4EO_io_8R41uVDsbCj+_Fn8YOQ8e+JmAC27dFW7UA@mail.gmail.com","subject":"Re: Incorrect diff-parseopt conversion?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-24T10:08:53Z","receivedAt":"2019-05-24T10:08:57Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 24 2019, Duy Nguyen wrote:\n\n> On Wed, May 22, 2019 at 7:35 AM Ramsay Jones\n> wrote:\n>>\n>>\n>>\n>> On 22/05/2019 01:11, Duy Nguyen wrote:\n>> > On Wed, May 22, 2019 at 2:56 AM Ramsay Jones\n>> > wrote:\n>> >>\n>> >> Hi Duy,\n>> >>\n>> >> I am in the middle of rebasing a long running branch onto\n>> >> current master (v2.22.0-rc1) and noticed something odd with\n>> >> commit af2f368091 (\"diff-parseopt: convert --output-*\",\n>> >> 2019-02-21).\n>> >>\n>> >> As part of the branch I am rebasing, I have defined a new\n>> >> OPT_LL_CALLBACK() macro[1], which I had intended to apply to\n>> >> the 'output' option to diff. However, commit af2f368091\n>> >> defines that option thus:\n>> >>\n>> >> + { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n>> >> + N_(\"Output to a specific file\"),\n>> >> + PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n>> >>\n>> >> Note that the 'option type' is given as OPTION_CALLBACK, not\n>> >> as OPTION_LOWLEVEL_CALLBACK. Is this intended?\n>> >\n>> > Yeah I think this is correct (phew!).\n>>\n>> OK, I just had a look at the code in parse-options.c.\n>> Hmm, somewhat ugly! :-D\n>\n> Yeah it's a bit hackish. The problem is parse-options (and also config\n> parser) does not allow passing user data to the callback. Changing it\n> would affect all callsites at once. It's just too scary for me to do\n> it and I chose an uglier way but with smaller impact.\n\nAs an aside because I've sometimes wanted to pass such arbitrary data to\nthe callback.\n\nDo you mean it's painful to change \"struct option\" because of all the\nmacros around it?\n\nInstead of all callsites, wouldn't that just be a patch simliar to\nbf3ff338a2 (\"parse-options: stop abusing 'callback' for lowlevel\ncallbacks\", 2019-01-27)?\n\nI.e. a special callback that'll be called to return a void* to arbitrary\ndata, and then we'll call the callback with that.\n\nOr, just passing it around in a global variable, since we know the flow\nof when we setup the data, and when parse_options() is called.\n"},{"id":"376213","messageId":"CACsJy8CrTEAHesK3ZpwLJqu4fCvMGcW3YH-AkSYN79Db_KmdtQ@mail.gmail.com","threadId":"51145","inReplyTo":"87mujcf9l6.fsf@evledraar.gmail.com","subject":"Re: Incorrect diff-parseopt conversion?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-25T10:37:02Z","receivedAt":"2019-05-25T10:39:10Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 24, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason\n wrote:\n> >> OK, I just had a look at the code in parse-options.c.\n> >> Hmm, somewhat ugly! :-D\n> >\n> > Yeah it's a bit hackish. The problem is parse-options (and also config\n> > parser) does not allow passing user data to the callback. Changing it\n> > would affect all callsites at once. It's just too scary for me to do\n> > it and I chose an uglier way but with smaller impact.\n>\n> As an aside because I've sometimes wanted to pass such arbitrary data to\n> the callback.\n>\n> Do you mean it's painful to change \"struct option\" because of all the\n> macros around it?\n\nNo, it's updating all the callback functions to take a new parameter.\n\n> Instead of all callsites, wouldn't that just be a patch simliar to\n> bf3ff338a2 (\"parse-options: stop abusing 'callback' for lowlevel\n> callbacks\", 2019-01-27)?\n>\n> I.e. a special callback that'll be called to return a void* to arbitrary\n> data, and then we'll call the callback with that.\n\nThat allows more gradual conversion, but we'll add a bunch new OPT_\nmacros. Not exactly nice.\n\nIf we do this, I'd rather go with converting all callback functions at\nonce. Though we probably want to avoid going through the same pain\nagain, by passing a single struct instead, e.g.\n\nint some_callback(const struct option *opt, const struct optval *val);\n\n\"arg\" and \"unset\" are in this \"val\", callback data is in it too. And\nit may help reduce those -Wunused-paramters that Jeff's trying to\nkill.\n\n> Or, just passing it around in a global variable, since we know the flow\n> of when we setup the data, and when parse_options() is called.\n\nThis might work fine with parse_options() at top level (e.g. cmd_xx\nfunctions) but apply.c, diff.c and other should not really depend on\nit. And we could almost avoid that with parse_opt_ctx_t.\n-- \nDuy\n"}]}