Re: [PATCH v3 0/4] showing existing ws breakage
Jeff King writes:
> On Wed, May 27, 2015 at 01:46:26PM -0700, Junio C Hamano wrote:
>
>> Jeff King writes:
>>
>> > Do you want me to also eradicate PLAIN from the code itself? It's a
>> > rather simple change, but it does touch a lot of places.
>>
>> Nah, that is not user-facing. We do not do s/cache/index/ in the
>> code, either.
>>
>> Besides, I actually find plain much easier to type than context ;-)
>
> OK. I just did it while our emails were in flight, so here it is just
> for reference.
I actually like that; the changes are fairly isolated and contained.
>
> -- >8 --
> Subject: diff.h: rename DIFF_PLAIN color slot to DIFF_CONTEXT
>
> The latter is a much more descriptive name (and we support
> "color.diff.context" now). This also updates the name of any
> local variables which were used to store the color.
>
> Signed-off-by: Jeff King
> ---
> combine-diff.c | 6 +++---
> diff.c | 26 +-
> diff.h | 2 +-
> line-log.c | 6 +++---
> 4 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 8eb7278..30c7eb6 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -730,7 +730,7 @@ static void dump_sline(struct sline *sline, const char
> *line_prefix,
> const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
> const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
> const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
> - const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
> + const char *c_context = diff_get_color(use_color, DIFF_CONTEXT);
> const char *c_reset = diff_get_color(use_color, DIFF_RESET);
>
> if (result_deleted)
> @@ -793,7 +793,7 @@ static void dump_sline(struct sline *sline, const char
> *line_prefix,
> }
> if (comment_end)
> printf("%s%s %s%s", c_reset,
> - c_plain, c_reset,
> + c_context, c_reset,
> c_func);
> for (i = 0; i < comment_end; i++)
> putchar(hunk_comment[i]);
> @@ -828,7 +828,7 @@ static void dump_sline(struct sline *sline, const char
> *line_prefix,
>*/
> if (!context)
> continue;
> - fputs(c_plain, stdout);
> + fputs(c_context, stdout);
> }
> else
> fputs(c_new, stdout);
> diff --git a/diff.c b/diff.c
> index 27bd371..100773f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -42,7 +42,7 @@ static long diff_algorithm;
>
> static char diff_colors[][COLOR_MAXLEN] = {
> GIT_COLOR_RESET,
> - GIT_COLOR_NORMAL, /* PLAIN */
> + GIT_COLOR_NORMAL, /* CONTEXT */
> GIT_COLOR_BOLD, /* METAINFO */
> GIT_COLOR_CYAN, /* FRAGINFO */
> GIT_COLOR_RED, /* OLD */
> @@ -55,7 +55,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
> static int parse_diff_color_slot(const char *var)
> {
> if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
> - return DIFF_PLAIN;
> + return DIFF_CONTEXT;
> if (!strcasecmp(var, "meta"))
> return DIFF_METAINFO;
> if (!strcasecmp(var, "frag"))
> @@ -501,7 +501,7 @@ static void emit_add_line(const char *reset,
> static void emit_hunk_header(struct emit_callback *ecbdata,
>const char *line, int len)
> {
> - const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
> + const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
> const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
> const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
> const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
> @@ -518,7 +518,7 @@ static void emit_hunk_header(struct emit_callback
> *ecbdata,
> if (len < 10 ||
> memcmp(line, atat, 2) ||
> !(ep = memmem(line + 2, len - 2, atat, 2))) {
> - emit_line(ecbdata->opt, plain, reset, line, len);
> + emit_line(ecbdata->opt, context, reset, line, len);
> return;
> }
> ep += 2; /* skip over @@ */
> @@ -540,7 +540,7 @@ static void emit_hunk_header(struct emit_callback
> *ecbdata,
> if (*ep != ' ' && *ep != '\t')
> break;
> if (ep != cp) {
> - strbuf_addstr(&msgbuf, plain);
> + strbuf_addstr(&msgbuf, context);
> strbuf_add(&msgbuf, cp, ep - cp);
> strbuf_addstr(&msgbuf, reset);
> }
> @@ -623,10 +623,10 @@ static void emit_rewrite_lines(struct emit_callb
Re: [PATCH v3 0/4] showing existing ws breakage
On Wed, May 27, 2015 at 03:22:18AM -0400, Jeff King wrote: > In color.diff.*, these are called "new", "old", and "plain". I am of the > opinion that "context" is a far better name than "plain", but perhaps we > should support both for consistency. > > Here's a patch for the color.diff side, if we want to go that route. So I had originally thought we would support both names in both places. But since the diff patch we ended up with is basically "the real name is context; plain is just a historical anomaly", I do not see any need to support "plain" in your new whitespace code. I suspect you came to the same conclusion independently, but I wanted to make sure what I had written before didn't leave anybody confused. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] showing existing ws breakage
On Wed, May 27, 2015 at 01:46:26PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > Do you want me to also eradicate PLAIN from the code itself? It's a
> > rather simple change, but it does touch a lot of places.
>
> Nah, that is not user-facing. We do not do s/cache/index/ in the
> code, either.
>
> Besides, I actually find plain much easier to type than context ;-)
OK. I just did it while our emails were in flight, so here it is just
for reference.
-- >8 --
Subject: diff.h: rename DIFF_PLAIN color slot to DIFF_CONTEXT
The latter is a much more descriptive name (and we support
"color.diff.context" now). This also updates the name of any
local variables which were used to store the color.
Signed-off-by: Jeff King
---
combine-diff.c | 6 +++---
diff.c | 26 +-
diff.h | 2 +-
line-log.c | 6 +++---
4 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index 8eb7278..30c7eb6 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -730,7 +730,7 @@ static void dump_sline(struct sline *sline, const char
*line_prefix,
const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
- const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
+ const char *c_context = diff_get_color(use_color, DIFF_CONTEXT);
const char *c_reset = diff_get_color(use_color, DIFF_RESET);
if (result_deleted)
@@ -793,7 +793,7 @@ static void dump_sline(struct sline *sline, const char
*line_prefix,
}
if (comment_end)
printf("%s%s %s%s", c_reset,
- c_plain, c_reset,
+ c_context, c_reset,
c_func);
for (i = 0; i < comment_end; i++)
putchar(hunk_comment[i]);
@@ -828,7 +828,7 @@ static void dump_sline(struct sline *sline, const char
*line_prefix,
*/
if (!context)
continue;
- fputs(c_plain, stdout);
+ fputs(c_context, stdout);
}
else
fputs(c_new, stdout);
diff --git a/diff.c b/diff.c
index 27bd371..100773f 100644
--- a/diff.c
+++ b/diff.c
@@ -42,7 +42,7 @@ static long diff_algorithm;
static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
- GIT_COLOR_NORMAL, /* PLAIN */
+ GIT_COLOR_NORMAL, /* CONTEXT */
GIT_COLOR_BOLD, /* METAINFO */
GIT_COLOR_CYAN, /* FRAGINFO */
GIT_COLOR_RED, /* OLD */
@@ -55,7 +55,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
static int parse_diff_color_slot(const char *var)
{
if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
- return DIFF_PLAIN;
+ return DIFF_CONTEXT;
if (!strcasecmp(var, "meta"))
return DIFF_METAINFO;
if (!strcasecmp(var, "frag"))
@@ -501,7 +501,7 @@ static void emit_add_line(const char *reset,
static void emit_hunk_header(struct emit_callback *ecbdata,
const char *line, int len)
{
- const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+ const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
@@ -518,7 +518,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
if (len < 10 ||
memcmp(line, atat, 2) ||
!(ep = memmem(line + 2, len - 2, atat, 2))) {
- emit_line(ecbdata->opt, plain, reset, line, len);
+ emit_line(ecbdata->opt, context, reset, line, len);
return;
}
ep += 2; /* skip over @@ */
@@ -540,7 +540,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
if (*ep != ' ' && *ep != '\t')
break;
if (ep != cp) {
- strbuf_addstr(&msgbuf, plain);
+ strbuf_addstr(&msgbuf, context);
strbuf_add(&msgbuf, cp, ep - cp);
strbuf_addstr(&msgbuf, reset);
}
@@ -623,10 +623,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
data += len;
}
if (!endp) {
- const char *plain = diff_get_color(ecb->color_diff,
- DIFF_PL
Re: [PATCH v3 0/4] showing existing ws breakage
Jeff King writes: > Do you want me to also eradicate PLAIN from the code itself? It's a > rather simple change, but it does touch a lot of places. Nah, that is not user-facing. We do not do s/cache/index/ in the code, either. Besides, I actually find plain much easier to type than context ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] showing existing ws breakage
On Wed, May 27, 2015 at 11:57:15AM -0700, Junio C Hamano wrote: > > -- >8 -- > > Subject: diff: accept color.diff.context as a synonym for "plain" > > > > The term "plain" is a bit ambiguous; let's allow the more > > specific "context", but keep "plain" around for > > compatibility. > > > > Signed-off-by: Jeff King > > --- > > I didn't bother mentioning the historical "plain" in the documentation. > > I don't know if it's better to (for people who find it in the wild and > > wonder what it means) or if it simply clutters the description. > > 'plain' does sound a misnomer, as these slot names are about "what" > are painted, not "how" they are painted. The latter is what their > values represent. Whoever named that slot was confused by the fact > that 'context' (i.e. "what") lines are by default painted in 'plain' > color without frills (i.e. "how"). > > We usually try to give a brief mention to historical names primarily > to silence those who pick up stale information from the Web, get > curious, and then complain loudly after finding that we no longer > document them even though we keep accepting them silently, so I am > somewhat tempted to do this on top. Yeah, I waffled on doing it myself. If you take the patch, please do squash that in. Do you want me to also eradicate PLAIN from the code itself? It's a rather simple change, but it does touch a lot of places. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] showing existing ws breakage
Jeff King writes: > In color.diff.*, these are called "new", "old", and "plain". I am of the > opinion that "context" is a far better name than "plain", but perhaps we > should support both for consistency. > > Here's a patch for the color.diff side, if we want to go that route. > > -- >8 -- > Subject: diff: accept color.diff.context as a synonym for "plain" > > The term "plain" is a bit ambiguous; let's allow the more > specific "context", but keep "plain" around for > compatibility. > > Signed-off-by: Jeff King > --- > I didn't bother mentioning the historical "plain" in the documentation. > I don't know if it's better to (for people who find it in the wild and > wonder what it means) or if it simply clutters the description. 'plain' does sound a misnomer, as these slot names are about "what" are painted, not "how" they are painted. The latter is what their values represent. Whoever named that slot was confused by the fact that 'context' (i.e. "what") lines are by default painted in 'plain' color without frills (i.e. "how"). We usually try to give a brief mention to historical names primarily to silence those who pick up stale information from the Web, get curious, and then complain loudly after finding that we no longer document them even though we keep accepting them silently, so I am somewhat tempted to do this on top. diff --git a/Documentation/config.txt b/Documentation/config.txt index 0a7ffa5..b458590 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -870,7 +870,8 @@ command line with the `--color[=]` option. color.diff.:: Use customized color for diff colorization. `` specifies which part of the patch to use the specified color, and is one - of `context` (context text), `meta` (metainformation), `frag` + of `context` (context text - `plain` is a historical synonym), + `meta` (metainformation), `frag` (hunk header), 'func' (function in hunk header), `old` (removed lines), `new` (added lines), `commit` (commit headers), or `whitespace` (highlighting whitespace errors). The values of these variables may be -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] showing existing ws breakage
On Tue, May 26, 2015 at 11:30:28PM -0700, Junio C Hamano wrote:
> The fourth one in v2 used a new option "--[no-]ws-check-deleted",
> but in this round a new option "--ws-error-highlight=" is
> defined instead. With that, you can say
>
> diff --ws-error-highlight=new,old
>
> to say "I want to see whitespace errors on new and old lines", and
>
> diff --ws-error-highlight=new,old,context
> diff --ws-error-highlight=all
>
> can be used to also see whitespace errors on context lines. Being
> able to see whitespace errors on context lines, i.e. the ones that
> were there in the original and you left intact, would help you see
> how prevalent whitespace errors are in the original and hopefully
> would make it easier for you to decide if a separate preliminary
> clean-up to only fix these whitespace errors is warranted.
That makes sense. Neat feature.
In color.diff.*, these are called "new", "old", and "plain". I am of the
opinion that "context" is a far better name than "plain", but perhaps we
should support both for consistency.
Here's a patch for the color.diff side, if we want to go that route.
-- >8 --
Subject: diff: accept color.diff.context as a synonym for "plain"
The term "plain" is a bit ambiguous; let's allow the more
specific "context", but keep "plain" around for
compatibility.
Signed-off-by: Jeff King
---
I didn't bother mentioning the historical "plain" in the documentation.
I don't know if it's better to (for people who find it in the wild and
wonder what it means) or if it simply clutters the description. It may
also be that people don't find "plain" as meaningless as I do, and would
rather leave it as the primary in the documentation (and accepting
"context" is just a nicety).
I also resisted the urge to refactor every internal variable and enum
mentioning "plain" into "context". I guess whether that is a good idea
depends on how strongly you agree that "plain" is a bad name. :)
Documentation/config.txt | 2 +-
diff.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f76e8c..34ef9fe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -914,7 +914,7 @@ command line with the `--color[=]` option.
color.diff.::
Use customized color for diff colorization. `` specifies
which part of the patch to use the specified color, and is one
- of `plain` (context text), `meta` (metainformation), `frag`
+ of `context` (context text), `meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
`new` (added lines), `commit` (commit headers), or `whitespace`
(highlighting whitespace errors).
diff --git a/diff.c b/diff.c
index 7500c55..27bd371 100644
--- a/diff.c
+++ b/diff.c
@@ -54,7 +54,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
static int parse_diff_color_slot(const char *var)
{
- if (!strcasecmp(var, "plain"))
+ if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
return DIFF_PLAIN;
if (!strcasecmp(var, "meta"))
return DIFF_METAINFO;
--
2.4.1.552.g6de66a4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] showing existing ws breakage
We paint whitespace breakages in new (i.e. added or updated) lines when showing the "git diff" output to help people avoid introducing them with their changes. The basic premise is that people would want to avoid touching existing lines only to fix whitespace errors in a patch that does other changes of substance, and that is why we traditionally did not paint whitespace breakages in existing (i.e. deleted or context) lines. However, some people would want to keep existing breakages when they are doing other changes of substance; "new" lines in such a patch would show existing whitespace breakages painted, and it is not apparent if the breakages were inherited from the original or newly introduced. Christian Brabandt had an interesting idea to help users in this situation; why not give them a mode to paint whitespace breakages in "old" (i.e. deleted or was replaced) lines, too? http://thread.gmane.org/gmane.comp.version-control.git/269912/focus=269956 This series is a reroll of the previous one http://thread.gmane.org/gmane.comp.version-control.git/269972 which built on Christian'sidea, but with a different implementation (Christian's original painted trailing whitespaces only). The first two patches are unchanged since v2; they are preliminary clean-ups. The third one extends the corresponding patch since v2; not only a helper for "deleted" lines but also another helper for "context" lines is added. The fourth one in v2 used a new option "--[no-]ws-check-deleted", but in this round a new option "--ws-error-highlight=" is defined instead. With that, you can say diff --ws-error-highlight=new,old to say "I want to see whitespace errors on new and old lines", and diff --ws-error-highlight=new,old,context diff --ws-error-highlight=all can be used to also see whitespace errors on context lines. Being able to see whitespace errors on context lines, i.e. the ones that were there in the original and you left intact, would help you see how prevalent whitespace errors are in the original and hopefully would make it easier for you to decide if a separate preliminary clean-up to only fix these whitespace errors is warranted. Junio C Hamano (4): t4015: modernise style t4015: separate common setup and per-test expectation diff.c: add emit_del_line() and emit_context_line() diff.c: --ws-error-highlight= option Documentation/diff-options.txt | 10 + diff.c | 122 -- diff.h | 5 + t/t4015-diff-whitespace.sh | 508 ++--- 4 files changed, 385 insertions(+), 260 deletions(-) -- 2.4.2-503-g3e4528a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
