[RFH] convert shortlog to use parse_options

[RFH] convert shortlog to use parse_options

[Jeff King]

Pierre,

I am converting shortlog to use parse_options, but I have hit a behavior
I can't seem to represent.

The "-w" option has an optional argument, and we used to accept only
"-w" or "-wX,Y,Z". However, parse_options allows "-w X,Y,Z".

This means that "-w HEAD" used to mean "turn on wrapping
with default parameters, look at HEAD" and now translates to
"-w with parameter HEAD".

Is there a way to do this currently, or can we add an option flag?

---
 builtin-shortlog.c |  111 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 3d8d709..37fdb45 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -6,9 +6,12 @@
 #include "revision.h"
 #include "utf8.h"
 #include "mailmap.h"
+#include "parse-options.h"
 
-static const char shortlog_usage[] =
-"git-shortlog [-n] [-s] [<commit-id>... ]";
+static const char * const shortlog_usage[] = {
+	"git-shortlog [-n] [-s] [-e] [<commit-id>... ]",
+	NULL
+};
 
 static char *common_repo_prefix;
 static int email;
@@ -181,34 +184,45 @@ static int parse_uint(char const **arg, int comma)
 }
 
 static const char wrap_arg_usage[] = "-w[<width>[,<indent1>[,<indent2>]]]";
+struct wrap_arg {
+	int enabled;
+	int len;
+	int in1;
+	int in2;
+};
 #define DEFAULT_WRAPLEN 76
 #define DEFAULT_INDENT1 6
 #define DEFAULT_INDENT2 9
 
-static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)
+static int parse_wrap(const struct option *opt, const char *arg, int unset)
 {
-	arg += 2; /* skip -w */
+	struct wrap_arg *wrap = opt->value;
 
-	*wrap = parse_uint(&arg, ',');
-	if (*wrap < 0)
+	wrap->enabled = 1;
+
+	if (!arg)
+		return 0;
+	wrap->len = parse_uint(&arg, ',');
+	if (wrap->len < 0)
 		die(wrap_arg_usage);
-	*in1 = parse_uint(&arg, ',');
-	if (*in1 < 0)
+	wrap->in1 = parse_uint(&arg, ',');
+	if (wrap->in1 < 0)
 		die(wrap_arg_usage);
-	*in2 = parse_uint(&arg, '\0');
-	if (*in2 < 0)
+	wrap->in2 = parse_uint(&arg, '\0');
+	if (wrap->in2 < 0)
 		die(wrap_arg_usage);
 
-	if (!*wrap)
-		*wrap = DEFAULT_WRAPLEN;
-	if (!*in1)
-		*in1 = DEFAULT_INDENT1;
-	if (!*in2)
-		*in2 = DEFAULT_INDENT2;
-	if (*wrap &&
-	    ((*in1 && *wrap <= *in1) ||
-	     (*in2 && *wrap <= *in2)))
+	if (!wrap->len)
+		wrap->len = DEFAULT_WRAPLEN;
+	if (!wrap->in1)
+		wrap->in1 = DEFAULT_INDENT1;
+	if (!wrap->in2)
+		wrap->in2 = DEFAULT_INDENT2;
+	if (wrap->len &&
+	    ((wrap->in1 && wrap->len <= wrap->in1) ||
+	     (wrap->in2 && wrap->len <= wrap->in2)))
 		die(wrap_arg_usage);
+	return 0;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -216,34 +230,31 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 	struct path_list list = { NULL, 0, 0, 1 };
 	int i, j, sort_by_number = 0, summary = 0;
-	int wrap_lines = 0;
-	int wrap = DEFAULT_WRAPLEN;
-	int in1 = DEFAULT_INDENT1;
-	int in2 = DEFAULT_INDENT2;
-
-	/* since -n is a shadowed rev argument, parse our args first */
-	while (argc > 1) {
-		if (!strcmp(argv[1], "-n") || !strcmp(argv[1], "--numbered"))
-			sort_by_number = 1;
-		else if (!strcmp(argv[1], "-s") ||
-				!strcmp(argv[1], "--summary"))
-			summary = 1;
-		else if (!strcmp(argv[1], "-e") ||
-			 !strcmp(argv[1], "--email"))
-			email = 1;
-		else if (!prefixcmp(argv[1], "-w")) {
-			wrap_lines = 1;
-			parse_wrap_args(argv[1], &in1, &in2, &wrap);
-		}
-		else if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
-			usage(shortlog_usage);
-		else
-			break;
-		argv++;
-		argc--;
-	}
+	struct wrap_arg wrap = {
+		0,
+		DEFAULT_WRAPLEN,
+		DEFAULT_INDENT1,
+		DEFAULT_INDENT2
+	};
+	struct option options[] = {
+		OPT_BOOLEAN('n', "numbered", &sort_by_number, "sort by number"),
+		OPT_BOOLEAN('s', "summary", &summary,
+				"show commit count summary"),
+		OPT_BOOLEAN('e', "email", &email, "show email addresses"),
+		{
+			OPTION_CALLBACK, 'w', NULL, &wrap, "wrap",
+			"line-wrap commit message", PARSE_OPT_OPTARG,
+			parse_wrap
+		},
+	};
+
+	argc = parse_options(argc, argv, options, shortlog_usage, 0);
+
 	init_revisions(&rev, prefix);
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	/* setup_revisions expects to have the program name in argv[0],
+	 * but parse_options has removed it. We have to do parse_options
+	 * before setup_revisions, though, to claim "-n". */
+	argc = setup_revisions(argc+1, argv-1, &rev, NULL);
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
@@ -272,9 +283,11 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 			for (j = onelines->nr - 1; j >= 0; j--) {
 				const char *msg = onelines->items[j].path;
 
-				if (wrap_lines) {
-					int col = print_wrapped_text(msg, in1, in2, wrap);
-					if (col != wrap)
+				if (wrap.enabled) {
+					int col = print_wrapped_text(msg,
+							wrap.in1, wrap.in2,
+							wrap.len);
+					if (col != wrap.len)
 						putchar('\n');
 				}
 				else
-- 
1.5.4.rc0.1088.gfa79c-dirty

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

On Thu, Dec 13, 2007 at 05:52:26AM +0000, Jeff King wrote:
> Pierre,
> 
> I am converting shortlog to use parse_options, but I have hit a behavior
> I can't seem to represent.
> 
> The "-w" option has an optional argument, and we used to accept only
> "-w" or "-wX,Y,Z". However, parse_options allows "-w X,Y,Z".
> 
> This means that "-w HEAD" used to mean "turn on wrapping
> with default parameters, look at HEAD" and now translates to
> "-w with parameter HEAD".
> 
> Is there a way to do this currently, or can we add an option flag?

No we can't. And I believe that such a thing is definitely bad practice
:/ So if you really need to, we will have to add some PARSE_OPT_STICKARG
or sth alike that would check that the argument was "sticked" to the
option either with `-wA,B,C` or `--long-opt=A,B,C` depending on the fact
that an option is short or long.

Though: `git shortlog -w -- HEAD` will work properly because option
arguments don't take the next token as an argument thing if it starts
with a dash.

Though note that you can't migrate things that use init_revisions and so
on to parseoptions yet, because revisions also have dashed tokens (--not
e.g.) and that the first run of parse_options will just hate it and
fail. Maybe we can do parse_options work in multiple passes though, but
that would require a quite extensive rethought of the module, the
introduction of a parseopt context to be freed after the last pass
(because we will have a lot of small allocation stuff going on). I'll
try to see what I can do in that direction.

For that we must migrate diff and revisions option parser as big macros
(I started[0] it but didn't had the time to complete it yet, and it's
quite a huge task, because there is no incremental upgrade path here,
and there are commands using both diff and revisions options so we must
migrate both at once) and use aggregated parseoptions specifiers.

  [0] http://git.madism.org/?p=git.git;a=commitdiff;h=059cfb6d4cfbdff68d81577d00c9dbce6fed443e

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Jeff King]

On Thu, Dec 13, 2007 at 10:06:04AM +0100, Pierre Habouzit wrote:

> No we can't. And I believe that such a thing is definitely bad practice
> :/ So if you really need to, we will have to add some PARSE_OPT_STICKARG
> or sth alike that would check that the argument was "sticked" to the
> option either with `-wA,B,C` or `--long-opt=A,B,C` depending on the fact
> that an option is short or long.

Yes, I am not sure if the right solution is to just say "we are changing
how -w works". Because it either must change, or it must be inconsistent
with the rest of the option parsing for the rest of eternity.

> Though note that you can't migrate things that use init_revisions and so
> on to parseoptions yet, because revisions also have dashed tokens (--not
> e.g.) and that the first run of parse_options will just hate it and

Ah, yes. I hadn't really considered that angle yet (my basic testing had
just been on non-dashed revision parameters).

I will table this patch for now, then, since it obviously is too complex
for v1.5.4.

Thanks for your input.

-Peff

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On jeu, déc 13, 2007 at 09:06:04 +0000, Pierre Habouzit wrote:
> fail. Maybe we can do parse_options work in multiple passes though, but
> that would require a quite extensive rethought of the module, the

  In fact it's not doable because of short options. I knew I already had
that idea but dropped it, the reason is that if you have the atom:

  -ab

  That the first pass knows about a -b, and that the second pass knows
about -a, then you have a problem. Because at soon you meet a flag that
you don't know about, you cannot parse any further. So we're back at
what I said, we must migrate revs and diff options parsing first, and
that's a huge task.


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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

On Thu, Dec 13, 2007 at 09:10:56AM +0000, Jeff King wrote:
> On Thu, Dec 13, 2007 at 10:06:04AM +0100, Pierre Habouzit wrote:
> 
> > No we can't. And I believe that such a thing is definitely bad practice
> > :/ So if you really need to, we will have to add some PARSE_OPT_STICKARG
> > or sth alike that would check that the argument was "sticked" to the
> > option either with `-wA,B,C` or `--long-opt=A,B,C` depending on the fact
> > that an option is short or long.
> 
> Yes, I am not sure if the right solution is to just say "we are changing
> how -w works". Because it either must change, or it must be inconsistent
> with the rest of the option parsing for the rest of eternity.

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case. Because this kind of backward incompatibility I totally missed in
the first place is unacceptable. Patch on its way.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/2] parseopt: Enforce the use of the sticked form for optional arguments.

[Pierre Habouzit]

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index e12b428..7a08a0c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -89,7 +89,7 @@ static int get_value(struct optparse_t *p,
 			*(const char **)opt->value = NULL;
 			return 0;
 		}
-		if (opt->flags & PARSE_OPT_OPTARG && (!arg || *arg == '-')) {
+		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
 			*(const char **)opt->value = (const char *)opt->defval;
 			return 0;
 		}
@@ -103,7 +103,7 @@ static int get_value(struct optparse_t *p,
 			return (*opt->callback)(opt, NULL, 1);
 		if (opt->flags & PARSE_OPT_NOARG)
 			return (*opt->callback)(opt, NULL, 0);
-		if (opt->flags & PARSE_OPT_OPTARG && (!arg || *arg == '-'))
+		if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
 			return (*opt->callback)(opt, NULL, 0);
 		if (!arg)
 			return opterror(opt, "requires a value", flags);
@@ -114,7 +114,7 @@ static int get_value(struct optparse_t *p,
 			*(int *)opt->value = 0;
 			return 0;
 		}
-		if (opt->flags & PARSE_OPT_OPTARG && (!arg || !isdigit(*arg))) {
+		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
 			*(int *)opt->value = opt->defval;
 			return 0;
 		}
-- 
1.5.3.7.2249.g89c99-dirty

[PATCH 2/2] parseopt: Add a gitcli(5) man page.

[Pierre Habouzit]

This page should hold every information about the git ways to parse command
lines, and best practices to be used for scripting.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/Makefile   |    2 +-
 Documentation/gitcli.txt |  104 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/gitcli.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 37ec355..f58f947 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -2,7 +2,7 @@ MAN1_TXT= \
 	$(filter-out $(addsuffix .txt, $(ARTICLES) $(SP_ARTICLES)), \
 		$(wildcard git-*.txt)) \
 	gitk.txt
-MAN5_TXT=gitattributes.txt gitignore.txt gitmodules.txt
+MAN5_TXT=gitattributes.txt gitignore.txt gitcli.txt gitmodules.txt
 MAN7_TXT=git.txt
 
 MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
new file mode 100644
index 0000000..3b3f5e4
--- /dev/null
+++ b/Documentation/gitcli.txt
@@ -0,0 +1,104 @@
+gitcli(5)
+=========
+
+NAME
+----
+gitcli - git command line interface and its usual conventions
+
+SYNOPSIS
+--------
+gitcli
+
+
+DESCRIPTION
+-----------
+This manual intends to describe best practice in how to use git CLI.  Here are
+the rules that you should follow when you are scripting git:
+
+ * it's preferred to use the non dashed form of git commands, which means that
+   you should prefer `"git foo"` to `"git-foo"`.
+
+ * splitting short option switches in separate atoms (prefer `"git foo -a -b"`
+   to `"git foo -ab"`, the latter may not even work).
+
+ * when a command line switch takes an argument, use the 'sticked' form, which
+   means that you must prefer `"git foo -oArg"` to `"git foo -o Arg"` for short
+   option switches, and `"git foo --long-opt=Arg"` to `"git foo --long-opt Arg"`
+   for long switches.
+
+
+ENHANCED CLI
+------------
+From the git 1.5.4 series and further, git commands (not all of them at the
+time of the writing though) come with an enhanced option parser with nice
+facilities. Here is an exhaustive list of them
+
+Magic Options
+~~~~~~~~~~~~~
+Commands which have the enhanced option parser activated all understand a
+couple of magic command line switches:
+
+-h::
+	gives a pretty printed usage of the command.
++
+---------------------------------------------
+$ git describe -h
+usage: git-describe [options] <committish>*
+
+    --contains            find the tag that comes after the commit
+    --debug               debug search strategy on stderr
+    --all                 use any ref in .git/refs
+    --tags                use any tag in .git/refs/tags
+    --abbrev [<n>]        use <n> digits to display SHA-1s
+    --candidates <n>      consider <n> most recent tags (default: 10)
+---------------------------------------------
+
+--help-all::
+	Some git commands takes options that are only used for plumbing or that
+	are deprecated, and such options are hidden from the default usage. This
+	switch gives the full list of options.
+
+
+Negating options
+~~~~~~~~~~~~~~~~
+Another things to keep in mind is that long options can be negated. For
+example, `"git branch"` has the option `"--track"` which is 'on' by default. You
+can use `"--no-track"` to override that behaviour. The same goes for `"--color"`
+and `"--no-color"`.
+
+
+Aggregating short options
+~~~~~~~~~~~~~~~~~~~~~~~~~
+Commands that support the enhanced option parser allow you to aggregate short
+options. This means that you can for example use `"git rm -rf"` or
+`"git clean -fdx"`.
+
+
+Separating argument from the switch
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Also for option switches that take a mandatory argument, you can separate it
+from the switch. That means that all the following uses are correct:
+
+----------------------------
+$ git foo --long-opt=Arg
+$ git foo --long-opt Arg
+$ git foo -oArg
+$ git foo -o Arg
+----------------------------
+
+However, this is *NOT* possible for switches with an optionnal value, where the
+'sticked' form must be used:
+----------------------------
+$ git describe --abbrev HEAD     # correct
+$ git describe --abbrev=10 HEAD  # correct
+$ git describe --abbrev 10 HEAD  # NOT WHAT YOU MEANT
+----------------------------
+
+
+Documentation
+-------------
+Documentation by Pierre Habouzit.
+
+GIT
+---
+Part of the gitlink:git[7] suite
-- 
1.5.3.7.2249.g89c99-dirty

Re: [PATCH 2/2] parseopt: Add a gitcli(5) man page.

[Wincent Colaiuta]

El 13/12/2007, a las 11:27, Pierre Habouzit escribió:

> This page should hold every information about the git ways to parse  
> command
> lines, and best practices to be used for scripting.

Some feedback from a native English speaker follows...

> @@ -0,0 +1,104 @@
> +gitcli(5)
> +=========
> +
> +NAME
> +----
> +gitcli - git command line interface and its usual conventions

"git command line interface and conventions" sounds better; the  
"usual" is redundant.

Or did you mean "*usage* conventions"? If that is the case, "git  
command line interface and usage" is better, but just "git command  
line interface" is enough.

> +DESCRIPTION
> +-----------
> +This manual intends to describe best practice in how to use git  
> CLI.  Here are
> +the rules that you should follow when you are scripting git:

Suggest "how to use the git CLI".

> + * it's preferred to use the non dashed form of git commands, which  
> means that
> +   you should prefer `"git foo"` to `"git-foo"`.

"non-dashed", and in any case, this could be more concise. How about:

* the non-dashed form of git commands is preferred; use `"git foo"`  
rather than
  `"git-foo"`

> + * splitting short option switches in separate atoms (prefer `"git  
> foo -a -b"`
> +   to `"git foo -ab"`, the latter may not even work).

"*split* short option switches *into* separate atoms"

And the comma before "the latter may not even work" should be a semi- 
colon.

> + * when a command line switch takes an argument, use the 'sticked'  
> form, which
> +   means that you must prefer `"git foo -oArg"` to `"git foo -o  
> Arg"` for short
> +   option switches, and `"git foo --long-opt=Arg"` to `"git foo -- 
> long-opt Arg"`
> +   for long switches.

Again this could be more concise. Instead of:

	"which means that you must prefer .... to ..."

you could just say:

	"use ... instead of ..."

> +ENHANCED CLI
> +------------
> +From the git 1.5.4 series and further, git commands (not all of  
> them at the
> +time of the writing though) come with an enhanced option parser  
> with nice
> +facilities. Here is an exhaustive list of them

How about:

"From git 1.5.4 onwards, many git commands come with an enhanced  
option parser..."

> +Magic Options
> +~~~~~~~~~~~~~
> +Commands which have the enhanced option parser activated all  
> understand a
> +couple of magic command line switches:

"Commands which use the enhanced option parser all understand..."

> +
> +-h::
> +	gives a pretty printed usage of the command.

"pretty-printed"

> +--help-all::
> +	Some git commands takes options that are only used for plumbing or  
> that
> +	are deprecated, and such options are hidden from the default  
> usage. This
> +	switch gives the full list of options.

"Some git commands *take* options that are deprecated or used only  
*by* plumbing"

And:

"such options are *not included in* the default usage" ("hidden from"  
sounds awkward).

> +Negating options
> +~~~~~~~~~~~~~~~~
> +Another things to keep in mind is that long options can be negated.

"Another *thing*"

But you could replace the whole sentence with just:

"Long options can be negated."

> +Separating argument from the switch
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

"Separating *the* argument from the switch"

Or if you prefer

"Separating *arguments* from the *switches*"

> +However, this is *NOT* possible for switches with an optionnal  
> value, where the
> +'sticked' form must be used:

Typo: "optional"

>
Cheers,
Wincent

Re: [PATCH 2/2] parseopt: Add a gitcli(5) man page.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On jeu, déc 13, 2007 at 11:04:08 +0000, Wincent Colaiuta wrote:
> El 13/12/2007, a las 11:27, Pierre Habouzit escribió:
> 
> >This page should hold every information about the git ways to parse 
> >command
> >lines, and best practices to be used for scripting.
> 
> Some feedback from a native English speaker follows...

  FWIW, like always when it comes to documentation, you're welcome to
send a patch superseding my 2/2. I'm not a good English writer, I won't
be offended at all. I care about the page being here, not about it being
written by me.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> In fact we have kind of the issue for every single optional argument out
> there:
>
> $ git describe --abbrev HEAD
> error: option `abbrev' expects a numerical value
> [...]
>
>   *ouch*
>
> So I believe that with optional arguments we must change the way we do
> things, and that we _must_ enforce the argument to be sticked in that
> case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

Among the above cases, the third one through the last one will get slightly
different questions.  The third and fourth ones get "You are given this
parameter and it must be yours", and the last one gets "You are on the
command line, and were not given any parameter."

The parser can do one of these things:

 * Inspect the token, if exists, and see if it is appropriate for it.

   * If not

     - if it is optional, then take the default value, and answer "I
       handled myself Ok, but that HEAD is not mine";

     - if it "must be yours" (the third case), barf.

   * If so

     - Use that given value and answer "I handled myself Ok, and that
       parameter 7 is mine";  this includes the fourth case as well.

And this does not have to be callback for common types like integers.

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > In fact we have kind of the issue for every single optional argument out
> > there:
> >
> > $ git describe --abbrev HEAD
> > error: option `abbrev' expects a numerical value
> > [...]
> >
> >   *ouch*
> >
> > So I believe that with optional arguments we must change the way we do
> > things, and that we _must_ enforce the argument to be sticked in that
> > case.
> 
> I think "Must" is a bit too strong an expression.
> 
> 	git describe --abbrev 7 HEAD
>         git describe --abbrev HEAD
>         git describe --abbrev=HEAD
> 	git describe --abbrev=7 HEAD
> 	git describe --abbrev
> 
> The --abbrev parser in this case could be asked with this question: "You
> are on the command line.  There is a token after you.  Is it your
> parameter?".

  I thought of that, but it's really convoluted and can definitely lead
to very subtle issues. The number of git commands with optional
arguments is quite low, mostly due to legacy, I don't expect _new_
commands to take optional arguments. I don't really like the ambiguity
it creates, and in some cases you just won't be able to disambiguate at
all. Here it looks nice because --abbrev takes an integer argument, and
it's likely that no branch nor reference names will be only made of
digits. Though for commands taking an optional string[0] argument this is
way more fishy.

  *I* (and it's my opinion, maybe other don't see it that way) see the
parse-option module as a convenience given to people using the CLI UI in
an interactive shell. So it tries to achieve a good balance between
brevity and error detection. Here I think it's quite error prone and
gives almost no help to the user: if there is a gain to type git repack
-afd vs. git repack -a -f -d, I see no real gain in --abbrev 10 vs.
--abbrev=10.


  [0] OTOH I'm not sure there will ever be optional arguments that
      aren't integers in git, but I may be wrong.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Thu, Dec 13, 2007 at 06:03:47PM +0000, Pierre Habouzit wrote:
> On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > 
> > > In fact we have kind of the issue for every single optional argument out
> > > there:
> > >
> > > $ git describe --abbrev HEAD
> > > error: option `abbrev' expects a numerical value
> > > [...]
> > >
> > >   *ouch*
> > >
> > > So I believe that with optional arguments we must change the way we do
> > > things, and that we _must_ enforce the argument to be sticked in that
> > > case.
> > 
> > I think "Must" is a bit too strong an expression.
> > 
> > 	git describe --abbrev 7 HEAD
> >         git describe --abbrev HEAD
> >         git describe --abbrev=HEAD
> > 	git describe --abbrev=7 HEAD
> > 	git describe --abbrev
> > 
> > The --abbrev parser in this case could be asked with this question: "You
> > are on the command line.  There is a token after you.  Is it your
> > parameter?".

The other issue is that when you had --abbrev=foo or --abbrev foo, the
first one has to generate an error, whereas the second one should just
say "foo" is not for me. The point being that the callback is not really
aware of how the argument got assigned to it.

That means that we will have to pass more flags to callabacks, making it
less easy to write them. Again, I'm not sure that the coding hassle is
worth of the small gain.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Kristian Høgsberg]

On Thu, 2007-12-13 at 19:03 +0100, Pierre Habouzit wrote:
> On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > 
> > > In fact we have kind of the issue for every single optional argument out
> > > there:
> > >
> > > $ git describe --abbrev HEAD
> > > error: option `abbrev' expects a numerical value
> > > [...]
> > >
> > >   *ouch*
> > >
> > > So I believe that with optional arguments we must change the way we do
> > > things, and that we _must_ enforce the argument to be sticked in that
> > > case.
> > 
> > I think "Must" is a bit too strong an expression.
> > 
> > 	git describe --abbrev 7 HEAD
> >         git describe --abbrev HEAD
> >         git describe --abbrev=HEAD
> > 	git describe --abbrev=7 HEAD
> > 	git describe --abbrev
> > 
> > The --abbrev parser in this case could be asked with this question: "You
> > are on the command line.  There is a token after you.  Is it your
> > parameter?".
> 
>   I thought of that, but it's really convoluted and can definitely lead
> to very subtle issues. The number of git commands with optional
> arguments is quite low, mostly due to legacy, I don't expect _new_
> commands to take optional arguments. I don't really like the ambiguity
> it creates, and in some cases you just won't be able to disambiguate at
> all. Here it looks nice because --abbrev takes an integer argument, and
> it's likely that no branch nor reference names will be only made of
> digits. Though for commands taking an optional string[0] argument this is
> way more fishy.

My 

Re: [RFH] convert shortlog to use parse_options

[Kristian Høgsberg]

On Thu, 2007-12-13 at 13:28 -0500, Kristian Høgsberg wrote:
> On Thu, 2007-12-13 at 19:03 +0100, Pierre Habouzit wrote:
> > On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:
> > > Pierre Habouzit <madcoder@debian.org> writes:
> > > 
> > > > In fact we have kind of the issue for every single optional argument out
> > > > there:
> > > >
> > > > $ git describe --abbrev HEAD
> > > > error: option `abbrev' expects a numerical value
> > > > [...]
> > > >
> > > >   *ouch*
> > > >
> > > > So I believe that with optional arguments we must change the way we do
> > > > things, and that we _must_ enforce the argument to be sticked in that
> > > > case.
> > > 
> > > I think "Must" is a bit too strong an expression.
> > > 
> > > 	git describe --abbrev 7 HEAD
> > >         git describe --abbrev HEAD
> > >         git describe --abbrev=HEAD
> > > 	git describe --abbrev=7 HEAD
> > > 	git describe --abbrev
> > > 
> > > The --abbrev parser in this case could be asked with this question: "You
> > > are on the command line.  There is a token after you.  Is it your
> > > parameter?".
> > 
> >   I thought of that, but it's really convoluted and can definitely lead
> > to very subtle issues. The number of git commands with optional
> > arguments is quite low, mostly due to legacy, I don't expect _new_
> > commands to take optional arguments. I don't really like the ambiguity
> > it creates, and in some cases you just won't be able to disambiguate at
> > all. Here it looks nice because --abbrev takes an integer argument, and
> > it's likely that no branch nor reference names will be only made of
> > digits. Though for commands taking an optional string[0] argument this is
> > way more fishy.
> 
> My 

Oops, sorry about that.  I just wanted to say we shouldn't jump through
all these hoops to make the option parser support every type of option
there ever was in the git command line ui.  A lot of these were probably
decided somewhat arbitrarily by whoever implemented the command.
Instead it's an opportunity to retroactively enforce some consistency
and predictability to the various option-styles that have been
hand-rolled over time in different git commands.

Kristian

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> I thought of that, but it's really convoluted and can definitely lead
> to very subtle issues. The number of git commands with optional
> arguments is quite low, mostly due to legacy, I don't expect _new_
> commands to take optional arguments.

I do not think we want to assume anything like that for a generic API.

> I don't really like the ambiguity
> it creates, and in some cases you just won't be able to disambiguate at
> all. Here it looks nice because --abbrev takes an integer argument, and
> it's likely that no branch nor reference names will be only made of
> digits. Though for commands taking an optional string[0] argument this is
> way more fishy.

I thought about ambiguity issues and I was only 70% sure about my
suggestion.  If you have an object, the initial unique part of whose
name consists only of decimal digits, you could get:

	git describe --abbrev 7 538538538
	git describe --abbrev 538538538 HEAD
	git describe --abbrev 538538538

and the last case needs to be disambiguated (either show HEAD with
abbreviation of 540 million digits, or show that object abbreviated to
the default length).  Because we are discussing a generic "optional
integer with default value" parser, let's not argue that only a small
integer between 0..40 makes sense to --abbrev flag in this context and
we should use that knowledge to further disambiguate [*1*].

You could say "If a flag takes an optional parameter and has a default,
an empty string means use the default", and disambiguate the lat example
in the above this way:

	git describe --abbrev 538538538
	git describe --abbrev '' 538538538

But it is not prettier than always requiring the optional parameter to
be sticked with the flag, as you suggest, like this;

	git describe --abbrev 538538538
	git describe --abbrev=538538538

So I am not entirely opposed to your version, nor I am claiming my
suggestion is better.  However, I just thought that "some parameters you
MUST stick to the flag" might create confusion to the end users, and I
wanted to see if others can come up with a less confusing alternative.
And the way I did so was to keep the discussion going by stirring the
pot a bit.

>   [0] OTOH I'm not sure there will ever be optional arguments that
>       aren't integers in git, but I may be wrong.

We could make HEAD as the default for "git branch --contains", if you
want an example.

[Footnote]

*1* We could introduce "optional integer with valid range with default",
and that would fit naturally into the scheme I outlined.  Even if the
next token parses as an integer, if it is out of range, you can say it
is not yours, and if it has to be yours, you can barf, saying "that's
out of range".

"The valid range" would be useful regardless of disambiguation. I wished
for "only valid is 1 or more" when I adjusted one of the commands to
parse_options().

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> The other issue is that when you had --abbrev=foo or --abbrev foo, the
> first one has to generate an error, whereas the second one should just
> say "foo" is not for me. The point being that the callback is not really
> aware of how the argument got assigned to it.

That obviously needs to be fixed ("this is the next token that could be
yours" vs "this must be yours"), if we were to go the route I suggested.

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> Oops, sorry about that.  I just wanted to say we shouldn't jump through
> all these hoops to make the option parser support every type of option
> there ever was in the git command line ui.  A lot of these were probably
> decided somewhat arbitrarily by whoever implemented the command.
> Instead it's an opportunity to retroactively enforce some consistency
> and predictability to the various option-styles that have been
> hand-rolled over time in different git commands.

That principle is fine, but I do not think it is relevant to what is
being discussed.  The issue is what to do with a flag that can
optionally take a parameter but works fine without because it has a
default.

 * You can obviously disallow such a flag, and call the result
   "consistent".  I do not think we want to go that route.  --abbrev, -M
   (to diff) and -w (to shortlog) are good examples why this is a good
   thing.  You want to use the default most of the time, but want to be
   able to tweak the value sometimes.

 * You can alternatively require parameters to such a flag always stuck
   with the flag, which is what Pierre did.  I suspected that is
   introducing an inconsistency and may be confusing to the users ("I
   can write -n=1 or -n 2, but why not --abbrev 7???") and wanted to see
   if somebody can come up with a better alternative.

 * You can try to make the parser a bit more context sensitive by
   looking at the possible parameter and see if it is plausible, which
   hopefully would work for most of the real life cases (e.g. "--abbrev
   7 HEAD" vs "--abbrev HEAD").  I however agree with Pierre that "DWIM
   works most of the time" is not good enough if there is no way to
   disambiguate in cases that fall outside.

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

On Thu, Dec 13, 2007 at 07:31:24PM +0000, Junio C Hamano wrote:
> So I am not entirely opposed to your version, nor I am claiming my
> suggestion is better.  However, I just thought that "some parameters you
> MUST stick to the flag" might create confusion to the end users, and I
> wanted to see if others can come up with a less confusing alternative.
> And the way I did so was to keep the discussion going by stirring the
> pot a bit.

I understand that, and I hope I wasn't sounding harsh at all, I was just
debating. Note that I don't like the asymmetry of some options needing a
specific syntax whereas all the rest is lax. It's cumbersome at the very
least. Though, to me there is one gain: when the user uses the
--long-opt=foo version you are _sure_ about what he meant. When you have
--long-opt foo you're not.

Your proposal tries hard to do what the user meant, with a reasonable
chance of being wrong. My proposal is on the conservative side, but
generate totally incomprehensible errors: if you do
  $ git describe --abbrev 10 HEAD
with my patch you get a not very nice
  fatal: Not a valid object name 10
as an answer. which is kind of going back to the kind of situations
parse-options is trying to avoid in the first place. I wouldn't be
really annoyed by my solution if we were able to generate an
intelligible error message instead. But guess what, if we were able to
do so, we would be able to fix the ambiguity in the first place *sigh*.


So maybe the thing would be to sleep on a it a bit and not taking any of
my patches yet (not even in pu) and let people think. It's not a major
problem, though it's one that must be fixed before 1.5.4 because we
don't want a broken option parser ever be released.


Note that there is another path, that would be to disallow mixing of
options with real arguments, and have an option separator (though with
`--` having a distinct meaning in git, that wouldn't be that easy). But
sadly some git commands already allowed such a thing, so imposing it in
parse-options would be backward incompatible for those. And I believe
many people are attached to this feature anyways.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Jeff King]

On Thu, Dec 13, 2007 at 01:47:36PM -0500, Kristian Høgsberg wrote:

> Oops, sorry about that.  I just wanted to say we shouldn't jump through
> all these hoops to make the option parser support every type of option
> there ever was in the git command line ui.  A lot of these were probably
> decided somewhat arbitrarily by whoever implemented the command.
> Instead it's an opportunity to retroactively enforce some consistency
> and predictability to the various option-styles that have been
> hand-rolled over time in different git commands.

I agree. I am already a little bit uncomfortable with the "--abbrev 10"
won't work but "--foo 10" will, because it requires that the user
remember which arguments are optional and which are required. But
switching it to "--abbrev 10 works, but --abbrev $foo does not, unless
of course $foo is an integer, in which case you must use --abbrev=$foo"
is just a little bit too DWIM. E.g., if you are scripting, it's just one
more source of error (if I have $foo, how must I write --abbrev $foo for
it to ensure that I _don't_ trigger the optional argument?).

-Peff

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I agree. I am already a little bit uncomfortable with the "--abbrev 10"
> won't work but "--foo 10" will, because it requires that the user
> remember which arguments are optional and which are required. But
> switching it to "--abbrev 10 works, but --abbrev $foo does not, unless
> of course $foo is an integer, in which case you must use --abbrev=$foo"
> is just a little bit too DWIM. E.g., if you are scripting, it's just one
> more source of error (if I have $foo, how must I write --abbrev $foo for
> it to ensure that I _don't_ trigger the optional argument?).

Oh, there is no question that scripted callers need a way to safely
disambiguate, and be able to call "git cmd" with always the default
abbreviation for whatever $foo it gets from the user unambiguously.

I do not disagree with that.

But that is different from making it harder for end users (not scripters
but command line users).

Again, I am not saying that my suggested alternative is superiour; I
just threw it out to as an example of a different approach to achieve
disambiguation than the "some flags must have its parameter stuck with
it, some don't" behaviour, which will surely be confusing to the command
line end users.

To contrast the two, with Pierre's, if you want $n-digit abbreviations
and $cnt-lines of output in your script, your script would say:

	git cmd --abbrev=$n -n $cnt $foo

because you as a script writer are required to know --abbrev is such a
magic flag that take optional parameter and cannot be fed as two options
(you can also write "-n=$cnt", but I am talking about --abbrev part).

If you are accepting the default abbreviation, on the other hand:

	git cmd --abbrev -n $cnt $foo

which looks nice.  The latter needs to be written in a funky way:

	git cmd --abbrev= $foo

if we take "empty parameter to a flag that take optional parameter means
use the default setting"; we could introduce a magic 'default' token to
read it slightly better:

	git cmd --abbrev=default $foo

but that does not change the fact that it makes it harder for
scripters.  I do not disagree with that.

The command line end users, who want to do the same, but has a bit more
concrete than unknown $n, $cnt, or $foo, can do

	git cmd --abbrev HEAD~4

in either approach.  However, with Pierre's, the command line end users
can say either:

	git cmd --abbrev=10 -n=4
	git cmd --abbrev=10 -n 4
	git cmd --abbrev=10 -n4

but they cannot say:

	git cmd --abbrev 10 -n 4

They need to learn the difference between --abbrev and -n, because you
avoid DWIMmery for the sake of script writers.  I have a slight
suspicion that it is backwards.

If there is _no_ existing users and previous versions of git, one
plausible alternative that would be much cleaner than anything we
discussed so far would be to always require '-n=4' (or "-n4") form and
never accept "-n 4", even for a flag that takes mandatory parameter.
Then there is no room for confusion.  Users (both command line end users
and script writers) need to learn only one rule: flag parameters are
always with the flag, not given as two words.

I really wish the world were that simple, but I do no think that would
fly well with the existing users' fingers.

Re: [RFH] convert shortlog to use parse_options

[Andreas Ericsson]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> I agree. I am already a little bit uncomfortable with the "--abbrev 10"
>> won't work but "--foo 10" will, because it requires that the user
>> remember which arguments are optional and which are required. But
>> switching it to "--abbrev 10 works, but --abbrev $foo does not, unless
>> of course $foo is an integer, in which case you must use --abbrev=$foo"
>> is just a little bit too DWIM. E.g., if you are scripting, it's just one
>> more source of error (if I have $foo, how must I write --abbrev $foo for
>> it to ensure that I _don't_ trigger the optional argument?).
> 
> Oh, there is no question that scripted callers need a way to safely
> disambiguate, and be able to call "git cmd" with always the default
> abbreviation for whatever $foo it gets from the user unambiguously.
> 
> I do not disagree with that.
> 
> But that is different from making it harder for end users (not scripters
> but command line users).
> 
> Again, I am not saying that my suggested alternative is superiour; I
> just threw it out to as an example of a different approach to achieve
> disambiguation than the "some flags must have its parameter stuck with
> it, some don't" behaviour, which will surely be confusing to the command
> line end users.
> 
> To contrast the two, with Pierre's, if you want $n-digit abbreviations
> and $cnt-lines of output in your script, your script would say:
> 
> 	git cmd --abbrev=$n -n $cnt $foo
> 
> because you as a script writer are required to know --abbrev is such a
> magic flag that take optional parameter and cannot be fed as two options
> (you can also write "-n=$cnt", but I am talking about --abbrev part).
> 
> If you are accepting the default abbreviation, on the other hand:
> 
> 	git cmd --abbrev -n $cnt $foo
> 
> which looks nice.  The latter needs to be written in a funky way:
> 
> 	git cmd --abbrev= $foo
> 
> if we take "empty parameter to a flag that take optional parameter means
> use the default setting"; we could introduce a magic 'default' token to
> read it slightly better:
> 
> 	git cmd --abbrev=default $foo
> 
> but that does not change the fact that it makes it harder for
> scripters.  I do not disagree with that.
> 
> The command line end users, who want to do the same, but has a bit more
> concrete than unknown $n, $cnt, or $foo, can do
> 
> 	git cmd --abbrev HEAD~4
> 
> in either approach.  However, with Pierre's, the command line end users
> can say either:
> 
> 	git cmd --abbrev=10 -n=4
> 	git cmd --abbrev=10 -n 4
> 	git cmd --abbrev=10 -n4
> 
> but they cannot say:
> 
> 	git cmd --abbrev 10 -n 4
> 
> They need to learn the difference between --abbrev and -n, because you
> avoid DWIMmery for the sake of script writers.  I have a slight
> suspicion that it is backwards.
> 
> If there is _no_ existing users and previous versions of git, one
> plausible alternative that would be much cleaner than anything we
> discussed so far would be to always require '-n=4' (or "-n4") form and
> never accept "-n 4", even for a flag that takes mandatory parameter.
> Then there is no room for confusion.  Users (both command line end users
> and script writers) need to learn only one rule: flag parameters are
> always with the flag, not given as two words.
> 
> I really wish the world were that simple, but I do no think that would
> fly well with the existing users' fingers.

I think you're right in that last sentence. It would also be terribly
inconsistent with the other 3000 or so command-line programs one has
in ones path.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

On Fri, Dec 14, 2007 at 05:59:52AM +0000, Junio C Hamano wrote:
> 	git cmd --abbrev=10 -n=4

  actually -n=4 isn't understood atm, only -n4 and -n 4 are.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Fri, Dec 14, 2007 at 05:59:52AM +0000, Junio C Hamano wrote:
>> 	git cmd --abbrev=10 -n=4
>
>   actually -n=4 isn't understood atm, only -n4 and -n 4 are.

Ah, my mistake.  And I do not think accepting -n=4 is a good idea (it is
not historically done).

After thinking about it a bit more, I think I was worried too much about
burdening the users to remember the differences between options with,
without and optional option-arguments [*1*].  They need to know the
difference between options with and without option-arguments already
because single letter options can be combined if they are without
option-arguments, and they have to write "shortlog -new72" but not
"shortlog -wen72".  If they want to be extra sure, they can be more
explicit and say "shortlog -n -e -w72".

So let's go with the version you outlined --- options that take optional
option-arguments must get their option-arguments stuck to them, but
otherwise option-arguments can also be given as a separate word that
follows the option.

[Footnote]

*1* The fact some of our commands support options with optional
option-arguments is already against Guideline #7 in "12.2 Utility Syntax
Guidelines", so other POSIX guidelines are not useful for us in deciding
what behaviour to model after.

Re: [RFH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On ven, déc 14, 2007 at 08:34:58 +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Fri, Dec 14, 2007 at 05:59:52AM +0000, Junio C Hamano wrote:
> >> 	git cmd --abbrev=10 -n=4
> >
> >   actually -n=4 isn't understood atm, only -n4 and -n 4 are.
> 
> Ah, my mistake.  And I do not think accepting -n=4 is a good idea (it is
> not historically done).

> After thinking about it a bit more, I think I was worried too much about
> burdening the users to remember the differences between options with,
> without and optional option-arguments [*1*].  They need to know the
> difference between options with and without option-arguments already
> because single letter options can be combined if they are without
> option-arguments, and they have to write "shortlog -new72" but not
> "shortlog -wen72".  If they want to be extra sure, they can be more
> explicit and say "shortlog -n -e -w72".

  Yep, let's do it then. Note that's the reason why I felt we need a
manual page about this, because we should give some guidelines of what
is safe for scripting.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   Yep, let's do it then. Note that's the reason why I felt we need a
> manual page about this, because we should give some guidelines of what
> is safe for scripting.

There are some fallouts from the series, though.  I've fixed up git-tag
but I strongly suspect all of parseopt users now need careful auditing.
If we cannot be confident with the parseoptified commands within
reasonably short timeframe by -rc1, we may end up having to revert the
parseopt changes from them, which I'd rather avoid, but if you look at
the git-tag change (especially for -l) you would understand it.  The
"must stick" restriction feels Ok on paper but in practice it looks
rather draconian and very user unfriendly.

I'll be sending out two patches shortly.  The result will sit in 'pu'
waiting for an Ack and/or improvement, but that will happen only when I
got around pushing things out, which may come later than you see the
patches.

[PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

The command freely used optional option-argments for its -l and -n options.
I think allowing "git tag -n xxx" without barfing was an error to begin with,
but not supporting "git tag -l pattern" form is a serious regression.

So this fixes the handling of -l to reinstate the original behaviour while
detecting a user error "git tag -l pattern garbage", and adjusts tests that
use "-n param" form to use "-nparam".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-tag.c  |   15 +++++++-
 t/t7004-tag.sh |  110 +++++++++++++++++++++++++------------------------------
 2 files changed, 63 insertions(+), 62 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..5213c62 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -371,7 +371,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	int annotate = 0, sign = 0, force = 0, lines = 0,
 					delete = 0, verify = 0;
-	char *list = NULL, *msgfile = NULL, *keyid = NULL;
+	const char *list = NULL, *msgfile = NULL, *keyid = NULL;
 	const char *no_pattern = "NO_PATTERN";
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct option options[] = {
@@ -407,8 +407,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (sign)
 		annotate = 1;
 
-	if (list)
+	if (list) {
+		/*
+		 * This is unfortunate but requiring "git tag -lpattern" and not
+		 * allowing "git tag -l pattern" is a serious regression.
+		 */
+		if (argc && list == no_pattern) {
+			list = argv[0];
+			argc--;
+		}
+		if (argc)
+			die("extra argument after -l[pattern]: %s", argv[0]);
 		return list_tags(list == no_pattern ? NULL : list, lines);
+	}
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 09d56e0..7f58038 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -491,25 +491,19 @@ test_expect_success \
 	echo "tag-one-line" >expect &&
 	git-tag -l | grep "^tag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^tag-one-line" >actual &&
+	git-tag -n0 -l | grep "^tag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l tag-one-line >actual &&
+	git-tag -n0 -l tag-one-line >actual &&
 	git diff expect actual &&
 
 	echo "tag-one-line    A msg" >expect &&
-	git-tag -n xxx -l | grep "^tag-one-line" >actual &&
-	git diff expect actual &&
-	git-tag -n "" -l | grep "^tag-one-line" >actual &&
-	git diff expect actual &&
-	git-tag -n 1 -l | grep "^tag-one-line" >actual &&
-	git diff expect actual &&
 	git-tag -n -l | grep "^tag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l tag-one-line >actual &&
+	git-tag -n1 -l | grep "^tag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l tag-one-line >actual &&
+	git-tag -n2 -l tag-one-line >actual &&
 	git diff expect actual &&
-	git-tag -n 999 -l tag-one-line >actual &&
+	git-tag -n999 -l tag-one-line >actual &&
 	git diff expect actual
 '
 
@@ -520,21 +514,21 @@ test_expect_success \
 	echo "tag-zero-lines" >expect &&
 	git-tag -l | grep "^tag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^tag-zero-lines" >actual &&
+	git-tag -n0 -l | grep "^tag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l tag-zero-lines >actual &&
+	git-tag -n0 -l tag-zero-lines >actual &&
 	git diff expect actual &&
 
 	echo "tag-zero-lines  " >expect &&
-	git-tag -n 1 -l | grep "^tag-zero-lines" >actual &&
+	git-tag -n1 -l | grep "^tag-zero-lines" >actual &&
 	git diff expect actual &&
 	git-tag -n -l | grep "^tag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l tag-zero-lines >actual &&
+	git-tag -n1 -l tag-zero-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l tag-zero-lines >actual &&
+	git-tag -n2 -l tag-zero-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 999 -l tag-zero-lines >actual &&
+	git-tag -n999 -l tag-zero-lines >actual &&
 	git diff expect actual
 '
 
@@ -548,37 +542,37 @@ test_expect_success \
 	echo "tag-lines" >expect &&
 	git-tag -l | grep "^tag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^tag-lines" >actual &&
+	git-tag -n0 -l | grep "^tag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l tag-lines >actual &&
+	git-tag -n0 -l tag-lines >actual &&
 	git diff expect actual &&
 
 	echo "tag-lines       tag line one" >expect &&
-	git-tag -n 1 -l | grep "^tag-lines" >actual &&
+	git-tag -n1 -l | grep "^tag-lines" >actual &&
 	git diff expect actual &&
 	git-tag -n -l | grep "^tag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l tag-lines >actual &&
+	git-tag -n1 -l tag-lines >actual &&
 	git diff expect actual &&
 
 	echo "    tag line two" >>expect &&
-	git-tag -n 2 -l | grep "^ *tag.line" >actual &&
+	git-tag -n2 -l | grep "^ *tag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l tag-lines >actual &&
+	git-tag -n2 -l tag-lines >actual &&
 	git diff expect actual &&
 
 	echo "    tag line three" >>expect &&
-	git-tag -n 3 -l | grep "^ *tag.line" >actual &&
+	git-tag -n3 -l | grep "^ *tag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 3 -l tag-lines >actual &&
+	git-tag -n3 -l tag-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 4 -l | grep "^ *tag.line" >actual &&
+	git-tag -n4 -l | grep "^ *tag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 4 -l tag-lines >actual &&
+	git-tag -n4 -l tag-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 99 -l | grep "^ *tag.line" >actual &&
+	git-tag -n99 -l | grep "^ *tag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 99 -l tag-lines >actual &&
+	git-tag -n99 -l tag-lines >actual &&
 	git diff expect actual
 '
 
@@ -906,25 +900,21 @@ test_expect_success \
 	echo "stag-one-line" >expect &&
 	git-tag -l | grep "^stag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^stag-one-line" >actual &&
+	git-tag -n0 -l | grep "^stag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l stag-one-line >actual &&
+	git-tag -n0 -l stag-one-line >actual &&
 	git diff expect actual &&
 
 	echo "stag-one-line   A message line signed" >expect &&
-	git-tag -n xxx -l | grep "^stag-one-line" >actual &&
-	git diff expect actual &&
-	git-tag -n "" -l | grep "^stag-one-line" >actual &&
-	git diff expect actual &&
-	git-tag -n 1 -l | grep "^stag-one-line" >actual &&
-	git diff expect actual &&
 	git-tag -n -l | grep "^stag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l stag-one-line >actual &&
+	git-tag -n1 -l | grep "^stag-one-line" >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l stag-one-line >actual &&
+	git-tag -n1 -l stag-one-line >actual &&
 	git diff expect actual &&
-	git-tag -n 999 -l stag-one-line >actual &&
+	git-tag -n2 -l stag-one-line >actual &&
+	git diff expect actual &&
+	git-tag -n999 -l stag-one-line >actual &&
 	git diff expect actual
 '
 
@@ -935,21 +925,21 @@ test_expect_success \
 	echo "stag-zero-lines" >expect &&
 	git-tag -l | grep "^stag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^stag-zero-lines" >actual &&
+	git-tag -n0 -l | grep "^stag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l stag-zero-lines >actual &&
+	git-tag -n0 -l stag-zero-lines >actual &&
 	git diff expect actual &&
 
 	echo "stag-zero-lines " >expect &&
-	git-tag -n 1 -l | grep "^stag-zero-lines" >actual &&
-	git diff expect actual &&
 	git-tag -n -l | grep "^stag-zero-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l stag-zero-lines >actual &&
+	git-tag -n1 -l | grep "^stag-zero-lines" >actual &&
+	git diff expect actual &&
+	git-tag -n1 -l stag-zero-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l stag-zero-lines >actual &&
+	git-tag -n2 -l stag-zero-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 999 -l stag-zero-lines >actual &&
+	git-tag -n999 -l stag-zero-lines >actual &&
 	git diff expect actual
 '
 
@@ -963,37 +953,37 @@ test_expect_success \
 	echo "stag-lines" >expect &&
 	git-tag -l | grep "^stag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l | grep "^stag-lines" >actual &&
+	git-tag -n0 -l | grep "^stag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 0 -l stag-lines >actual &&
+	git-tag -n0 -l stag-lines >actual &&
 	git diff expect actual &&
 
 	echo "stag-lines      stag line one" >expect &&
-	git-tag -n 1 -l | grep "^stag-lines" >actual &&
-	git diff expect actual &&
 	git-tag -n -l | grep "^stag-lines" >actual &&
 	git diff expect actual &&
-	git-tag -n 1 -l stag-lines >actual &&
+	git-tag -n1 -l | grep "^stag-lines" >actual &&
+	git diff expect actual &&
+	git-tag -n1 -l stag-lines >actual &&
 	git diff expect actual &&
 
 	echo "    stag line two" >>expect &&
-	git-tag -n 2 -l | grep "^ *stag.line" >actual &&
+	git-tag -n2 -l | grep "^ *stag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 2 -l stag-lines >actual &&
+	git-tag -n2 -l stag-lines >actual &&
 	git diff expect actual &&
 
 	echo "    stag line three" >>expect &&
-	git-tag -n 3 -l | grep "^ *stag.line" >actual &&
+	git-tag -n3 -l | grep "^ *stag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 3 -l stag-lines >actual &&
+	git-tag -n3 -l stag-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 4 -l | grep "^ *stag.line" >actual &&
+	git-tag -n4 -l | grep "^ *stag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 4 -l stag-lines >actual &&
+	git-tag -n4 -l stag-lines >actual &&
 	git diff expect actual &&
-	git-tag -n 99 -l | grep "^ *stag.line" >actual &&
+	git-tag -n99 -l | grep "^ *stag.line" >actual &&
 	git diff expect actual &&
-	git-tag -n 99 -l stag-lines >actual &&
+	git-tag -n99 -l stag-lines >actual &&
 	git diff expect actual
 '
 
-- 
1.5.4.rc0.54.g3eb2

[PATCH] (squashme) gitcli documentation fixups

[Junio C Hamano]

This comes directly on top of gitcli documentation patch and is intended
to be squashed into it.

I looked around to see if there is a good place to add gitlink:gitcli[5]
but unfortunately I did not find any.  One possibility, once this
document is enhanced enough to be usable as "introduction to scripting
using plumbing", we could add it to near the top of git(7) where we
refer to the tutorial, user manual and the everyday document, but in the
current form it is too sketchy and does not cover enough.  But we have
to start somewhere.

I think we should add the first rule in the bulletted list.

 * avoid reinventing the wheel.

but it needs a bit more explanation.  Quite a few people seem to try to
reinvent "git rev-parse --verify HEAD" in their scripts using much
higher level "git show -s -1 --pretty=format:xxx", which is unfortunate
and disgusting at the same time.

---

 Documentation/gitcli.txt |   45 +++++++++++++++++++++++++++------------------
 Makefile                 |    1 +
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index 3b3f5e4..f790b78 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -3,7 +3,7 @@ gitcli(5)
 
 NAME
 ----
-gitcli - git command line interface and its usual conventions
+gitcli - git command line interface and conventions
 
 SYNOPSIS
 --------
@@ -12,31 +12,40 @@ gitcli
 
 DESCRIPTION
 -----------
-This manual intends to describe best practice in how to use git CLI.  Here are
+
+This manual describes best practice in how to use git CLI.  Here are
 the rules that you should follow when you are scripting git:
 
  * it's preferred to use the non dashed form of git commands, which means that
    you should prefer `"git foo"` to `"git-foo"`.
 
- * splitting short option switches in separate atoms (prefer `"git foo -a -b"`
+ * splitting short options to separate words (prefer `"git foo -a -b"`
    to `"git foo -ab"`, the latter may not even work).
 
- * when a command line switch takes an argument, use the 'sticked' form, which
-   means that you must prefer `"git foo -oArg"` to `"git foo -o Arg"` for short
-   option switches, and `"git foo --long-opt=Arg"` to `"git foo --long-opt Arg"`
-   for long switches.
+ * when a command line option takes an argument, use the 'sticked' form.  In
+   other words, write `"git foo -oArg"` instead of `"git foo -o Arg"` for short
+   options, and `"git foo --long-opt=Arg"` instead of `"git foo --long-opt Arg"`
+   for long options.  An option that takes optional option-argument must be
+   written in the 'sticked' form.
+
+ * when you give a revision parameter to a command, make sure the parameter is
+   not ambiguous with a name of a file in the work tree.  E.g. do not write
+   `"git log -1 HEAD"` but write `"git log -1 HEAD --"`; the former will not work
+   if you happen to have a file called `HEAD` in the work tree.
 
 
 ENHANCED CLI
 ------------
-From the git 1.5.4 series and further, git commands (not all of them at the
-time of the writing though) come with an enhanced option parser with nice
-facilities. Here is an exhaustive list of them
+From the git 1.5.4 series and further, many git commands (not all of them at the
+time of the writing though) come with an enhanced option parser.
+
+Here is an exhaustive list of the facilities provided by this option parser.
+
 
 Magic Options
 ~~~~~~~~~~~~~
 Commands which have the enhanced option parser activated all understand a
-couple of magic command line switches:
+couple of magic command line options:
 
 -h::
 	gives a pretty printed usage of the command.
@@ -54,14 +63,14 @@ usage: git-describe [options] <committish>*
 ---------------------------------------------
 
 --help-all::
-	Some git commands takes options that are only used for plumbing or that
+	Some git commands take options that are only used for plumbing or that
 	are deprecated, and such options are hidden from the default usage. This
-	switch gives the full list of options.
+	option gives the full list of options.
 
 
 Negating options
 ~~~~~~~~~~~~~~~~
-Another things to keep in mind is that long options can be negated. For
+Boolean options with long option names can be negated by prefixing `"--no-"`. For
 example, `"git branch"` has the option `"--track"` which is 'on' by default. You
 can use `"--no-track"` to override that behaviour. The same goes for `"--color"`
 and `"--no-color"`.
@@ -74,10 +83,10 @@ options. This means that you can for example use `"git rm -rf"` or
 `"git clean -fdx"`.
 
 
-Separating argument from the switch
+Separating argument from the option
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Also for option switches that take a mandatory argument, you can separate it
-from the switch. That means that all the following uses are correct:
+You can write the mandatory option parameter to an option as a separate
+word on the command line.  That means that all the following uses work:
 
 ----------------------------
 $ git foo --long-opt=Arg
@@ -86,7 +95,7 @@ $ git foo -oArg
 $ git foo -o Arg
 ----------------------------
 
-However, this is *NOT* possible for switches with an optionnal value, where the
+However, this is *NOT* allowed for switches with an optionnal value, where the
 'sticked' form must be used:
 ----------------------------
 $ git describe --abbrev HEAD     # correct
diff --git a/Makefile b/Makefile
index 617e5f5..fd9b939 100644
--- a/Makefile
+++ b/Makefile
@@ -1172,6 +1172,7 @@ check-docs::
 		documented,gitattributes | \
 		documented,gitignore | \
 		documented,gitmodules | \
+		documented,gitcli | \
 		documented,git-tools | \
 		sentinel,not,matching,is,ok ) continue ;; \
 		esac; \
-- 
1.5.4.rc0.54.g3eb2

Re: [RFH] convert shortlog to use parse_options

[Andreas Ericsson]

Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
>>   Yep, let's do it then. Note that's the reason why I felt we need a
>> manual page about this, because we should give some guidelines of what
>> is safe for scripting.
> 
> There are some fallouts from the series, though.  I've fixed up git-tag
> but I strongly suspect all of parseopt users now need careful auditing.
> If we cannot be confident with the parseoptified commands within
> reasonably short timeframe by -rc1, we may end up having to revert the
> parseopt changes from them, which I'd rather avoid, but if you look at
> the git-tag change (especially for -l) you would understand it.  The
> "must stick" restriction feels Ok on paper but in practice it looks
> rather draconian and very user unfriendly.
> 

Usually, optional arguments warrant adding a second parameter. This can
often even improve usability, as it's never unclear or ambiguous what's
happening. For the 'git tag -l' case, I'd use something like
'git tag -l --match="regex"' or some such, or perhaps make "-l" its own
subcommand ("list") with a built-in alias of -l. That means "-l" has to
be the first argument after "git tag" on the command-line, but I suspect
it doesn't make much sense to use it along with other options anyway, so
perhaps that's not much of an issue.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [RFH] convert shortlog to use parse_options

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Junio C Hamano wrote:
> 
>> ...  The
>> "must stick" restriction feels Ok on paper but in practice it looks
>> rather draconian and very user unfriendly.
>
> Usually, optional arguments warrant adding a second parameter. This can
> often even improve usability, as it's never unclear or ambiguous what's
> happening. For the 'git tag -l' case, I'd use something like
> 'git tag -l --match="regex"' or some such,...

That is essentially arguing for POSIXly correct "do not allow optional
option-arguments" (utility syntax guidelines #7).  That position might
be politically correct, but I am already discussing beyond that:
usability.

For "git tag -l", the fix was rather simple, as the option would either
have taken a zero pattern (list all) or a single pattern (list matching
this pattern), and the command itself did not take any extra arguments,
so that was what I did in the patch.  Compare your POSIXly correct
version:

        git tag -l			(ok)
        git tag -l pattern		(not ok)
	git tag -l --match=pattern	(ok)

with the traditional (and fixed):

        git tag -l			(ok)
        git tag -l pattern		(ok)
	git tag -l pattern garbage	(not ok)

Which one is easier for the user?

Re: [PATCH] (squashme) gitcli documentation fixups

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On Mon, Dec 17, 2007 at 07:28:47AM +0000, Junio C Hamano wrote:
> This comes directly on top of gitcli documentation patch and is intended
> to be squashed into it.

  I obviously ack.
> -Another things to keep in mind is that long options can be negated. For
> +Boolean options with long option names can be negated by prefixing `"--no-"`. For
   ^^^^^^^
Though this isn't correct: you can negate any kind of option, even one
with strings arguments, and it does makes sense. E.g. if you have some:

  foo.stringOpt = "value"

in your gitconfig file, then it's very handy to be able to write:

  $ git foo --no-string-opt

to be sure the gitconfig from the user won't mess with what you intend
to do. The negation of commands can be disabled (in the recent
iterations of parseopt) using a flag I don't recall the name, but it's
on by default even for non boolean options. It may make sense to do a
re-read pass of all options and see which ones it makes sense to negate
and which not.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] (squashme) gitcli documentation fixups

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Mon, Dec 17, 2007 at 07:28:47AM +0000, Junio C Hamano wrote:
>  * avoid reinventing the wheel.
> 
> but it needs a bit more explanation.  Quite a few people seem to try to
> reinvent "git rev-parse --verify HEAD" in their scripts using much
> higher level "git show -s -1 --pretty=format:xxx", which is unfortunate
> and disgusting at the same time.

Oh and about that, the point is, users don't always know the wheel
exists because they don't know where to look in the first place. Maybe
gitcli(5) will become the right place to explain this kind of usual
tricks under a "git scripting idioms" section.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Mon, Dec 17, 2007 at 07:28:41AM +0000, Junio C Hamano wrote:
> The command freely used optional option-argments for its -l and -n options.
> I think allowing "git tag -n xxx" without barfing was an error to begin with,
> but not supporting "git tag -l pattern" form is a serious regression.
> 
> So this fixes the handling of -l to reinstate the original behaviour while
> detecting a user error "git tag -l pattern garbage", and adjusts tests that
> use "-n param" form to use "-nparam".

> -	if (list)
> +	if (list) {
> +		/*
> +		 * This is unfortunate but requiring "git tag -lpattern" and not
> +		 * allowing "git tag -l pattern" is a serious regression.
> +		 */
> +		if (argc && list == no_pattern) {
> +			list = argv[0];
> +			argc--;
> +		}
> +		if (argc)
> +			die("extra argument after -l[pattern]: %s", argv[0]);
>  		return list_tags(list == no_pattern ? NULL : list, lines);
> +	}

  Okay this is kind of disgusting, and I'm absolutely not pleased with
it (I mean I'm not pleased that parse_opt forces us to write things like
that). This hack allows:

  git tag -l -n10 <pattern>

and will then attach the <pattern> to the '-l' switch, and I find it
nowhere near acceptable. I believe the fix is worse than the disease.
I'll try to think harder about what we can do about it. Though for now,
we will have to go for it for a while.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
>> Junio C Hamano wrote:
>>
>>> ...  The
>>> "must stick" restriction feels Ok on paper but in practice it looks
>>> rather draconian and very user unfriendly.
>> Usually, optional arguments warrant adding a second parameter. This can
>> often even improve usability, as it's never unclear or ambiguous what's
>> happening. For the 'git tag -l' case, I'd use something like
>> 'git tag -l --match="regex"' or some such,...
> 
> That is essentially arguing for POSIXly correct "do not allow optional
> option-arguments" (utility syntax guidelines #7).  That position might
> be politically correct, but I am already discussing beyond that:
> usability.
> 
> For "git tag -l", the fix was rather simple, as the option would either
> have taken a zero pattern (list all) or a single pattern (list matching
> this pattern), and the command itself did not take any extra arguments,
> so that was what I did in the patch.  Compare your POSIXly correct
> version:
> 
>         git tag -l			(ok)
>         git tag -l pattern		(not ok)
> 	git tag -l --match=pattern	(ok)
> 
> with the traditional (and fixed):
> 
>         git tag -l			(ok)
>         git tag -l pattern		(ok)
> 	git tag -l pattern garbage	(not ok)
> 
> Which one is easier for the user?


git tag -l pattern, although I suspect newcomers often write it as
"git tag -l | grep pattern" anyways.

If -l was a short-hand for "git tag list", and "list" was a subcommand
to git tag, the whole business would be explainable. The fact that
"git tag" lists all tag but doesn't take a patter, while "git tag -l"
does exactly the same thing, but *does* take an (optional) pattern
means the --match functionality could just as well be written as
"git tag -m pattern" (and let '-m' imply '-l', which is sensible).
"-l" can then be deprecated, as its syntax doesn't match that of
the other way to list tags.

Otoh, I don't care all that deeply about it. It's just nicer to explain
the UI if there are no optional arguments, primarily because it involves
exceptions, and secondarily because many programs follow the posixly
correct thing of "no optional arguments", so they're a definitive
minority in the program argument jungle.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   Okay this is kind of disgusting, and I'm absolutely not pleased with
> it (I mean I'm not pleased that parse_opt forces us to write things like
> that).
> ...
> I'll try to think harder about what we can do about it. Though for now,
> we will have to go for it for a while.

This is just a quick idea before I go back to sleep, but your earlier
comment on "--no-<an-option-that-is-not-even-boolean>" made me realize
that the alternative I was suggesting earlier would actually work much
nicer, if you introduce "--<an-option-that-take-optional-arg>-default"
magic.

Then, normal users who know what the value of $foo is (iow, not scripts)
can say:

	git cmd --abbrev 10
        git cmd --abbrev HEAD
        git cmd --abbrev=10 HEAD

and scripts that want to have $foo to be treated as rev, even when it
consists entirely of digits, can say:

	git cmd --abbrev-default $foo

to disambiguate (i.e.  like "--no-" magic, "-default" is a magic, and it
tells the parser that "there is no option-argument given to this").

To make sure $foo is treated as the precision, the script can say:

	git cmd --abbrev=$foo

If the script wants DWIM just like human users want, it can do:

	git cmd --abbrev $foo

There of course is a little details called coding, but I think this is
probably the most user friendly to the users and the scripts alike.  It
certainly is nicer than what the current parse_options() does, and/or
the git-tag workaround does.

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

On Mon, Dec 17, 2007 at 10:53:00AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   Okay this is kind of disgusting, and I'm absolutely not pleased with
> > it (I mean I'm not pleased that parse_opt forces us to write things like
> > that).
> > ...
> > I'll try to think harder about what we can do about it. Though for now,
> > we will have to go for it for a while.
> 
> This is just a quick idea before I go back to sleep, but your earlier
> comment on "--no-<an-option-that-is-not-even-boolean>" made me realize
> that the alternative I was suggesting earlier would actually work much
> nicer, if you introduce "--<an-option-that-take-optional-arg>-default"
> magic.

  meeeow I love the idea !

> Then, normal users who know what the value of $foo is (iow, not scripts)
> can say:
> 
> 	git cmd --abbrev 10
>         git cmd --abbrev HEAD
>         git cmd --abbrev=10 HEAD
> 
> and scripts that want to have $foo to be treated as rev, even when it
> consists entirely of digits, can say:
> 
> 	git cmd --abbrev-default $foo
> 
> to disambiguate (i.e.  like "--no-" magic, "-default" is a magic, and it
> tells the parser that "there is no option-argument given to this").
> 
> To make sure $foo is treated as the precision, the script can say:
> 
> 	git cmd --abbrev=$foo
> 
> If the script wants DWIM just like human users want, it can do:
> 
> 	git cmd --abbrev $foo
> 
> There of course is a little details called coding, but I think this is
> probably the most user friendly to the users and the scripts alike.  It
> certainly is nicer than what the current parse_options() does, and/or
> the git-tag workaround does.

I like the idea, this way we can do the "let's pass the argument as an
option to the callback and let it say if it likes it or not, and have a
quite not so bad way to help the guy scripting this disambiguate. I like
it a lot, and it shouldn't be that hard to deal with. I'll work on it,
and propose new patches ASAP.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   Okay this is kind of disgusting, and I'm absolutely not pleased with
> it (I mean I'm not pleased that parse_opt forces us to write things like
> that). This hack allows:
>
>   git tag -l -n10 <pattern>
>
> and will then attach the <pattern> to the '-l' switch,...

Heh, it turns out that we were both stupid and blind.

Look at contrib/examples/git-tag.sh again.

The original addition of "-n <count>" was suboptimal and did not allow
"git tag -l -n10 <glob>", but I would actually call that a bug of the -n
<count> implementation (it wanted to affect working of other option, so
at that point it should have restructured the loop and made parsing of
options and carrying out of actions separate steps).

The -l option tells "git tag" to work in "list tags" mode, and in that
mode, the command itself takes zero or one (we could have taken more but
we didn't) glob to limit the listing.  The <glob> is not even an option
argument to option -l, but the arguments to "git tag" command itself.

So "git-tag -l" was a bad example of an option that takes optional
option-argument, and you can see that the conversion to parse_options()
done in 396865859918e9c7bf8ce74aae137c57da134610 (Make builtin-tag.c use
parse_options.) broke it.

IOW, the fixup I posted was not a workaround but happens to be a bugfix
to the above commit that did parse_options() conversion.

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Mon, Dec 17, 2007 at 10:53:00AM +0000, Junio C Hamano wrote:
> ...
>> This is just a quick idea before I go back to sleep, but your earlier
>> comment on "--no-<an-option-that-is-not-even-boolean>" made me realize
>> that the alternative I was suggesting earlier would actually work much
>> nicer, if you introduce "--<an-option-that-take-optional-arg>-default"
>> magic.
>
>   meeeow I love the idea !

There is a bit more serious issue than coding, actually.

Short options.

A script wants to use default rename detection threshold for unknown
commit $foo whose name might look like a number.  IOW, this

	git diff -M $foo

could be ambiguous.  Obviously, "git diff -M-default $foo" would not fly
very well.

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

On Mon, Dec 17, 2007 at 11:13:14AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   Okay this is kind of disgusting, and I'm absolutely not pleased with
> > it (I mean I'm not pleased that parse_opt forces us to write things like
> > that). This hack allows:
> >
> >   git tag -l -n10 <pattern>
> >
> > and will then attach the <pattern> to the '-l' switch,...
> 
> Heh, it turns out that we were both stupid and blind.

  [...]

  indeed, but then this happens to be a better patch than yours IMHO:


>From 5a3cdd255f17c7d7bc9245881f0d50146413113f Mon Sep 17 00:00:00 2001
From: Pierre Habouzit <madcoder@debian.org>
Date: Mon, 17 Dec 2007 12:54:55 +0100
Subject: [PATCH] git-tag: fix -l switch handling regression.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-tag.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..219633d 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -16,7 +16,7 @@
 static const char * const git_tag_usage[] = {
 	"git-tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git-tag -d <tagname>...",
-	"git-tag [-n [<num>]] -l [<pattern>]",
+	"git-tag -l [-n [<num>]] [<pattern>]",
 	"git-tag -v <tagname>...",
 	NULL
 };
@@ -370,13 +370,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_lock *lock;
 
 	int annotate = 0, sign = 0, force = 0, lines = 0,
-					delete = 0, verify = 0;
-	char *list = NULL, *msgfile = NULL, *keyid = NULL;
-	const char *no_pattern = "NO_PATTERN";
+		list = 0, delete = 0, verify = 0;
+	char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct option options[] = {
-		{ OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
-			PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
+		OPT_INTEGER('l', NULL, &list, "list tag names"),
 		{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
 				"print n lines of each tag message",
 				PARSE_OPT_OPTARG, NULL, 1 },
@@ -408,7 +406,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		annotate = 1;
 
 	if (list)
-		return list_tags(list == no_pattern ? NULL : list, lines);
+		return list_tags(argv[0], lines);
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)
-- 
debian.1.5.3.7.1-dirty

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]

On Mon, Dec 17, 2007 at 11:56:48AM +0000, Pierre Habouzit wrote:
> On Mon, Dec 17, 2007 at 11:13:14AM +0000, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > 
> > >   Okay this is kind of disgusting, and I'm absolutely not pleased with
> > > it (I mean I'm not pleased that parse_opt forces us to write things like
> > > that). This hack allows:
> > >
> > >   git tag -l -n10 <pattern>
> > >
> > > and will then attach the <pattern> to the '-l' switch,...
> > 
> > Heh, it turns out that we were both stupid and blind.
> 
>   [...]
> 
>   indeed, but then this happens to be a better patch than yours IMHO:
> 
> 
> From 5a3cdd255f17c7d7bc9245881f0d50146413113f Mon Sep 17 00:00:00 2001
> From: Pierre Habouzit <madcoder@debian.org>
> Date: Mon, 17 Dec 2007 12:54:55 +0100
> Subject: [PATCH] git-tag: fix -l switch handling regression.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  builtin-tag.c |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin-tag.c b/builtin-tag.c
> index 274901a..219633d 100644
> --- a/builtin-tag.c
> +++ b/builtin-tag.c
> @@ -16,7 +16,7 @@
>  static const char * const git_tag_usage[] = {
>  	"git-tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
>  	"git-tag -d <tagname>...",
> -	"git-tag [-n [<num>]] -l [<pattern>]",
> +	"git-tag -l [-n [<num>]] [<pattern>]",
>  	"git-tag -v <tagname>...",
>  	NULL
>  };
> @@ -370,13 +370,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct ref_lock *lock;
>  
>  	int annotate = 0, sign = 0, force = 0, lines = 0,
> -					delete = 0, verify = 0;
> -	char *list = NULL, *msgfile = NULL, *keyid = NULL;
> -	const char *no_pattern = "NO_PATTERN";
> +		list = 0, delete = 0, verify = 0;
> +	char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct option options[] = {
> -		{ OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
> -			PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
> +		OPT_INTEGER('l', NULL, &list, "list tag names"),
                ^^^^^^^^^^^
      that should obviously be OPT_BOOLEAN

>  		{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
>  				"print n lines of each tag message",
>  				PARSE_OPT_OPTARG, NULL, 1 },
> @@ -408,7 +406,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		annotate = 1;
>  
>  	if (list)
> -		return list_tags(list == no_pattern ? NULL : list, lines);
> +		return list_tags(argv[0], lines);
>  	if (delete)
>  		return for_each_tag_name(argv, delete_tag);
>  	if (verify)
> -- 
> debian.1.5.3.7.1-dirty
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Mon, Dec 17, 2007 at 11:21:11AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Mon, Dec 17, 2007 at 10:53:00AM +0000, Junio C Hamano wrote:
> > ...
> >> This is just a quick idea before I go back to sleep, but your earlier
> >> comment on "--no-<an-option-that-is-not-even-boolean>" made me realize
> >> that the alternative I was suggesting earlier would actually work much
> >> nicer, if you introduce "--<an-option-that-take-optional-arg>-default"
> >> magic.
> >
> >   meeeow I love the idea !
> 
> There is a bit more serious issue than coding, actually.
> 
> Short options.
> 
> A script wants to use default rename detection threshold for unknown
> commit $foo whose name might look like a number.  IOW, this
> 
> 	git diff -M $foo
> 
> could be ambiguous.  Obviously, "git diff -M-default $foo" would not fly
> very well.

Yes, I thought about that too actually.

After having written this mail 4 time already, I came up with an idea I
kind of like: like find, we could make {} be a placeholder for the
"default" argument. For example:

  $ git foo --abbrev {} 10
  $ git log -M {} 1
  ...

{} would have the same semantics as your --long-opt-default. It tells the
option parser that "no there isn't anything to grok for that command thank you
very much". Of course if for some reason you really want to pass "{}" to the
command, the stuck form holds:

  $ git foo --long-opt={}
  $ git foo -o{}

What do you think ?

PS: I know that in some shells {} needs escaping, which isn't nice. I chose it
    because it's the same as find(1) but we could e.g. use '_' that is less
    "conventional" (if it even makes sense) but is a bit easier to type than \{}.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] convert shortlog to use parse_options

[Kristian Høgsberg]

On Sun, 2007-12-16 at 23:59 -0800, Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> > Junio C Hamano wrote:
> > 
> >> ...  The
> >> "must stick" restriction feels Ok on paper but in practice it looks
> >> rather draconian and very user unfriendly.
> >
> > Usually, optional arguments warrant adding a second parameter. This can
> > often even improve usability, as it's never unclear or ambiguous what's
> > happening. For the 'git tag -l' case, I'd use something like
> > 'git tag -l --match="regex"' or some such,...
> 
> That is essentially arguing for POSIXly correct "do not allow optional
> option-arguments" (utility syntax guidelines #7).  That position might
> be politically correct, but I am already discussing beyond that:
> usability.
> 
> For "git tag -l", the fix was rather simple, as the option would either
> have taken a zero pattern (list all) or a single pattern (list matching
> this pattern), and the command itself did not take any extra arguments,
> so that was what I did in the patch.  Compare your POSIXly correct
> version:
> 
>         git tag -l			(ok)
>         git tag -l pattern		(not ok)
> 	git tag -l --match=pattern	(ok)
> 
> with the traditional (and fixed):
> 
>         git tag -l			(ok)
>         git tag -l pattern		(ok)
> 	git tag -l pattern garbage	(not ok)
> 
> Which one is easier for the user?

git tag now lists tags by default so there's a couple of other options
to consider:

        git tag				(ok)
        git tag --match pattern		(ok)
	git tag --match=pattern     	(ok)
	git tag --match=pattern -l	(ok)
	git tag -l pattern		(not ok)

Or we could repurpose -l as the match option:

	git tag			(ok)
	git tag -l		(not ok)
	git tag -l pattern	(ok)

And that was the point I was trying to make earlier with my rather
abstract sounding post about jumping through hoops.  If we can't break
the options interface to make git sane, we'll be stuck with a broken
command line interface and must write complicated documents on what option
sticking is and how it works.

cheers,
Kristian

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> After having written this mail 4 time already, I came up with an idea I
> kind of like: like find, we could make {} be a placeholder for the
> "default" argument. For example:
>
>   $ git foo --abbrev {} 10
>   $ git log -M {} 1
>   ...
>
> {} would have the same semantics as your --long-opt-default. It tells the
> option parser that "no there isn't anything to grok for that command thank you
> very much". Of course if for some reason you really want to pass "{}" to the
> command, the stuck form holds:
>
>   $ git foo --long-opt={}
>   $ git foo -o{}
>
> What do you think ?

1. {} means a completely different thing to find ("place the real value
   here"); there is no similarity.  I would strongly oppose to it.  If
   you want to invoke opt with default but still want to pass "{}" as an
   argument unrelated to that opt, you would do "--opt={} {}".  That's
   double ugly.

2. For a long option with optional option-argument, --abbrev-default (or
   in the other order, --default-abbrev) to mark "there is no option
   argument, do not do your context sensitive parsing" and using an
   explicit '=' (e.g. --abbrev=<value>) to mark "this is the argument,
   do not do your context sensitive parsing" is much more readable.

3. There are only handful options with optional option-argument that
   does not have long format.  I think it is reasonable to require
   "stuck argument" to them.  For most of the short options that take
   optional option-argument, traditionally we did not allow them to be
   spelled as separate words, so there is no regression to introduce
   such a behaviour.  -B/-M/-C options to diff family would be handled
   sanely this way.

   Another possibility, which I do not like very much, is to add long
   format to them, if only for paranoid scripters who want rename
   detection with the default threshold and cannot say "diff -M $foo".
   They can say "diff --detect-rename-default $foo" instead ("-M" is a
   bad example here, as giving a single path never makes sense for -M so
   $foo cannot be a file whose name is e.g. "20", and default number of
   abbreviated commit object name is longer than 2 which means it would
   make it longer than "percentage" form of threshold).

So in short, for an option that takes optional option-argument:

   - if it is given as "--<long-name>-default", there is no optional
     argument, period.

   - if it is given as "--long-name" but there is no next word, there is
     no optional argument, either.

   - if it is given as "--long-name=value", that "value" is the
     argument.  Barf if it does not validate.

   - if it is given as "--long-name", and there is a next word, see if
     that is plausible as its argument.  Get it and signal the caller
     you consumed it, if it is.  Ignore it and signal the caller you
     didn't, if it isn't.

   - if it is given as "-svalue", that "value" is the argument.  Barf if
     it does not validate.

   - if it is given as "-s", and there is a next word, and if the option
     has long format counterpart as well, then see if the next word is
     plausible as its argument.  Get it and signal the caller you
     consumed it, if it is.  Ignore it and signal the caller you didn't,
     if it isn't.

   - if it is given as "-s" but the previous rule did not trigger, there
     is no optional argument.

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Jeff King]

On Mon, Dec 17, 2007 at 11:52:29AM -0800, Junio C Hamano wrote:

> So in short, for an option that takes optional option-argument:

I agree with everything you said, except...

>    - if it is given as "--long-name", and there is a next word, see if
>      that is plausible as its argument.  Get it and signal the caller
>      you consumed it, if it is.  Ignore it and signal the caller you
>      didn't, if it isn't.

This "plausible" makes me a little nervous, and I wonder why we want to
support this at all. Is it

  1. We have traditionally supported "--abbrev 10"? I don't think this
     is the case.
  2. Consistency with "--non-optional-arg foo"? Do we have any such
     non-optional long arguments? I didn't see any; I think we stick
     with --non-optional-arg=foo everywhere.
  3. More convenience to the user? I don't see how " " is easier than
     "=".

>    - if it is given as "-s", and there is a next word, and if the option
>      has long format counterpart as well, then see if the next word is
>      plausible as its argument.  Get it and signal the caller you
>      consumed it, if it is.  Ignore it and signal the caller you didn't,
>      if it isn't.

Similarly, what is the goal here?

  1. Have we ever supported "-s foo"? Not for -B/-M/-C, nor for
     shortlog's -w.
  2. This would add consistency to non-optional arguments.
  3. It's longer to type.

So I see a slight case for "-s foo", but none at all for "--long foo".

-Peff

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Mon, Dec 17, 2007 at 08:31:43PM +0000, Jeff King wrote:
> On Mon, Dec 17, 2007 at 11:52:29AM -0800, Junio C Hamano wrote:
> 
> > So in short, for an option that takes optional option-argument:
> 
> I agree with everything you said, except...
> 
> >    - if it is given as "--long-name", and there is a next word, see if
> >      that is plausible as its argument.  Get it and signal the caller
> >      you consumed it, if it is.  Ignore it and signal the caller you
> >      didn't, if it isn't.
> 
> This "plausible" makes me a little nervous, and I wonder why we want to
> support this at all. Is it
> 
>   1. We have traditionally supported "--abbrev 10"? I don't think this
>      is the case.

  Yes, that's why the restriction bugs me a bit too.

>   2. Consistency with "--non-optional-arg foo"? Do we have any such
>      non-optional long arguments? I didn't see any; I think we stick
>      with --non-optional-arg=foo everywhere.

  there are some, I don't recall the exact commands, but option parsing
was quite inconsistent in git (well still is), there are the very simple
loops that just do strcmp and look for the '=', there are the loops that
allow interleaving of options and arguments (and that rewrite argc/argv
a bit like parseopt does) and also the ones that allow the separate
mode, and the one that do both.

  The force-stick-mode is a regression for them.

> >    - if it is given as "-s", and there is a next word, and if the option
> >      has long format counterpart as well, then see if the next word is
> >      plausible as its argument.  Get it and signal the caller you
> >      consumed it, if it is.  Ignore it and signal the caller you didn't,
> >      if it isn't.
> 
> Similarly, what is the goal here?
> 
>   1. Have we ever supported "-s foo"? Not for -B/-M/-C, nor for
>      shortlog's -w.

  Yes for git tag -n for example, and there are some other
examples, look at maint for commands that have been migrated to
parse_options, some behave like that, and more than one for sure.

>   3. It's longer to type.

  It's way more readable, but YMMV.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 17, 2007 at 11:52:29AM -0800, Junio C Hamano wrote:
>
>> So in short, for an option that takes optional option-argument:
>
> I agree with everything you said, except...
>
>>    - if it is given as "--long-name", and there is a next word, see if
>>      that is plausible as its argument.  Get it and signal the caller
>>      you consumed it, if it is.  Ignore it and signal the caller you
>>      didn't, if it isn't.
>
> This "plausible" makes me a little nervous, and I wonder why we want to
> support this at all. Is it
>
>   1. We have traditionally supported "--abbrev 10"? I don't think this
>      is the case.
>   2. Consistency with "--non-optional-arg foo"? Do we have any such
>      non-optional long arguments? I didn't see any; I think we stick
>      with --non-optional-arg=foo everywhere.
>   3. More convenience to the user? I don't see how " " is easier than
>      "=".

You forgot one case.

    4. Everybody who does _not_ know that we traditionally did not
       support the form would expect "--abbrev 10" and "-n 4" to work.

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Jeff King]

On Mon, Dec 17, 2007 at 12:42:51PM -0800, Junio C Hamano wrote:

> You forgot one case.
> 
>     4. Everybody who does _not_ know that we traditionally did not
>        support the form would expect "--abbrev 10" and "-n 4" to work.

I would expect "-n 4" to work, but not "--abbrev 10". But perhaps that
is just me. If that is the expectation, I think the behavior you
outlined is sensible.

-Peff

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Mon, Dec 17, 2007 at 08:42:04PM +0000, Pierre Habouzit wrote:
> On Mon, Dec 17, 2007 at 08:31:43PM +0000, Jeff King wrote:
> > On Mon, Dec 17, 2007 at 11:52:29AM -0800, Junio C Hamano wrote:
> > 
> > > So in short, for an option that takes optional option-argument:
> > 
> > I agree with everything you said, except...
> > 
> > >    - if it is given as "--long-name", and there is a next word, see if
> > >      that is plausible as its argument.  Get it and signal the caller
> > >      you consumed it, if it is.  Ignore it and signal the caller you
> > >      didn't, if it isn't.
> > 
> > This "plausible" makes me a little nervous, and I wonder why we want to
> > support this at all. Is it
> > 
> >   1. We have traditionally supported "--abbrev 10"? I don't think this
> >      is the case.
> 
>   Yes, that's why the restriction bugs me a bit too.

  Err I misread your point, _yes_ we do, see builtin-show-ref.c, or see
--start-number in builtin-log.c. There is a precedent.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

On Mon, Dec 17, 2007 at 07:52:29PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > After having written this mail 4 time already, I came up with an idea I
> > kind of like: like find, we could make {} be a placeholder for the
> > "default" argument. For example:
> >
> >   $ git foo --abbrev {} 10
> >   $ git log -M {} 1
> >   ...
> >
> > {} would have the same semantics as your --long-opt-default. It tells the
> > option parser that "no there isn't anything to grok for that command thank you
> > very much". Of course if for some reason you really want to pass "{}" to the
> > command, the stuck form holds:
> >
> >   $ git foo --long-opt={}
> >   $ git foo -o{}
> >
> > What do you think ?
> 
> 1. {} means a completely different thing to find ("place the real value
>    here"); there is no similarity.  I would strongly oppose to it.  If

  okay we could make it be '_', but …

>    you want to invoke opt with default but still want to pass "{}" as an
>    argument unrelated to that opt, you would do "--opt={} {}".  That's
>    double ugly.

Actually that would be --opt {} {} (or --opt _ _) and indeed it's not
very nice (euphemism for it sucks hard).

I like the *-default idea a lot, but it's not really useful if we cannot
fix the single letter switches at the same time. I had the idea to use !
(or any other less shell-magical char) like this: --abbrev! but it
doesn't fly a lot better for single letter switches.

Well maybe it's not worth fighting any longer, we should stick to the
stuck form then :)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Mon, Dec 17, 2007 at 08:53:55PM +0000, Jeff King wrote:
> On Mon, Dec 17, 2007 at 12:42:51PM -0800, Junio C Hamano wrote:
> 
> > You forgot one case.
> > 
> >     4. Everybody who does _not_ know that we traditionally did not
> >        support the form would expect "--abbrev 10" and "-n 4" to work.
> 
> I would expect "-n 4" to work, but not "--abbrev 10". But perhaps that
> is just me. If that is the expectation, I think the behavior you
> outlined is sensible.

FWIW that's exactly the opposite for me. -n4 is easy to type, and I
always do that. Though on a keyboard, ' ' is under the thumb, '=' is
harder to catch, so I tend to prefer when CLIs don't force me to use =.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Jeff King]

On Mon, Dec 17, 2007 at 10:01:16PM +0100, Pierre Habouzit wrote:

>   Err I misread your point, _yes_ we do, see builtin-show-ref.c, or see
> --start-number in builtin-log.c. There is a precedent.

Ugh. Well, in that case, it seems we are stuck with it, and I think
the behavior Junio laid out is the right course of action.

-Peff

Re: [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Mon, Dec 17, 2007 at 11:07:29PM +0000, Jeff King wrote:
> On Mon, Dec 17, 2007 at 10:01:16PM +0100, Pierre Habouzit wrote:
> 
> >   Err I misread your point, _yes_ we do, see builtin-show-ref.c, or see
> > --start-number in builtin-log.c. There is a precedent.
> 
> Ugh. Well, in that case, it seems we are stuck with it, and I think
> the behavior Junio laid out is the right course of action.

Well I agree, I was mostly trying to show what the code could look like
if we tried to be more clever. I'm fine for enforcing the sticked usage
for optional flags, I was the one advocating it in the first place
anyways. I just wanted to be sure we didn't missed something obvious.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]