Re: [PATCH] git-send-email: parse all messages in mbox

Vitaly Mayatskikh <v.mayatskih@gmail.com> writes:

I suspect nobody would comment on it because with reindentation this was extremely painful to review.

I _think_ the gist of your change is that (1) here you stop slurping the body when you see a line that begins with "From "...

... and (2) the "last unless (<F>)" at the end of the loop where you attempt to see if there is any more lines to be read from the stream (and if so go back and continue from the header parsing).

But I think the code structure is wrong. When you said "last if /^From /" earlier, you have already read that line (and you do not unread it), and here with "unless (<F>)" you are reading yet another line (one line after the UNIX-From line) and discarding it. The next round loses the real RFC2822 header line you discarded with this unless (<F>), and begins from the next line, and would break "First unfold multiline header fields" logic among other things.

But this is only from reviewing a patch with reindentation noise so I might have missed some subtle issues.

Can you make this into two patches for easier review? One to split out the existing loop for a single input stream into a helper function without changing any behaviour (i.e. the loop reads everything to the end), and then as a follow-up patch introduce "when we see a UNIX-From line we are at the beginning of the next message so return early" logic to the helper?

IOW, after the two-patch series, the current main-loop may look something like:

	my $unread_line = undef;
	while (1) {
		$unread_line = handle_one_stream(\*F, $unread_line);
                last if (!defined $unread_line);
	}
        close(\*F);

and your new handle_one_stream() sub will look something like:

	sub handle_one_stream {
		my ($fh, $last_line) = @_;
		local ($_);

		my $author = undef;
                my $author_encoding;
                my $has_content_type;
                my $body_encoding;
                @cc = ();
                @xh = ();
                my $input_format = undef;
                my @header = ();
                $message = "";
                $message_num++;

                # First unfold multiline header fields
                while (1) {
			if (defined $last_line) {
				$_ = $last_line;
                                $last_line = undef;
			} else {
                        	$_ = <F>;
			}
                        ...
		}
                # Now parse the header
                ...
                # Now parse the message body
                $last_line = undef;
                while (1) {
                	$_ = <F>;
			if (/^From /) {
				# This is the beginning of the
                                # next message; unread it.
				$last_line = $_;
                                last;
                        }
                        $message .= $_;
			...
		}
		return $last_line;
	}