> From dae20e25960c73bd7ccc0939fe096bb68a009fb5 Mon Sep 17 00:00:00 2001
> From: erwangg <erwangg@xxxxxxxxxxxxxxxxx>
> Date: Wed, 27 Aug 2008 12:22:43 +0800
> Subject: [PATCH] ingore cr at eol when diff with color=auto
>
> ---
Thanks. A few comments.
> diff --git a/diff.c b/diff.c
> index 18fa7a7..846a9af 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -517,9 +517,16 @@ static void emit_line(FILE *file, const char *set,
> const char *reset, const char
> if (has_trailing_newline)
> len--;
>
> + int has_trailing_return = (len > 0 && line[len-1] == '\r');
decl-after-statement.
> + if (has_trailing_return)
> + len--;
> +
> fputs(set, file);
> fwrite(line, len, 1, file);
> fputs(reset, file);
> +
> + if (has_trailing_return)
> + fputc('\r', file);
> if (has_trailing_newline)
> fputc('\n', file);
> }
We indent with tab which jumps the cursor to next column that is multiple
of 8.
Otherwise this hunk is good.
> @@ -535,7 +542,7 @@ static void emit_add_line(const char *reset, struct
> emit_callback *ecbdata, cons
> /* Emit just the prefix, then the rest. */
> emit_line(ecbdata->file, set, reset, line, ecbdata->nparents);
> ws_check_emit(line + ecbdata->nparents,
> - len - ecbdata->nparents, ecbdata->ws_rule,
> + len - ecbdata->nparents, ecbdata->ws_rule|WS_CR_AT_EOL,
> ecbdata->file, set, reset, ws);
> }
> }
I do not think you want to force WS_CR_AT_EOL here.
That is a policy issue (not "git policy", but the policy of the contents
that is managed by git, in other words, your repository) that you should
control with core.whitespace (and if you want finer grain control, then
with .gitattributes).
Your patch is whitespace damanged, and has style issues, and lacks any
comment to defend the change. It does not have a Sign-off, either. Even
though the intent is good, I cannot apply this as-is. Please look at
Documentation/SubmittingPatches and emulate patches from other git
regulars.
You say "when color=auto" in the patch title, but there is nothing that
restricts the effect of this change only to that case in your patch. A
comment to defend the change should address things like that, and here is
how I would write it.
When the tracked contents have CRLF line endings, colored diff output
shows "^M" at the end of output lines, which is distracting, even
though the pager we use by default ("less") knows to hide them.
The problem is that "less" hides a carriage-return only at the end of
the line, immediately before a line feed. The colored diff output
does not take this into account, and emits four element sequence for
each line:
- force this color;
- the line up to but not including the terminating line feed;
- reset color
- line feed.
By including the carriage return at the end of the line in the second
item, we are breaking the smart our pager has in order not to show
"^M". This can be fixed by changing the sequence to:
- force this color;
- the line up to but not including the terminating end-of-line;
- reset color
- end-of-line.
where end-of-line is either a single linefeed or a CRLF pair.
When the output is not colored, "force this color" and "reset color"
sequences are both empty, so we won't have this problem with or
without this patch.
If we drop the hunk to emit_add_line() that forces WS_CR_AT_EOL, I find
that the intent and the approach of the patch is quite good. I suspect
that combined-diff output routines have the same issue that you need to
fix the same way, but I didn't look.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
