Re: git merge <tag>: Spawning an editor can't be disabled

Re: git merge <tag>: Spawning an editor can't be disabled

[Jonathan Nieder]

Hi,

Guido Günther wrote[1]:

> as of 1.7.9 merging in a tag unconditionally spawns an editor. I tried
> turning this of with --no-edit but to no avail. This is a behaviour
> change that breaks tools like git-buildpackage. I wonder if this should
> be turned off by default?

Thanks.  I can confirm this: ever since commit fab47d05 (merge: force
edit and no-ff mode when merging a tag object, 2011-11-07), running
"git checkout github/maint-1.5.6 && git merge --no-edit v1.7.2"
launches an editor window despite the caller's request.  And I agree
that it is counter-intuitive.

Here's a quick band-aid.  Unlike the case of [2], this does not
suppress the pulling-signed-tag magic when it kicks in, so the
resulting commit will record the signature and message of the tag you
merged, even though you have not carefully looked it over.  Not sure
if that's a good or bad thing yet.

Patch relies on "merge: use editor by default in interactive sessions"
from master.  If this looks like a sane approach, I can resend with a
proposed log message and a test for t/t7600-merge.sh.  (Or if someone
else wants to do it first, even better.)

Hmm?
Jonathan

[1] http://bugs.debian.org/659255
[2] http://thread.gmane.org/gmane.comp.version-control.git/189825/focus=189989

diff --git i/builtin/merge.c w/builtin/merge.c
index 62c7b681..c401106e 100644
--- i/builtin/merge.c
+++ w/builtin/merge.c
@@ -1323,7 +1323,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (merge_remote_util(commit) &&
 		    merge_remote_util(commit)->obj &&
 		    merge_remote_util(commit)->obj->type == OBJ_TAG) {
-			option_edit = 1;
+			if (option_edit < 0)
+				option_edit = 1;
 			allow_fast_forward = 0;
 		}
 	}

Re: git merge <tag>: Spawning an editor can't be disabled

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> as of 1.7.9 merging in a tag unconditionally spawns an editor. I tried
>> turning this of with --no-edit but to no avail. This is a behaviour
>> change that breaks tools like git-buildpackage. I wonder if this should
>> be turned off by default?
>
> Thanks.  I can confirm this: ever since commit fab47d05 (merge: force
> edit and no-ff mode when merging a tag object, 2011-11-07), running
> "git checkout github/maint-1.5.6 && git merge --no-edit v1.7.2"
> launches an editor window despite the caller's request.  And I agree
> that it is counter-intuitive.

Thanks; it is unquestionably a bug that "git merge --no-edit v1.7.2"
spawns an editor.

The real question is what should happen.

If the editor is not spawned, there is no way for the user to review the
result of signature verification before deciding to accept the merge.
"git merge --no-edit v1.7.2" could error out saying "you cannot create
this merge without reviewing".  Or it could behave as if it was asked to
"git merge --no-edit v1.7.2^0", dropping the signature verification and
recording part altogether.

Re: git merge <tag>: Spawning an editor can't be disabled

[Johannes Sixt]

Am 09.02.2012 19:11, schrieb Junio C Hamano:
> If the editor is not spawned, there is no way for the user to review the
> result of signature verification before deciding to accept the merge.
> "git merge --no-edit v1.7.2" could error out saying "you cannot create
> this merge without reviewing".  Or it could behave as if it was asked to
> "git merge --no-edit v1.7.2^0", dropping the signature verification and
> recording part altogether.

It should behave as if the editor was spawned and the user did not
change the content of the commit message.

Use case: First, you merge ordinarily, that is, you review the signature
and the contents, and you are satisfied. Shortly later, you discover
that a fix should be applied before the merge. So you rewind the branch
before the merge, and commit the fix. Now you can repeat the merge with
--no-edit because you have already seen the contents.

Contrived? Dunno.

-- Hannes

Re: git merge <tag>: Spawning an editor can't be disabled

[Jonathan Nieder]

Junio C Hamano wrote:

> If the editor is not spawned, there is no way for the user to review the
> result of signature verification before deciding to accept the merge.
> "git merge --no-edit v1.7.2" could error out saying "you cannot create
> this merge without reviewing".  Or it could behave as if it was asked to
> "git merge --no-edit v1.7.2^0", dropping the signature verification and
> recording part altogether.

Others might want to read the tag message and check its signature,
even though the original committer was in a non-interactive rush.

Here's a toy patch (untested) to avoid wasting time verifying a
signature only to throw away the result.  It's not actually a no-op,
since if conflicts are encountered, the operator who intervenes won't
get to see whether the signature was valid when her editor of choice
is launched.

diff --git i/builtin.h w/builtin.h
index 857b9c8a..62dffbdc 100644
--- i/builtin.h
+++ w/builtin.h
@@ -17,6 +17,7 @@ extern void prune_packed_objects(int);
 
 struct fmt_merge_msg_opts {
 	unsigned add_title:1;
+	unsigned skip_tag_verification:1;
 	int shortlog_len;
 };
 
diff --git i/builtin/fmt-merge-msg.c w/builtin/fmt-merge-msg.c
index c81a7fef..e99030d3 100644
--- i/builtin/fmt-merge-msg.c
+++ w/builtin/fmt-merge-msg.c
@@ -316,7 +316,8 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
 	strbuf_add_lines(tagbuf, "# ", sig->buf, sig->len);
 }
 
-static void fmt_merge_msg_sigs(struct strbuf *out)
+static void fmt_merge_msg_sigs(struct strbuf *out,
+			       struct fmt_merge_msg_opts *opts)
 {
 	int i, tag_number = 0, first_tag = 0;
 	struct strbuf tagbuf = STRBUF_INIT;
@@ -332,8 +333,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 			goto next;
 		len = parse_signature(buf, size);
 
-		if (size == len)
-			; /* merely annotated */
+		if (size == len || opts->skip_tag_verification)
+			; /* merely annotated, or caller disabled signature check */
 		else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig)) {
 			if (!sig.len)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
@@ -400,7 +401,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 		fmt_merge_msg_title(out, current_branch);
 
 	if (origins.nr)
-		fmt_merge_msg_sigs(out);
+		fmt_merge_msg_sigs(out, opts);
 
 	if (opts->shortlog_len) {
 		struct commit *head;
diff --git i/builtin/merge.c w/builtin/merge.c
index c401106e..75e027b6 100644
--- i/builtin/merge.c
+++ w/builtin/merge.c
@@ -1295,6 +1295,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			opts.add_title = !have_message;
 			opts.shortlog_len = shortlog_len;
 
+			assert(0 <= option_edit);
+			if (option_commit && !option_edit)
+				opts.skip_tag_verification = 1;
+
 			fmt_merge_msg(&merge_names, &merge_msg, &opts);
 			if (merge_msg.len)
 				strbuf_setlen(&merge_msg, merge_msg.len - 1);

Re: git merge <tag>: Spawning an editor can't be disabled

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> It should behave as if the editor was spawned and the user did not
> change the content of the commit message.
> ...
> Contrived? Dunno.

No, just "Sane".

Thanks. And I think Jonathan's original patch is just fine as-is; the
commented verification result should be stripspace()'ed out when we make
the final commit if we do so ourselves, and will stay if the user ends up
having to help resolving the merge.

Re: git merge <tag>: Spawning an editor can't be disabled

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Patch relies on "merge: use editor by default in interactive sessions"
> from master.  If this looks like a sane approach, I can resend with a
> proposed log message and a test for t/t7600-merge.sh.  (Or if someone
> else wants to do it first, even better.)

Please do not depend on that one that will never be merged to 1.7.9.x
maintenance track.

Your approach is the right one, though.  A patch for 1.7.9.1, on top of
b5c9f1c (merge: do not create a signed tag merge under --ff-only option,
2012-02-05), would look like this, I think.

I think various "0 < option_edit" we can find in f824628 (merge: use
editor by default in interactive sessions, 2012-01-10) and f26af3f (merge:
add instructions to the commit message when editing, 2012-01-30) can and
should be reverted back to just "option_edit", as nobody should be looking
at option_edit before the "if edit is negative, set it to the default" we
can see in the last hunk of this patch (except for the "merging tag? turn
editing on by default unless --no-edit is given" that is the topic of this
thread).

Thanks.

-- >8 --
Subject: merge: do not launch an editor on "--no-edit $tag"

When the user explicitly asked us not to, don't launch an editor.

But do everything else the same way as the "edit" case, i.e. leave the
comment with verification result in the log template and record the
mergesig in the resulting merge commit for later inspection.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/merge.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b4fbc60..f385b8a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -48,7 +48,7 @@ static const char * const builtin_merge_usage[] = {
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit;
+static int fast_forward_only, option_edit = -1;
 static int allow_trivial = 1, have_message;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -193,7 +193,7 @@ static struct option builtin_merge_options[] = {
 		"create a single commit instead of doing a merge"),
 	OPT_BOOLEAN(0, "commit", &option_commit,
 		"perform a commit if the merge succeeds (default)"),
-	OPT_BOOLEAN('e', "edit", &option_edit,
+	OPT_BOOL('e', "edit", &option_edit,
 		"edit message before committing"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast-forward (default)"),
@@ -1287,11 +1287,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		    merge_remote_util(commit) &&
 		    merge_remote_util(commit)->obj &&
 		    merge_remote_util(commit)->obj->type == OBJ_TAG) {
-			option_edit = 1;
+			if (option_edit < 0)
+				option_edit = 1;
 			allow_fast_forward = 0;
 		}
 	}
 
+	if (option_edit < 0)
+		option_edit = 0;
+
 	if (!use_strategies) {
 		if (!remoteheads->next)
 			add_strategies(pull_twohead, DEFAULT_TWOHEAD);

Re: git merge <tag>: Spawning an editor can't be disabled

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Your approach is the right one, though.  A patch for 1.7.9.1, on top of
> b5c9f1c (merge: do not create a signed tag merge under --ff-only option,
> 2012-02-05), would look like this, I think.
> ...

I tweaked the message to credit your initial analysis and added the
following test (again on top of b5c9f1c).

Thanks.

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index a598dfa..9e27bbf 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -683,4 +683,16 @@ test_expect_success GPG 'merge --ff-only tag' '
 	test_cmp actual expect
 '
 
+test_expect_success GPG 'merge --no-edit tag should skip editor' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git tag -f -s -m "A newer commit" signed &&
+	git reset --hard c0 &&
+
+	EDITOR=false git merge --no-edit signed &&
+	git rev-parse signed^0 >expect &&
+	git rev-parse HEAD^2 >actual &&
+	test_cmp actual expect
+'
+
 test_done