[PATCHv2] tag: add --edit option

[PATCHv2] 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>
---

Changes since v1:
- Fix usage string
- Use write_script to generate editor
- Rename editor to fakeeditor to match the other tests in the testsuite
- I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails

 Documentation/git-tag.txt |  6 ++++++
 builtin/tag.c             | 11 +++++++++--
 t/t7004-tag.sh            | 30 ++++++++++++++++++++++++++++++
 3 files changed, 45 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..ce5cac3dd23f 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 tag message")),
 		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..063996ddc05c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,21 @@ 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' '
+	write_script fakeeditor <<-\EOF
+	sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+'
+test_expect_success \
+	'creating an annotated tag with -m message --edit should succeed' '
+	EDITOR=./fakeeditor	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 +480,21 @@ 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' '
+	write_script fakeeditor <<-\EOF
+	sed -e "s/Another message/Another edited message/g" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+'
+test_expect_success \
+	'creating an annotated tag with -F messagefile --edit should succeed' '
+	EDITOR=./fakeeditor	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: [PATCHv2] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 12:21 PM, 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>
> ---
> Changes since v1:
> - Fix usage string
> - Use write_script to generate editor
> - Rename editor to fakeeditor to match the other tests in the testsuite

Thanks for explaining what changed since the previous attempt. It is
also helpful for reviewers if you include a reference to the previous
iteration, like this:
https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7f7c@suse.com/T/#u

Cc:'ing reviewers of previous iterations is also good etiquette when
submitting a new version.

> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails

Typically, it's easier on Junio, from a patch management standpoint,
if you submit all these related patches as a single series.
Alternately, if you do want to submit those changes separately, before
the current patch lands in "master", be sure to mention atop which
patch (this one) the additional patch(es) should live. Thanks.

> diff --git 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.
> +-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.

You probably ought to add this new option to the command synopsis. In
the git-commit man page, the synopsis mentions only '-e' (not --edit),
so perhaps this man page could mirror that one. (Sorry for not
noticing this earlier.)

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -452,6 +452,21 @@ test_expect_success \
> +get_tag_header annotated-tag-edit $commit commit $time >expect
> +echo "An edited message" >>expect

Modern practice is to perform these "expect" setup actions (and all
other actions) within tests themselves rather than outside of tests.
However, consistency also has value, and since this test script is
filled with this sort of stylized "expect" setup already, this may be
fine, and probably not worth a re-roll. (A "modernization" patch by
someone can come later if warranted.)

> +test_expect_success 'set up editor' '
> +       write_script fakeeditor <<-\EOF
> +       sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> +       mv "$1-" "$1"
> +       EOF
> +'

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

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 12:21 PM, 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>
>> ---
>> Changes since v1:
>> - Fix usage string
>> - Use write_script to generate editor
>> - Rename editor to fakeeditor to match the other tests in the testsuite
> Thanks for explaining what changed since the previous attempt. It is
> also helpful for reviewers if you include a reference to the previous
> iteration, like this:
> https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7f7c@suse.com/T/#u
>
> Cc:'ing reviewers of previous iterations is also good etiquette when
> submitting a new version.

I thought I did. My script might be glitchy. Sorry for that.

>
>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails
> Typically, it's easier on Junio, from a patch management standpoint,
> if you submit all these related patches as a single series.
> Alternately, if you do want to submit those changes separately, before
> the current patch lands in "master", be sure to mention atop which
> patch (this one) the additional patch(es) should live. Thanks.

Well this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.
Plus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),
and I don't want this patch to get stuck in the pipe for that :)

>
>> diff --git 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.
>> +-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.
> You probably ought to add this new option to the command synopsis. In
> the git-commit man page, the synopsis mentions only '-e' (not --edit),
> so perhaps this man page could mirror that one. (Sorry for not
> noticing this earlier.)

Yep makes sense.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,21 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
> Modern practice is to perform these "expect" setup actions (and all
> other actions) within tests themselves rather than outside of tests.
> However, consistency also has value, and since this test script is
> filled with this sort of stylized "expect" setup already, this may be
> fine, and probably not worth a re-roll. (A "modernization" patch by
> someone can come later if warranted.)
>
>> +test_expect_success 'set up editor' '
>> +       write_script fakeeditor <<-\EOF
>> +       sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +       mv "$1-" "$1"
>> +       EOF
>> +'

It's probably worth doing a whole cleanup of these (and switch to write script) in a dedicated patch series.

Nicolas

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

[Eric Sunshine]

On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin
<nmoreychaisemartin@suse.com> wrote:
> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>> <nmoreychaisemartin@suse.com> wrote:
>>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails
>> Typically, it's easier on Junio, from a patch management standpoint,
>> if you submit all these related patches as a single series.
>> Alternately, if you do want to submit those changes separately, before
>> the current patch lands in "master", be sure to mention atop which
>> patch (this one) the additional patch(es) should live. Thanks.
>
> Well this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.

Yup, that's a reasonable way to look at it. I see them as related
(thus a potential patch series) simply because the existing error
message in tag.c is fine, as is, until the --edit option is
introduced, after which it becomes a bit iffy. Not a big deal, though.

> Plus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),

As long as you just change the content of double-quoted string, you
shouldn't have to worry about localization. The localization folks
will handle the .po files and whatnot.

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

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 10:57, Eric Sunshine a écrit :
> On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemartin@suse.com> wrote:
>> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
>>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>>> <nmoreychaisemartin@suse.com> wrote:
>>>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails
>>> Typically, it's easier on Junio, from a patch management standpoint,
>>> if you submit all these related patches as a single series.
>>> Alternately, if you do want to submit those changes separately, before
>>> the current patch lands in "master", be sure to mention atop which
>>> patch (this one) the additional patch(es) should live. Thanks.
>> Well this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.
> Yup, that's a reasonable way to look at it. I see them as related
> (thus a potential patch series) simply because the existing error
> message in tag.c is fine, as is, until the --edit option is
> introduced, after which it becomes a bit iffy. Not a big deal, though.

OK ok I'll do it :)
What message do you suggest ?
As I said in a previous mail, a simple "Editor failure, cancelling {commit, tag}" should be enough as launch_editor already outputs error messages describing what issue the editor had.

I don't think suggesting moving to --no-edit || -m || -F is that helpful.
It's basically saying your setup is broken, but you can workaround by setting those options (and not saying that you're going to have some more issues later one).

>> Plus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),
> As long as you just change the content of double-quoted string, you
> shouldn't have to worry about localization. The localization folks
> will handle the .po files and whatnot.

Sounds easy enough :)

Nicolas

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

[Eric Sunshine]

On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin
<nmoreychaisemartin@suse.com> wrote:
> What message do you suggest ?  As I said in a previous mail, a
> simple "Editor failure, cancelling {commit, tag}" should be enough
> as launch_editor already outputs error messages describing what
> issue the editor had.
>
> I don't think suggesting moving to --no-edit || -m || -F is that
> helpful.  It's basically saying your setup is broken, but you can
> workaround by setting those options (and not saying that you're
> going to have some more issues later one).

If it's the case the launch_editor() indeed outputs an appropriate
error message, then the existing error message from tag.c is already
appropriate when --edit is not specified. It's only the --edit case
that the tag.c's additional message is somewhat weird. And, in fact,
suppressing tag.c's message might be the correct thing to do in the
--edit case:

    static void create_tag(...) {
        ...
if (launch_editor(...)) {
   if (!opt->use_editor)
       fprintf(stderr, _("... use either -m or -F ..."));
            exit(1);
}

I don't feel strongly about it either way and am fine with just
punting on the issue until someone actually complains about it.

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

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 20:16, Eric Sunshine a écrit :
> On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemartin@suse.com> wrote:
>> What message do you suggest ?  As I said in a previous mail, a
>> simple "Editor failure, cancelling {commit, tag}" should be enough
>> as launch_editor already outputs error messages describing what
>> issue the editor had.
>>
>> I don't think suggesting moving to --no-edit || -m || -F is that
>> helpful.  It's basically saying your setup is broken, but you can
>> workaround by setting those options (and not saying that you're
>> going to have some more issues later one).
> If it's the case the launch_editor() indeed outputs an appropriate
> error message, then the existing error message from tag.c is already
> appropriate when --edit is not specified.

I don't fully agree with the current message. The right thing to do is to fix the editor, not to hide the issue.
A better message would be "Editor failed. Fix it, or supply the message using either..."
At least we suggest the right way to do it first.

>  It's only the --edit case
> that the tag.c's additional message is somewhat weird. And, in fact,
> suppressing tag.c's message might be the correct thing to do in the
> --edit case:
>
>     static void create_tag(...) {
>         ...
> if (launch_editor(...)) {
>    if (!opt->use_editor)
>        fprintf(stderr, _("... use either -m or -F ..."));
>             exit(1);
> }
>
> I don't feel strongly about it either way and am fine with just
> punting on the issue until someone actually complains about it.

The test should be opt->message_given && opt->use_editor.
If just --edit is provided but no -m/-F, --edit does not have any effect and it should be the same error message as when no option is given.

Nicolas