On 05/13/2017 09:01 PM, Stefan Beller wrote:
In 250f79930d (diff.c: split emit_line() from the first char and the rest
of the line, 2009-09-14) we introduced the local variable 'nofirst' that
indicates if we have no first sign character. With the given implementation
we had to use an extra variable unlike reusing 'first' because the lines
first character could be '\0'.
Change the meaning of the 'first' argument to not mean the first character
of the line, but rather just containing the sign that is prepended to the
line. Refactor emit_line to not include the lines first character, but pass
the complete line as well as a '\0' sign, which now serves as an indication
not to print a sign.
With this patch other callers hard code the sign (which are '+', '-',
' ' and '\\') such that we do not run into unexpectedly emitting an
error-nous '\0'.
"erroneous"?
I also don't understand the meaning of this paragraph - if you mean that
this patch teaches other callers to hardcode the sign, I don't see any
such changes in the diff below.
After reading the patch below, would this commit message be better:
[begin]
diff.c: teach emit_line_0 to accept sign parameter
Instead of a separate "first" parameter representing the first character
of the line to be printed, make emit_line_0 take an optional "sign"
parameter specifically intended to hold the sign of the line. Callers
that store the sign and line separately can use the "sign" parameter
like they used the "first" parameter previously, and callers that store
the sign and line together (or do not have a sign) no longer need to
manipulate their arguments to fit the requirements of emit_line_0.
(And then mention that you have checked all the callers and that none of
them send '\n' or '\r' as the sign, as you have done in this version.)
[end]
The audit of the caller revealed that the sign cannot be '\n' or '\r',
so remove that condition for trailing newline or carriage return in the
sign; the else part of the condition handles the len==0 perfectly,
so we can drop the if/else construct.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
---
diff.c | 40 +++++++++++++++++-----------------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/diff.c b/diff.c
index c2ed605cd0..4269b8dccf 100644
--- a/diff.c
+++ b/diff.c
@@ -517,33 +517,24 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
}
static void emit_line_0(struct diff_options *o, const char *set, const char *reset,
- int first, const char *line, int len)
+ int sign, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
- int nofirst;
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
- if (len == 0) {
- has_trailing_newline = (first == '\n');
- has_trailing_carriage_return = (!has_trailing_newline &&
- (first == '\r'));
- nofirst = has_trailing_newline || has_trailing_carriage_return;
- } else {
- has_trailing_newline = (len > 0 && line[len-1] == '\n');
- if (has_trailing_newline)
- len--;
- has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
- if (has_trailing_carriage_return)
- len--;
- nofirst = 0;
- }
+ has_trailing_newline = (len > 0 && line[len-1] == '\n');
+ if (has_trailing_newline)
+ len--;
+ has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+ if (has_trailing_carriage_return)
+ len--;
- if (len || !nofirst) {
+ if (len || sign) {
fputs(set, file);
- if (!nofirst)
- fputc(first, file);
+ if (sign)
+ fputc(sign, file);
fwrite(line, len, 1, file);
fputs(reset, file);
}
@@ -556,7 +547,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
static void emit_line(struct diff_options *o, const char *set, const char *reset,
const char *line, int len)
{
- emit_line_0(o, set, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, reset, 0, line, len);
}
Maybe this function is unnecessary now that emit_line_0 can take the
line directly.
static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
@@ -4822,9 +4813,12 @@ void diff_flush(struct diff_options *options)
if (output_format & DIFF_FORMAT_PATCH) {
if (separator) {
- fprintf(options->file, "%s%c",
- diff_line_prefix(options),
- options->line_termination);
+ char term[2];
+ term[0] = options->line_termination;
+ term[1] = '\0';
+
+ emit_line(options, NULL, NULL,
+ term, 1);
If options->line_termination is 0, this is actually a zero-length string
(not 1).
if (options->stat_sep) {
/* attach patch instead of inline */
fputs(options->stat_sep, options->file);