Re: [PATCH] parse-options: fix the description of defval

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> writes:

> On Sun, Mar 29, 2015 at 4:32 PM, Ivan Ukhov <ivan.ukhov@gmail.com> wrote:
>> Since the deletion of OPT_SET_PTR, defval can no longer contain a pointer.
>>
>
> Actually, it can contain a pointer for OPTION_CMDMODE, OPTION_STRING
> and OPTION_FILENAME. Since we are on the topic of updating the
> documentation, I think it would be great if the documentation
> mentioned these option types as well.

Actually, both of you are correct ;-)

The patch text you are responding to is not saying anything wrong.
The only thing Ivan stated incorrectly is the log message.

    parse-options.h: OPTION_{BIT,SET_INT} do not store pointer to defval

    When 20d1c652 (parse-options: remove unused OPT_SET_PTR,
    2014-03-30) removed OPT_SET_PTR, the comment in the header that
    describes what the option did to defval field was left behind by
    mistake.  Remove it.

or something, perhaps?

It is a different issue if we want to describe uses of `defval` by
all other macros like OPTION_STRING.  We should make it easier for
our contributors (me included) to find how each option macros can be
used, and how OPTION_XYZ uses defval must be described somewhere,
but I personally think bloating the description of `defval` is not a
good way to do so.  Description of OPTION_XYZ may be the first place
for programmers to go to find how it should be used, so perhaps it
is a better idea to enrich descriptions there instead of here.

In other words, it may be an improvement to say only the general
principle shared across all uses e.g. "default value to fill .value
with", without mentioning specifics of exceptions (e.g. "for
OPTION_BIT it is not even a default, it is _the_ value") in this
section.  Instead, comment OPTION_BIT with "the defval field is used
to store the bitmask used to set/clear/flip" or something.

But as I said, that is a different issue.

> diff --git a/parse-options.h b/parse-options.h
> index 7940bc7..c71e9da 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -95,8 +95,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *
>   * `defval`::
>   *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
> - *   OPTION_{BIT,SET_INT} store the {mask,integer,pointer} to put in
> - *   the value when met.
> + *   OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
>   *   CALLBACKS can use it like they want.
>   */