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

On Mon, Nov 14, 2011 at 02:20:23PM -0800, Junio C Hamano wrote:

"Kirill A. Shutemov" [off-list ref] writes:

quoted

From: "Kirill A. Shutemov" <redacted>

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

quoted

Signed-off-by: Kirill A. Shutemov <redacted>

quoted

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.

quoted

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?

quoted

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