On Mon, Jan 23, 2012 at 11:18 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Traditionally, a cleanly resolved merge was committed by "git merge" using
> the auto-generated merge commit log message with invoking the editor.
>
> After 5 years of use in the field, it turns out that people perform too
> many unjustified merges of the upstream history into their topic branches.
> These merges are not just useless, but they are often not explained well,
> and making the end result unreadable when it gets time for merging their
> history back to their upstream.
>
> Earlier we added the "--edit" option to the command, so that people can
> edit the log message to explain and justify their merge commits. Let's
> take it one step further and spawn the editor by default when we are in an
> interactive session (i.e. the standard input and the standard output are
> pointing at the same tty device).
>
> There may be existing scripts that leave the standard input and the
> standard output of the "git merge" connected to whatever environment the
> scripts were started, and such invocation might trigger the above
> "interactive session" heuristics. GIT_MERGE_AUTOEDIT environment variable
> can be set to "no" at the beginning of such scripts to use the historical
> behaviour while the script runs.
>
> Note that this backward compatibility is meant only for scripts, and we
> deliberately do *not* support "merge.edit = yes/no/auto" configuration
> option to allow people to keep the historical behaviour.
>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
>
> * With an update to the documentation, so that I won't forget, even
> though this topic came too late in the cycle and not meant for the
> upcoming 1.7.9.
>
> Documentation/git-merge.txt | 2 +-
> Documentation/merge-options.txt | 16 +++++++++++++---
> builtin/merge.c | 38 ++++++++++++++++++++++++++++++++++----
> t/test-lib.sh | 3 ++-
> 4 files changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index e2e6aba..3ceefb8 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -9,7 +9,7 @@ git-merge - Join two or more development histories together
> SYNOPSIS
> --------
> [verse]
> -'git merge' [-n] [--stat] [--no-commit] [--squash]
> +'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
> [-s <strategy>] [-X <strategy-option>]
> [--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
> 'git merge' <msg> HEAD <commit>...
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 6bd0b04..f2f1d0f 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -8,10 +8,20 @@ failed and do not autocommit, to give the user a chance to
> inspect and further tweak the merge result before committing.
>
> --edit::
> --e::
> +--no-edit::
> + Invoke an editor before committing successful mechanical merge to
> + further edit the auto-generated merge message, so that the user
> + can explain and justify the merge. The `--no-edit` option can be
> + used to accept the auto-generated message (this is generally
> + discouraged). The `--edit` option is still useful if you are
> + giving a draft message with the `-m` option from the command line
> + and want to edit it in the editor.
> +
> - Invoke editor before committing successful merge to further
> - edit the default merge message.
> +Older scripts may depend on the historical behaviour of not allowing the
> +user to edit the merge log message. They will see an editor opened when
> +they run `git merge`. To make it easier to adjust such scripts to the
> +updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
> +set to `no` at the beginning of them.
>
> --ff::
> --no-ff::
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 99f1429..0006175 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {
>
> static int show_diffstat = 1, shortlog_len, 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 struct strbuf merge_msg;
> static struct commit_list *remoteheads;
> @@ -189,7 +189,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)"),
> @@ -877,12 +877,12 @@ static void prepare_to_commit(void)
> write_merge_msg(&msg);
> run_hook(get_index_file(), "prepare-commit-msg",
> git_path("MERGE_MSG"), "merge", NULL, NULL);
> - if (option_edit) {
> + if (0 < option_edit) {
> if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
> abort_commit(NULL);
> }
> read_merge_msg(&msg);
> - stripspace(&msg, option_edit);
> + stripspace(&msg, 0 < option_edit);
> if (!msg.len)
> abort_commit(_("Empty commit message."));
> strbuf_release(&merge_msg);
> @@ -1076,6 +1076,33 @@ static void write_merge_state(void)
> close(fd);
> }
>
> +static int default_edit_option(void)
> +{
> + static const char name[] = "GIT_MERGE_AUTOEDIT";
> + const char *e = getenv(name);
> + struct stat st_stdin, st_stdout;
> +
> + if (have_message)
> + /* an explicit -m msg without --[no-]edit */
> + return 0;
> +
> + if (e) {
> + int v = git_config_maybe_bool(name, e);
> + if (v < 0)
> + die("Bad value '%s' in environment '%s'", e, name);
> + return v;
> + }
> +
> + /* Use editor if stdin and stdout are the same and is a tty */
> + return (!fstat(0, &st_stdin) &&
> + !fstat(1, &st_stdout) &&
> + isatty(0) &&
> + st_stdin.st_dev == st_stdout.st_dev &&
> + st_stdin.st_ino == st_stdout.st_ino &&
> + st_stdin.st_mode == st_stdout.st_mode);
I just stumbled over this code, and I got a bit worried; the
stat-implementation we use on Windows sets st_ino to 0, so
"st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
Perhaps we should add a check for isatty(1) here as well? There's max
one console per process on Windows (AllocConsole fails if one has
already been create, see
http://msdn.microsoft.com/en-us/library/windows/desktop/ms681944(v=vs.85).aspx
for details), so I think if both stdin and stout are consoles, we
should be good.
Or is there something I'm missing here?
---
diff --git a/builtin/merge.c b/builtin/merge.c
index ed0f959..bef01e3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1130,6 +1130,7 @@ static int default_edit_option(void)
return (!fstat(0, &st_stdin) &&
!fstat(1, &st_stdout) &&
isatty(0) &&
+ isatty(1) &&
st_stdin.st_dev == st_stdout.st_dev &&
st_stdin.st_ino == st_stdout.st_ino &&
st_stdin.st_mode == st_stdout.st_mode);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html