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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Feb 23, 2012, 20:31 UTC
Message-ID: <CABPQNSagJ+vDcrcVTJdPc0i+ODeNZ21hUmRO9LBNzS9xFXg_zg@mail.gmail.com>
In-Reply-To: <7vlint2t5h.fsf@alter.siamese.dyndns.org>

On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 37 quoted lines

> 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"?

Show 8 quoted lines

>    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.