Make builtin-tag.c use parse_options.

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

---

Applied to "next".
Comments welcomed.

 builtin-tag.c  |  141 ++++++++++++++++++++++++--------------------------------
 t/t7004-tag.sh |    8 +---
 2 files changed, 61 insertions(+), 88 deletions(-)

index 66e5a58..5af1950 100644
@@ -11,9 +11,15 @@
 #include "refs.h"
 #include "tag.h"
 #include "run-command.h"
-
-static const char builtin_tag_usage[] =
-  "git-tag [-n [<num>]] -l [<pattern>] | [-a | -s | -u <key-id>] [-f | -d | -v] [-m <msg> | -F <file>] <tagname> [<head>]";
+#include "parse-options.h"
+
+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 -v <tagname>...",
+	NULL
+};
 
 static char signingkey[1000];
 

@@ -308,101 +314,74 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
 	unsigned char object[20], prev[20];
-	int annotate = 0, sign = 0, force = 0, lines = 0, message = 0;
 	char ref[PATH_MAX];
 	const char *object_ref, *tag;
-	int i;
 	struct ref_lock *lock;
 
+	int annotate = 0, sign = 0, force = 0, lines = 0,
+					delete = 0, verify = 0;
+	char *list = NULL, *msg = NULL, *msgfile = NULL, *keyid = NULL;
+	const char *no_pattern = "NO_PATTERN";
+	struct option options[] = {
+		{ OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
+			PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
+		{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
+				"print n lines of each tag message",
+				PARSE_OPT_OPTARG, NULL, 1 },
+		OPT_BOOLEAN('d', NULL, &delete, "delete tags"),
+		OPT_BOOLEAN('v', NULL, &verify, "verify tags"),
+
+		OPT_GROUP("Tag creation options"),
+		OPT_BOOLEAN('a', NULL, &annotate,
+					"annotated tag, needs a message"),
+		OPT_STRING('m', NULL, &msg, "msg", "message for the tag"),
+		OPT_STRING('F', NULL, &msgfile, "file", "message in a file"),
+		OPT_BOOLEAN('s', NULL, &sign, "annotated and GPG-signed tag"),
+		OPT_STRING('u', NULL, &keyid, "key-id",
+					"use another key to sign the tag"),
+		OPT_BOOLEAN('f', NULL, &force, "replace the tag if exists"),
+		OPT_END()
+	};
+
 	git_config(git_tag_config);
-	strbuf_init(&buf, 0);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, options, git_tag_usage, 0);
 
-		if (arg[0] != '-')
-			break;
-		if (!strcmp(arg, "-a")) {
-			annotate = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-s")) {
-			annotate = 1;
-			sign = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f")) {
-			force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n")) {
-			if (i + 1 == argc || *argv[i + 1] == '-')
-				/* no argument */
-				lines = 1;
-			else
-				lines = isdigit(*argv[++i]) ?
-					atoi(argv[i]) : 1;
-			continue;
-		}
-		if (!strcmp(arg, "-m")) {
-			annotate = 1;
-			i++;
-			if (i == argc)
-				die("option -m needs an argument.");
-			if (message)
-				die("only one -F or -m option is allowed.");
-			strbuf_addstr(&buf, argv[i]);
-			message = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-F")) {
-			annotate = 1;
-			i++;
-			if (i == argc)
-				die("option -F needs an argument.");
-			if (message)
-				die("only one -F or -m option is allowed.");
-
-			if (!strcmp(argv[i], "-")) {
+	if (list)
+		return list_tags(list == no_pattern ? NULL : list, lines);
+	if (delete)
+		return for_each_tag_name(argv, delete_tag);
+	if (verify)
+		return for_each_tag_name(argv, verify_tag);
+
+	strbuf_init(&buf, 0);
+	if (msg || msgfile) {
+		if (msg && msgfile)
+			die("only one -F or -m option is allowed.");
+		annotate = 1;
+		if (msg)
+			strbuf_addstr(&buf, msg);
+		else {
+			if (!strcmp(msgfile, "-")) {
 				if (strbuf_read(&buf, 0, 1024) < 0)
-					die("cannot read %s", argv[i]);
+					die("cannot read %s", msgfile);
 			} else {
-				if (strbuf_read_file(&buf, argv[i], 1024) < 0)
+				if (strbuf_read_file(&buf, msgfile, 1024) < 0)
 					die("could not open or read '%s': %s",
-						argv[i], strerror(errno));
+						msgfile, strerror(errno));
 			}
-			message = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u")) {
-			annotate = 1;
-			sign = 1;
-			i++;
-			if (i == argc)
-				die("option -u needs an argument.");
-			if (strlcpy(signingkey, argv[i], sizeof(signingkey))
-							>= sizeof(signingkey))
-				die("argument to option -u too long");
-			continue;
 		}
-		if (!strcmp(arg, "-l"))
-			return list_tags(argv[i + 1], lines);
-		if (!strcmp(arg, "-d"))
-			return for_each_tag_name(argv + i + 1, delete_tag);
-		if (!strcmp(arg, "-v"))
-			return for_each_tag_name(argv + i + 1, verify_tag);
-		usage(builtin_tag_usage);
 	}
 
-	if (i == argc) {
+	if (argc == 0) {
 		if (annotate)
-			usage(builtin_tag_usage);
+			usage_with_options(git_tag_usage, options);
 		return list_tags(NULL, lines);
 	}
-	tag = argv[i++];
+	tag = argv[0];
 
-	object_ref = i < argc ? argv[i] : "HEAD";
-	if (i + 1 < argc)
+	object_ref = argc == 2 ? argv[1] : "HEAD";
+	if (argc > 2)
 		die("too many params");
 
 	if (get_sha1(object_ref, object))

@@ -419,7 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die("tag '%s' already exists", tag);
 
 	if (annotate)
-		create_tag(object, tag, &buf, message, sign, object);
+		create_tag(object, tag, &buf, msg || msgfile, sign, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)

index 0d07bc3..4b09d28 100755
@@ -339,20 +339,14 @@ test_expect_success \
 '
 
 test_expect_success \
-	'trying to create tags giving many -m or -F options should fail' '
+	'trying to create tags giving both -m or -F options should fail' '
 	echo "message file 1" >msgfile1 &&
 	echo "message file 2" >msgfile2 &&
 	! tag_exists msgtag &&
-	! git-tag -m "message 1" -m "message 2" msgtag &&
-	! tag_exists msgtag &&
-	! git-tag -F msgfile1 -F msgfile2 msgtag &&
-	! tag_exists msgtag &&
 	! git-tag -m "message 1" -F msgfile1 msgtag &&
 	! tag_exists msgtag &&
 	! git-tag -F msgfile1 -m "message 1" msgtag &&
 	! tag_exists msgtag &&
-	! git-tag -F msgfile1 -m "message 1" -F msgfile2 msgtag &&
-	! tag_exists msgtag &&
 	! git-tag -m "message 1" -F msgfile1 -m "message 2" msgtag &&
 	! tag_exists msgtag
 '

1.5.3.4

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

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

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

2007/11/10, Junio C Hamano <[email protected]>:

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

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

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

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

int parse_opt_stringlist(const struct option *opt, const char *arg, int unset)

{
    string_list **l = opt->value;
    string_list_elem *e;

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

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

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

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

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Alas, this exposes a regression in kh/commit series.

 t/t7501-commit.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

index 1b444d4..bf5dd86 100644
@@ -178,4 +178,27 @@ test_expect_success 'amend commit to fix author' '
 	diff expected current
 
 '
+
+test_expect_success 'sign off' '
+
+	>positive &&
+	git add positive &&
+	git commit -s -m "thank you" &&
+	actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
+	expected=$(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") &&
+	test "z$actual" = "z$expected"
+
+'
+
+test_expect_success 'multiple -m' '
+
+	>negative &&
+	git add negative &&
+	git commit -m "one" -m "two" -m "three" &&
+	actual=$(git cat-file commit HEAD | sed -e "1,/^\$/d") &&
+	expected=$(echo one; echo; echo two; echo; echo three) &&
+	test "z$actual" = "z$expected"
+
+'
+
 test_done

This is an updated patch to the test script...

 t/t7501-commit.sh |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

index b151b51..4dc35bd 100644
@@ -163,4 +163,73 @@ test_expect_success 'partial commit that involves removal (3)' '
 
 '
 
+author="The Real Author <[email protected]>"
+test_expect_success 'amend commit to fix author' '
+
+	oldtick=$GIT_AUTHOR_DATE &&
+	test_tick &&
+	git reset --hard &&
+	git cat-file -p HEAD |
+	sed -e "s/author.*/author $author $oldtick/" \
+		-e "s/^\(committer.*> \).*$/\1$GIT_COMMITTER_DATE/" > \
+		expected &&
+	git commit --amend --author="$author" &&
+	git cat-file -p HEAD > current &&
+	diff expected current
+
+'
+
+test_expect_success 'sign off (1)' '
+
+	echo 1 >positive &&
+	git add positive &&
+	git commit -s -m "thank you" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	(
+		echo thank you
+		echo
+		git var GIT_COMMITTER_IDENT |
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+	) >expected &&
+	diff -u expected actual
+
+'
+
+test_expect_success 'sign off (2)' '
+
+	echo 2 >positive &&
+	git add positive &&
+	existing="Signed-off-by: Watch This <[email protected]>" &&
+	git commit -s -m "thank you
+
+$existing" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	(
+		echo thank you
+		echo
+		echo $existing
+		git var GIT_COMMITTER_IDENT |
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+	) >expected &&
+	diff -u expected actual
+
+'
+
+test_expect_success 'multiple -m' '
+
+	>negative &&
+	git add negative &&
+	git commit -m "one" -m "two" -m "three" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	(
+		echo one
+		echo
+		echo two
+		echo
+		echo three
+	) >expected &&
+	diff -u expected actual
+
+'
+
 test_done

2007/11/10, Junio C Hamano <[email protected]>:

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

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

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

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

...

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

static int opt_parse_F(const struct option *opt, const char *arg, int unset)

{
        struct strbuf *buf = opt->value;

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

Shouldn't be too hard :)
Kristian

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

Also note that:
(1) repeating options was already silent in many git commands, so it's

    not really a regression ;

(2) for many commands it actually make sense to allow repeating (for _BOOLEAN e.g.). And I'd argue that for OPTION_BIT it also makes sense as well.

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

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Hi,

[re Cc:ing jasam]

I guess it is because of the PARSE_OPT_OPTARG thing, together with no_pattern. We need to know if -l was specified, even if no argument was passed in.

Hth,
Dscho