Re: Please make git-am handle \r\n-damaged patches

Alex Riesen [off-list ref] writes:

Maybe it is as simple as that (not tested yet,
and sent through gmail, so please be careful):

I thought about this approach, but it made me worried about a case where
an otherwise sane piece of e-mail has \r at the end of one line as the
real payload.  But as long as we are talking about a text e-mail (and we
are talking about mailsplit here and a binary payload with a CTE applied
counts as text), I think we can safely use an approach like this.

It is not that uncommon to have mails with DOS line-ending, notably
Thunderbird and Gmail (when saving what they call "original" message).

Noticed by H. Peter Anvin.

Signed-off-by: Alex Riesen <redacted>
---

Alex Riesen [off-list ref] writes:

Maybe it is as simple as that (not tested yet,
and sent through gmail, so please be careful):

That's wrong, of course. I missed the fact that \n stays in the
buffer. Corrected.

 builtin-mailsplit.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index ad5f6b5..188ffec 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -58,6 +58,8 @@ int read_line_with_nul(char *buf, int size, FILE *in)
 		if (c == '\n' || len + 1 >= size)
 			break;
 	}
+	if (len > 1 && buf[len - 2] == '\r')
+		buf[--len - 1] = '\n';
 	buf[len] = '\0';
 
 	return len;

-- 
1.6.4.30.gd4c0

Heya,

On Tue, Aug 4, 2009 at 10:26, Alex Riesen[off-list ref] wrote:

       }
+       if (len > 1 && buf[len - 2] == '\r')
+               buf[--len - 1] = '\n';
       buf[len] = '\0';

How about something like:

+       if (len > 1 && buf[len - 2] == '\r' && (buf[len - 1] == '\n'
|| buf[len - 1] == '\0'))
+               buf[--len - 1] = '\n';

To make sure that we're not erasing a \r somewhere in the middle of the content?

-- 
Cheers,

Sverre Rabbelier

Sverre Rabbelier wrote:

Heya,

On Tue, Aug 4, 2009 at 10:26, Alex Riesen[off-list ref] wrote:

quoted

       }
+       if (len > 1 && buf[len - 2] == '\r')
+               buf[--len - 1] = '\n';
       buf[len] = '\0';

How about something like:

+       if (len > 1 && buf[len - 2] == '\r' && (buf[len - 1] == '\n'
|| buf[len - 1] == '\0'))
+               buf[--len - 1] = '\n';

To make sure that we're not erasing a \r somewhere in the middle of the content?

You may want to push the \r back into the buffer if it is the last character read
too.  We may reach the limit of size characters without finding a \n, and so we
can't tell whether the last \r we read was a solitary \r or whether it is the
beginning of \r\n sequence.

So maybe we need something like this after the 'for' loop instead:


   if (c == '\n') {
   	if (len > 1 && buf[len - 2] == '\r')
		buf[--len - 1] = '\n';
   } else if (c == '\r') {
	ungetc(c, in);
	len--;
   }

-brandon

On Tue, Aug 4, 2009 at 19:34, Sverre Rabbelier[off-list ref] wrote:

On Tue, Aug 4, 2009 at 10:26, Alex Riesen[off-list ref] wrote:

quoted

       }
+       if (len > 1 && buf[len - 2] == '\r')
+               buf[--len - 1] = '\n';
       buf[len] = '\0';

How about something like:

+       if (len > 1 && buf[len - 2] == '\r' && (buf[len - 1] == '\n'
|| buf[len - 1] == '\0'))
+               buf[--len - 1] = '\n';

To make sure that we're not erasing a \r somewhere in the middle of the content?

I think this should be enough:

	if (c == '\n' && len > 1 && buf[len - 2] == '\r')
		buf[--len - 1] = '\n';

I'll resend.

On Tue, Aug 4, 2009 at 8:46 AM, Junio C Hamano[off-list ref] wrote:

I thought about this approach, but it made me worried about a case where
an otherwise sane piece of e-mail has \r at the end of one line as the
real payload. But as long as we are talking about a text e-mail (and we
are talking about mailsplit here and a binary payload with a CTE applied
counts as text), I think we can safely use an approach like this.

RFC 2822, section 2.3 explicitly states that a CR should not occur
without a LF (and vice versa, but the e-mail client might convert CRLF
to LF when saving to file), so I think this should be safe.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Noticed by H. Peter Anvin.

It is not that uncommon to have mails with DOS line-ending,
notably Thunderbird and web mailers like Gmail (when saving
what they call "original" message).

Signed-off-by: Alex Riesen <redacted>
---

Corrected bug with unconditonal last (or very long) line shortening if
it contains a CR in next-to-last character. Noticed by Sverre Rabbelier.

It should also handle the case mentioned by Brandon Casey.

 builtin-mailsplit.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index ad5f6b5..48285a0 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -58,6 +58,8 @@ int read_line_with_nul(char *buf, int size, FILE *in)
 		if (c == '\n' || len + 1 >= size)
 			break;
 	}
+	if (c == '\n' && len > 1 && buf[len - 2] == '\r')
+		buf[--len - 1] = '\n';
 	buf[len] = '\0';
 
 	return len;

-- 
1.6.4.34.gc3135e

On Tue, Aug 4, 2009 at 22:59, Erik Faye-Lund[off-list ref] wrote:

On Tue, Aug 4, 2009 at 8:46 AM, Junio C Hamano[off-list ref] wrote:

quoted

I thought about this approach, but it made me worried about a case where
an otherwise sane piece of e-mail has \r at the end of one line as the
real payload. But as long as we are talking about a text e-mail (and we
are talking about mailsplit here and a binary payload with a CTE applied
counts as text), I think we can safely use an approach like this.

RFC 2822, section 2.3 explicitly states that a CR should not occur
without a LF (and vice versa, but the e-mail client might convert CRLF
to LF when saving to file), so I think this should be safe.

You missed this line in original posters e-mail:
"In a serious case of craniorectal immersion..."

We are not safe from that and alike.

Quoting Junio C Hamano [off-list ref]

Alex Riesen [off-list ref] writes:

quoted

Maybe it is as simple as that (not tested yet,
and sent through gmail, so please be careful):

I thought about this approach, but it made me worried about a case where
an otherwise sane piece of e-mail has \r at the end of one line as the
real payload.  But as long as we are talking about a text e-mail (and we
are talking about mailsplit here and a binary payload with a CTE applied
counts as text), I think we can safely use an approach like this.

Is it safe to rebase a commit that introduces a carriage-return at the end of the line using the updated program?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

On Tue, Aug 4, 2009 at 11:05 PM, Alex Riesen[off-list ref] wrote:

You missed this line in original posters e-mail:
"In a serious case of craniorectal immersion..."

We are not safe from that and alike.

Storing e-mails with CRLFs intact is perfectly sane. Depending on them
not being stored that way is not.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Alex Riesen wrote:

What if \r lands at character 99 and \n is at character 100?  If buf has
exactly 100 characters available for writing.  Wouldn't the '\r' be stored
into buf, but not the '\n'?  Then the 'for' loop would terminate since len + 1
would be >= size and the code above would test whether c == '\n', and it
would not, so the '\r' would not be removed from buf as it should be.

At the point where buf has been filled, and the last character read is a '\r',
we can not tell whether the next character is a '\n' or not, so we do not know
if it is a solitary '\r' or whether it is the start of a '\r\n' sequence.  It
seems to me that we must push the '\r' back into the stream and allow the next
call to read_line_with_nul() handle it, or peek to see if the next character
is a '\n'.

Be aware that if we use the version I suggested which pushes the '\r' back
into the input stream, I think we risk an infinite loop if size == 1.  I don't
think that is possible from the current callers though.

-brandon

Brandon Casey wrote:

Be aware that if we use the version I suggested which pushes the '\r' back
into the input stream, I think we risk an infinite loop if size == 1.  I don't
think that is possible from the current callers though.

I think something like this would avoid any potential infinite loop, however improbable:

        if (c == '\n') {
                if (len > 1 && buf[len - 2] == '\r')
                        buf[--len - 1] = '\n';
        } else if (c == '\r') {
                c = getc(in);
                if (c == '\n')
                        buf[len - 1] = '\n';
                else if (c != EOF)
                        ungetc(c, in);
        }

-brandon

On Tue, Aug 4, 2009 at 23:29, Brandon
Casey[off-list ref] wrote:

Alex Riesen wrote:

quoted

+     if (c == '\n' && len > 1 && buf[len - 2] == '\r')
+             buf[--len - 1] = '\n';
      buf[len] = '\0';

      return len;

What if \r lands at character 99 and \n is at character 100?  If buf has
exactly 100 characters available for writing. ...

Ah, yes. You're right.

I have strong dislike towards unget, though. How about this, instead:

int read_line_with_nul(char *buf, int size, FILE *in)
{
	int len = 0, c;

	while (len < size) {
		c = getc(in);
		if (c == EOF)
			break;
		buf[len++] = c;
		if (c == '\n')
			break;
		else if (len == size)
			c = 0;
	}
	if (c == '\n' && len > 1 && buf[len - 2] == '\r')
		buf[--len - 1] = '\n';
	buf[len] = '\0';

	return len;
}

On Tue, Aug 4, 2009 at 23:58, Alex Riesen[off-list ref] wrote:

int read_line_with_nul(char *buf, int size, FILE *in)
{
       int len = 0, c;

       while (len < size) {

"for (;;)", of course. Leftover from development process

               c = getc(in);
               if (c == EOF)
                       break;
               buf[len++] = c;
               if (c == '\n')
                       break;
               else if (len == size)
                       c = 0;
       }
       if (c == '\n' && len > 1 && buf[len - 2] == '\r')
               buf[--len - 1] = '\n';
       buf[len] = '\0';

       return len;
}

On Wed, Aug 5, 2009 at 00:03, Alex Riesen[off-list ref] wrote:

On Tue, Aug 4, 2009 at 23:58, Alex Riesen[off-list ref] wrote:

quoted

int read_line_with_nul(char *buf, int size, FILE *in)
{
       int len = 0, c;

       while (len < size) {

"for (;;)", of course. Leftover from development process

Ahm... No, its not! Its correct :)
Good night, everybody.

quoted

               c = getc(in);
               if (c == EOF)
                       break;
               buf[len++] = c;
               if (c == '\n')
                       break;
               else if (len == size)
                       c = 0;
       }
       if (c == '\n' && len > 1 && buf[len - 2] == '\r')
               buf[--len - 1] = '\n';
       buf[len] = '\0';

       return len;
}

Nanako Shiraishi [off-list ref] writes:

Quoting Junio C Hamano [off-list ref]

quoted

Alex Riesen [off-list ref] writes:

quoted

Maybe it is as simple as that (not tested yet,
and sent through gmail, so please be careful):

I thought about this approach, but it made me worried about a case where
an otherwise sane piece of e-mail has \r at the end of one line as the
real payload.  But as long as we are talking about a text e-mail (and we
are talking about mailsplit here and a binary payload with a CTE applied
counts as text), I think we can safely use an approach like this.

Is it safe to rebase a commit that introduces a carriage-return at the end of the line using the updated program?

Hmmm, good point.  The approach does break rebase.

At least we could patch it up like this.

 builtin-mailinfo.c  |    2 +-
 builtin-mailsplit.c |   11 +++++++++--
 builtin.h           |    2 +-
 git-am.sh           |    8 +++++++-
 t/t3400-rebase.sh   |   26 ++++++++++++++++++++++++--
 5 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 92637ac..6e44d2c 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c

@@ -835,7 +835,7 @@ static void handle_body(void)
 		strbuf_reset(&line);
 		if (strbuf_avail(&line) < 100)
 			strbuf_grow(&line, 100);
-	} while ((len = read_line_with_nul(line.buf, strbuf_avail(&line), fin)));
+	} while ((len = read_line_with_nul(line.buf, strbuf_avail(&line), fin, 0)));
 
 handle_body_out:
 	strbuf_release(&prev);

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index ad5f6b5..f774b04 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -45,8 +45,10 @@ static int is_from_line(const char *line, int len)
 /* Could be as small as 64, enough to hold a Unix "From " line. */
 static char buf[4096];
 
+static int keep_cr;
+
 /* We cannot use fgets() because our lines can contain NULs */
-int read_line_with_nul(char *buf, int size, FILE *in)
+int read_line_with_nul(char *buf, int size, FILE *in, int nuke_cr_at_eol)
 {
 	int len = 0, c;
 

@@ -58,6 +60,9 @@ int read_line_with_nul(char *buf, int size, FILE *in)
 		if (c == '\n' || len + 1 >= size)
 			break;
 	}
+	if (nuke_cr_at_eol &&
+	    (1 < len && buf[len - 2] == '\r' && buf[len - 1] == '\n'))
+		buf[len-- - 2] = '\n';
 	buf[len] = '\0';
 
 	return len;

@@ -93,7 +98,7 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 		if (fwrite(buf, 1, len, output) != len)
 			die_errno("cannot write output");
 
-		len = read_line_with_nul(buf, sizeof(buf), mbox);
+		len = read_line_with_nul(buf, sizeof(buf), mbox, !keep_cr);
 		if (len == 0) {
 			if (feof(mbox)) {
 				status = 1;

@@ -248,6 +253,8 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)
 			nr = strtol(arg+2, NULL, 10);
 		} else if ( arg[1] == 'b' && !arg[2] ) {
 			allow_bare = 1;
+		} else if (!strcmp(arg, "--keep-cr")) {
+			keep_cr = 1;
 		} else if ( arg[1] == 'o' && arg[2] ) {
 			dir = arg+2;
 		} else if ( arg[1] == '-' && !arg[2] ) {

diff --git a/builtin.h b/builtin.h
index 825a96f..59dddee 100644
--- a/builtin.h
+++ b/builtin.h

@@ -13,7 +13,7 @@ extern const char git_more_info_string[];
 extern void list_common_cmds_help(void);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void prune_packed_objects(int);
-extern int read_line_with_nul(char *buf, int size, FILE *file);
+extern int read_line_with_nul(char *buf, int size, FILE *file, int nuke_cr_at_eol);
 extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
 	struct strbuf *out);
 extern int commit_tree(const char *msg, unsigned char *tree,

diff --git a/git-am.sh b/git-am.sh
index d64d997..985226b 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -197,7 +197,13 @@ check_patch_format () {
 split_patches () {
 	case "$patch_format" in
 	mbox)
-		git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||
+		case "$rebasing" in
+		'')
+			keep_cr= ;;
+		?*)
+			keep_cr=--keep-cr ;;
+		esac
+		git mailsplit -d"$prec" -o"$dotest" -b $keep_cr -- "$@" > "$dotest/last" ||
 		clean_abort
 		;;
 	stgit-series)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index c5c29cc..4e6a44b 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh

@@ -3,9 +3,10 @@
 # Copyright (c) 2005 Amos Waterland
 #
 
-test_description='git rebase should not destroy author information
+test_description='git rebase assorted tests
 
-This test runs git rebase and checks that the author information is not lost.
+This test runs git rebase and checks that the author information is not lost
+among other things.
 '
 . ./test-lib.sh
 

@@ -133,4 +134,25 @@ test_expect_success 'rebase -q is quiet' '
      test ! -s output.out
 '
 
+q_to_cr () {
+	tr Q '\015'
+}
+
+test_expect_success 'Rebase a commit that sprinkles CRs in' '
+	(
+		echo "One"
+		echo "TwoQ"
+		echo "Three"
+		echo "FQur"
+		echo "Five"
+	) | q_to_cr >CR &&
+	git add CR &&
+	test_tick &&
+	git commit -a -m "A file with a line with CR" &&
+	git tag file-with-cr &&
+	git checkout HEAD^0 &&
+	git rebase --onto HEAD^^ HEAD^ &&
+	git diff --exit-code file-with-cr:CR HEAD:CR
+'
+
 test_done

Alex Riesen wrote:

On Tue, Aug 4, 2009 at 23:29, Brandon
Casey[off-list ref] wrote:

quoted

Alex Riesen wrote:

quoted

+     if (c == '\n' && len > 1 && buf[len - 2] == '\r')
+             buf[--len - 1] = '\n';
      buf[len] = '\0';

      return len;

What if \r lands at character 99 and \n is at character 100?  If buf has
exactly 100 characters available for writing. ...

Ah, yes. You're right.

I have strong dislike towards unget, though. How about this, instead:

int read_line_with_nul(char *buf, int size, FILE *in)
{
	int len = 0, c;

	while (len < size) {
		c = getc(in);
		if (c == EOF)
			break;
		buf[len++] = c;
		if (c == '\n')
			break;
		else if (len == size)
			c = 0;
	}
	if (c == '\n' && len > 1 && buf[len - 2] == '\r')
		buf[--len - 1] = '\n';
	buf[len] = '\0';

	return len;
}

I don't see how this solves the problem.  Still if the buffer is filled,
and the last character read is '\r', and the next character that has not
yet been read is '\n', then the '\r' will erroneously be returned in buf.

Plus, I think you'll have a problem at buf[len] = '\0' if the loop runs to
completion and len == size.

-brandon

From: Brandon Casey <redacted>

Convert mailinfo and mailsplit to use strbufs before attempting to address
the CRLF issue for saved emails.

Brandon Casey (3):
  strbuf: add new function strbuf_getwholeline()
  builtin-mailinfo,builtin-mailsplit: use strbufs
  builtin-mailsplit.c: remove read_line_with_nul() since it is no
    longer used

Junio C Hamano (1):
  Allow mailsplit to handle mails with CRLF line-endings

 builtin-mailinfo.c  |    8 +-------
 builtin-mailsplit.c |   44 ++++++++++++++------------------------------
 builtin.h           |    1 -
 git-am.sh           |    8 +++++++-
 strbuf.c            |   15 ++++++++++++---
 strbuf.h            |    1 +
 t/t3400-rebase.sh   |   26 ++++++++++++++++++++++++--
 7 files changed, 59 insertions(+), 44 deletions(-)

From: Brandon Casey <redacted>

This function is just like strbuf_getline() except it retains the
line-termination character.  This function will be used by the mailinfo
and mailsplit builtins which require the entire line for parsing.

Signed-off-by: Brandon Casey <redacted>
---
 strbuf.c |   15 ++++++++++++---
 strbuf.h |    1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index f03d117..a6153dc 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -322,7 +322,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 	return -1;
 }
 
-int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
+int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
 	int ch;
 

@@ -332,10 +332,10 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 
 	strbuf_reset(sb);
 	while ((ch = fgetc(fp)) != EOF) {
-		if (ch == term)
-			break;
 		strbuf_grow(sb, 1);
 		sb->buf[sb->len++] = ch;
+		if (ch == term)
+			break;
 	}
 	if (ch == EOF && sb->len == 0)
 		return EOF;

@@ -344,6 +344,15 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 	return 0;
 }
 
+int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
+{
+	if (strbuf_getwholeline(sb, fp, term))
+		return EOF;
+	if (sb->buf[sb->len-1] == term)
+		strbuf_setlen(sb, sb->len-1);
+	return 0;
+}
+
 int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 {
 	int fd, len;

diff --git a/strbuf.h b/strbuf.h
index eaa8704..d05e056 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -126,6 +126,7 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
 
+extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);

-- 
1.6.4

From: Junio C Hamano <redacted>

It is not that uncommon to have mails with DOS line-ending, notably
Thunderbird and web mailers like Gmail (when saving what they call
"original" message).  So modify mailsplit to convert CRLF line-endings to
just LF.

Since git-rebase is built on top of git-am, add an option to mailsplit to
be used by git-am when it is acting on behalf of git-rebase, to refrain
from doing this conversion.

And add a test to make sure that rebase still works.

Signed-off-by: Brandon Casey <redacted>
---
 builtin-mailsplit.c |    6 ++++++
 git-am.sh           |    8 +++++++-
 t/t3400-rebase.sh   |   26 ++++++++++++++++++++++++--
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index d288fde..c9c85c3 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -44,6 +44,7 @@ static int is_from_line(const char *line, int len)
 }
 
 static struct strbuf buf = STRBUF_INIT;
+static int keep_cr;
 
 /* Called with the first line (potentially partial)
  * already in buf[] -- normally that should begin with

@@ -69,6 +70,12 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 	 * "From " and having something that looks like a date format.
 	 */
 	for (;;) {
+		if (!keep_cr && buf.len > 1 && buf.buf[buf.len-1] == '\n' &&
+			buf.buf[buf.len-2] == '\r') {
+			strbuf_setlen(&buf, buf.len-2);
+			strbuf_addch(&buf, '\n');
+		}
+
 		if (fwrite(buf.buf, 1, buf.len, output) != buf.len)
 			die_errno("cannot write output");
 

@@ -226,6 +233,8 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)
 			nr = strtol(arg+2, NULL, 10);
 		} else if ( arg[1] == 'b' && !arg[2] ) {
 			allow_bare = 1;
+		} else if (!strcmp(arg, "--keep-cr")) {
+			keep_cr = 1;
 		} else if ( arg[1] == 'o' && arg[2] ) {
 			dir = arg+2;
 		} else if ( arg[1] == '-' && !arg[2] ) {

diff --git a/git-am.sh b/git-am.sh
index d64d997..985226b 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -197,7 +197,13 @@ check_patch_format () {
 split_patches () {
 	case "$patch_format" in
 	mbox)
-		git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||
+		case "$rebasing" in
+		'')
+			keep_cr= ;;
+		?*)
+			keep_cr=--keep-cr ;;
+		esac
+		git mailsplit -d"$prec" -o"$dotest" -b $keep_cr -- "$@" > "$dotest/last" ||
 		clean_abort
 		;;
 	stgit-series)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index c5c29cc..4e6a44b 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh

@@ -3,9 +3,10 @@
 # Copyright (c) 2005 Amos Waterland
 #
 
-test_description='git rebase should not destroy author information
+test_description='git rebase assorted tests
 
-This test runs git rebase and checks that the author information is not lost.
+This test runs git rebase and checks that the author information is not lost
+among other things.
 '
 . ./test-lib.sh
 

@@ -133,4 +134,25 @@ test_expect_success 'rebase -q is quiet' '
      test ! -s output.out
 '
 
+q_to_cr () {
+	tr Q '\015'
+}
+
+test_expect_success 'Rebase a commit that sprinkles CRs in' '
+	(
+		echo "One"
+		echo "TwoQ"
+		echo "Three"
+		echo "FQur"
+		echo "Five"
+	) | q_to_cr >CR &&
+	git add CR &&
+	test_tick &&
+	git commit -a -m "A file with a line with CR" &&
+	git tag file-with-cr &&
+	git checkout HEAD^0 &&
+	git rebase --onto HEAD^^ HEAD^ &&
+	git diff --exit-code file-with-cr:CR HEAD:CR
+'
+
 test_done

-- 
1.6.4

From: Brandon Casey <redacted>

There should be no functional change.  Just the necessary changes and
simplifications associated with calling strbuf_getwholeline() rather
than an internal function or fgets.

Signed-off-by: Brandon Casey <redacted>
---
 builtin-mailinfo.c  |    8 +-------
 builtin-mailsplit.c |   20 ++++++++------------
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 92637ac..b0b5d8f 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c

@@ -765,7 +765,6 @@ static void handle_filter(struct strbuf *line)
 
 static void handle_body(void)
 {
-	int len = 0;
 	struct strbuf prev = STRBUF_INIT;
 
 	/* Skip up to the first boundary */

@@ -775,8 +774,6 @@ static void handle_body(void)
 	}
 
 	do {
-		strbuf_setlen(&line, line.len + len);
-
 		/* process any boundary lines */
 		if (*content_top && is_multipart_boundary(&line)) {
 			/* flush any leftover */

@@ -832,10 +829,7 @@ static void handle_body(void)
 			handle_filter(&line);
 		}
 
-		strbuf_reset(&line);
-		if (strbuf_avail(&line) < 100)
-			strbuf_grow(&line, 100);
-	} while ((len = read_line_with_nul(line.buf, strbuf_avail(&line), fin)));
+	} while (!strbuf_getwholeline(&line, fin, '\n'));
 
 handle_body_out:
 	strbuf_release(&prev);

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index ad5f6b5..c2ec6ea 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -7,6 +7,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "string-list.h"
+#include "strbuf.h"
 
 static const char git_mailsplit_usage[] =
 "git mailsplit [-d<prec>] [-f<n>] [-b] -o<directory> [<mbox>|<Maildir>...]";

@@ -42,8 +43,7 @@ static int is_from_line(const char *line, int len)
 	return 1;
 }
 
-/* Could be as small as 64, enough to hold a Unix "From " line. */
-static char buf[4096];
+static struct strbuf buf = STRBUF_INIT;
 
 /* We cannot use fgets() because our lines can contain NULs */
 int read_line_with_nul(char *buf, int size, FILE *in)

@@ -71,10 +71,9 @@ int read_line_with_nul(char *buf, int size, FILE *in)
 static int split_one(FILE *mbox, const char *name, int allow_bare)
 {
 	FILE *output = NULL;
-	int len = strlen(buf);
 	int fd;
 	int status = 0;
-	int is_bare = !is_from_line(buf, len);
+	int is_bare = !is_from_line(buf.buf, buf.len);
 
 	if (is_bare && !allow_bare)
 		goto corrupt;

@@ -88,20 +87,17 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 	 * "From " and having something that looks like a date format.
 	 */
 	for (;;) {
-		int is_partial = len && buf[len-1] != '\n';
-
-		if (fwrite(buf, 1, len, output) != len)
+		if (fwrite(buf.buf, 1, buf.len, output) != buf.len)
 			die_errno("cannot write output");
 
-		len = read_line_with_nul(buf, sizeof(buf), mbox);
-		if (len == 0) {
+		if (strbuf_getwholeline(&buf, mbox, '\n')) {
 			if (feof(mbox)) {
 				status = 1;
 				break;
 			}
 			die_errno("cannot read mbox");
 		}
-		if (!is_partial && !is_bare && is_from_line(buf, len))
+		if (!is_bare && is_from_line(buf.buf, buf.len))
 			break; /* done with one message */
 	}
 	fclose(output);

@@ -166,7 +162,7 @@ static int split_maildir(const char *maildir, const char *dir,
 			goto out;
 		}
 
-		if (fgets(buf, sizeof(buf), f) == NULL) {
+		if (strbuf_getwholeline(&buf, f, '\n')) {
 			error("cannot read mail %s (%s)", file, strerror(errno));
 			goto out;
 		}

@@ -203,7 +199,7 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
 	} while (isspace(peek));
 	ungetc(peek, f);
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (strbuf_getwholeline(&buf, f, '\n')) {
 		/* empty stdin is OK */
 		if (f != stdin) {
 			error("cannot read mbox %s", file);

-- 
1.6.4

From: Brandon Casey <redacted>


Signed-off-by: Brandon Casey <redacted>
---


Squash this with the previous one if you like.

-brandon


 builtin-mailsplit.c |   18 ------------------
 builtin.h           |    1 -
 2 files changed, 0 insertions(+), 19 deletions(-)

diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index c2ec6ea..d288fde 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c

@@ -45,24 +45,6 @@ static int is_from_line(const char *line, int len)
 
 static struct strbuf buf = STRBUF_INIT;
 
-/* We cannot use fgets() because our lines can contain NULs */
-int read_line_with_nul(char *buf, int size, FILE *in)
-{
-	int len = 0, c;
-
-	for (;;) {
-		c = getc(in);
-		if (c == EOF)
-			break;
-		buf[len++] = c;
-		if (c == '\n' || len + 1 >= size)
-			break;
-	}
-	buf[len] = '\0';
-
-	return len;
-}
-
 /* Called with the first line (potentially partial)
  * already in buf[] -- normally that should begin with
  * the Unix "From " line.  Write it into the specified

diff --git a/builtin.h b/builtin.h
index 20427d2..9993dfc 100644
--- a/builtin.h
+++ b/builtin.h

@@ -13,7 +13,6 @@ extern const char git_more_info_string[];
 extern void list_common_cmds_help(void);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void prune_packed_objects(int);
-extern int read_line_with_nul(char *buf, int size, FILE *file);
 extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
 	struct strbuf *out);
 extern int commit_tree(const char *msg, unsigned char *tree,

-- 
1.6.4

On Wed, Aug 5, 2009 at 05:31, Brandon Casey[off-list ref] wrote:

That's much better then any on-the-fly corrections :)

On Tue, 4 Aug 2009, Erik Faye-Lund wrote:

RFC 2822, section 2.3 explicitly states that a CR should not occur
without a LF (and vice versa, but the e-mail client might convert CRLF
to LF when saving to file), so I think this should be safe.

The rare BINARYMIME extension relaxes this requirement.

Tony.
-- 
f.anthony.n.finch  [off-list ref]  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.