Re: [PATCH] Make builtin-tag.c use parse_options.

Carlos Rica [off-list ref] writes:

Also, this removes those tests ensuring that repeated
-m options don't allocate memory more than once, because now
this is done after parsing options, using the last one
when more are given. The same for -F.

The reason for this change is...?  Is this because it is
cumbersome to detect and refuse multiple -m options using the
parseopt API?  If so, the API may be what needs to be fixed.
Taking the last one and discarding earlier ones feels to me an
arbitrary choice.

While I freely admit that I do not particularly find the "One -m
introduces one new line, concatenated to form the final
paragraph" handling of multiple -m options done by git-commit
nice nor useful, I suspect that it would make more sense to make
git-tag and git-commit handle multiple -m option consistently,
if you are going to change the existing semantics.  Since some
people really seem to like multiple -m handling of git-commit,
the avenue of the least resistance for better consistency would
be to accept and concatenate (with LF in between) multiple -m
options.

With multiple -F, I think erroring out would be the sensible
thing to do, but some people might prefer concatenation.  I do
not care either way as long as commit and tag behave
consistently.

2007/11/10, Junio C Hamano [off-list ref]:

Carlos Rica [off-list ref] writes:

quoted

Also, this removes those tests ensuring that repeated
-m options don't allocate memory more than once, because now
this is done after parsing options, using the last one
when more are given. The same for -F.

The reason for this change is...?  Is this because it is
cumbersome to detect and refuse multiple -m options using the
parseopt API?  If so, the API may be what needs to be fixed.
Taking the last one and discarding earlier ones feels to me an
arbitrary choice.

While I freely admit that I do not particularly find the "One -m
introduces one new line, concatenated to form the final
paragraph" handling of multiple -m options done by git-commit
nice nor useful, I suspect that it would make more sense to make
git-tag and git-commit handle multiple -m option consistently,
if you are going to change the existing semantics.  Since some
people really seem to like multiple -m handling of git-commit,
the avenue of the least resistance for better consistency would
be to accept and concatenate (with LF in between) multiple -m
options.

With multiple -F, I think erroring out would be the sensible
thing to do, but some people might prefer concatenation.  I do
not care either way as long as commit and tag behave
consistently.

A solution not needing memory allocation into the option parser
could be setting a callback running over the repeated option
arguments, passing them to the function one per each call.
Then, the user will be able to decide if he wants the arguments
concatenated or only need one of them and prefers erroring out.

Is this already possible with the current parser or the callback
mode only calls using the last option?

On Sat, Nov 10, 2007 at 12:25:44PM +0000, Carlos Rica wrote:

2007/11/10, Junio C Hamano [off-list ref]:

quoted

Carlos Rica [off-list ref] writes:

A solution not needing memory allocation into the option parser
could be setting a callback running over the repeated option
arguments, passing them to the function one per each call.
Then, the user will be able to decide if he wants the arguments
concatenated or only need one of them and prefers erroring out.

Is this already possible with the current parser or the callback
mode only calls using the last option?

  Everything is possible, you just have to code it. With a callback
you have in the struct option two places to store "things". The void*
value pointer and the intptr_t defval. _Usually_ the void* is the
pointer to the data that will be _written_ and the defval the data that
will be put into the void* under some circumstances (e.g. when your
option is negated).

  For Your case I'd go with some kind of string list pointed into the
void * value, defval has no or little use. You don't really care about
allocating memory in the option parser, I mean, option parsing is done
once at the initialization phase. It's not evil. In pseudo-C here is how
I would write the callback:

int parse_opt_stringlist(const struct option *opt, const char *arg, int unset)
{
    string_list **l = opt->value;
    string_list_elem *e;

    if (unset) { /* negationg option clears the list */
	while (*l) {
	    string_list_elem_free(string_list_pop(l));
	}
	return 0;
    }

    e = string_list_elem_new();
    e->data = arg;
    string_list_push(l, e);
    return 0;
}

  And you're done, you can do what you want with that list from the caller.
There probably is such a structure in git, if not, it can probably be hacked
in a few lines.

  Remember, callbacks give you _full_ control on what you can do in the option
parser, and if you're not happy with Turing complete expressivity, there isn't
anything I can do for you :P Note that if you do write such a generic
callback, it belongs to parse-options.[hc].

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

2007/11/10, Junio C Hamano [off-list ref]:

Carlos Rica [off-list ref] writes:

quoted

Also, this removes those tests ensuring that repeated
-m options don't allocate memory more than once, because now
this is done after parsing options, using the last one
when more are given. The same for -F.

The reason for this change is...?  Is this because it is
cumbersome to detect and refuse multiple -m options using the
parseopt API?  If so, the API may be what needs to be fixed.
Taking the last one and discarding earlier ones feels to me an
arbitrary choice.

You can do many things with repeated options.
Here in git-tag we considered two different ways to manage them:
Concatenating values for the option and/or refusing more than one.
I found that current option-parser can do both from the client
using callbacks, as Pierre shows me, so I think it is the right way to do it.

Pierre, by default, I think that the parser should print an error
when more than one option of the same type is given,
in order to report it to the command-line user,
but make this behaviour optional for the programmer.
Specifically, I thought in this last option:

enum parse_opt_option_flags {
	PARSE_OPT_OPTARG  = 1,
	PARSE_OPT_NOARG   = 2,
	PARSE_OPT_ALLOWREP = 4
};

While I freely admit that I do not particularly find the "One -m
introduces one new line, concatenated to form the final
paragraph" handling of multiple -m options done by git-commit
nice nor useful, I suspect that it would make more sense to make
git-tag and git-commit handle multiple -m option consistently,
if you are going to change the existing semantics.  Since some
people really seem to like multiple -m handling of git-commit,
the avenue of the least resistance for better consistency would
be to accept and concatenate (with LF in between) multiple -m
options.

With multiple -F, I think erroring out would be the sensible
thing to do, but some people might prefer concatenation.  I do
not care either way as long as commit and tag behave
consistently.

Then, Kristian, what are you willing to do in such case?
It seems easier for me to concatenate of -m and -F options, even when
both types are given. I don't know why "people" want multiple -m options,
but I think that mixing -m and -F options could be interesting for them too.
If someone know if this have been discussed and decided already,
please give me the link.

On Mon, Nov 12, 2007 at 01:09:37PM +0000, Carlos Rica wrote:

2007/11/10, Junio C Hamano [off-list ref]:

quoted

Carlos Rica [off-list ref] writes:

quoted

Also, this removes those tests ensuring that repeated
-m options don't allocate memory more than once, because now
this is done after parsing options, using the last one
when more are given. The same for -F.

The reason for this change is...?  Is this because it is
cumbersome to detect and refuse multiple -m options using the
parseopt API?  If so, the API may be what needs to be fixed.
Taking the last one and discarding earlier ones feels to me an
arbitrary choice.

You can do many things with repeated options.
Here in git-tag we considered two different ways to manage them:
Concatenating values for the option and/or refusing more than one.
I found that current option-parser can do both from the client
using callbacks, as Pierre shows me, so I think it is the right way to do it.

Pierre, by default, I think that the parser should print an error
when more than one option of the same type is given,

  I beg to differ. It makes sense for OPTION_STRING options, but not for
other. Though you cannot always detect that.

Also note that:
(1) repeating options was already silent in many git commands, so it's
    not really a regression ;
(2) for many commands it actually make sense to allow repeating (for
    _BOOLEAN e.g.). And I'd argue that for OPTION_BIT it also makes
    sense as well.

in order to report it to the command-line user, but make this
behaviour optional for the programmer.  Specifically, I thought in
this last option:

enum parse_opt_option_flags {
	PARSE_OPT_OPTARG   = 1,
	PARSE_OPT_NOARG    = 2,
	PARSE_OPT_ALLOWREP = 4
};

  To do that you need to keep a list of the triggered commands to do
that, there is no way to achieve that reliably right now. As taking the
last one and discarding the other is the usual way for option parsers I
never saw this as a big issue.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Mon, 2007-11-12 at 14:09 +0100, Carlos Rica wrote:

2007/11/10, Junio C Hamano [off-list ref]:

quoted

Carlos Rica [off-list ref] writes:

...

Then, Kristian, what are you willing to do in such case?
It seems easier for me to concatenate of -m and -F options, even when
both types are given. I don't know why "people" want multiple -m options,
but I think that mixing -m and -F options could be interesting for them too.
If someone know if this have been discussed and decided already,
please give me the link.

I should be pretty easy to just append the contents of multiple fies,
even inter-mingled with -m options.  We just do a callback like Johannes
just did for -m in builtin-commit.c for -F and append to the same
strbuf.  strbuf_read() already appends, so the callback could look
something like:

static int opt_parse_F(const struct option *opt, const char *arg, int
unset)
{
        struct strbuf *buf = opt->value;

	if (!strcmp(arg, "-")) {
                if (isatty(0))
                        fprintf(stderr, "(reading log message from
standard input)\n");
                if (strbuf_read(&sb, 0, 0) < 0)
                        die("could not read log from standard input");
	} else {
                if (strbuf_read_file(&sb, logfile, 0) < 0)
                        die("could not read log file '%s': %s",
                            logfile, strerror(errno));
	}
}

Shouldn't be too hard :)
Kristian