{"thread":{"id":"29580","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","startedAt":"2012-02-09T16:08:18Z","lastAt":"2012-02-09T21:34:25Z","messageCount":7,"participants":["Jonathan Nieder","Junio C Hamano","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"184240","messageId":"20120209160803.GA5742@burratino","threadId":"29580","inReplyTo":"20120209153431.GA24033@godiug.sigxcpu.org","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-09T16:08:18Z","receivedAt":"2012-02-09T16:08:18Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nGuido Günther wrote[1]:\n\n> as of 1.7.9 merging in a tag unconditionally spawns an editor. I tried\n> turning this of with --no-edit but to no avail. This is a behaviour\n> change that breaks tools like git-buildpackage. I wonder if this should\n> be turned off by default?\n\nThanks.  I can confirm this: ever since commit fab47d05 (merge: force\nedit and no-ff mode when merging a tag object, 2011-11-07), running\n\"git checkout github/maint-1.5.6 && git merge --no-edit v1.7.2\"\nlaunches an editor window despite the caller's request.  And I agree\nthat it is counter-intuitive.\n\nHere's a quick band-aid.  Unlike the case of [2], this does not\nsuppress the pulling-signed-tag magic when it kicks in, so the\nresulting commit will record the signature and message of the tag you\nmerged, even though you have not carefully looked it over.  Not sure\nif that's a good or bad thing yet.\n\nPatch relies on \"merge: use editor by default in interactive sessions\"\nfrom master.  If this looks like a sane approach, I can resend with a\nproposed log message and a test for t/t7600-merge.sh.  (Or if someone\nelse wants to do it first, even better.)\n\nHmm?\nJonathan\n\n[1] http://bugs.debian.org/659255\n[2] http://thread.gmane.org/gmane.comp.version-control.git/189825/focus=189989\n\ndiff --git i/builtin/merge.c w/builtin/merge.c\nindex 62c7b681..c401106e 100644\n--- i/builtin/merge.c\n+++ w/builtin/merge.c\n@@ -1323,7 +1323,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tif (merge_remote_util(commit) &&\n \t\t    merge_remote_util(commit)->obj &&\n \t\t    merge_remote_util(commit)->obj->type == OBJ_TAG) {\n-\t\t\toption_edit = 1;\n+\t\t\tif (option_edit < 0)\n+\t\t\t\toption_edit = 1;\n \t\t\tallow_fast_forward = 0;\n \t\t}\n \t}\n"},{"id":"184245","messageId":"7vzkcrx4f2.fsf@alter.siamese.dyndns.org","threadId":"29580","inReplyTo":"20120209160803.GA5742@burratino","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T18:11:45Z","receivedAt":"2012-02-09T18:11:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> as of 1.7.9 merging in a tag unconditionally spawns an editor. I tried\n>> turning this of with --no-edit but to no avail. This is a behaviour\n>> change that breaks tools like git-buildpackage. I wonder if this should\n>> be turned off by default?\n>\n> Thanks.  I can confirm this: ever since commit fab47d05 (merge: force\n> edit and no-ff mode when merging a tag object, 2011-11-07), running\n> \"git checkout github/maint-1.5.6 && git merge --no-edit v1.7.2\"\n> launches an editor window despite the caller's request.  And I agree\n> that it is counter-intuitive.\n\nThanks; it is unquestionably a bug that \"git merge --no-edit v1.7.2\"\nspawns an editor.\n\nThe real question is what should happen.\n\nIf the editor is not spawned, there is no way for the user to review the\nresult of signature verification before deciding to accept the merge.\n\"git merge --no-edit v1.7.2\" could error out saying \"you cannot create\nthis merge without reviewing\".  Or it could behave as if it was asked to\n\"git merge --no-edit v1.7.2^0\", dropping the signature verification and\nrecording part altogether.\n"},{"id":"184247","messageId":"4F34158D.3030903@kdbg.org","threadId":"29580","inReplyTo":"7vzkcrx4f2.fsf@alter.siamese.dyndns.org","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-02-09T18:50:53Z","receivedAt":"2012-02-09T18:50:53Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.02.2012 19:11, schrieb Junio C Hamano:\n> If the editor is not spawned, there is no way for the user to review the\n> result of signature verification before deciding to accept the merge.\n> \"git merge --no-edit v1.7.2\" could error out saying \"you cannot create\n> this merge without reviewing\".  Or it could behave as if it was asked to\n> \"git merge --no-edit v1.7.2^0\", dropping the signature verification and\n> recording part altogether.\n\nIt should behave as if the editor was spawned and the user did not\nchange the content of the commit message.\n\nUse case: First, you merge ordinarily, that is, you review the signature\nand the contents, and you are satisfied. Shortly later, you discover\nthat a fix should be applied before the merge. So you rewind the branch\nbefore the merge, and commit the fix. Now you can repeat the merge with\n--no-edit because you have already seen the contents.\n\nContrived? Dunno.\n\n-- Hannes\n"},{"id":"184250","messageId":"20120209190131.GG6249@burratino","threadId":"29580","inReplyTo":"7vzkcrx4f2.fsf@alter.siamese.dyndns.org","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-09T19:01:31Z","receivedAt":"2012-02-09T19:01:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> If the editor is not spawned, there is no way for the user to review the\n> result of signature verification before deciding to accept the merge.\n> \"git merge --no-edit v1.7.2\" could error out saying \"you cannot create\n> this merge without reviewing\".  Or it could behave as if it was asked to\n> \"git merge --no-edit v1.7.2^0\", dropping the signature verification and\n> recording part altogether.\n\nOthers might want to read the tag message and check its signature,\neven though the original committer was in a non-interactive rush.\n\nHere's a toy patch (untested) to avoid wasting time verifying a\nsignature only to throw away the result.  It's not actually a no-op,\nsince if conflicts are encountered, the operator who intervenes won't\nget to see whether the signature was valid when her editor of choice\nis launched.\n\ndiff --git i/builtin.h w/builtin.h\nindex 857b9c8a..62dffbdc 100644\n--- i/builtin.h\n+++ w/builtin.h\n@@ -17,6 +17,7 @@ extern void prune_packed_objects(int);\n \n struct fmt_merge_msg_opts {\n \tunsigned add_title:1;\n+\tunsigned skip_tag_verification:1;\n \tint shortlog_len;\n };\n \ndiff --git i/builtin/fmt-merge-msg.c w/builtin/fmt-merge-msg.c\nindex c81a7fef..e99030d3 100644\n--- i/builtin/fmt-merge-msg.c\n+++ w/builtin/fmt-merge-msg.c\n@@ -316,7 +316,8 @@ static void fmt_tag_signature(struct strbuf *tagbuf,\n \tstrbuf_add_lines(tagbuf, \"# \", sig->buf, sig->len);\n }\n \n-static void fmt_merge_msg_sigs(struct strbuf *out)\n+static void fmt_merge_msg_sigs(struct strbuf *out,\n+\t\t\t       struct fmt_merge_msg_opts *opts)\n {\n \tint i, tag_number = 0, first_tag = 0;\n \tstruct strbuf tagbuf = STRBUF_INIT;\n@@ -332,8 +333,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\t\tgoto next;\n \t\tlen = parse_signature(buf, size);\n \n-\t\tif (size == len)\n-\t\t\t; /* merely annotated */\n+\t\tif (size == len || opts->skip_tag_verification)\n+\t\t\t; /* merely annotated, or caller disabled signature check */\n \t\telse if (verify_signed_buffer(buf, len, buf + len, size - len, &sig)) {\n \t\t\tif (!sig.len)\n \t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n@@ -400,7 +401,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t\tfmt_merge_msg_title(out, current_branch);\n \n \tif (origins.nr)\n-\t\tfmt_merge_msg_sigs(out);\n+\t\tfmt_merge_msg_sigs(out, opts);\n \n \tif (opts->shortlog_len) {\n \t\tstruct commit *head;\ndiff --git i/builtin/merge.c w/builtin/merge.c\nindex c401106e..75e027b6 100644\n--- i/builtin/merge.c\n+++ w/builtin/merge.c\n@@ -1295,6 +1295,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\topts.add_title = !have_message;\n \t\t\topts.shortlog_len = shortlog_len;\n \n+\t\t\tassert(0 <= option_edit);\n+\t\t\tif (option_commit && !option_edit)\n+\t\t\t\topts.skip_tag_verification = 1;\n+\n \t\t\tfmt_merge_msg(&merge_names, &merge_msg, &opts);\n \t\t\tif (merge_msg.len)\n \t\t\t\tstrbuf_setlen(&merge_msg, merge_msg.len - 1);\n"},{"id":"184259","messageId":"7v4nuzwzn4.fsf@alter.siamese.dyndns.org","threadId":"29580","inReplyTo":"4F34158D.3030903@kdbg.org","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T19:54:55Z","receivedAt":"2012-02-09T19:54:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> It should behave as if the editor was spawned and the user did not\n> change the content of the commit message.\n> ...\n> Contrived? Dunno.\n\nNo, just \"Sane\".\n\nThanks. And I think Jonathan's original patch is just fine as-is; the\ncommented verification result should be stripspace()'ed out when we make\nthe final commit if we do so ourselves, and will stay if the user ends up\nhaving to help resolving the merge.\n"},{"id":"184269","messageId":"7vpqdnvh3b.fsf@alter.siamese.dyndns.org","threadId":"29580","inReplyTo":"20120209160803.GA5742@burratino","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T21:20:56Z","receivedAt":"2012-02-09T21:20:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Patch relies on \"merge: use editor by default in interactive sessions\"\n> from master.  If this looks like a sane approach, I can resend with a\n> proposed log message and a test for t/t7600-merge.sh.  (Or if someone\n> else wants to do it first, even better.)\n\nPlease do not depend on that one that will never be merged to 1.7.9.x\nmaintenance track.\n\nYour approach is the right one, though.  A patch for 1.7.9.1, on top of\nb5c9f1c (merge: do not create a signed tag merge under --ff-only option,\n2012-02-05), would look like this, I think.\n\nI think various \"0 < option_edit\" we can find in f824628 (merge: use\neditor by default in interactive sessions, 2012-01-10) and f26af3f (merge:\nadd instructions to the commit message when editing, 2012-01-30) can and\nshould be reverted back to just \"option_edit\", as nobody should be looking\nat option_edit before the \"if edit is negative, set it to the default\" we\ncan see in the last hunk of this patch (except for the \"merging tag? turn\nediting on by default unless --no-edit is given\" that is the topic of this\nthread).\n\nThanks.\n\n-- >8 --\nSubject: merge: do not launch an editor on \"--no-edit $tag\"\n\nWhen the user explicitly asked us not to, don't launch an editor.\n\nBut do everything else the same way as the \"edit\" case, i.e. leave the\ncomment with verification result in the log template and record the\nmergesig in the resulting merge commit for later inspection.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/merge.c |   10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b4fbc60..f385b8a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -48,7 +48,7 @@ static const char * const builtin_merge_usage[] = {\n \n static int show_diffstat = 1, shortlog_len = -1, squash;\n static int option_commit = 1, allow_fast_forward = 1;\n-static int fast_forward_only, option_edit;\n+static int fast_forward_only, option_edit = -1;\n static int allow_trivial = 1, have_message;\n static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n@@ -193,7 +193,7 @@ static struct option builtin_merge_options[] = {\n \t\t\"create a single commit instead of doing a merge\"),\n \tOPT_BOOLEAN(0, \"commit\", &option_commit,\n \t\t\"perform a commit if the merge succeeds (default)\"),\n-\tOPT_BOOLEAN('e', \"edit\", &option_edit,\n+\tOPT_BOOL('e', \"edit\", &option_edit,\n \t\t\"edit message before committing\"),\n \tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n \t\t\"allow fast-forward (default)\"),\n@@ -1287,11 +1287,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t    merge_remote_util(commit) &&\n \t\t    merge_remote_util(commit)->obj &&\n \t\t    merge_remote_util(commit)->obj->type == OBJ_TAG) {\n-\t\t\toption_edit = 1;\n+\t\t\tif (option_edit < 0)\n+\t\t\t\toption_edit = 1;\n \t\t\tallow_fast_forward = 0;\n \t\t}\n \t}\n \n+\tif (option_edit < 0)\n+\t\toption_edit = 0;\n+\n \tif (!use_strategies) {\n \t\tif (!remoteheads->next)\n \t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n"},{"id":"184271","messageId":"7vliobvggu.fsf@alter.siamese.dyndns.org","threadId":"29580","inReplyTo":"7vpqdnvh3b.fsf@alter.siamese.dyndns.org","subject":"Re: git merge <tag>: Spawning an editor can't be disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T21:34:25Z","receivedAt":"2012-02-09T21:34:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Your approach is the right one, though.  A patch for 1.7.9.1, on top of\n> b5c9f1c (merge: do not create a signed tag merge under --ff-only option,\n> 2012-02-05), would look like this, I think.\n> ...\n\nI tweaked the message to credit your initial analysis and added the\nfollowing test (again on top of b5c9f1c).\n\nThanks.\n\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex a598dfa..9e27bbf 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -683,4 +683,16 @@ test_expect_success GPG 'merge --ff-only tag' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success GPG 'merge --no-edit tag should skip editor' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit tag -f -s -m \"A newer commit\" signed &&\n+\tgit reset --hard c0 &&\n+\n+\tEDITOR=false git merge --no-edit signed &&\n+\tgit rev-parse signed^0 >expect &&\n+\tgit rev-parse HEAD^2 >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n"}]}