preparatory diff improvements for moved color detection

PATCH In review Stefan Beller 3 participants 9 messages Sep 7, 2016 – Sep 8, 2016

Share & tools

Stefan Beller

author

2016-09-07 23:36 UTC #0 lore raw

cover preparatory diff improvements for moved color detection

Motivated by the feedback on the "[PATCHv4] diff.c: emit moved lines with a different color"[1], I started refactoring the fn_out_consume function in diff.c. This lead to a huge amount of tiny patches so far, but nothing to present as an end result.

These patches are preparatory for this effort and I want them out such that I do not need to worry about them later.

Thanks,
Stefan

[1] https://public-inbox.org/git/[email protected]/

Stefan Beller (3):
diff.c: use diff_options directly
diff: omit found pointer from emit_callback
diff: remove dead code

 diff.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

Signature · 1 line

2.10.0.2.g0676c79.dirty

Stefan Beller

author

2016-09-07 23:37 UTC #1 lore raw

PATCH 1/3 diff.c: use diff_options directly

The value of ecbdata->opt is accessible via the short variable o already, so let's use that instead.

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff.c +10 ~~-11~~

index 534c12e..4a6501c 100644
@@ -1217,7 +1217,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	const char *line_prefix = diff_line_prefix(o);
 
 	if (ecbdata->header) {
-		fprintf(ecbdata->opt->file, "%s", ecbdata->header->buf);
+		fprintf(o->file, "%s", ecbdata->header->buf);
 		strbuf_reset(ecbdata->header);
 		ecbdata->header = NULL;
 	}

@@ -1229,9 +1229,9 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
 		name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
 
-		fprintf(ecbdata->opt->file, "%s%s--- %s%s%s\n",
+		fprintf(o->file, "%s%s--- %s%s%s\n",
 			line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
-		fprintf(ecbdata->opt->file, "%s%s+++ %s%s%s\n",
+		fprintf(o->file, "%s%s+++ %s%s%s\n",
 			line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
 		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
 	}

@@ -1249,15 +1249,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		find_lno(line, ecbdata);
 		emit_hunk_header(ecbdata, line, len);
 		if (line[len-1] != '\n')
-			putc('\n', ecbdata->opt->file);
+			putc('\n', o->file);
 		return;
 	}
 
 	if (len < 1) {
-		emit_line(ecbdata->opt, reset, reset, line, len);
+		emit_line(o, reset, reset, line, len);
 		if (ecbdata->diff_words
 		    && ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
-			fputs("~\n", ecbdata->opt->file);
+			fputs("~\n", o->file);
 		return;
 	}

@@ -1282,8 +1282,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		}
 		diff_words_flush(ecbdata);
 		if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
-			emit_line(ecbdata->opt, context, reset, line, len);
-			fputs("~\n", ecbdata->opt->file);
+			emit_line(o, context, reset, line, len);
+			fputs("~\n", o->file);
 		} else {
 			/*
 			 * Skip the prefix character, if any.  With

@@ -1294,7 +1294,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			      line++;
 			      len--;
 			}
-			emit_line(ecbdata->opt, context, reset, line, len);
+			emit_line(o, context, reset, line, len);
 		}
 		return;
 	}

@@ -1316,8 +1316,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	default:
 		/* incomplete line at the end */
 		ecbdata->lno_in_preimage++;
-		emit_line(ecbdata->opt,
-			  diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
+		emit_line(o, diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
 			  reset, line, len);
 		break;
 	}

Signature · 1 line

2.10.0.2.g0676c79.dirty

Jacob Keller

2016-09-08 06:32 UTC #2 lore raw

Re: [PATCH 1/3] diff.c: use diff_options directly

Quoted 4 lines · Stefan Beller

The value of ecbdata->opt is accessible via the short variable o already, so let's use that instead.
Signed-off-by Stefan Beller <[email protected]>

Seems reasonable.

Quoted 81 lines

---
 diff.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff.c +10 ~~-12~~

index 534c12e..4a6501c 100644
@@ -1217,7 +1217,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
        const char *line_prefix = diff_line_prefix(o);
 
        if (ecbdata->header) {
-               fprintf(ecbdata->opt->file, "%s", ecbdata->header->buf);
+               fprintf(o->file, "%s", ecbdata->header->buf);
                strbuf_reset(ecbdata->header);
                ecbdata->header = NULL;
        }

@@ -1229,9 +1229,9 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
                name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
                name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
 
-               fprintf(ecbdata->opt->file, "%s%s--- %s%s%s\n",
+               fprintf(o->file, "%s%s--- %s%s%s\n",
                        line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
-               fprintf(ecbdata->opt->file, "%s%s+++ %s%s%s\n",
+               fprintf(o->file, "%s%s+++ %s%s%s\n",
                        line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
                ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
        }

@@ -1249,15 +1249,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
                find_lno(line, ecbdata);
                emit_hunk_header(ecbdata, line, len);
                if (line[len-1] != '\n')
-                       putc('\n', ecbdata->opt->file);
+                       putc('\n', o->file);
                return;
        }
 
        if (len < 1) {
-               emit_line(ecbdata->opt, reset, reset, line, len);
+               emit_line(o, reset, reset, line, len);
                if (ecbdata->diff_words
                    && ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
-                       fputs("~\n", ecbdata->opt->file);
+                       fputs("~\n", o->file);
                return;
        }

@@ -1282,8 +1282,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
                }
                diff_words_flush(ecbdata);
                if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
-                       emit_line(ecbdata->opt, context, reset, line, len);
-                       fputs("~\n", ecbdata->opt->file);
+                       emit_line(o, context, reset, line, len);
+                       fputs("~\n", o->file);
                } else {
                        /*
                         * Skip the prefix character, if any.  With

@@ -1294,7 +1294,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
                              line++;
                              len--;
                        }
-                       emit_line(ecbdata->opt, context, reset, line, len);
+                       emit_line(o, context, reset, line, len);
                }
                return;
        }

@@ -1316,8 +1316,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
        default:
                /* incomplete line at the end */
                ecbdata->lno_in_preimage++;
-               emit_line(ecbdata->opt,
-                         diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
+               emit_line(o, diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
                          reset, line, len);
                break;
        }
--

2.10.0.2.g0676c79.dirty

Stefan Beller

author

2016-09-07 23:37 UTC #3 lore raw

PATCH 1/2 diff: omit found pointer from emit_callback

In reply to Stefan Beller

We keep the actual data in the diff options, which are just as accessible. Remove the pointer stored in struct emit_callback for readability.

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff.c +1 -4

index 4a6501c..79ad91d 100644
@@ -354,7 +354,6 @@ struct emit_callback {
 	const char **label_path;
 	struct diff_words_data *diff_words;
 	struct diff_options *opt;
-	int *found_changesp;
 	struct strbuf *header;
 };

@@ -722,7 +721,6 @@ static void emit_rewrite_diff(const char *name_a,
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
 	ecbdata.color_diff = want_color(o->use_color);
-	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b);
 	ecbdata.opt = o;
 	if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {

@@ -1215,13 +1213,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 	struct diff_options *o = ecbdata->opt;
 	const char *line_prefix = diff_line_prefix(o);
+	o->found_changes = 1;
 
 	if (ecbdata->header) {
 		fprintf(o->file, "%s", ecbdata->header->buf);
 		strbuf_reset(ecbdata->header);
 		ecbdata->header = NULL;
 	}
-	*(ecbdata->found_changesp) = 1;
 
 	if (ecbdata->label_path[0]) {
 		const char *name_a_tab, *name_b_tab;

@@ -2437,7 +2435,6 @@ static void builtin_diff(const char *name_a,
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
 		ecbdata.color_diff = want_color(o->use_color);
-		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
 			check_blank_at_eof(&mf1, &mf2, &ecbdata);

Signature · 1 line

2.10.0.2.g0676c79.dirty

Stefan Beller

author

2016-09-07 23:37 UTC #4 lore raw

PATCH 2/3 diff: omit found pointer from emit_callback

In reply to Stefan Beller

We keep the actual data in the diff options, which are just as accessible. Remove the pointer stored in struct emit_callback for readability.

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff.c +1 -4

index 4a6501c..79ad91d 100644
@@ -354,7 +354,6 @@ struct emit_callback {
 	const char **label_path;
 	struct diff_words_data *diff_words;
 	struct diff_options *opt;
-	int *found_changesp;
 	struct strbuf *header;
 };

@@ -722,7 +721,6 @@ static void emit_rewrite_diff(const char *name_a,
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
 	ecbdata.color_diff = want_color(o->use_color);
-	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b);
 	ecbdata.opt = o;
 	if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {

@@ -1215,13 +1213,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 	struct diff_options *o = ecbdata->opt;
 	const char *line_prefix = diff_line_prefix(o);
+	o->found_changes = 1;
 
 	if (ecbdata->header) {
 		fprintf(o->file, "%s", ecbdata->header->buf);
 		strbuf_reset(ecbdata->header);
 		ecbdata->header = NULL;
 	}
-	*(ecbdata->found_changesp) = 1;
 
 	if (ecbdata->label_path[0]) {
 		const char *name_a_tab, *name_b_tab;

@@ -2437,7 +2435,6 @@ static void builtin_diff(const char *name_a,
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
 		ecbdata.color_diff = want_color(o->use_color);
-		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
 			check_blank_at_eof(&mf1, &mf2, &ecbdata);

Signature · 1 line

2.10.0.2.g0676c79.dirty

Junio C Hamano

2016-09-08 20:53 UTC #5 lore raw

Re: [PATCH 2/3] diff: omit found pointer from emit_callback

diff.c struct emit_callback { · line 358

Quoted 11 lines · Stefan Beller

diff.c +0 -1

index 4a6501c..79ad91d 100644
@@ -354,7 +354,6 @@ struct emit_callback {
 	const char **label_path;
 	struct diff_words_data *diff_words;
 	struct diff_options *opt;
-	int *found_changesp;
 	struct strbuf *header;
 };

I briefly wondered if we have some callsites that do not want o->found_changes to be modified (hence pointing this field at elsewhere), but the fact that you can _remove_ this field means that there is no such use case, which is good.

Quoted 10 lines

diff.c +0 -1

@@ -722,7 +721,6 @@ static void emit_rewrite_diff(const char *name_a,
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
 	ecbdata.color_diff = want_color(o->use_color);
-	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b);
 	ecbdata.opt = o;
 	if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {

@@ -1215,13 +1213,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);

diff.c static void fn_out_consume(void *priv, char *line, unsigned long len) · line 1224

Quoted 10 lines

diff.c +1 -1

 	struct diff_options *o = ecbdata->opt;
 	const char *line_prefix = diff_line_prefix(o);
+	o->found_changes = 1;
 
 	if (ecbdata->header) {
 		fprintf(o->file, "%s", ecbdata->header->buf);
 		strbuf_reset(ecbdata->header);
 		ecbdata->header = NULL;
 	}
-	*(ecbdata->found_changesp) = 1;

Is there a good reason to move the assignment up? "The fact that this function was called even once means we found some change" is probably a good argument, but then I'd prefer to have a blank before it to separate it (the first statement) from the block of decls.

No need to resend. Thanks.

Stefan Beller

author

2016-09-07 23:37 UTC #6 lore raw

PATCH 3/3 diff: remove dead code

In reply to Stefan Beller

When len < 1, len has to be 0 or negative, emit_line will then remove the first character and by then len would be negative. As this doesn't happen, it is safe to assume it is dead code.

This continues to simplify the code, which was started in b8d9c1a66b (2009-09-03, diff.c: the builtin_diff() deals with only two-file comparison).

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 8 --------
 1 file changed, 8 deletions(-)

diff.c +0 -8

index 79ad91d..c143019 100644
@@ -1251,14 +1251,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (len < 1) {
-		emit_line(o, reset, reset, line, len);
-		if (ecbdata->diff_words
-		    && ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
-			fputs("~\n", o->file);
-		return;
-	}
-
 	if (ecbdata->diff_words) {
 		if (line[0] == '-') {
 			diff_words_append(line, len,

Signature · 1 line

2.10.0.2.g0676c79.dirty

Junio C Hamano

2016-09-08 21:07 UTC #7 lore raw

Re: [PATCH 3/3] diff: remove dead code

Quoted 7 lines · Stefan Beller

When len < 1, len has to be 0 or negative, emit_line will then remove the first character and by then len would be negative. As this doesn't happen, it is safe to assume it is dead code.
This continues to simplify the code, which was started in b8d9c1a66b (2009-09-03, diff.c: the builtin_diff() deals with only two-file comparison).

We look at line[0] to see if it is '@' before this check, which would have been wrong if "len < 1" were ever true.

Quoted 25 lines

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 8 --------
 1 file changed, 8 deletions(-)

diff.c +0 -8

index 79ad91d..c143019 100644
@@ -1251,14 +1251,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (len < 1) {
-		emit_line(o, reset, reset, line, len);
-		if (ecbdata->diff_words
-		    && ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
-			fputs("~\n", o->file);
-		return;
-	}
-
 	if (ecbdata->diff_words) {
 		if (line[0] == '-') {
 			diff_words_append(line, len,

Stefan Beller

author

2016-09-07 23:37 UTC #8 lore raw

PATCH 2/2 diff: remove dead code

In reply to Stefan Beller

When len < 1, len has to be 0 or negative, emit_line will then remove the first character and by then len would be negative. As this doesn't happen, it is safe to assume it is dead code.

This continues to simplify the code, which was started in b8d9c1a66b (2009-09-03, diff.c: the builtin_diff() deals with only two-file comparison).

Signed-off-by Stefan Beller <[email protected]>

---
 diff.c | 8 --------
 1 file changed, 8 deletions(-)

diff.c +0 -8

index 79ad91d..c143019 100644
@@ -1251,14 +1251,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		return;
 	}
 
-	if (len < 1) {
-		emit_line(o, reset, reset, line, len);
-		if (ecbdata->diff_words
-		    && ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
-			fputs("~\n", o->file);
-		return;
-	}
-
 	if (ecbdata->diff_words) {
 		if (line[0] == '-') {
 			diff_words_append(line, len,

Signature · 1 line

2.10.0.2.g0676c79.dirty