From: Patrick Steinhardt <ps@pks.im>
To: John Passaro via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, John Passaro <john.a.passaro@gmail.com>
Subject: Re: [PATCH] builtin/tag.c: add --trailer arg
Date: Mon, 29 Apr 2024 08:50:01 +0200 [thread overview]
Message-ID: <Zi9DGYwlT7VnW7oj@tanuki> (raw)
In-Reply-To: <pull.1723.git.1714365076246.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7952 bytes --]
On Mon, Apr 29, 2024 at 04:31:15AM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <john.a.passaro@gmail.com>
>
> Teach git-tag to accept --trailer option to add trailers to annotated
> tag messages, like git-commit.
>
> Signed-off-by: John Passaro <john.a.passaro@gmail.com>
This feels like a sensible addition to me indeed, thanks!
[snip]
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9a33cb50b45..0334a5d15ec 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -28,9 +28,11 @@
> #include "date.h"
> #include "write-or-die.h"
> #include "object-file-convert.h"
> +#include "run-command.h"
>
> static const char * const git_tag_usage[] = {
> N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> + " [(--trailer <token>[(=|:)<value>])...]\n"
> " <tagname> [<commit> | <object>]"),
> N_("git tag -d <tagname>..."),
> N_("git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]\n"
> @@ -290,10 +292,11 @@ 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)
This line is overly long now, let's break it.
> {
> 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,14 +316,18 @@ 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) {
> write_or_die(fd, buf->buf, buf->len);
> + if (trailer_args->nr && buf->buf[buf->len-1] != '\n') {
> + write_or_die(fd, "\n", 1);
> + }
We avoid braces around single-line statements.
I was also wondering whether we can simplify this to:
if (opt->message_given && buf->len) {
strbuf_complete(buf, '\n');
write_or_die(fd, buf->buf, buf->len);
}
Or does changing `buf` cause problems for us?
> strbuf_reset(buf);
> } else if (!is_null_oid(prev)) {
> write_tag_body(fd, prev);
> @@ -338,10 +345,31 @@ 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) {
> + struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> + strvec_pushl(&run_trailer.args, "interpret-trailers",
> + "--in-place", "--no-divider",
> + path, NULL);
> + strvec_pushv(&run_trailer.args, trailer_args->v);
> + run_trailer.git_cmd = 1;
> + if (run_command(&run_trailer))
> + die(_("unable to pass trailers to --trailers"));
> + }
This part is copied from `builtin/commit.c`. Would it make sense to move
it into a function `amend_trailers_to_file()` or similar that we add to
our trailer API so that we can avoid the code duplication?
> + if (should_edit) {
> + if (launch_editor(path, buf, NULL)) {
> + fprintf(stderr,
> + _("Please supply the message using either -m or -F option.\n"));
> + exit(1);
> + }
I know you simply re-indented the block here, but let's also fix the
indentation of the second argument to fprintf(3P) while at it.
> + } else if (trailer_args->nr) {
> + 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);
> + }
> }
> }
>
> @@ -416,6 +444,14 @@ struct msg_arg {
> struct strbuf buf;
> };
>
> +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
> +{
> + BUG_ON_OPT_NEG(unset);
> +
> + strvec_pushl(opt->value, "--trailer", arg, NULL);
> + return 0;
> +}
> +
> static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> {
> struct msg_arg *msg = opt->value;
> @@ -463,6 +499,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> struct ref_sorting *sorting;
> struct string_list sorting_options = STRING_LIST_INIT_DUP;
> struct ref_format format = REF_FORMAT_INIT;
> + struct strvec trailer_args = STRVEC_INIT;
> int icase = 0;
> int edit_flag = 0;
> struct option options[] = {
> @@ -479,6 +516,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> OPT_CALLBACK_F('m', "message", &msg, N_("message"),
> N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
> OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> + OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
> OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
> OPT_CLEANUP(&cleanup_arg),
> @@ -548,7 +586,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> opt.sign = 1;
> set_signing_key(keyid);
> }
> - create_tag_object = (opt.sign || annotate || msg.given || msgfile);
> + create_tag_object = (opt.sign || annotate || msg.given || msgfile || edit_flag || trailer_args.nr);
>
> if ((create_tag_object || force) && (cmdmode != 0))
> usage_with_options(git_tag_usage, options);
> @@ -635,7 +673,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> else if (!force)
> die(_("tag '%s' already exists"), tag);
>
> - opt.message_given = msg.given || msgfile;
> + opt.message_given = msg.given || (msgfile != NULL);
> opt.use_editor = edit_flag;
Besides being not required, this change also violates our coding style
where we don't explicitly check for NULL pointers.
> if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
> @@ -653,7 +691,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> if (force_sign_annotate && !annotate)
> opt.sign = 1;
> path = git_pathdup("TAG_EDITMSG");
> - create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
> + create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object, &trailer_args,
> path);
Nit: let's move `&trailer_args` to the next line to not make it overly
long.
> }
>
> @@ -686,6 +724,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> strbuf_release(&reflog_msg);
> strbuf_release(&msg.buf);
> strbuf_release(&err);
> + strvec_clear(&trailer_args);
> free(msgfile);
> return ret;
> }
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 696866d7794..364db2b4685 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -668,6 +668,105 @@ test_expect_success \
> test_cmp expect actual
> '
>
> +# trailers
> +
> +get_tag_header tag-with-inline-message-and-trailers $commit commit $time >expect
> +cat >>expect <<EOF
> +create tag with trailers
> +
> +my-trailer: here
> +alt-trailer: there
> +EOF
You probably just follow precedent in this file, but our modern coding
style sets up the `expect` file inside of the test body itself. You also
do it like that in other tests, so let's be consistent.
The same comment applies to other tests, as well.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-04-29 6:50 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 4:31 [PATCH] builtin/tag.c: add --trailer arg John Passaro via GitGitGadget
2024-04-29 6:50 ` Patrick Steinhardt [this message]
2024-04-29 14:50 ` John Passaro
2024-04-29 15:05 ` John Passaro
2024-04-29 17:07 ` Junio C Hamano
2024-04-29 15:29 ` Junio C Hamano
2024-04-29 16:38 ` John Passaro
2024-04-29 17:04 ` Junio C Hamano
2024-04-29 16:53 ` [PATCH v2] " John Passaro via GitGitGadget
2024-04-29 18:54 ` [PATCH v3 0/3] builtin/tag.c: add --trailer option John Passaro via GitGitGadget
2024-04-29 18:54 ` [PATCH v3 1/3] builtin/commit.c: refactor --trailer logic John Passaro via GitGitGadget
2024-04-30 5:54 ` Patrick Steinhardt
2024-04-30 16:38 ` Junio C Hamano
2024-04-29 18:54 ` [PATCH v3 2/3] builtin/tag.c: add --trailer arg John Passaro via GitGitGadget
2024-04-30 5:54 ` Patrick Steinhardt
2024-04-30 16:53 ` Junio C Hamano
2024-04-30 21:48 ` John Passaro
2024-04-30 22:23 ` Junio C Hamano
2024-05-05 18:59 ` John Passaro
2024-04-29 18:54 ` [PATCH v3 3/3] po: update git-tag translations John Passaro via GitGitGadget
2024-04-29 19:22 ` Junio C Hamano
2024-04-29 19:28 ` John Passaro
2024-04-30 14:41 ` [PATCH v4 0/3] builtin/tag.c: add --trailer option John Passaro via GitGitGadget
2024-04-30 14:41 ` [PATCH v4 1/3] builtin/commit.c: remove bespoke option callback John Passaro via GitGitGadget
2024-05-02 6:27 ` Patrick Steinhardt
2024-04-30 14:41 ` [PATCH v4 2/3] builtin/commit.c: refactor --trailer logic John Passaro via GitGitGadget
2024-05-02 6:27 ` Patrick Steinhardt
2024-04-30 14:41 ` [PATCH v4 3/3] builtin/tag.c: add --trailer option John Passaro via GitGitGadget
2024-05-02 6:27 ` [PATCH v4 0/3] " Patrick Steinhardt
2024-05-05 18:49 ` [PATCH v5 " John Passaro via GitGitGadget
2024-05-05 18:49 ` [PATCH v5 1/3] builtin/commit: use ARGV macro to collect trailers John Passaro via GitGitGadget
2024-05-07 15:38 ` John Passaro
2024-05-07 17:06 ` Junio C Hamano
2024-05-05 18:49 ` [PATCH v5 2/3] builtin/commit: refactor --trailer logic John Passaro via GitGitGadget
2024-05-05 18:49 ` [PATCH v5 3/3] builtin/tag: add --trailer option John Passaro via GitGitGadget
2024-05-06 5:40 ` [PATCH v5 0/3] builtin/tag.c: " Patrick Steinhardt
2024-05-06 17:52 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zi9DGYwlT7VnW7oj@tanuki \
--to=ps@pks.im \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=john.a.passaro@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).