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

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

[Junio C Hamano]

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);
+}
+
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -1261,6 +1288,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (option_edit < 0)
+		option_edit = default_edit_option();
+
 	if (!use_strategies) {
 		if (!remoteheads->next)
 			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..ed32c2a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -63,7 +63,8 @@ GIT_AUTHOR_NAME='A U Thor'
 GIT_COMMITTER_EMAIL=committer@example.com
 GIT_COMMITTER_NAME='C O Mitter'
 GIT_MERGE_VERBOSITY=5
-export GIT_MERGE_VERBOSITY
+GIT_MERGE_AUTOEDIT=no
+export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
-- 
1.7.9.rc2.48.g92994

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

[Michael Haggerty]

On 01/23/2012 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.

s/with/without/

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 01/23/2012 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.
>
> s/with/without/

Oops, thanks, ... goes to find a hole to crawl into ...

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

[Thomas Rast]

On Mon, 23 Jan 2012 14:18:40 -0800, 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.

Ok, so I'm late to the party and perhaps I missed the discussion about
this, but...

I think the proposed commit message should have a comment, just like for
an ordinary commit, that explains why we are showing the user an editor.
(I'm too lazy to check, but I suspect we *always* give a comment about
what is going on when we fire up an editor.)

I would suggest something like

# Please enter the commit message for your merge commit.  Lines starting
# with '#' will be ignored, and an empty message aborts the commit.

or if you feel comfortable with educating the user in a
workflow-specific way, even

# Please enter the commit message for your merge commit.  You should
# justify it especially if it merges an updated upstream into a topic
# branch.
# 
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> I would suggest something like
>
> # Please enter the commit message for your merge commit.  Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
>
> or if you feel comfortable with educating the user in a
> workflow-specific way, even
>
> # Please enter the commit message for your merge commit.  You should
> # justify it especially if it merges an updated upstream into a topic
> # branch.
> # 
> # Lines starting with '#' will be ignored, and an empty message aborts
> # the commit.

Sounds like a good thing to do.
Please make it so.

[PATCH] merge: add instructions to the commit message when editing

[Thomas Rast]

Before f824628 (merge: use editor by default in interactive sessions,
2012-01-10), git-merge only started an editor if the user explicitly
asked for it with --edit.  Thus it seemed unlikely that the user would
need extra guidance.

After f824628 the _normal_ thing is to start an editor.  Give at least
an indication of why we are doing it.

The sentence about justification is one of the few things about
standard git that are not agnostic to the workflow that the user
chose.  However, f824628 was proposed by Linus specifically to
discourage users from merging unrelated upstream progress into topic
branches.  So we may as well take another step in the same direction.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/merge.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bfb7547..ed628b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -885,11 +885,22 @@ static void abort_commit(const char *err_msg)
 	exit(1);
 }
 
+static const char merge_editor_comment[] =
+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");
+
 static void prepare_to_commit(void)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *comment = _(merge_editor_comment);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
+	if (0 < option_edit)
+		strbuf_add_lines(&msg, "# ", comment, strlen(comment));
 	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
-- 
1.7.9.350.ga960f

Re: [PATCH] merge: add instructions to the commit message when editing

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> The sentence about justification is one of the few things about
> standard git that are not agnostic to the workflow that the user
> chose.

We try to be agnostic at plumbing level, but I do not think we ever made
such a promise at the Porcelain level like "git merge". On the contrary,
we try to encourage good workflows by coding behaviours to support BCP to
Porcelain commands.  Am I misreading what you were trying to say here?

> diff --git a/builtin/merge.c b/builtin/merge.c
> index bfb7547..ed628b8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -885,11 +885,22 @@ static void abort_commit(const char *err_msg)
>  	exit(1);
>  }
>  
> +static const char merge_editor_comment[] =
> +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");

I am tempted to rewrite this a bit, perhaps something like ...

  Please enter the commit message for your merge commit.  Explain
  why the merge is necessary, especially if it merges an updated
  upstream into a topic branch.

... because people who need to be told to "justify it" would probably be
helped by a more explicit "explain _why_ it is needed".

Re: [PATCH] merge: add instructions to the commit message when editing

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> The sentence about justification is one of the few things about
>> standard git that are not agnostic to the workflow that the user
>> chose.
>
> We try to be agnostic at plumbing level, but I do not think we ever made
> such a promise at the Porcelain level like "git merge". On the contrary,
> we try to encourage good workflows by coding behaviours to support BCP to
> Porcelain commands.  Am I misreading what you were trying to say here?

Oh, I was just trying to preempt a possible argument why this is wrong.
Maybe I was a bit over-eager in doing so ;-)

>> +static const char merge_editor_comment[] =
>> +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");
>
> I am tempted to rewrite this a bit, perhaps something like ...
>
>   Please enter the commit message for your merge commit.  Explain
>   why the merge is necessary, especially if it merges an updated
>   upstream into a topic branch.
>
> ... because people who need to be told to "justify it" would probably be
> helped by a more explicit "explain _why_ it is needed".

Why not.  The "explain..." might be construed as a bit too coercive, but
I cannot come up with a way to defuse it (well, except again tacking on
"you should") and yours is certainly much clearer.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] merge: add instructions to the commit message when editing

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

>>   Please enter the commit message for your merge commit.  Explain
>>   why the merge is necessary, especially if it merges an updated
>>   upstream into a topic branch.
>>
>> ... because people who need to be told to "justify it" would probably be
>> helped by a more explicit "explain _why_ it is needed".
>
> Why not.  The "explain..." might be construed as a bit too coercive, but
> I cannot come up with a way to defuse it (well, except again tacking on
> "you should") ...

Would "Please enter the commit message for your merge commit, to explain
why ..." work?

Re: [PATCH] merge: add instructions to the commit message when editing

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>>>   Please enter the commit message for your merge commit.  Explain
>>>   why the merge is necessary, especially if it merges an updated
>>>   upstream into a topic branch.
>>>
>>> ... because people who need to be told to "justify it" would probably be
>>> helped by a more explicit "explain _why_ it is needed".
>>
>> Why not.  The "explain..." might be construed as a bit too coercive, but
>> I cannot come up with a way to defuse it (well, except again tacking on
>> "you should") ...
>
> Would "Please enter the commit message for your merge commit, to explain
> why ..." work?

Yes, though it winds up as a rather long sentence.  I could then shorten
it to

  Please enter a commit message that explains why the merge commit is
  necessary, especially if it merges an updated upstream into a topic
  branch.

though that's only _implying_ that you're completing a merge.

Your pick ;-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Erik Faye-Lund]

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);

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

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

>> +       /* 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? ...
> Or is there something I'm missing here?

No, the intention was not "we do this whether standard output is tty or
not", but was "we check that fd#0 and fd#1 are connected to the same
device by trusting stat() to do the right thing, so checking isatty(0)
is sufficient".  As that "trusting stat()" assumption does not hold for
your platform, we would need to add isatty(1) to accomodate it.

Thanks for a set of sharp eyes.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> +       /* 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? ...
>> Or is there something I'm missing here?
>
> No, the intention was ...

s/No,/No, you are not missing anything./;

I'll queue it with this explanation:

    merge: do not trust fstat(2) too much when checking interactiveness
    
    The heuristic used by "git merge" to decide if it automatically gives an
    editor upon clean automerge is to see if the standard input and the
    standard output is the same device and is a tty, we are in an interactive
    session.  "The same device" test was done by comparing fstat(2) result on
    the two file descriptors (and they must match), and we asked isatty() only
    for the standard input (we insist that they are the same device and there
    is no point asking tty-ness of the standard output).
    
    The stat(2) emulation on Windows port however does not give a usable value
    in st_ino field, so even if the standard output is connected to something
    different from the standard input, "The same device" test may incorrectly
    return true. To accomodate it, add another isatty() check for the standard
    output stream as well.
    
    Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks.

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

[Erik Faye-Lund]

On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>>> +       /* 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? ...
>>> Or is there something I'm missing here?
>>
>> No, the intention was ...
>
> s/No,/No, you are not missing anything./;
>
> I'll queue it with this explanation:
>
>    merge: do not trust fstat(2) too much when checking interactiveness
>
>    The heuristic used by "git merge" to decide if it automatically gives an
>    editor upon clean automerge is to see if the standard input and the
>    standard output is the same device and is a tty, we are in an interactive
>    session.  "The same device" test was done by comparing fstat(2) result on
>    the two file descriptors (and they must match), and we asked isatty() only
>    for the standard input (we insist that they are the same device and there
>    is no point asking tty-ness of the standard output).
>
>    The stat(2) emulation on Windows port however does not give a usable value
>    in st_ino field, so even if the standard output is connected to something

Shouldn't that be "emulation _in the_ Windows port" and "in _the_ st_ino field"?

>    different from the standard input, "The same device" test may incorrectly
>    return true. To accomodate it, add another isatty() check for the standard
>    output stream as well.
>
>    Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks.

I just sent a mail with a proper-ish commit message, but I like yours
better as it explains the symptom a bit.