[PATCH, v4] git-tag: introduce --cleanup option

[PATCH, v4] git-tag: introduce --cleanup option

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

Normally git tag stripes tag message lines starting with '#', trailing
spaces from every line and empty lines from the beginning and end.

--cleanup allows to select different cleanup modes for tag message.
It provides the same interface as --cleanup option in git-commit.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 Documentation/git-tag.txt |   10 ++++++
 builtin/tag.c             |   69 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index c83cb13..c2d73f3 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,6 +99,16 @@ OPTIONS
 	Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
 	is given.
 
+--cleanup=<mode>::
+	This option sets how the tag message is cleaned up.
+	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
+	and 'default'. The 'default' mode will strip leading and
+	trailing empty lines and #commentary from the tag message
+	only if the message is to be edited. Otherwise only whitespace
+	removed. The 'verbatim' mode does not change message at all,
+	'whitespace' removes just leading/trailing whitespace lines
+	and 'strip' removes both whitespace and commentary.
+
 <tagname>::
 	The name of the tag to create, delete, or describe.
 	The new tag name must pass all checks defined by
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..27a66a3 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -268,6 +268,15 @@ static const char tag_template[] =
 	N_("\n"
 	"#\n"
 	"# Write a tag message\n"
+	"# Lines starting with '#' will be ignored.\n"
+	"#\n");
+
+static const char tag_template_nocleanup[] =
+	N_("\n"
+	"#\n"
+	"# Write a tag message\n"
+	"# Lines starting with '#' will be kept; you may remove them"
+	" yourself if you want to.\n"
 	"#\n");
 
 static void set_signingkey(const char *value)
@@ -319,8 +328,18 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 	return 0;
 }
 
+struct create_tag_options {
+	unsigned int message;
+	unsigned int sign;
+	enum {
+		CLEANUP_NONE,
+		CLEANUP_SPACE,
+		CLEANUP_ALL
+	} cleanup_mode;
+};
+
 static void create_tag(const unsigned char *object, const char *tag,
-		       struct strbuf *buf, int message, int sign,
+		       struct strbuf *buf, struct create_tag_options *opt,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -345,7 +364,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (header_len > sizeof(header_buf) - 1)
 		die(_("tag header too big."));
 
-	if (!message) {
+	if (!opt->message) {
 		int fd;
 
 		/* write the template message before editing: */
@@ -356,8 +375,12 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 		if (!is_null_sha1(prev))
 			write_tag_body(fd, prev);
+		else if (opt->cleanup_mode == CLEANUP_ALL)
+			write_or_die(fd, _(tag_template),
+					strlen(_(tag_template)));
 		else
-			write_or_die(fd, _(tag_template), strlen(_(tag_template)));
+			write_or_die(fd, _(tag_template_nocleanup),
+					strlen(_(tag_template_nocleanup)));
 		close(fd);
 
 		if (launch_editor(path, buf, NULL)) {
@@ -367,14 +390,15 @@ static void create_tag(const unsigned char *object, const char *tag,
 		}
 	}
 
-	stripspace(buf, 1);
+	if (opt->cleanup_mode != CLEANUP_NONE)
+		stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
 
-	if (!message && !buf->len)
+	if (opt->message && !buf->len)
 		die(_("no tag message?"));
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, opt->sign, result) < 0) {
 		if (path)
 			fprintf(stderr, _("The tag message has been left in %s\n"),
 				path);
@@ -422,9 +446,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct ref_lock *lock;
-
-	int annotate = 0, sign = 0, force = 0, lines = -1,
-		list = 0, delete = 0, verify = 0;
+	struct create_tag_options opt;
+	char *cleanup_arg = NULL;
+	int annotate = 0, force = 0, lines = -1, list = 0,
+	    delete = 0, verify = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -442,7 +467,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &msg, "message",
 			     "tag message", parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, "read message from file"),
-		OPT_BOOLEAN('s', "sign", &sign, "annotated and GPG-signed tag"),
+		OPT_BOOLEAN('s', "sign", &opt.sign, "annotated and GPG-signed tag"),
+		OPT_STRING(0, "cleanup", &cleanup_arg, "default",
+			"how to strip spaces and #comments from message"),
 		OPT_STRING('u', "local-user", &keyid, "key-id",
 					"use another key to sign the tag"),
 		OPT__FORCE(&force, "replace the tag if exists"),
@@ -459,13 +486,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_tag_config, NULL);
 
+	opt.sign = 0;
+
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
 	if (keyid) {
-		sign = 1;
+		opt.sign = 1;
 		set_signingkey(keyid);
 	}
-	if (sign)
+	if (opt.sign)
 		annotate = 1;
 	if (argc == 0 && !(delete || verify))
 		list = 1;
@@ -523,9 +552,21 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else if (!force)
 		die(_("tag '%s' already exists"), tag);
 
+	opt.message = msg.given || msgfile;
+
+	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
+		opt.cleanup_mode = !opt.message ? CLEANUP_ALL : CLEANUP_SPACE;
+	else if (!strcmp(cleanup_arg, "verbatim"))
+		opt.cleanup_mode = CLEANUP_NONE;
+	else if (!strcmp(cleanup_arg, "whitespace"))
+		opt.cleanup_mode = CLEANUP_SPACE;
+	else if (!strcmp(cleanup_arg, "strip"))
+		opt.cleanup_mode = CLEANUP_ALL;
+	else
+		die(_("Invalid cleanup mode %s"), cleanup_arg);
+
 	if (annotate)
-		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+		create_tag(object, tag, &buf, &opt, prev, object);
 
 	lock = lock_any_ref_for_update(ref.buf, prev, 0);
 	if (!lock)
-- 
1.7.7.3

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Jeff King]

On Sun, Dec 04, 2011 at 06:20:26AM +0200, Kirill A. Shutemov wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> Normally git tag stripes tag message lines starting with '#', trailing
> spaces from every line and empty lines from the beginning and end.

s/stripes/strips

> --cleanup allows to select different cleanup modes for tag message.
> It provides the same interface as --cleanup option in git-commit.

Thanks, I think this is better, though it would be better still if they
could share the code. As a minor nit, I think the patch would be a
little easier to read and review if you split the actual changes from
the refactoring to use the "struct create_tag_options".

More importantly, though, this seems to break t6300 badly. I haven't
looked into why yet, though.

-Peff

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Jeff King]

On Sun, Dec 04, 2011 at 06:20:26AM +0200, Kirill A. Shutemov wrote:

> @@ -367,14 +390,15 @@ static void create_tag(const unsigned char *object, const char *tag,
> [...]
> -	if (!message && !buf->len)
> +	if (opt->message && !buf->len)
>  		die(_("no tag message?"));

Ah, this is the hunk that causes t6300 to fail. You accidentally removed
the negation when converting the "message" flag over.

This was much easier to find by splitting the refactoring (where the bug
is) away from the new feature (which is where I assumed the bug was).
Here's the first half of the "split". You can rebase your original patch
on top to get the second half.

I also looked at factoring out the "which cleanup mode to select" logic
from builtin/commit.c, but it turned out to just make things harder to
follow.

-- >8 --
From: "Kirill A. Shutemov" <kirill@shutemov.name>
Subject: [PATCH] tag: refactor passing tag creation options

Rather than continually adding parameters to the create_tag
function, we can put all of the flags in a struct.

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, I'm not sure this really buys us much as a refactoring. It
saves a parameter in the function, but it's not like we end up passing
all those parameters to sub-functions, where something like this would
increase readability. I'm fine with this, but I'd also be fine with just
dropping this half and passing the cleanup_mode parameter directly.

 builtin/tag.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..e5bd708 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -319,8 +319,13 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 	return 0;
 }
 
+struct create_tag_options {
+	unsigned int message;
+	unsigned int sign;
+};
+
 static void create_tag(const unsigned char *object, const char *tag,
-		       struct strbuf *buf, int message, int sign,
+		       struct strbuf *buf, struct create_tag_options *opt,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -345,7 +350,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (header_len > sizeof(header_buf) - 1)
 		die(_("tag header too big."));
 
-	if (!message) {
+	if (!opt->message) {
 		int fd;
 
 		/* write the template message before editing: */
@@ -369,12 +374,12 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	stripspace(buf, 1);
 
-	if (!message && !buf->len)
+	if (!opt->message && !buf->len)
 		die(_("no tag message?"));
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, opt->sign, result) < 0) {
 		if (path)
 			fprintf(stderr, _("The tag message has been left in %s\n"),
 				path);
@@ -422,9 +427,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct ref_lock *lock;
-
-	int annotate = 0, sign = 0, force = 0, lines = -1,
-		list = 0, delete = 0, verify = 0;
+	struct create_tag_options opt;
+	int annotate = 0, force = 0, lines = -1, list = 0,
+	    delete = 0, verify = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -442,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &msg, "message",
 			     "tag message", parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, "read message from file"),
-		OPT_BOOLEAN('s', "sign", &sign, "annotated and GPG-signed tag"),
+		OPT_BOOLEAN('s', "sign", &opt.sign, "annotated and GPG-signed tag"),
 		OPT_STRING('u', "local-user", &keyid, "key-id",
 					"use another key to sign the tag"),
 		OPT__FORCE(&force, "replace the tag if exists"),
@@ -459,13 +464,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_tag_config, NULL);
 
+	opt.sign = 0;
+
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
 	if (keyid) {
-		sign = 1;
+		opt.sign = 1;
 		set_signingkey(keyid);
 	}
-	if (sign)
+	if (opt.sign)
 		annotate = 1;
 	if (argc == 0 && !(delete || verify))
 		list = 1;
@@ -523,9 +530,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else if (!force)
 		die(_("tag '%s' already exists"), tag);
 
+	opt.message = msg.given || msgfile;
+
 	if (annotate)
-		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+		create_tag(object, tag, &buf, &opt, prev, object);
 
 	lock = lock_any_ref_for_update(ref.buf, prev, 0);
 	if (!lock)
-- 
1.7.8.rc4.4.g884ec

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Jeff King]

On Mon, Dec 05, 2011 at 05:27:24PM -0500, Jeff King wrote:

> I also looked at factoring out the "which cleanup mode to select" logic
> from builtin/commit.c, but it turned out to just make things harder to
> follow.

While I was doing that, I also noticed this minor fix:

-- >8 --
Subject: [PATCH] stripspace: fix outdated comment

The comment on top of stripspace() claims that the buffer
will no longer be NUL-terminated. However, this has not been
the case at least since the move to using strbuf in 2007.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/stripspace.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 4d3b93f..1288ffc 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -22,8 +22,6 @@ static size_t cleanup(char *line, size_t len)
  * Remove empty lines from the beginning and end
  * and also trailing spaces from every line.
  *
- * Note that the buffer will not be NUL-terminated.
- *
  * Turn multiple consecutive empty lines between paragraphs
  * into just one empty line.
  *
-- 
1.7.8.rc4.4.g884ec

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> More importantly, though, this seems to break t6300 badly. I haven't
> looked into why yet, though.

Also breaks 7004 which is _about_ tags.

Rolling a broken patch in quick succession to v4 without ever running
tests (and not adding new test pieces to protect new feature) is not a
very productive way to use the reviewer bandwidth on this list.

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> More importantly, though, this seems to break t6300 badly. I haven't
> looked into why yet, though.

Probably two issues.

 - opt.message (and the original 'message') was misnamed and confused the
   patch author what "if (!message && !buf->len)" meant.

 - "opt" is a structure meant to be extensible, but is not initialized as
   a whole, inviting future errors.

It still seems to be broken with respect to the primary thing the patch
wanted to do (t7400 "git tag -F commentsfile comments-annotated-tag" does
not seem to produce an expected result), so I'll kick it back to the
Kirill to look at.

Thanks.

 builtin/tag.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 27a66a3..7883720 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -329,7 +329,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 }
 
 struct create_tag_options {
-	unsigned int message;
+	unsigned int message_given:1;
 	unsigned int sign;
 	enum {
 		CLEANUP_NONE,
@@ -364,7 +364,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (header_len > sizeof(header_buf) - 1)
 		die(_("tag header too big."));
 
-	if (!opt->message) {
+	if (!opt->message_given) {
 		int fd;
 
 		/* write the template message before editing: */
@@ -393,7 +393,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (opt->cleanup_mode != CLEANUP_NONE)
 		stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
 
-	if (opt->message && !buf->len)
+	if (!opt->message_given && !buf->len)
 		die(_("no tag message?"));
 
 	strbuf_insert(buf, 0, header_buf, header_len);
@@ -486,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_tag_config, NULL);
 
-	opt.sign = 0;
+	memset(&opt, 0, sizeof(opt));
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
@@ -552,10 +552,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else if (!force)
 		die(_("tag '%s' already exists"), tag);
 
-	opt.message = msg.given || msgfile;
+	opt.message_given = msg.given || msgfile;
 
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
-		opt.cleanup_mode = !opt.message ? CLEANUP_ALL : CLEANUP_SPACE;
+		opt.cleanup_mode = !opt.message_given ? CLEANUP_ALL : CLEANUP_SPACE;
 	else if (!strcmp(cleanup_arg, "verbatim"))
 		opt.cleanup_mode = CLEANUP_NONE;
 	else if (!strcmp(cleanup_arg, "whitespace"))

Re: [PATCH, v4] git-tag: introduce --cleanup option

[Kirill A. Shutemov]

On Mon, Dec 05, 2011 at 02:41:26PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > More importantly, though, this seems to break t6300 badly. I haven't
> > looked into why yet, though.
> 
> Probably two issues.
> 
>  - opt.message (and the original 'message') was misnamed and confused the
>    patch author what "if (!message && !buf->len)" meant.
> 
>  - "opt" is a structure meant to be extensible, but is not initialized as
>    a whole, inviting future errors.

Sorry for that and thanks for the investigation.

> It still seems to be broken with respect to the primary thing the patch
> wanted to do (t7400 "git tag -F commentsfile comments-annotated-tag" does
> not seem to produce an expected result), so I'll kick it back to the
> Kirill to look at.

CLEANUP_ALL should always be default regardless of opt.message_given.

I'll send new version.

-- 
 Kirill A. Shutemov