Re: [PATCH 2/4] diff.c: add --output-indicator-{new, old, context}

On Mon, Aug 13, 2018 at 4:47 AM Johannes Schindelin
[off-list ref] wrote:

Hi Steafn,

On Fri, 10 Aug 2018, Stefan Beller wrote:

quoted

This will prove useful in range-diff in a later patch as we will be able to
differentiate between adding a new file (that line is starting with +++
and then the file name) and regular new lines.

Very good!

quoted

It could also be useful for experimentation in new patch formats, i.e.
we could teach git to emit moved lines with lines other than +/-.

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 21 +++++++++++++++++----
 diff.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b3cb73eb69a..b75eb085cb3 100644
--- a/diff.c
+++ b/diff.c

@@ -1237,7 +1237,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
                                       struct emitted_diff_symbol *eds)
 {
      static const char *nneof = " No newline at end of file\n";
-     const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
+     const char *context, *reset, *set, *set_sign, *meta, *fraginfo, *first;
      struct strbuf sb = STRBUF_INIT;

      enum diff_symbol s = eds->s;

@@ -1288,7 +1288,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
                      else if (c == '-')
                              set = diff_get_color_opt(o, DIFF_FILE_OLD);
              }
-             emit_line_ws_markup(o, set_sign, set, reset, " ", line, len,
+             first = o->output_indicators[OI_CONTEXT] ?
+                     o->output_indicators[OI_CONTEXT] : " ";
+             emit_line_ws_markup(o, set_sign, set, reset, first, line, len,

Instead of doing this over and over again, how about

1) setting o->output_indicators to " " in diff_setup()?

... and when parsing the command line options we could already overwrite it
  in place.

2) passing OI_CONTEXT to emit_line_ws_markup() instead of `first`? I.e.
   change it to the index in the output_indicators, with -1 indicating
   "none"?

That sounds like an elegant design, as then it is super clear that 'first'
can only ever be a sign (or character that we chose), but
giving -1 for "none" sounds cumbersome. I'll take a look into that.

quoted

+#define OI_NEW 0
+#define OI_OLD 1
+#define OI_CONTEXT 2

I could imagine that OI_* is too generic a prefix, and that we would want
to have a prefix that is less prone to collide with other global
constants, such as OUTPUT_INDICATOR_*.

I agree on that; will take the suggestion of
OUTPUT_INDICATOR_*.