Re: [PATCH 15/26] submodule.c: migrate diff output to use emit_diff_symbol

[Jonathan Tan]

On Mon, 19 Jun 2017 19:48:05 -0700
Stefan Beller  wrote:

> As the submodule process is no longer attached to the same stdout as
> the superprojects process, we need to pass coloring explicitly.

I found this confusing - what difference does the stdout make? If they
were the same stdout, one process could set a color for the other
process to follow, but it wouldn't be able to affect color changes
inside the output, right?

> Remove the colors from the function signatures, as all the coloring
> decisions will be made either inside the child process or the final
> emit_diff_symbol.

This seems to be of the same pattern as the others. I would write this
as:

Create diff_emit_submodule_.* functions that make its own coloring
decisions and migrate submodule.c to use them. This centralizes
diff output, including coloring information, in one file.

or something like that.

[PATCH 15/26] submodule.c: migrate diff output to use emit_diff_symbol

[Stefan Beller]

As the submodule process is no longer attached to the same stdout as
the superprojects process, we need to pass coloring explicitly.

Remove the colors from the function signatures, as all the coloring
decisions will be made either inside the child process or the final
emit_diff_symbol.

Signed-off-by: Stefan Beller 
---
 diff.c  | 89 -
 diff.h  |  9 +++
 submodule.c | 85 ++
 submodule.h | 13 +++--
 4 files changed, 128 insertions(+), 68 deletions(-)

diff --git a/diff.c b/diff.c
index 96ce53c5cf..42a9020d95 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,13 @@ enum diff_symbol {
DIFF_SYMBOL_HEADER,
DIFF_SYMBOL_BINARY_FILES,
DIFF_SYMBOL_REWRITE_DIFF,
+   DIFF_SYMBOL_SUBMODULE_ADD,
+   DIFF_SYMBOL_SUBMODULE_DEL,
+   DIFF_SYMBOL_SUBMODULE_UNTRACKED,
+   DIFF_SYMBOL_SUBMODULE_MODIFIED,
+   DIFF_SYMBOL_SUBMODULE_HEADER,
+   DIFF_SYMBOL_SUBMODULE_ERROR,
+   DIFF_SYMBOL_SUBMODULE_PIPETHROUGH,
 };
 /*
  * Flags for content lines:
@@ -700,11 +707,77 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
reset = diff_get_color_opt(o, DIFF_RESET);
emit_line(o, fraginfo, reset, line, len);
break;
+   case DIFF_SYMBOL_SUBMODULE_ADD:
+   set = diff_get_color_opt(o, DIFF_FILE_NEW);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, set, reset, line, len);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_DEL:
+   set = diff_get_color_opt(o, DIFF_FILE_OLD);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, set, reset, line, len);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_UNTRACKED:
+   fprintf(o->file, "%sSubmodule %s contains untracked content\n",
+   diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_MODIFIED:
+   fprintf(o->file, "%sSubmodule %s contains modified content\n",
+   diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_HEADER:
+   fprintf(o->file, "%s%s", diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_ERROR:
+   emit_line(o, "", "", line, len);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_PIPETHROUGH:
+   emit_line(o, "", "", line, len);
+   break;
default:
die("BUG: unknown diff symbol");
}
 }
 
+void diff_emit_submodule_del(struct diff_options *o, const char *line)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_DEL, line, strlen(line), 0);
+}
+
+void diff_emit_submodule_add(struct diff_options *o, const char *line)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_ADD, line, strlen(line), 0);
+}
+
+void diff_emit_submodule_untracked(struct diff_options *o, const char *path)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_UNTRACKED,
+path, strlen(path), 0);
+}
+
+void diff_emit_submodule_modified(struct diff_options *o, const char *path)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_MODIFIED,
+path, strlen(path), 0);
+}
+
+void diff_emit_submodule_header(struct diff_options *o, const char *header)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_HEADER,
+header, strlen(header), 0);
+}
+
+void diff_emit_submodule_error(struct diff_options *o, const char *err)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_ERROR, err, strlen(err), 0);
+}
+
+void diff_emit_submodule_pipethrough(struct diff_options *o,
+const char *line, int len)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_PIPETHROUGH, line, len, 0);
+}
+
 static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char 
*line, int len)
 {
if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
@@ -2465,24 +2538,16 @@ static void builtin_diff(const char *name_a,
if (o->submodule_format == DIFF_SUBMODULE_LOG &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
-   const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
-   const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
-   show_submodule_summary(o->file, one->path ? one->path : 
two->path,
-   line_prefix,
+   show_submodule_summary(o, one->path ? one->path : two->path,
>oid, >oid,
-   two->dirty_submodule,
-   meta, del, add, reset);
+   two->dirty_submodule);
return;
} else if (o->submodule_format ==