{"thread":{"id":"29433","subject":"[PATCH] merge: use editor by default in interactive sessions","startedAt":"2012-01-23T22:18:40Z","lastAt":"2012-02-23T20:31:25Z","messageCount":14,"participants":["Junio C Hamano","Michael Haggerty","Thomas Rast","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182988","messageId":"7vipk26p1b.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":null,"subject":"[PATCH] merge: use editor by default in interactive sessions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-23T22:18:40Z","receivedAt":"2012-01-23T22:18:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Traditionally, a cleanly resolved merge was committed by \"git merge\" using\nthe auto-generated merge commit log message with invoking the editor.\n\nAfter 5 years of use in the field, it turns out that people perform too\nmany unjustified merges of the upstream history into their topic branches.\nThese merges are not just useless, but they are often not explained well,\nand making the end result unreadable when it gets time for merging their\nhistory back to their upstream.\n\nEarlier we added the \"--edit\" option to the command, so that people can\nedit the log message to explain and justify their merge commits. Let's\ntake it one step further and spawn the editor by default when we are in an\ninteractive session (i.e. the standard input and the standard output are\npointing at the same tty device).\n\nThere may be existing scripts that leave the standard input and the\nstandard output of the \"git merge\" connected to whatever environment the\nscripts were started, and such invocation might trigger the above\n\"interactive session\" heuristics. GIT_MERGE_AUTOEDIT environment variable\ncan be set to \"no\" at the beginning of such scripts to use the historical\nbehaviour while the script runs.\n\nNote that this backward compatibility is meant only for scripts, and we\ndeliberately do *not* support \"merge.edit = yes/no/auto\" configuration\noption to allow people to keep the historical behaviour.\n\nSuggested-by: Linus Torvalds \nSigned-off-by: Junio C Hamano \n---\n\n * With an update to the documentation, so that I won't forget, even\n though this topic came too late in the cycle and not meant for the\n upcoming 1.7.9.\n\n Documentation/git-merge.txt | 2 +-\n Documentation/merge-options.txt | 16 +++++++++++++---\n builtin/merge.c | 38 ++++++++++++++++++++++++++++++++++----\n t/test-lib.sh | 3 ++-\n 4 files changed, 50 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex e2e6aba..3ceefb8 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -9,7 +9,7 @@ git-merge - Join two or more development histories together\n SYNOPSIS\n --------\n [verse]\n-'git merge' [-n] [--stat] [--no-commit] [--squash]\n+'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n \t[-s ] [-X ]\n \t[--[no-]rerere-autoupdate] [-m ] [...]\n 'git merge' HEAD ...\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 6bd0b04..f2f1d0f 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -8,10 +8,20 @@ failed and do not autocommit, to give the user a chance to\n inspect and further tweak the merge result before committing.\n \n --edit::\n--e::\n+--no-edit::\n+\tInvoke an editor before committing successful mechanical merge to\n+\tfurther edit the auto-generated merge message, so that the user\n+\tcan explain and justify the merge. The `--no-edit` option can be\n+\tused to accept the auto-generated message (this is generally\n+\tdiscouraged). The `--edit` option is still useful if you are\n+\tgiving a draft message with the `-m` option from the command line\n+\tand want to edit it in the editor.\n +\n-\tInvoke editor before committing successful merge to further\n-\tedit the default merge message.\n+Older scripts may depend on the historical behaviour of not allowing the\n+user to edit the merge log message. They will see an editor opened when\n+they run `git merge`. To make it easier to adjust such scripts to the\n+updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be\n+set to `no` at the beginning of them.\n \n --ff::\n --no-ff::\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 99f1429..0006175 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {\n \n static int show_diffstat = 1, shortlog_len, 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 struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n@@ -189,7 +189,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@@ -877,12 +877,12 @@ static void prepare_to_commit(void)\n \twrite_merge_msg(&msg);\n \trun_hook(get_index_file(), \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n-\tif (option_edit) {\n+\tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n \t\t\tabort_commit(NULL);\n \t}\n \tread_merge_msg(&msg);\n-\tstripspace(&msg, option_edit);\n+\tstripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\n \t\tabort_commit(_(\"Empty commit message.\"));\n \tstrbuf_release(&merge_msg);\n@@ -1076,6 +1076,33 @@ static void write_merge_state(void)\n \tclose(fd);\n }\n \n+static int default_edit_option(void)\n+{\n+\tstatic const char name[] = \"GIT_MERGE_AUTOEDIT\";\n+\tconst char *e = getenv(name);\n+\tstruct stat st_stdin, st_stdout;\n+\n+\tif (have_message)\n+\t\t/* an explicit -m msg without --[no-]edit */\n+\t\treturn 0;\n+\n+\tif (e) {\n+\t\tint v = git_config_maybe_bool(name, e);\n+\t\tif (v < 0)\n+\t\t\tdie(\"Bad value '%s' in environment '%s'\", e, name);\n+\t\treturn v;\n+\t}\n+\n+\t/* Use editor if stdin and stdout are the same and is a tty */\n+\treturn (!fstat(0, &st_stdin) &&\n+\t\t!fstat(1, &st_stdout) &&\n+\t\tisatty(0) &&\n+\t\tst_stdin.st_dev == st_stdout.st_dev &&\n+\t\tst_stdin.st_ino == st_stdout.st_ino &&\n+\t\tst_stdin.st_mode == st_stdout.st_mode);\n+}\n+\n+\n int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n@@ -1261,6 +1288,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (option_edit < 0)\n+\t\toption_edit = default_edit_option();\n+\n \tif (!use_strategies) {\n \t\tif (!remoteheads->next)\n \t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..ed32c2a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -63,7 +63,8 @@ GIT_AUTHOR_NAME='A U Thor'\n GIT_COMMITTER_EMAIL=committer@example.com\n GIT_COMMITTER_NAME='C O Mitter'\n GIT_MERGE_VERBOSITY=5\n-export GIT_MERGE_VERBOSITY\n+GIT_MERGE_AUTOEDIT=no\n+export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT\n export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n export EDITOR\n-- \n1.7.9.rc2.48.g92994\n"},{"id":"182990","messageId":"4F1DDECB.5080904@alum.mit.edu","threadId":"29433","inReplyTo":"7vipk26p1b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-23T22:27:23Z","receivedAt":"2012-01-23T22:27:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/23/2012 11:18 PM, Junio C Hamano wrote:\n> Traditionally, a cleanly resolved merge was committed by \"git merge\" using\n> the auto-generated merge commit log message with invoking the editor.\n\ns/with/without/\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"182991","messageId":"7vaa5e6oas.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"4F1DDECB.5080904@alum.mit.edu","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-23T22:34:35Z","receivedAt":"2012-01-23T22:34:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 01/23/2012 11:18 PM, Junio C Hamano wrote:\n>> Traditionally, a cleanly resolved merge was committed by \"git merge\" using\n>> the auto-generated merge commit log message with invoking the editor.\n>\n> s/with/without/\n\nOops, thanks, ... goes to find a hole to crawl into ...\n"},{"id":"183338","messageId":"87hazdazmb.fsf@thomas.inf.ethz.ch","threadId":"29433","inReplyTo":"7vipk26p1b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-30T17:06:52Z","receivedAt":"2012-01-30T17:06:52Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Mon, 23 Jan 2012 14:18:40 -0800, Junio C Hamano wrote:\n> Traditionally, a cleanly resolved merge was committed by \"git merge\" using\n> the auto-generated merge commit log message with invoking the editor.\n> \n> After 5 years of use in the field, it turns out that people perform too\n> many unjustified merges of the upstream history into their topic branches.\n> These merges are not just useless, but they are often not explained well,\n> and making the end result unreadable when it gets time for merging their\n> history back to their upstream.\n\nOk, so I'm late to the party and perhaps I missed the discussion about\nthis, but...\n\nI think the proposed commit message should have a comment, just like for\nan ordinary commit, that explains why we are showing the user an editor.\n(I'm too lazy to check, but I suspect we *always* give a comment about\nwhat is going on when we fire up an editor.)\n\nI would suggest something like\n\n# Please enter the commit message for your merge commit. Lines starting\n# with '#' will be ignored, and an empty message aborts the commit.\n\nor if you feel comfortable with educating the user in a\nworkflow-specific way, even\n\n# Please enter the commit message for your merge commit. You should\n# justify it especially if it merges an updated upstream into a topic\n# branch.\n# \n# Lines starting with '#' will be ignored, and an empty message aborts\n# the commit.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183350","messageId":"7vd3a1caxb.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"87hazdazmb.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-30T18:17:20Z","receivedAt":"2012-01-30T18:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> I would suggest something like\n>\n> # Please enter the commit message for your merge commit. Lines starting\n> # with '#' will be ignored, and an empty message aborts the commit.\n>\n> or if you feel comfortable with educating the user in a\n> workflow-specific way, even\n>\n> # Please enter the commit message for your merge commit. You should\n> # justify it especially if it merges an updated upstream into a topic\n> # branch.\n> # \n> # Lines starting with '#' will be ignored, and an empty message aborts\n> # the commit.\n\nSounds like a good thing to do.\nPlease make it so.\n"},{"id":"183379","messageId":"0c9a880c7dca27520f957446c6b0e72e93609b03.1327954927.git.trast@student.ethz.ch","threadId":"29433","inReplyTo":"7vd3a1caxb.fsf@alter.siamese.dyndns.org","subject":"[PATCH] merge: add instructions to the commit message when editing","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-30T20:25:30Z","receivedAt":"2012-01-30T20:25:30Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Before f824628 (merge: use editor by default in interactive sessions,\n2012-01-10), git-merge only started an editor if the user explicitly\nasked for it with --edit. Thus it seemed unlikely that the user would\nneed extra guidance.\n\nAfter f824628 the _normal_ thing is to start an editor. Give at least\nan indication of why we are doing it.\n\nThe sentence about justification is one of the few things about\nstandard git that are not agnostic to the workflow that the user\nchose. However, f824628 was proposed by Linus specifically to\ndiscourage users from merging unrelated upstream progress into topic\nbranches. So we may as well take another step in the same direction.\n\nSigned-off-by: Thomas Rast \n---\n builtin/merge.c | 11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bfb7547..ed628b8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -885,11 +885,22 @@ static void abort_commit(const char *err_msg)\n \texit(1);\n }\n \n+static const char merge_editor_comment[] =\n+N_(\"Please enter the commit message for your merge commit. You should\\n\"\n+\"justify it especially if it merges an updated upstream into a topic\\n\"\n+\"branch.\\n\"\n+\"\\n\"\n+\"Lines starting with '#' will be ignored, and an empty message aborts\\n\"\n+\"the commit.\\n\");\n+\n static void prepare_to_commit(void)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n+\tconst char *comment = _(merge_editor_comment);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n+\tif (0 < option_edit)\n+\t\tstrbuf_add_lines(&msg, \"# \", comment, strlen(comment));\n \twrite_merge_msg(&msg);\n \trun_hook(get_index_file(), \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n-- \n1.7.9.350.ga960f\n"},{"id":"183385","messageId":"7vy5soaons.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"0c9a880c7dca27520f957446c6b0e72e93609b03.1327954927.git.trast@student.ethz.ch","subject":"Re: [PATCH] merge: add instructions to the commit message when editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-30T21:03:35Z","receivedAt":"2012-01-30T21:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> The sentence about justification is one of the few things about\n> standard git that are not agnostic to the workflow that the user\n> chose.\n\nWe try to be agnostic at plumbing level, but I do not think we ever made\nsuch a promise at the Porcelain level like \"git merge\". On the contrary,\nwe try to encourage good workflows by coding behaviours to support BCP to\nPorcelain commands. Am I misreading what you were trying to say here?\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index bfb7547..ed628b8 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -885,11 +885,22 @@ static void abort_commit(const char *err_msg)\n> \texit(1);\n> }\n> \n> +static const char merge_editor_comment[] =\n> +N_(\"Please enter the commit message for your merge commit. You should\\n\"\n> +\"justify it especially if it merges an updated upstream into a topic\\n\"\n> +\"branch.\\n\"\n> +\"\\n\"\n> +\"Lines starting with '#' will be ignored, and an empty message aborts\\n\"\n> +\"the commit.\\n\");\n\nI am tempted to rewrite this a bit, perhaps something like ...\n\n Please enter the commit message for your merge commit. Explain\n why the merge is necessary, especially if it merges an updated\n upstream into a topic branch.\n\n... because people who need to be told to \"justify it\" would probably be\nhelped by a more explicit \"explain _why_ it is needed\".\n"},{"id":"183392","messageId":"878vkoamu3.fsf@thomas.inf.ethz.ch","threadId":"29433","inReplyTo":"7vy5soaons.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: add instructions to the commit message when editing","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-01-30T21:43:00Z","receivedAt":"2012-01-30T21:43:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> The sentence about justification is one of the few things about\n>> standard git that are not agnostic to the workflow that the user\n>> chose.\n>\n> We try to be agnostic at plumbing level, but I do not think we ever made\n> such a promise at the Porcelain level like \"git merge\". On the contrary,\n> we try to encourage good workflows by coding behaviours to support BCP to\n> Porcelain commands. Am I misreading what you were trying to say here?\n\nOh, I was just trying to preempt a possible argument why this is wrong.\nMaybe I was a bit over-eager in doing so ;-)\n\n>> +static const char merge_editor_comment[] =\n>> +N_(\"Please enter the commit message for your merge commit. You should\\n\"\n>> +\"justify it especially if it merges an updated upstream into a topic\\n\"\n>> +\"branch.\\n\"\n>> +\"\\n\"\n>> +\"Lines starting with '#' will be ignored, and an empty message aborts\\n\"\n>> +\"the commit.\\n\");\n>\n> I am tempted to rewrite this a bit, perhaps something like ...\n>\n> Please enter the commit message for your merge commit. Explain\n> why the merge is necessary, especially if it merges an updated\n> upstream into a topic branch.\n>\n> ... because people who need to be told to \"justify it\" would probably be\n> helped by a more explicit \"explain _why_ it is needed\".\n\nWhy not. The \"explain...\" might be construed as a bit too coercive, but\nI cannot come up with a way to defuse it (well, except again tacking on\n\"you should\") and yours is certainly much clearer.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183395","messageId":"7vhazcamdl.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"878vkoamu3.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] merge: add instructions to the commit message when editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-30T21:52:54Z","receivedAt":"2012-01-30T21:52:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n>> Please enter the commit message for your merge commit. Explain\n>> why the merge is necessary, especially if it merges an updated\n>> upstream into a topic branch.\n>>\n>> ... because people who need to be told to \"justify it\" would probably be\n>> helped by a more explicit \"explain _why_ it is needed\".\n>\n> Why not. The \"explain...\" might be construed as a bit too coercive, but\n> I cannot come up with a way to defuse it (well, except again tacking on\n> \"you should\") ...\n\nWould \"Please enter the commit message for your merge commit, to explain\nwhy ...\" work?\n"},{"id":"183405","messageId":"87k4488gcj.fsf@thomas.inf.ethz.ch","threadId":"29433","inReplyTo":"7vhazcamdl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: add instructions to the commit message when editing","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-01-31T07:46:04Z","receivedAt":"2012-01-31T07:46:04Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>>> Please enter the commit message for your merge commit. Explain\n>>> why the merge is necessary, especially if it merges an updated\n>>> upstream into a topic branch.\n>>>\n>>> ... because people who need to be told to \"justify it\" would probably be\n>>> helped by a more explicit \"explain _why_ it is needed\".\n>>\n>> Why not. The \"explain...\" might be construed as a bit too coercive, but\n>> I cannot come up with a way to defuse it (well, except again tacking on\n>> \"you should\") ...\n>\n> Would \"Please enter the commit message for your merge commit, to explain\n> why ...\" work?\n\nYes, though it winds up as a rather long sentence. I could then shorten\nit to\n\n Please enter a commit message that explains why the merge commit is\n necessary, especially if it merges an updated upstream into a topic\n branch.\n\nthough that's only _implying_ that you're completing a merge.\n\nYour pick ;-)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185270","messageId":"CABPQNSZVOjOKpqv4s1ZCEQRd_yT3us3mqC9aN-KK5PHqztYQQg@mail.gmail.com","threadId":"29433","inReplyTo":"7vipk26p1b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-02-23T12:43:52Z","receivedAt":"2012-02-23T12:43:52Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Jan 23, 2012 at 11:18 PM, Junio C Hamano wrote:\n> Traditionally, a cleanly resolved merge was committed by \"git merge\" using\n> the auto-generated merge commit log message with invoking the editor.\n>\n> After 5 years of use in the field, it turns out that people perform too\n> many unjustified merges of the upstream history into their topic branches.\n> These merges are not just useless, but they are often not explained well,\n> and making the end result unreadable when it gets time for merging their\n> history back to their upstream.\n>\n> Earlier we added the \"--edit\" option to the command, so that people can\n> edit the log message to explain and justify their merge commits. Let's\n> take it one step further and spawn the editor by default when we are in an\n> interactive session (i.e. the standard input and the standard output are\n> pointing at the same tty device).\n>\n> There may be existing scripts that leave the standard input and the\n> standard output of the \"git merge\" connected to whatever environment the\n> scripts were started, and such invocation might trigger the above\n> \"interactive session\" heuristics.  GIT_MERGE_AUTOEDIT environment variable\n> can be set to \"no\" at the beginning of such scripts to use the historical\n> behaviour while the script runs.\n>\n> Note that this backward compatibility is meant only for scripts, and we\n> deliberately do *not* support \"merge.edit = yes/no/auto\" configuration\n> option to allow people to keep the historical behaviour.\n>\n> Suggested-by: Linus Torvalds \n> Signed-off-by: Junio C Hamano \n> ---\n>\n>  * With an update to the documentation, so that I won't forget, even\n>   though this topic came too late in the cycle and not meant for the\n>   upcoming 1.7.9.\n>\n>  Documentation/git-merge.txt     |    2 +-\n>  Documentation/merge-options.txt |   16 +++++++++++++---\n>  builtin/merge.c                 |   38 ++++++++++++++++++++++++++++++++++----\n>  t/test-lib.sh                   |    3 ++-\n>  4 files changed, 50 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> index e2e6aba..3ceefb8 100644\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -9,7 +9,7 @@ git-merge - Join two or more development histories together\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git merge' [-n] [--stat] [--no-commit] [--squash]\n> +'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n>        [-s ] [-X ]\n>        [--[no-]rerere-autoupdate] [-m ] [...]\n>  'git merge' HEAD ...\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 6bd0b04..f2f1d0f 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -8,10 +8,20 @@ failed and do not autocommit, to give the user a chance to\n>  inspect and further tweak the merge result before committing.\n>\n>  --edit::\n> --e::\n> +--no-edit::\n> +       Invoke an editor before committing successful mechanical merge to\n> +       further edit the auto-generated merge message, so that the user\n> +       can explain and justify the merge. The `--no-edit` option can be\n> +       used to accept the auto-generated message (this is generally\n> +       discouraged). The `--edit` option is still useful if you are\n> +       giving a draft message with the `-m` option from the command line\n> +       and want to edit it in the editor.\n>  +\n> -       Invoke editor before committing successful merge to further\n> -       edit the default merge message.\n> +Older scripts may depend on the historical behaviour of not allowing the\n> +user to edit the merge log message. They will see an editor opened when\n> +they run `git merge`. To make it easier to adjust such scripts to the\n> +updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be\n> +set to `no` at the beginning of them.\n>\n>  --ff::\n>  --no-ff::\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 99f1429..0006175 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {\n>\n>  static int show_diffstat = 1, shortlog_len, 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 struct strbuf merge_msg;\n>  static struct commit_list *remoteheads;\n> @@ -189,7 +189,7 @@ static struct option builtin_merge_options[] = {\n>                \"create a single commit instead of doing a merge\"),\n>        OPT_BOOLEAN(0, \"commit\", &option_commit,\n>                \"perform a commit if the merge succeeds (default)\"),\n> -       OPT_BOOLEAN('e', \"edit\", &option_edit,\n> +       OPT_BOOL('e', \"edit\", &option_edit,\n>                \"edit message before committing\"),\n>        OPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n>                \"allow fast-forward (default)\"),\n> @@ -877,12 +877,12 @@ static void prepare_to_commit(void)\n>        write_merge_msg(&msg);\n>        run_hook(get_index_file(), \"prepare-commit-msg\",\n>                 git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n> -       if (option_edit) {\n> +       if (0 < option_edit) {\n>                if (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n>                        abort_commit(NULL);\n>        }\n>        read_merge_msg(&msg);\n> -       stripspace(&msg, option_edit);\n> +       stripspace(&msg, 0 < option_edit);\n>        if (!msg.len)\n>                abort_commit(_(\"Empty commit message.\"));\n>        strbuf_release(&merge_msg);\n> @@ -1076,6 +1076,33 @@ static void write_merge_state(void)\n>        close(fd);\n>  }\n>\n> +static int default_edit_option(void)\n> +{\n> +       static const char name[] = \"GIT_MERGE_AUTOEDIT\";\n> +       const char *e = getenv(name);\n> +       struct stat st_stdin, st_stdout;\n> +\n> +       if (have_message)\n> +               /* an explicit -m msg without --[no-]edit */\n> +               return 0;\n> +\n> +       if (e) {\n> +               int v = git_config_maybe_bool(name, e);\n> +               if (v < 0)\n> +                       die(\"Bad value '%s' in environment '%s'\", e, name);\n> +               return v;\n> +       }\n> +\n> +       /* Use editor if stdin and stdout are the same and is a tty */\n> +       return (!fstat(0, &st_stdin) &&\n> +               !fstat(1, &st_stdout) &&\n> +               isatty(0) &&\n> +               st_stdin.st_dev == st_stdout.st_dev &&\n> +               st_stdin.st_ino == st_stdout.st_ino &&\n> +               st_stdin.st_mode == st_stdout.st_mode);\n\nI just stumbled over this code, and I got a bit worried; the\nstat-implementation we use on Windows sets st_ino to 0, so\n\"st_stdin.st_ino == st_stdout.st_ino\" will always evaluate to true.\n\nPerhaps we should add a check for isatty(1) here as well? There's max\none console per process on Windows (AllocConsole fails if one has\nalready been create, see\nhttp://msdn.microsoft.com/en-us/library/windows/desktop/ms681944(v=vs.85).aspx\nfor details), so I think if both stdin and stout are consoles, we\nshould be good.\n\nOr is there something I'm missing here?\n\n---\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ed0f959..bef01e3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1130,6 +1130,7 @@ static int default_edit_option(void)\n \treturn (!fstat(0, &st_stdin) &&\n \t\t!fstat(1, &st_stdout) &&\n \t\tisatty(0) &&\n+\t\tisatty(1) &&\n \t\tst_stdin.st_dev == st_stdout.st_dev &&\n \t\tst_stdin.st_ino == st_stdout.st_ino &&\n \t\tst_stdin.st_mode == st_stdout.st_mode);\n"},{"id":"185291","messageId":"7vd3954ame.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"CABPQNSZVOjOKpqv4s1ZCEQRd_yT3us3mqC9aN-KK5PHqztYQQg@mail.gmail.com","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T19:23:37Z","receivedAt":"2012-02-23T19:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n>> + /* Use editor if stdin and stdout are the same and is a tty */\n>> + return (!fstat(0, &st_stdin) &&\n>> + !fstat(1, &st_stdout) &&\n>> + isatty(0) &&\n>> + st_stdin.st_dev == st_stdout.st_dev &&\n>> + st_stdin.st_ino == st_stdout.st_ino &&\n>> + st_stdin.st_mode == st_stdout.st_mode);\n>\n> I just stumbled over this code, and I got a bit worried; the\n> stat-implementation we use on Windows sets st_ino to 0, so\n> \"st_stdin.st_ino == st_stdout.st_ino\" will always evaluate to true.\n>\n> Perhaps we should add a check for isatty(1) here as well? ...\n> Or is there something I'm missing here?\n\nNo, the intention was not \"we do this whether standard output is tty or\nnot\", but was \"we check that fd#0 and fd#1 are connected to the same\ndevice by trusting stat() to do the right thing, so checking isatty(0)\nis sufficient\". As that \"trusting stat()\" assumption does not hold for\nyour platform, we would need to add isatty(1) to accomodate it.\n\nThanks for a set of sharp eyes.\n"},{"id":"185299","messageId":"7vlint2t5h.fsf@alter.siamese.dyndns.org","threadId":"29433","inReplyTo":"7vd3954ame.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T20:26:18Z","receivedAt":"2012-02-23T20:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Erik Faye-Lund writes:\n>\n>>> + /* Use editor if stdin and stdout are the same and is a tty */\n>>> + return (!fstat(0, &st_stdin) &&\n>>> + !fstat(1, &st_stdout) &&\n>>> + isatty(0) &&\n>>> + st_stdin.st_dev == st_stdout.st_dev &&\n>>> + st_stdin.st_ino == st_stdout.st_ino &&\n>>> + st_stdin.st_mode == st_stdout.st_mode);\n>>\n>> I just stumbled over this code, and I got a bit worried; the\n>> stat-implementation we use on Windows sets st_ino to 0, so\n>> \"st_stdin.st_ino == st_stdout.st_ino\" will always evaluate to true.\n>>\n>> Perhaps we should add a check for isatty(1) here as well? ...\n>> Or is there something I'm missing here?\n>\n> No, the intention was ...\n\ns/No,/No, you are not missing anything./;\n\nI'll queue it with this explanation:\n\n merge: do not trust fstat(2) too much when checking interactiveness\n \n The heuristic used by \"git merge\" to decide if it automatically gives an\n editor upon clean automerge is to see if the standard input and the\n standard output is the same device and is a tty, we are in an interactive\n session. \"The same device\" test was done by comparing fstat(2) result on\n the two file descriptors (and they must match), and we asked isatty() only\n for the standard input (we insist that they are the same device and there\n is no point asking tty-ness of the standard output).\n \n The stat(2) emulation on Windows port however does not give a usable value\n in st_ino field, so even if the standard output is connected to something\n different from the standard input, \"The same device\" test may incorrectly\n return true. To accomodate it, add another isatty() check for the standard\n output stream as well.\n \n Reported-by: Erik Faye-Lund \n Signed-off-by: Junio C Hamano \n\nThanks.\n"},{"id":"185301","messageId":"CABPQNSagJ+vDcrcVTJdPc0i+ODeNZ21hUmRO9LBNzS9xFXg_zg@mail.gmail.com","threadId":"29433","inReplyTo":"7vlint2t5h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: use editor by default in interactive sessions","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-02-23T20:31:25Z","receivedAt":"2012-02-23T20:31:25Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Erik Faye-Lund writes:\n>>\n>>>> +       /* Use editor if stdin and stdout are the same and is a tty */\n>>>> +       return (!fstat(0, &st_stdin) &&\n>>>> +               !fstat(1, &st_stdout) &&\n>>>> +               isatty(0) &&\n>>>> +               st_stdin.st_dev == st_stdout.st_dev &&\n>>>> +               st_stdin.st_ino == st_stdout.st_ino &&\n>>>> +               st_stdin.st_mode == st_stdout.st_mode);\n>>>\n>>> I just stumbled over this code, and I got a bit worried; the\n>>> stat-implementation we use on Windows sets st_ino to 0, so\n>>> \"st_stdin.st_ino == st_stdout.st_ino\" will always evaluate to true.\n>>>\n>>> Perhaps we should add a check for isatty(1) here as well? ...\n>>> Or is there something I'm missing here?\n>>\n>> No, the intention was ...\n>\n> s/No,/No, you are not missing anything./;\n>\n> I'll queue it with this explanation:\n>\n>    merge: do not trust fstat(2) too much when checking interactiveness\n>\n>    The heuristic used by \"git merge\" to decide if it automatically gives an\n>    editor upon clean automerge is to see if the standard input and the\n>    standard output is the same device and is a tty, we are in an interactive\n>    session.  \"The same device\" test was done by comparing fstat(2) result on\n>    the two file descriptors (and they must match), and we asked isatty() only\n>    for the standard input (we insist that they are the same device and there\n>    is no point asking tty-ness of the standard output).\n>\n>    The stat(2) emulation on Windows port however does not give a usable value\n>    in st_ino field, so even if the standard output is connected to something\n\nShouldn't that be \"emulation _in the_ Windows port\" and \"in _the_ st_ino field\"?\n\n>    different from the standard input, \"The same device\" test may incorrectly\n>    return true. To accomodate it, add another isatty() check for the standard\n>    output stream as well.\n>\n>    Reported-by: Erik Faye-Lund \n>    Signed-off-by: Junio C Hamano \n>\n> Thanks.\n\nI just sent a mail with a proper-ish commit message, but I like yours\nbetter as it explains the symptom a bit.\n"}]}