From: Erik Faye-Lund Date: Thu, 23 Feb 2012 20:31:25 GMT Subject: Re: [PATCH] merge: use editor by default in interactive sessions Message-ID: In-Reply-To: <7vlint2t5h.fsf@alter.siamese.dyndns.org> On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Erik Faye-Lund 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 >    Signed-off-by: Junio C Hamano > > Thanks. I just sent a mail with a proper-ish commit message, but I like yours better as it explains the symptom a bit.