git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Incorrect diff-parseopt conversion?
@ 2019-05-21 19:56 Ramsay Jones
  2019-05-22  0:11 ` Duy Nguyen
  0 siblings, 1 reply; 6+ messages in thread
From: Ramsay Jones @ 2019-05-21 19:56 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: GIT Mailing-list

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Incorrect diff-parseopt conversion?
  2019-05-21 19:56 Incorrect diff-parseopt conversion? Ramsay Jones
@ 2019-05-22  0:11 ` Duy Nguyen
  2019-05-22  0:35   ` Ramsay Jones
  0 siblings, 1 reply; 6+ messages in thread
From: Duy Nguyen @ 2019-05-22  0:11 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: GIT Mailing-list

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Incorrect diff-parseopt conversion?
  2019-05-22  0:11 ` Duy Nguyen
@ 2019-05-22  0:35   ` Ramsay Jones
  2019-05-24  9:40     ` Duy Nguyen
  0 siblings, 1 reply; 6+ messages in thread
From: Ramsay Jones @ 2019-05-22  0:35 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: GIT Mailing-list



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Incorrect diff-parseopt conversion?
  2019-05-22  0:35   ` Ramsay Jones
@ 2019-05-24  9:40     ` Duy Nguyen
  2019-05-24 10:08       ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 6+ messages in thread
From: Duy Nguyen @ 2019-05-24  9:40 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: GIT Mailing-list

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Incorrect diff-parseopt conversion?
  2019-05-24  9:40     ` Duy Nguyen
@ 2019-05-24 10:08       ` Ævar Arnfjörð Bjarmason
  2019-05-25 10:37         ` Duy Nguyen
  0 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-24 10:08 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Ramsay Jones, GIT Mailing-list


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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Incorrect diff-parseopt conversion?
  2019-05-24 10:08       ` Ævar Arnfjörð Bjarmason
@ 2019-05-25 10:37         ` Duy Nguyen
  0 siblings, 0 replies; 6+ messages in thread
From: Duy Nguyen @ 2019-05-25 10:37 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Ramsay Jones, GIT Mailing-list

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-05-25 10:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-21 19:56 Incorrect diff-parseopt conversion? Ramsay Jones
2019-05-22  0:11 ` Duy Nguyen
2019-05-22  0:35   ` Ramsay Jones
2019-05-24  9:40     ` Duy Nguyen
2019-05-24 10:08       ` Ævar Arnfjörð Bjarmason
2019-05-25 10:37         ` Duy Nguyen

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git