Commit with an empty message broken since v1.8.2.1

Commit a24a41ea9a928ccde2db074ab0835c4817223c9d introduces a bug which is still present in latest master.

This command 

    git commit -m "" --allow-empty --allow-empty-message

should create an empty commit with an empty message and never open a text editor for the commit message. Since the change, the editor is always opened.

My current workaround to skip the editor is setting the environment variable:

    GIT_EDITOR=true

If an empty message is specified with the option -m of git commit then
the editor is started.  That's unexpected and unnecessary.  Instead of
using the length of the message string for checking if the user
specified one, directly remember if the option -m was given.

Reported-by: Mislav Marohnić <redacted>
Signed-off-by: René Scharfe <redacted>
---
 builtin/commit.c  | 10 ++++++----
 t/t7502-commit.sh | 17 +++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d2f30d9..1621dfc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -107,7 +107,7 @@ static const char *cleanup_arg;
 
 static enum commit_whence whence;
 static int use_editor = 1, include_status = 1;
-static int show_ignored_in_status;
+static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 

@@ -121,9 +121,11 @@ static enum {
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
 	struct strbuf *buf = opt->value;
-	if (unset)
+	if (unset) {
+		have_option_m = 0;
 		strbuf_setlen(buf, 0);
-	else {
+	} else {
+		have_option_m = 1;
 		if (buf->len)
 			strbuf_addch(buf, '\n');
 		strbuf_addstr(buf, arg);

@@ -975,7 +977,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die(_("Using both --reset-author and --author does not make sense"));
 
-	if (logfile || message.len || use_message || fixup_message)
+	if (logfile || have_option_m || use_message || fixup_message)
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index a4938b1..6313da2 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -361,6 +361,23 @@ test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
 	test_cmp expect .git/result
 '
 
+test_expect_success 'do not fire editor if -m <msg> was given' '
+	echo tick >file &&
+	git add file &&
+	echo "editor not started" >.git/result &&
+	(GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" git commit -m tick) &&
+	test "$(cat .git/result)" = "editor not started"
+'
+
+test_expect_success 'do not fire editor if -m "" was given' '
+	echo tock >file &&
+	git add file &&
+	echo "editor not started" >.git/result &&
+	(GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" \
+	 git commit -m "" --allow-empty-message) &&
+	test "$(cat .git/result)" = "editor not started"
+'
+
 test_expect_success 'do not fire editor in the presence of conflicts' '
 
 	git clean -f &&

-- 
1.8.3

Commit a24a41e (git-commit: only append a newline to -m mesg if
necessary, 2013-02-18) introduced a regression: when
--allow-empty-message is used and an empty message is explicitly
specified with -m "", git commit still launches $EDITOR unnecessarily.
The commit (correctly) fixes opt_parse_m() to not fill in two newlines
into the message buffer unconditionally.  The real problem is that
launching $EDITOR only depends on use_editor and whether message is
empty.  Fix the problem by setting use_editor to 0 when
--allow-empty-message is specified in the codepath where an explicit
string is passed via -m.

Reported-by: Mislav Marohnić <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index d2f30d9..1f5da9d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -128,6 +128,8 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 			strbuf_addch(buf, '\n');
 		strbuf_addstr(buf, arg);
 		strbuf_complete_line(buf);
+		if (allow_empty_message)
+			use_editor = 0;
 	}
 	return 0;
 }

-- 
1.8.3.1.g33669de

Ramkumar Ramachandra wrote:

 builtin/commit.c | 2 ++
 1 file changed, 2 insertions(+)

I just made this dependent on the order in which options are parsed.
If --allow-empty-message is specified before -m "", it works.
Otherwise, not.

Sorry about the stupidity.

Commit a24a41e (git-commit: only append a newline to -m mesg if
necessary, 2013-02-18) introduced a regression: when
--allow-empty-message is used and an empty message is explicitly
specified with -m "", git commit still launches $EDITOR unnecessarily.
The commit (correctly) fixes opt_parse_m() to not fill in two newlines
into the message buffer unconditionally.  The real problem is that
launching $EDITOR only depends on use_editor and whether message is
empty.  Fix the problem by setting explicit_message in the codepath
where an explicit string is passed via -m, and then checking it before
launching $EDITOR.

Reported-by: Mislav Marohnić <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 Works?

 builtin/commit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d2f30d9..7d72ba7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -108,6 +108,7 @@ static const char *cleanup_arg;
 static enum commit_whence whence;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status;
+static int explicit_message = 0;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 

@@ -128,6 +129,7 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 			strbuf_addch(buf, '\n');
 		strbuf_addstr(buf, arg);
 		strbuf_complete_line(buf);
+		explicit_message = 1;
 	}
 	return 0;
 }

@@ -824,7 +826,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
-	if (use_editor) {
+	if (use_editor && !explicit_message) {
 		char index[PATH_MAX];
 		const char *env[2] = { NULL };
 		env[0] =  index;

-- 
1.8.3.1.g33669de.dirty

So now we have two fixes for the same issue, don't we ?
You probably missed $gmane/225534.

On Mon, May 27, 2013 at 4:20 PM, Ramkumar Ramachandra
[off-list ref] wrote:

Antoine Pelisse wrote:

So now we have two fixes for the same issue, don't we ?
You probably missed $gmane/225534.

Gah, missed that.  Sorry for the noise.