[PATCH] tag: add --edit option

[PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
---
 Documentation/git-tag.txt |  6 ++++++
 builtin/tag.c             | 11 +++++++++--
 t/t7004-tag.sh            | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..b9e5a993bea0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.
 	Implies `-a` if none of `-a`, `-s`, or `-u <keyid>`
 	is given.
 
+-e::
+--edit::
+	The message taken from file with `-F` and command line with
+	`-m` are usually used as the tag message unmodified.
+	This option lets you further edit the message taken from these sources.
+
 --cleanup=<mode>::
 	This option sets how the tag message is cleaned up.
 	The  '<mode>' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..91c60829d5f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu
 
 struct create_tag_options {
 	unsigned int message_given:1;
+	unsigned int use_editor:1;
 	unsigned int sign;
 	enum {
 		CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,
 		    tag,
 		    git_committer_info(IDENT_STRICT));
 
-	if (!opt->message_given) {
+	if (!opt->message_given || opt->use_editor) {
 		int fd;
 
 		/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, const char *tag,
 		if (fd < 0)
 			die_errno(_("could not create file '%s'"), path);
 
-		if (!is_null_oid(prev)) {
+		if (opt->message_given) {
+			write_or_die(fd, buf->buf, buf->len);
+			strbuf_reset(buf);
+		} else if (!is_null_oid(prev)) {
 			write_tag_body(fd, prev);
 		} else {
 			struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	struct ref_format format = REF_FORMAT_INIT;
 	int icase = 0;
+	int edit_flag = 0;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &msg, N_("message"),
 			     N_("tag message"), parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
+		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 		OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("mode"),
 			N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("tag '%s' already exists"), tag);
 
 	opt.message_given = msg.given || msgfile;
+	opt.use_editor = edit_flag;
 
 	if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
 		opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..60e3a53f297f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,23 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor
+'
+test_expect_success \
+	'creating an annotated tag with -m message --edit should succeed' '
+	EDITOR=./editor	git tag -m "A message" --edit annotated-tag-edit &&
+	get_tag_msg annotated-tag-edit >actual &&
+	test_cmp expect actual
+'
+
 cat >msgfile <<EOF
 Another message
 in a file.
@@ -465,6 +482,23 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+get_tag_header file-annotated-tag-edit $commit commit $time >expect
+sed -e "s/Another message/Another edited message/g" msgfile >>expect
+test_expect_success 'set up editor' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e "s/Another message/Another edited message/g" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor
+'
+test_expect_success \
+	'creating an annotated tag with -F messagefile --edit should succeed' '
+	EDITOR=./editor	git tag -F msgfile --edit file-annotated-tag-edit &&
+	get_tag_msg file-annotated-tag-edit >actual &&
+	test_cmp expect actual
+'
+
 cat >inputmsg <<EOF
 A message from the
 standard input
-- 
2.16.1.72.g5be1f00a9a70.dirty

Re: [PATCH] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
<nmoreychaisemartin@suse.com> wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
>
> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,
> -       if (!opt->message_given) {
> +       if (!opt->message_given || opt->use_editor) {
>
> -               if (!is_null_oid(prev)) {
> +               if (opt->message_given) {
> +                       write_or_die(fd, buf->buf, buf->len);
> +                       strbuf_reset(buf);
> +               } else if (!is_null_oid(prev)) {
>                         write_tag_body(fd, prev);
>                 } else {

A little below this change is where launch_editor() is actually
invoked. If it fails for some reason, it prints:

    Please supply the message using either -m or -F option.

which seems a bit counterintuitive if the user *did* specify one of
those options along with --edit. I wonder if that message needs to be
adjusted.

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -452,6 +452,23 @@ test_expect_success \
> +get_tag_header annotated-tag-edit $commit commit $time >expect
> +echo "An edited message" >>expect
> +test_expect_success 'set up editor' '
> +       cat >editor <<-\EOF &&
> +       #!/bin/sh
> +       sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> +       mv "$1-" "$1"
> +       EOF
> +       chmod 755 editor

If you use write_script() to create the fake editor, then it supplies
the #!/bin/sh line for you and does the 'chmod', so you only need to
supply the actual script payload. Also, other "editors" in this test
file are named "fakeeditor", so perhaps follow suit.

    write_script fakeeditor <<-\EOF
        sed -e "s/A message/An edited message/g" <"$1" >"$1-"
        mv "$1-" "$1"
    EOF

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemartin@suse.com> wrote:
>> Add a --edit option whichs allows modifying the messages provided by -m or -F,
>> the same way git commit --edit does.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
>> ---
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,
>> -       if (!opt->message_given) {
>> +       if (!opt->message_given || opt->use_editor) {
>>
>> -               if (!is_null_oid(prev)) {
>> +               if (opt->message_given) {
>> +                       write_or_die(fd, buf->buf, buf->len);
>> +                       strbuf_reset(buf);
>> +               } else if (!is_null_oid(prev)) {
>>                         write_tag_body(fd, prev);
>>                 } else {
> A little below this change is where launch_editor() is actually
> invoked. If it fails for some reason, it prints:
>
>     Please supply the message using either -m or -F option.
>
> which seems a bit counterintuitive if the user *did* specify one of
> those options along with --edit. I wonder if that message needs to be
> adjusted.
>
Yes I'll fix this.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,23 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
>> +test_expect_success 'set up editor' '
>> +       cat >editor <<-\EOF &&
>> +       #!/bin/sh
>> +       sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +       mv "$1-" "$1"
>> +       EOF
>> +       chmod 755 editor
> If you use write_script() to create the fake editor, then it supplies
> the #!/bin/sh line for you and does the 'chmod', so you only need to
> supply the actual script payload. Also, other "editors" in this test
> file are named "fakeeditor", so perhaps follow suit.
>
>     write_script fakeeditor <<-\EOF
>         sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>         mv "$1-" "$1"
>     EOF
>
I dumbly copied the test from commit --edit as it was my reference.
I'll fix the names and switch to write_script.

Thanks

Nicolas

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>
> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
>> <nmoreychaisemartin@suse.com> wrote:
>>> Add a --edit option whichs allows modifying the messages provided by -m or -F,
>>> the same way git commit --edit does.
>>>
>>> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
>>> ---
>>> diff --git a/builtin/tag.c b/builtin/tag.c
>>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,
>>> -       if (!opt->message_given) {
>>> +       if (!opt->message_given || opt->use_editor) {
>>>
>>> -               if (!is_null_oid(prev)) {
>>> +               if (opt->message_given) {
>>> +                       write_or_die(fd, buf->buf, buf->len);
>>> +                       strbuf_reset(buf);
>>> +               } else if (!is_null_oid(prev)) {
>>>                         write_tag_body(fd, prev);
>>>                 } else {
>> A little below this change is where launch_editor() is actually
>> invoked. If it fails for some reason, it prints:
>>
>>     Please supply the message using either -m or -F option.
>>
>> which seems a bit counterintuitive if the user *did* specify one of
>> those options along with --edit. I wonder if that message needs to be
>> adjusted.
>>
> Yes I'll fix this.
I just checked what commit.c does and it seems to behave as my patch:
        if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
            fprintf(stderr,
            _("Please supply the message using either -m or -F option.\n"));
            exit(1);
        }


To be honest the message is not that clear either.
If I'm reading launch_editor right most (or all) its falire are du to a failure to launch the editor or the editor crashed/exited with an error.
In this case, I wouldn't advise the user to use -m or -F but to fix its editor.

Nicolas

Re: [PATCH] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 5:43 AM, Nicolas Morey-Chaisemartin
<NMoreyChaisemartin@suse.de> wrote:
> Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>>> A little below this change is where launch_editor() is actually
>>> invoked. If it fails for some reason, it prints:
>>>
>>>     Please supply the message using either -m or -F option.
>>>
>>> which seems a bit counterintuitive if the user *did* specify one of
>>> those options along with --edit. I wonder if that message needs to be
>>> adjusted.
>>>
>> Yes I'll fix this.
> I just checked what commit.c does and it seems to behave as my patch:
>         if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>             fprintf(stderr,
>             _("Please supply the message using either -m or -F option.\n"));
>
> To be honest the message is not that clear either.
> If I'm reading launch_editor right most (or all) its falire are du to a failure to launch the editor or the editor crashed/exited with an error.
> In this case, I wouldn't advise the user to use -m or -F but to fix its editor.

Indeed, I also looked at the implementation of launch_editor(), and my
"wondering" about whether the message needed adjustment was just that.
The message seems somewhat counterintuitive in this case, but I didn't
necessarily have a better suggestion. A valid response, therefore,
might be to punt on it and leave that change for the future, or
perhaps take it on as a second patch which adjusts the message in both
commands. I don't have strong feelings about it at this time.

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:56, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 5:43 AM, Nicolas Morey-Chaisemartin
> <NMoreyChaisemartin@suse.de> wrote:
>> Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>>> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>>>> A little below this change is where launch_editor() is actually
>>>> invoked. If it fails for some reason, it prints:
>>>>
>>>>     Please supply the message using either -m or -F option.
>>>>
>>>> which seems a bit counterintuitive if the user *did* specify one of
>>>> those options along with --edit. I wonder if that message needs to be
>>>> adjusted.
>>>>
>>> Yes I'll fix this.
>> I just checked what commit.c does and it seems to behave as my patch:
>>         if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>>             fprintf(stderr,
>>             _("Please supply the message using either -m or -F option.\n"));
>>
>> To be honest the message is not that clear either.
>> If I'm reading launch_editor right most (or all) its falire are du to a failure to launch the editor or the editor crashed/exited with an error.
>> In this case, I wouldn't advise the user to use -m or -F but to fix its editor.
> Indeed, I also looked at the implementation of launch_editor(), and my
> "wondering" about whether the message needed adjustment was just that.
> The message seems somewhat counterintuitive in this case, but I didn't
> necessarily have a better suggestion. A valid response, therefore,
> might be to punt on it and leave that change for the future, or
> perhaps take it on as a second patch which adjusts the message in both
> commands. I don't have strong feelings about it at this time.

It seems all the error paths from launch_editor have an error message.
A simple "Editor failure, cancelling {commit, tag}" would probably be a better error message.
I'll post another series for that.

Re: [PATCH] tag: add --edit option

[Ramsay Jones]

On 01/02/18 09:49, Nicolas Morey-Chaisemartin wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
> 
> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
> ---
>  Documentation/git-tag.txt |  6 ++++++
>  builtin/tag.c             | 11 +++++++++--
>  t/t7004-tag.sh            | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 956fc019f984..b9e5a993bea0 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.
>  	Implies `-a` if none of `-a`, `-s`, or `-u <keyid>`
>  	is given.
>  
> +-e::
> +--edit::
> +	The message taken from file with `-F` and command line with
> +	`-m` are usually used as the tag message unmodified.
> +	This option lets you further edit the message taken from these sources.
> +
>  --cleanup=<mode>::
>  	This option sets how the tag message is cleaned up.
>  	The  '<mode>' can be one of 'verbatim', 'whitespace' and 'strip'.  The
> diff --git a/builtin/tag.c b/builtin/tag.c
> index a7e6a5b0f234..91c60829d5f9 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu
>  
>  struct create_tag_options {
>  	unsigned int message_given:1;
> +	unsigned int use_editor:1;
>  	unsigned int sign;
>  	enum {
>  		CLEANUP_NONE,
> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,
>  		    tag,
>  		    git_committer_info(IDENT_STRICT));
>  
> -	if (!opt->message_given) {
> +	if (!opt->message_given || opt->use_editor) {
>  		int fd;
>  
>  		/* write the template message before editing: */
> @@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, const char *tag,
>  		if (fd < 0)
>  			die_errno(_("could not create file '%s'"), path);
>  
> -		if (!is_null_oid(prev)) {
> +		if (opt->message_given) {
> +			write_or_die(fd, buf->buf, buf->len);
> +			strbuf_reset(buf);
> +		} else if (!is_null_oid(prev)) {
>  			write_tag_body(fd, prev);
>  		} else {
>  			struct strbuf buf = STRBUF_INIT;
> @@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>  	struct ref_format format = REF_FORMAT_INIT;
>  	int icase = 0;
> +	int edit_flag = 0;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
> @@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		OPT_CALLBACK('m', "message", &msg, N_("message"),
>  			     N_("tag message"), parse_msg_arg),
>  		OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
> +		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

s/commit/tag message/ ?

ATB,
Ramsay Jones