Re: [PATCH] log-tree: use custom line terminator in line termination mode

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2012, 20:03 UTC
Message-ID: <7vmx5r8y9i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vobq7al12.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

>> The correct output would have NUL after each commit, so "-z --format=%s"
>> would have a single-liner subject with the line-terminating LF replaced
>> with NUL, and "-p/--stat -z --format=%s" would have a single-liner subject
>> with its line-terminating LF, followed by the diff/diffstat in which the
>> terminating LF of the last line is replaced with NUL, but to be consistent
>> with what "-p/--stat -z --pretty=format:%s" does, I think it is OK to
>> append NUL to the diff/diffstat part instead of replacing its last LF with
>> NUL.
>
> In other words, this test on top (the last one only demonstrates the
> breakage).

Just a short hint if anybody wants to take a stab at it while I am deeply in today's integration cycle.

The code for "separator" semantics was a well tested code when "terminator" semantics was bolted on, and operated like this:

        for each record
        do
                if we have shown a record already
                then
                        show the termination character
                fi
                show the record
        done

The only difference between the two semantics is if we append the termination character after all the above is done, so the code should be structured that way, but that may not be how we currently do it. So the proper way to add the "terminator" semantics ought to be:

        if we have shown any record in the loop && opt->use_terminator
        then
                show the termination character
        fi

at the end of the above loop. If we see opt->use_terminator anywhere else in the existing code, it is an indication of a bug.

There is one small glitch. If a record is _not_ terminated with a newline, e.g. "log --pretty=format:%s" without --stat/-p, "separator" semantics will end up giving an incomplete line at the end. Most of the time we will be piping out output to the pager so it may not be a problem in the real life, but it would be nicer to also terminate such an output.

So the resulting logic should look something like:

        for each record
        do
                if we have shown a record already
                then
                        show the termination character
                fi
                show the record
                remember if the record ended with a LF
        done
        if we have shown any record in the loop &&
           (opt->use_terminator ||
            (opt->diffopt.line_termination == '\n' &&
             the last record did not end with a LF))
        then
                show the termination character
        fi