[RFH] convert shortlog to use parse_options

Subsystems: the rest

14 messages, 4 authors, 2016-06-15 · open the first message on its own page

[RFH] convert shortlog to use parse_options

From: Jeff King <hidden>
Date: 2016-06-15 22:43:58

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

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

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

Re: [RFH] convert shortlog to use parse_options

From: Jeff King <hidden>
Date: 2016-06-15 22:43:58

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

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

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

Re: [RFH] convert shortlog to use parse_options

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

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:
quoted
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

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

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

Signed-off-by: Pierre Habouzit <redacted>
---
 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.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

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 <redacted>
---
 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.

From: Wincent Colaiuta <hidden>
Date: 2016-06-15 22:43:58

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...
quoted hunk
@@ -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.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:58

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ó:
quoted
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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:59

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 <redacted>
---
 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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:59

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: [PATCH] (squashme) gitcli documentation fixups

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:59

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

Re: [PATCH] (squashme) gitcli documentation fixups

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:59

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

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

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:59

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help