From: Erik Faye-Lund Date: Thu, 23 Feb 2012 12:43:52 GMT Subject: Re: [PATCH] merge: use editor by default in interactive sessions Message-ID: In-Reply-To: <7vipk26p1b.fsf@alter.siamese.dyndns.org> On Mon, Jan 23, 2012 at 11:18 PM, Junio C Hamano 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 > Signed-off-by: Junio C Hamano > --- > >  * 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 ] [-X ] >        [--[no-]rerere-autoupdate] [-m ] [...] >  'git merge' HEAD ... > 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);