Subject: "Binary" I/O and subprocesses on DOS_NT

From: Eli Zaretskii <[email-masked]>
Date: Thu, 28 Aug 1997 16:58:53 +0300 (IDT)

I believe I've found a bug in call-process-region.  To reproduce, call 
hexl-find-file on a DOS-style text file (with CRLF EOLs), change it a 
bit, then save it: the file is written with Unix-style EOLs.

I think this is because call-process-region was incorrectly setting the 
coding systems for writing the region that serves as input to the process 
and for reading process output.  

First, if binary-process-input is nil, that means the input to process is
text, so setting the coding-system-for-write to nil (no conversion) is the
opposite of what should be done.

Also, call-process-region was setting the coding system for reading the
process output using binary-process-input (instead of
binary-process-output).  Actually, this latter part seems unnecessary at
all, since call-process does it itself, and it does it correctly.  So for 
now, I just ifdef'ed that part away, see the patch below.

I didn't install this change, because I would like you all to look at it 
carefully, in case I made some error.  But please read my other message 
about this before you look at the patch.  The whole issue is complicated, 
and I think it's a good idea that somebody else looks at what I've done.

Here's the patch:

1997-08-28 +03  Eli Zaretskii  <[email-masked]>

	* callproc.c (Fcall_process): Set EOL conversion type to LF when
	binary-process-output is non-nil.
	(Fcall_process_region): binary-process-XXXput only determines EOL
	conversion; if it is nil, convert LF <-> CRLF.  Don't bind
	coding-system-for-read, it is done in Fcall_process.

diff -c src/callproc.c~0 src/callproc.c
*** src/callproc.c~0	Sun Aug 24 00:19:34 1997
--- src/callproc.c	Thu Aug 28 15:44:20 1997
***************
*** 296,303 ****
  	  }
  	setup_coding_system (Fcheck_coding_system (val), &process_coding);
  #ifdef MSDOS
! 	/* On MSDOS, if the user did not ask for binary,
! 	   treat it as "text" which means doing CRLF conversion.  */
  	/* FIXME: this probably should be moved into the guts of
  	   `Ffind_operation_coding_system' for the case of `call-process'.  */
  	if (NILP (Vbinary_process_output))
--- 296,311 ----
  	  }
  	setup_coding_system (Fcheck_coding_system (val), &process_coding);
  #ifdef MSDOS
! 	/* On MSDOS, if the user did not ask for binary, treat it as
! 	   "text" which means doing CRLF conversion.  Otherwise, leave
! 	   the EOLs alone.
! 
! 	   Note that ``binary'' here only means whether EOLs should or
! 	   should not be converted, since that's what Vbinary_process_XXXput
! 	   meant in the days before the coding systems were introduced.
! 
! 	   For other conversions, the caller should set coding-system
! 	   variables explicitly, or rely on auto-detection.  */
  	/* FIXME: this probably should be moved into the guts of
  	   `Ffind_operation_coding_system' for the case of `call-process'.  */
  	if (NILP (Vbinary_process_output))
***************
*** 307,312 ****
--- 315,322 ----
  	      /* FIXME: should we set type to undecided?  */
  	      process_coding.type = coding_type_emacs_mule;
  	  }
+ 	else
+ 	  process_coding.eol_type = CODING_EOL_LF;
  #endif
        }
    }
***************
*** 801,813 ****
    start = args[0];
    end = args[1];
    /* Decide coding-system of the contents of the temporary file.  */
  #ifdef DOS_NT
!   specbind (Qbuffer_file_type, Vbinary_process_input);
!   if (NILP (Vbinary_process_input))
!     val = Qnil;
!   else
  #endif
-     {
        if (!NILP (Vcoding_system_for_write))
  	val = Vcoding_system_for_write;
        else if (NILP (current_buffer->enable_multibyte_characters))
--- 811,822 ----
    start = args[0];
    end = args[1];
    /* Decide coding-system of the contents of the temporary file.  */
+     {
  #ifdef DOS_NT
!       /* This is to cause find-buffer-file-type-coding-system (see
! 	 dos-w32.el) to choose correct EOL translation for write-region.  */
!       specbind (Qbuffer_file_type, Vbinary_process_input);
  #endif
        if (!NILP (Vcoding_system_for_write))
  	val = Vcoding_system_for_write;
        else if (NILP (current_buffer->enable_multibyte_characters))
***************
*** 825,834 ****
--- 834,860 ----
  	  else
  	    val = Qnil;
  	}
+ #ifdef DOS_NT
+       /* binary-process-input tells whether the buffer needs to be
+ 	 written with EOL conversions, but it doesn't say anything
+ 	 about the rest of text encoding.
+ 
+ 	 Don't let binary-process-input determine the EOL conversion if the
+ 	 coding system was set explicitly and it specified EOL handling.  */
+       if (NILP (val)
+ 	  || VECTORP (Fget (val, Qeol_type))
+ 	  || NILP (Vcoding_system_for_write))
+ 	{
+ 	  Fput (val, Qeol_type,
+ 		make_number (NILP (Vbinary_process_input) ? 1 : 0));
+ 	}
+ #endif
      }
    specbind (intern ("coding-system-for-write"), val);
    Fwrite_region (start, end, filename_string, Qnil, Qlambda, Qnil);
  
+   /* This is done by Fcall_process.  */
+ #if 0
  #ifdef DOS_NT
    if (NILP (Vbinary_process_input))
      val = Qnil;
***************
*** 853,858 ****
--- 879,885 ----
  	}
      }
    specbind (intern ("coding-system-for-read"), val);
+ #endif
  
    record_unwind_protect (delete_temp_file, filename_string);

---

From: Eli Zaretskii <[email-masked]>
Date: Fri, 29 Aug 1997 13:14:28 +0300 (IDT)

On Thu, 28 Aug 1997, Geoff Voelker wrote:

> Eli, I can't reproduce this.  I did
> 
> M-x hexl-find-file /tmp/text
> changed some characters
> C-x C-s
> y
> 
> then, in a command prompt, "od -a /tmp/text".  the lines still ended
> with CRLFs.

Maybe because hexl is called differently on NT?  At least one of the
places I patched (in call-process) are MSDOS only, and there are
numerous other ``ifdef MSDOS'' there.

But anyway, please look at the patches for call-process-region (which
are DOS_NT) and tell me whether they seem to be corect.  Maybe you
could come up with your own test case when you look at the present
code of call-process-region (for starters, it uses
Vbinary_process_input instead of Vbinary_process_output in the
fragment that I ifdef'ed away).

---

From: Richard Stallman <[email-masked]>
Date: Sun, 31 Aug 1997 15:40:56 -0400

Can you try this in the new pretest?