[PATCH, v2] tag: implement --[no-]strip option

[PATCH, v2] tag: implement --[no-]strip option

[Kirill A. Shutemov]

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

--strip::
	Remove from tag message lines staring with '#', trailing spaces
	from every line and empty lines from the beginning and end.
	Enabled by default. Use --no-strip to overwrite the behaviour.

--no-strip is useful if you want to take a tag message as-is, without
any stripping.

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

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index c83cb13..dbb76a6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,6 +99,11 @@ OPTIONS
 	Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
 	is given.
 
+--strip::
+	Remove from tag message lines staring with '#', trailing spaces
+	from every line and empty lines from the beginning and end.
+	Enabled by default. Use --no-strip to overwrite the behaviour.
+
 <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..05a1fd4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -319,8 +319,14 @@ 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;
+	unsigned int strip;
+};
+
 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 +351,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,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 		if (!is_null_sha1(prev))
 			write_tag_body(fd, prev);
-		else
+		else if (opt->strip)
 			write_or_die(fd, _(tag_template), strlen(_(tag_template)));
 		close(fd);
 
@@ -367,14 +373,15 @@ static void create_tag(const unsigned char *object, const char *tag,
 		}
 	}
 
-	stripspace(buf, 1);
+	if (opt->strip)
+		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);
@@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	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 = {
+		.sign = 0,
+		.strip = 1,
+	};
+
+	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 +454,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_BOOLEAN(0, "strip", &opt.strip,
+					"turn off tag message stripping"),
 		OPT_STRING('u', "local-user", &keyid, "key-id",
 					"use another key to sign the tag"),
 		OPT__FORCE(&force, "replace the tag if exists"),
@@ -462,10 +476,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	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 +537,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.7.2

Re: [PATCH, v2] tag: implement --[no-]strip option

[Junio C Hamano]

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

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>
> --strip::
> 	Remove from tag message lines staring with '#', trailing spaces
> 	from every line and empty lines from the beginning and end.
> 	Enabled by default. Use --no-strip to overwrite the behaviour.
>
> --no-strip is useful if you want to take a tag message as-is, without
> any stripping.

That is not a commit log message ;-)

> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index c83cb13..dbb76a6 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -99,6 +99,11 @@ OPTIONS
>  	Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
>  	is given.
>  
> +--strip::
> +	Remove from tag message lines staring with '#', trailing spaces
> +	from every line and empty lines from the beginning and end.
> +	Enabled by default. Use --no-strip to overwrite the behaviour.

s/overwrite/override/; or replace the last sentence with "With
`--no-strip`, the tag message given by the user is used as-is".

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9b6fd95..05a1fd4 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> ...
> @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>  
>  		if (!is_null_sha1(prev))
>  			write_tag_body(fd, prev);
> -		else
> +		else if (opt->strip)
>  			write_or_die(fd, _(tag_template), strlen(_(tag_template)));

Why are you not writing template when no strip is done? (Not an objection
disguised as a rhetorical question, but a question).

The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
find it useful to have commented instructions, once we start filling the
template with more useful information that is customized for the
situation (e.g. "git show -s --oneline" output), no?

> @@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	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 = {
> +		.sign = 0,
> +		.strip = 1,
> +	};

Avoid doing this.  Even though these C99 initializers are nicer and more
readable way to write this, we try to be gentle to people with older
compilers that do not grok the syntax.

Except for the above minor nits, the patch basically looks good. Please
hold onto it and resubmit after 1.7.8 final ships.

Thanks.

Re: [PATCH, v2] tag: implement --[no-]strip option

[Kirill A. Shutemov]

On Mon, Nov 14, 2011 at 02:20:23PM -0800, Junio C Hamano wrote:
> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> 
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> >
> > --strip::
> > 	Remove from tag message lines staring with '#', trailing spaces
> > 	from every line and empty lines from the beginning and end.
> > 	Enabled by default. Use --no-strip to overwrite the behaviour.
> >
> > --no-strip is useful if you want to take a tag message as-is, without
> > any stripping.
> 
> That is not a commit log message ;-)

Ok, I'll fix.

> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> 
> > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> > index c83cb13..dbb76a6 100644
> > --- a/Documentation/git-tag.txt
> > +++ b/Documentation/git-tag.txt
> > @@ -99,6 +99,11 @@ OPTIONS
> >  	Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
> >  	is given.
> >  
> > +--strip::
> > +	Remove from tag message lines staring with '#', trailing spaces
> > +	from every line and empty lines from the beginning and end.
> > +	Enabled by default. Use --no-strip to overwrite the behaviour.
> 
> s/overwrite/override/; or replace the last sentence with "With
> `--no-strip`, the tag message given by the user is used as-is".

Ok.

> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9b6fd95..05a1fd4 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > ...
> > @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
> >  
> >  		if (!is_null_sha1(prev))
> >  			write_tag_body(fd, prev);
> > -		else
> > +		else if (opt->strip)
> >  			write_or_die(fd, _(tag_template), strlen(_(tag_template)));
> 
> Why are you not writing template when no strip is done? (Not an objection
> disguised as a rhetorical question, but a question).
> 
> The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
> find it useful to have commented instructions, once we start filling the
> template with more useful information that is customized for the
> situation (e.g. "git show -s --oneline" output), no?

Yes. But in this case commented instructions will not be stripped and they
will go to the message. I think user will be confused.

We can show show some instructions before spawning the editor. What do
you think?

> > @@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >  	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 = {
> > +		.sign = 0,
> > +		.strip = 1,
> > +	};
> 
> Avoid doing this.  Even though these C99 initializers are nicer and more
> readable way to write this, we try to be gentle to people with older
> compilers that do not grok the syntax.

It's sad. Do you have a list of compilers which are important for the
project?

> Except for the above minor nits, the patch basically looks good. Please
> hold onto it and resubmit after 1.7.8 final ships.

Thanks.

-- 
 Kirill A. Shutemov

Re: [PATCH, v2] tag: implement --[no-]strip option

[Junio C Hamano]

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

>> > @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>> >  
>> >  		if (!is_null_sha1(prev))
>> >  			write_tag_body(fd, prev);
>> > -		else
>> > +		else if (opt->strip)
>> >  			write_or_die(fd, _(tag_template), strlen(_(tag_template)));
>> 
>> Why are you not writing template when no strip is done? (Not an objection
>> disguised as a rhetorical question, but a question).
>> 
>> The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
>> find it useful to have commented instructions, once we start filling the
>> template with more useful information that is customized for the
>> situation (e.g. "git show -s --oneline" output), no?
>
> Yes. But in this case commented instructions will not be stripped and they
> will go to the message. I think user will be confused.
>
> We can show show some instructions before spawning the editor. What do
> you think?

My knee-jerk reaction is that it would be worse than what your patch
does. I'd say we'd start from your patch and see how users of 'next'
reacts while the topic is cooking.

Re: [PATCH, v2] tag: implement --[no-]strip option

[Johannes Sixt]

Am 11/14/2011 22:43, schrieb Kirill A. Shutemov:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> --strip::
> 	Remove from tag message lines staring with '#', trailing spaces

s/staring/starting/

> 	from every line and empty lines from the beginning and end.
> 	Enabled by default. Use --no-strip to overwrite the behaviour.
> 
> --no-strip is useful if you want to take a tag message as-is, without
> any stripping.

I would like to know why this is useful. Tag messages are for human
consumption. What benefit is it that whitespace is not stripped? Why are
lines starting with '#' so important that they need to stay in the tag
message?

-- Hannes

Re: [PATCH, v2] tag: implement --[no-]strip option

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 11/14/2011 22:43, schrieb Kirill A. Shutemov:
>> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>> 
>> --strip::
>> 	Remove from tag message lines staring with '#', trailing spaces
>
> s/staring/starting/
>
>> 	from every line and empty lines from the beginning and end.
>> 	Enabled by default. Use --no-strip to overwrite the behaviour.
>> 
>> --no-strip is useful if you want to take a tag message as-is, without
>> any stripping.
>
> I would like to know why this is useful. Tag messages are for human
> consumption. What benefit is it that whitespace is not stripped? Why are
> lines starting with '#' so important that they need to stay in the tag
> message?

A conversion from foreign SCM comes to mind, but that is somewhat an
unfair comment, given that I know by heart how stripspace works and the
above documentation patch incorrectly describes what it does.

Besides stripping "# comment", stripspace collapses consecutive blank
lines, removes trailing blank lines and trailing whitespaces at the end of
lines.