[patch-readline] history file generation on minGW host

[patch-readline] history file generation on minGW host

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On minGW host, history file are open in text mode, that's imply windows 
specific
carriage return to be inserted ( \n -> \r\n conversion performed) and 
prevents windows history file to be compliant with linux one's.
When using current history file for windows, "^M" appears at each end of 
line.

This patch fixes this problem.
-- 
Denis



[-- Attachment #2: readline.patch --]
[-- Type: text/plain, Size: 1095 bytes --]

Index: readline/histfile.c
===================================================================
--- readline/histfile.c	(revision 368)
+++ readline/histfile.c	(working copy)
@@ -70,17 +70,18 @@
 #endif /* HAVE_MMAP */
 
 /* If we're compiling for __EMX__ (OS/2) or __CYGWIN__ (cygwin32 environment
-   on win 95/98/nt), we want to open files with O_BINARY mode so that there
+   on win 95/98/nt) or __MINGW32__ (pure windows environment),
+   we want to open files with O_BINARY mode so that there
    is no \n -> \r\n conversion performed.  On other systems, we don't want to
    mess around with O_BINARY at all, so we ensure that it's defined to 0. */
-#if defined (__EMX__) || defined (__CYGWIN__)
+#if defined (__EMX__) || defined (__CYGWIN__) || defined (__MINGW32__)
 #  ifndef O_BINARY
 #    define O_BINARY 0
 #  endif
-#else /* !__EMX__ && !__CYGWIN__ */
+#else /* !__EMX__ && !__CYGWIN__ && !__MINGW32__ */ 
 #  undef O_BINARY
 #  define O_BINARY 0
-#endif /* !__EMX__ && !__CYGWIN__ */
+#endif /* !__EMX__ && !__CYGWIN__ && !__MINGW32__ */
 
 #include <errno.h>
 #if !defined (errno)

[-- Attachment #3: ChangeLog.gdb --]
[-- Type: text/plain, Size: 118 bytes --]

2006-03-16  Denis Pilat <denis.pilat@st.com>

	* histfile.c (O_BINARY): Avoid \n -> \r\n conversion 
	for minGW host.

Re: [patch-readline] history file generation on minGW host

[Daniel Jacobowitz]

On Thu, Mar 16, 2006 at 02:44:38PM +0100, Denis PILAT wrote:
> On minGW host, history file are open in text mode, that's imply windows 
> specific
> carriage return to be inserted ( \n -> \r\n conversion performed) and 
> prevents windows history file to be compliant with linux one's.
> When using current history file for windows, "^M" appears at each end of 
> line.
> 
> This patch fixes this problem.

You should probably send this patch to the readline maintainers
instead?  It looks reasonable.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-readline] history file generation on minGW host

[Denis PILAT]

Daniel Jacobowitz wrote:

>On Thu, Mar 16, 2006 at 02:44:38PM +0100, Denis PILAT wrote:
>  
>
>>On minGW host, history file are open in text mode, that's imply windows 
>>specific
>>carriage return to be inserted ( \n -> \r\n conversion performed) and 
>>prevents windows history file to be compliant with linux one's.
>>When using current history file for windows, "^M" appears at each end of 
>>line.
>>
>>This patch fixes this problem.
>>    
>>
>
>You should probably send this patch to the readline maintainers
>instead?  It looks reasonable.
>
>  
>
It's done, I'm waiting for Chet reply

Re: [patch-readline] history file generation on minGW host

[Eli Zaretskii]

> Date: Thu, 16 Mar 2006 14:44:38 +0100
> From: Denis PILAT <denis.pilat@st.com>
> 
> On minGW host, history file are open in text mode, that's imply windows 
> specific
> carriage return to be inserted ( \n -> \r\n conversion performed) and 
> prevents windows history file to be compliant with linux one's.
> When using current history file for windows, "^M" appears at each end of 
> line.
> 
> This patch fixes this problem.

Thanks.

However, I think there's a better fix: teach readline to always remove
any CRs before an LF character, even on Posix platforms.  That way,
even if the history file was edited by some Windows editor that
doesn't honor the end-of-line format, it can still be read on any OS.
And as a bonus, we might get a cleaner code, without ugly OS-dependent
#ifdef's.

Unless Chet and others disagree, would you like to prepare a patch
along these lines?

Re: [patch-readline] history file generation on minGW host

[Denis PILAT]

Eli Zaretskii wrote:

>>Date: Thu, 16 Mar 2006 14:44:38 +0100
>>From: Denis PILAT <denis.pilat@st.com>
>>
>>On minGW host, history file are open in text mode, that's imply windows 
>>specific
>>carriage return to be inserted ( \n -> \r\n conversion performed) and 
>>prevents windows history file to be compliant with linux one's.
>>When using current history file for windows, "^M" appears at each end of 
>>line.
>>
>>This patch fixes this problem.
>>    
>>
>
>Thanks.
>
>However, I think there's a better fix: teach readline to always remove
>any CRs before an LF character, even on Posix platforms.  That way,
>even if the history file was edited by some Windows editor that
>doesn't honor the end-of-line format, it can still be read on any OS.
>And as a bonus, we might get a cleaner code, without ugly OS-dependent
>#ifdef's.
>
>Unless Chet and others disagree, would you like to prepare a patch
>along these lines?
>
Yes, good idea, I'll prepare that for next week.
Denis

>
>  
>

Re: [patch-readline] history file generation on minGW host

[Chet Ramey]

Denis PILAT wrote:

>> However, I think there's a better fix: teach readline to always remove
>> any CRs before an LF character, even on Posix platforms.  That way,
>> even if the history file was edited by some Windows editor that
>> doesn't honor the end-of-line format, it can still be read on any OS.
>> And as a bonus, we might get a cleaner code, without ugly OS-dependent
>> #ifdef's.
>>
>> Unless Chet and others disagree, would you like to prepare a patch
>> along these lines?
>>
> Yes, good idea, I'll prepare that for next week.

I'll wait for that one, then.

Chet


-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
( ``Discere est Dolere'' -- chet )
					   Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [patch-readline] history file reading

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]


Eli Zaretskii wrote:

>>Date: Thu, 16 Mar 2006 14:44:38 +0100
>>From: Denis PILAT <denis.pilat@st.com>
>>
>>On minGW host, history file are open in text mode, that's imply windows 
>>specific
>>carriage return to be inserted ( \n -> \r\n conversion performed) and 
>>prevents windows history file to be compliant with linux one's.
>>When using current history file for windows, "^M" appears at each end of 
>>line.
>>
>>This patch fixes this problem.
>>    
>>
>
>Thanks.
>
>However, I think there's a better fix: teach readline to always remove
>any CRs before an LF character, even on Posix platforms.  That way,
>even if the history file was edited by some Windows editor that
>doesn't honor the end-of-line format, it can still be read on any OS.
>And as a bonus, we might get a cleaner code, without ugly OS-dependent
>#ifdef's.
>
>Unless Chet and others disagree, would you like to prepare a patch
>along these lines?
>
>  
>
This new version of my patch changes the way we read history files to 
allow "\r\n" in end-of-line.
I don't change any more the way we write history files.

-- 
Denis


2006-03-21  Denis Pilat <denis.pilat@st.com>

    * histfile.c (read_history_range): remove '\r' character from 
history lines
    to allow reading files with Windows like end-of-line on unix host.




[-- Attachment #2: histfile.patch --]
[-- Type: text/plain, Size: 519 bytes --]

Index: histfile.c
===================================================================
--- histfile.c	(revision 386)
+++ histfile.c	(working copy)
@@ -228,7 +228,11 @@
   for (line_end = line_start; line_end < bufend; line_end++)
     if (*line_end == '\n')
       {
-	*line_end = '\0';
+	/* allow reading files with Windows like end-of-line */
+	if ( line_end - 1 >= line_start && *( line_end - 1 ) =='\r' )
+	  *( line_end - 1 )  = '\0';
+	else
+	  *line_end = '\0';
 
 	if (*line_start)
 	  add_history (line_start);

Re: [patch-readline] history file reading

[Chet Ramey]

Denis PILAT wrote:

>>
> This new version of my patch changes the way we read history files to
> allow "\r\n" in end-of-line.
> I don't change any more the way we write history files.

I don't see any problem with including this, or a variant.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
( ``Discere est Dolere'' -- chet )
				       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [patch-readline] history file reading

[Daniel Jacobowitz]

On Tue, Mar 21, 2006 at 09:47:30AM -0500, Chet Ramey wrote:
> Denis PILAT wrote:
> 
> >>
> > This new version of my patch changes the way we read history files to
> > allow "\r\n" in end-of-line.
> > I don't change any more the way we write history files.
> 
> I don't see any problem with including this, or a variant.

Fine by me too.

Denis, if you plan to continue submitting GDB patches, I think this is
the point where I talk about coding style :-)

On Tue, Mar 21, 2006 at 11:21:59AM +0100, Denis PILAT wrote:
> 2006-03-21  Denis Pilat <denis.pilat@st.com>
> 
>    * histfile.c (read_history_range): remove '\r' character from 
> history lines
>    to allow reading files with Windows like end-of-line on unix host.

Our ChangeLog entries have two spaces between date and name, and two
between name and date.  The indented portion starts with a tab on every
line.  The first character should usually be capitalized.  Also, they
cover only "what" and not "why".

2006-03-21  Denis Pilat  <denis.pilat@st.com>
 
	* histfile.c (read_history_range): Remove '\r' character from
	history lines.

> Index: histfile.c
> ===================================================================
> --- histfile.c	(revision 386)
> +++ histfile.c	(working copy)
> @@ -228,7 +228,11 @@
>    for (line_end = line_start; line_end < bufend; line_end++)
>      if (*line_end == '\n')
>        {
> -	*line_end = '\0';
> +	/* allow reading files with Windows like end-of-line */

Comments start with a capital, and end with a period and two spaces.
	/* Allow reading files with Windows-like end-of-line.  */

> +	if ( line_end - 1 >= line_start && *( line_end - 1 ) =='\r' )
> +	  *( line_end - 1 )  = '\0';

And you don't need the spaces inside the parentheses, but do usually
need spaces around operators:

	if (line_end - 1 >= line_start && *(line_end - 1) == '\r')
	  *(line_end - 1)  = '\0';

> +	else
> +	  *line_end = '\0';
>  
>  	if (*line_start)
>  	  add_history (line_start);

I've checked this in with formatting fixes.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-readline] history file reading

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

>> +	if ( line_end - 1 >= line_start && *( line_end - 1 ) =='\r' )
>> +	  *( line_end - 1 )  = '\0';
>
> And you don't need the spaces inside the parentheses, but do usually
> need spaces around operators:
>
> 	if (line_end - 1 >= line_start && *(line_end - 1) == '\r')
> 	  *(line_end - 1)  = '\0';

line_end[-1] whould be much better readable, IMHO.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch-readline] history file reading

[Bob Rossi]

On Tue, Mar 21, 2006 at 04:30:11PM +0100, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> >> +	if ( line_end - 1 >= line_start && *( line_end - 1 ) =='\r' )
> >> +	  *( line_end - 1 )  = '\0';
> >
> > And you don't need the spaces inside the parentheses, but do usually
> > need spaces around operators:
> >
> > 	if (line_end - 1 >= line_start && *(line_end - 1) == '\r')
> > 	  *(line_end - 1)  = '\0';
> 
> line_end[-1] whould be much better readable, IMHO.

Can you even index into an array with a negitive number?

Bob Rossi

Re: [patch-readline] history file reading

[Daniel Jacobowitz]

On Tue, Mar 21, 2006 at 10:33:08AM -0500, Bob Rossi wrote:
> On Tue, Mar 21, 2006 at 04:30:11PM +0100, Andreas Schwab wrote:
> > Daniel Jacobowitz <drow@false.org> writes:
> > 
> > >> +	if ( line_end - 1 >= line_start && *( line_end - 1 ) =='\r' )
> > >> +	  *( line_end - 1 )  = '\0';
> > >
> > > And you don't need the spaces inside the parentheses, but do usually
> > > need spaces around operators:
> > >
> > > 	if (line_end - 1 >= line_start && *(line_end - 1) == '\r')
> > > 	  *(line_end - 1)  = '\0';
> > 
> > line_end[-1] whould be much better readable, IMHO.
> 
> Can you even index into an array with a negitive number?

Certainly.  And I agree with Andreas (though not enough to go back and
change code someone else has written).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-readline] history file reading

[Eli Zaretskii]

> Date: Tue, 21 Mar 2006 10:33:08 -0500
> From: Bob Rossi <bob_rossi@cox.net>
> Cc: Denis PILAT <denis.pilat@st.com>, Chet Ramey <chet.ramey@case.edu>,
> 	Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com,
> 	bash-maintainers@gnu.org
> 
> > line_end[-1] whould be much better readable, IMHO.
> 
> Can you even index into an array with a negitive number?

Yes, provided that the result is valid, i.e. it doesn't reference an
address outside the array.

In fact, in C, `*(foo -1)' and `foo[-1]' is literally the same thing
by the language rules, and the compiler produces identical code for
both expressions.

Re: [patch-readline] history file reading

[Brian J. Fox]

> From: Daniel Jacobowitz <drow@false.org>
> Date: Tue, 21 Mar 2006 09:58:42 -0500
> To: Denis PILAT <denis.pilat@st.com>, Chet Ramey <chet.ramey@case.edu>
> Cc: Eli Zaretskii <eliz@gnu.org>, <gdb-patches@sources.redhat.com>,
> <bash-maintainers@gnu.org>
> Subject: Re: [patch-readline] history file reading
> 
> Our ChangeLog entries have two spaces between date and name, and two
> between name and date.  The indented portion starts with a tab on every
> line.  The first character should usually be capitalized.  Also, they
> cover only "what" and not "why".

Since when do ChangeLog entries not say "why"?  Without this information, it
is impossible to tell if the change needs to stay or not in the future.

I've certainly always included "why" in my ChangeLog entries.

Is this a personal preference, or are you quoting a new GNU mantra?

Thanks,

Brian

Re: [patch-readline] history file reading

[Daniel Jacobowitz]

On Tue, Mar 21, 2006 at 08:29:20AM -0800, Brian J. Fox wrote:
> > From: Daniel Jacobowitz <drow@false.org>
> > Date: Tue, 21 Mar 2006 09:58:42 -0500
> > To: Denis PILAT <denis.pilat@st.com>, Chet Ramey <chet.ramey@case.edu>
> > Cc: Eli Zaretskii <eliz@gnu.org>, <gdb-patches@sources.redhat.com>,
> > <bash-maintainers@gnu.org>
> > Subject: Re: [patch-readline] history file reading
> > 
> > Our ChangeLog entries have two spaces between date and name, and two
> > between name and date.  The indented portion starts with a tab on every
> > line.  The first character should usually be capitalized.  Also, they
> > cover only "what" and not "why".
> 
> Since when do ChangeLog entries not say "why"?  Without this information, it
> is impossible to tell if the change needs to stay or not in the future.
> 
> I've certainly always included "why" in my ChangeLog entries.
> 
> Is this a personal preference, or are you quoting a new GNU mantra?

In the GNU projects I've worked in, this is the usual interpretation of
the existing GNU coding standards:

http://www.gnu.org/prep/standards/html_node/Change-Log-Concepts.html#Change-Log-Concepts

  There's no need to describe the full purpose of the changes or how they
  work together. If you think that a change calls for explanation, you're
  probably right. Please do explain it - but please put the explanation in
  comments in the code, where people will see it whenever they see the
  code. For example, New function is enough for the change log when you
  add a function, because there should be a comment before the function
  definition to explain what it does.

And from GCC:

See also what the GNU Coding Standards have to say about what goes in
ChangeLogs; in particular, descriptions of the purpose of code and
changes should go in comments rather than the ChangeLog, though a
single line overall description of the changes may be useful above the
ChangeLog entry for a large batch of changes.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch-readline] history file reading

[Andreas Schwab]

"Brian J. Fox" <bfox@ai.mit.edu> writes:

> Since when do ChangeLog entries not say "why"?

http://www.gnu.org/prep/standards/html_node/Change-Log-Concepts.html#Change-Log-Concepts

> Without this information, it is impossible to tell if the change needs
> to stay or not in the future.

"Why" should be written in a comment in the code, because that's where you
are looking at if you wonder why something is done this way.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch-readline] history file reading

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Daniel Jacobowitz <drow@false.org>,
> 	Denis PILAT <denis.pilat@st.com>, Chet Ramey <chet.ramey@case.edu>,
> 	Eli Zaretskii <eliz@gnu.org>, <gdb-patches@sources.redhat.com>,
> 	<bash-maintainers@gnu.org>
> Date: Tue, 21 Mar 2006 17:46:21 +0100
> 
> "Brian J. Fox" <bfox@ai.mit.edu> writes:
> 
> > Since when do ChangeLog entries not say "why"?
> 
> http://www.gnu.org/prep/standards/html_node/Change-Log-Concepts.html#Change-Log-Concepts
> 
> > Without this information, it is impossible to tell if the change needs
> > to stay or not in the future.
> 
> "Why" should be written in a comment in the code, because that's where you
> are looking at if you wonder why something is done this way.

Right.

Re: [patch-readline] history file reading

[Eli Zaretskii]

> Date: Tue, 21 Mar 2006 09:58:42 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com, 	bash-maintainers@gnu.org
> 
> > > This new version of my patch changes the way we read history files to
> > > allow "\r\n" in end-of-line.
> > > I don't change any more the way we write history files.
> > 
> > I don't see any problem with including this, or a variant.
> 
> Fine by me too.

And fine with me, obviously.

> I've checked this in with formatting fixes.

Thanks.