Re: [PATCH v3 2/3] builtin/tag.c: add --trailer arg

[Junio C Hamano <gitster@pobox.com>]

"John Passaro via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Passaro <john.a.passaro@gmail.com>
>
> git-tag currently supports interpreting trailers from an annotated tag
> message, using --list --format="%(trailers)". There is no ergonomic way
> to add trailers to an annotated tag message.

Well said.  Drop "currently", though.  The usual way to compose a
log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

> In a previous patch, we refactored git-commit's implementation of its
> --trailer arg to the trailer.h API. Let's use that new function to teach
> git-tag the same --trailer argument, emulating as much of git-commit's
> behavior as much as possible.

Nicely described.

> @@ -178,6 +179,19 @@ This option is only applicable when listing tags without annotation lines.
>  	Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
>  	is given.
>  
> +--trailer <token>[(=|:)<value>]::
> +	Specify a (<token>, <value>) pair that should be applied as a
> +	trailer. (e.g. `git tag --trailer "Signed-off-by:T A Ger \
> +	<tagger@example.com>" --trailer "Helped-by:C O Mitter \
> +	<committer@example.com>"` will add the "Signed-off-by" trailer
> +	and the "Helped-by" trailer to the tag message.)
> +	The `trailer.*` configuration variables
> +	(linkgit:git-interpret-trailers[1]) can be used to define if
> +	a duplicated trailer is omitted, where in the run of trailers
> +	each trailer would appear, and other details.
> +	The trailers can be seen in `git tag --list` using
> +	`--format="%(trailers)"` placeholder.

I can see this was copied-and-pasted from git-commit, but I am not
sure if the ones used in the example are good fit for tag objects.
What does Helped-by even mean in the context of an annotated tag?

> @@ -290,10 +292,12 @@ static const char message_advice_nested_tag[] =
>  static void create_tag(const struct object_id *object, const char *object_ref,
>  		       const char *tag,
>  		       struct strbuf *buf, struct create_tag_options *opt,
> -		       struct object_id *prev, struct object_id *result, char *path)
> +		       struct object_id *prev, struct object_id *result,
> +		       struct strvec *trailer_args, char *path)
>  {
>  	enum object_type type;
>  	struct strbuf header = STRBUF_INIT;
> +	int should_edit;
>  
>  	type = oid_object_info(the_repository, object, NULL);
>  	if (type <= OBJ_NONE)
> @@ -313,13 +317,15 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>  		    tag,
>  		    git_committer_info(IDENT_STRICT));
>  
> -	if (!opt->message_given || opt->use_editor) {
> +	should_edit = opt->use_editor || !opt->message_given;
> +	if (should_edit || trailer_args->nr) {
>  		int fd;
>  
>  		/* write the template message before editing: */
>  		fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
>  
> -		if (opt->message_given) {
> +		if (opt->message_given && buf->len) {
> +			strbuf_complete(buf, '\n');
>  			write_or_die(fd, buf->buf, buf->len);
>  			strbuf_reset(buf);
>  		} else if (!is_null_oid(prev)) {
> @@ -338,10 +344,22 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>  		}
>  		close(fd);
>  
> -		if (launch_editor(path, buf, NULL)) {
> -			fprintf(stderr,
> -			_("Please supply the message using either -m or -F option.\n"));
> -			exit(1);
> +		if (trailer_args->nr && amend_file_with_trailers(path, trailer_args))
> +			die(_("unable to pass trailers to --trailers"));
> +
> +		if (should_edit) {
> +			if (launch_editor(path, buf, NULL)) {
> +				fprintf(stderr,
> +					_("Please supply the message using either -m or -F option.\n"));
> +				exit(1);
> +			}
> +		} else if (trailer_args->nr) {

When both should_edit and trailer_args->nr are true, this block will
not be entered.  We first do the "amend_file" thing, and then run an
editor on it, and that is the end of the story in that case.

When we do not have should_edit (e.g., -m "tag message" is given),
we would have run "amend_file" thing on it to tweak the message,
and we come in here.

> +			strbuf_reset(buf);
> +			if (strbuf_read_file(buf, path, 0) < 0) {
> +				fprintf(stderr,
> +					_("Please supply the message using either -m or -F option.\n"));
> +				exit(1);

Does this error message make sense here in this context?  The
earlier one was introduced by 7198203a (editor.c: Libify
launch_editor(), 2008-07-25)---after we fail to run the editor, as
we somehow seem to be unable to run an editor, we suggest the user
to give us a message in other ways.  But this one is different.  The
user gave us in one of these other ways already instead of using an
editor, but mucking with that with the "amend_file" thing somehow
made it unreadable.  Shouldn't it be more like

	die_errno(_("failed to read '%s'"), path);

or something along that line?