Re: [PATCH 2/4] diff.c: add --output-indicator-{new, old, context}
- From
Stefan Beller <sbeller@google.com>
- Date
- Aug 13, 2018, 18:23 UTC
- Message-ID
- <CAGZ79kbUKoVPXq_fz7YZMOcAWEe0uiZ04Fj9KD-J4eMmwjUXTw@mail.gmail.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1808131343310.71@tvgsbejvaqbjf.bet>
On Mon, Aug 13, 2018 at 4:47 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 45 quoted lines
>
> Hi Steafn,
>
> On Fri, 10 Aug 2018, Stefan Beller wrote:
>
> > 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!
>
> > 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 <sbeller@google.com>
> > ---
> > 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.
Show 7 quoted lines
> > +#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_*.