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

["Kirill A. Shutemov" <kirill@shutemov.name>]

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