Re: [PATCH] merge: use editor by default in interactive sessions

[Erik Faye-Lund <kusmabite@gmail.com>]

On Mon, Jan 23, 2012 at 11:18 PM, Junio C Hamano <gitster@pobox.com> 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@linux-foundation.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * 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);