Re: [PATCH 6/9] diff: add ability to insert additional headers for paths

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > In support of a remerge-diff ability we will add in a few commits, we
> > want to be able to provide additional headers to show along with a diff.
> > Add the plumbing necessary to enable this.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  diff.c | 34 +++++++++++++++++++++++++++++++++-
> >  diff.h |  1 +
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/diff.c b/diff.c
> > index 861282db1c3..a9490b9b2ba 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -27,6 +27,7 @@
> >  #include "help.h"
> >  #include "promisor-remote.h"
> >  #include "dir.h"
> > +#include "strmap.h"
> >
> >  #ifdef NO_FAST_WORKING_DIRECTORY
> >  #define FAST_WORKING_DIRECTORY 0
> > @@ -3406,6 +3407,33 @@ struct userdiff_driver *get_textconv(struct repository *r,
> >       return userdiff_get_textconv(r, one->driver);
> >  }
> >
> > +static struct strbuf* additional_headers(struct diff_options *o,
>
> Style.
>
> > +                                      const char *path)
> > +{
> > +     if (!o->additional_path_headers)
> > +             return NULL;
> > +     return strmap_get(o->additional_path_headers, path);
> > +}
> > +
> > +static void add_formatted_headers(struct strbuf *msg,
> > +                               struct strbuf *more_headers,
> > +                               const char *line_prefix,
> > +                               const char *meta,
> > +                               const char *reset)
> > +{
> > +     char *next, *newline;
> > +
> > +     next = more_headers->buf;
> > +     while ((newline = strchr(next, '\n'))) {
> > +             *newline = '\0';
> > +             strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
> > +             *newline = '\n';
> > +             next = newline + 1;
> > +     }
>
> The above is not wrong per-se, but we do not need to do the
> "temporarily terminate and then recover" dance, and avoiding it
> would make the code cleaner.
>
> Once you learn the value of "newline" [*], you know the number of
> bytes between "next" and "newline" so you can use safely "%.*s"
> format specifier without temporarily terminating the subsection of
> the string.
>
>         Side note. I would actually use strchrnul() instead, so that
>         we do not have to special case the end of the buffer.  For a
>         readily available example, see advice.c::vadvise().
>
> > +     if (*next)
> > +             strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
> > +}
>
> > @@ -4328,9 +4356,13 @@ static void fill_metainfo(struct strbuf *msg,
> >       const char *set = diff_get_color(use_color, DIFF_METAINFO);
> >       const char *reset = diff_get_color(use_color, DIFF_RESET);
> >       const char *line_prefix = diff_line_prefix(o);
> > +     struct strbuf *more_headers = NULL;
> >
> >       *must_show_header = 1;
> >       strbuf_init(msg, PATH_MAX * 2 + 300);
> > +     if ((more_headers = additional_headers(o, name)))
> > +             add_formatted_headers(msg, more_headers,
> > +                                   line_prefix, set, reset);
>
> So, we stuff what came via path_msg() without anything that allows
> readers to identify them to the header part?  Just like we have
> fixed and known string taken from a bounded vocabulary such as
> "index", "copy from", "old mode", etc., don't we want to prefix the
> hints that came from the merge machinery with some identifiable
> string?

> > @@ -5852,7 +5884,7 @@ int diff_unmodified_pair(struct diff_filepair *p)
> >
> >  static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
> >  {
> > -     if (diff_unmodified_pair(p))
> > +     if (diff_unmodified_pair(p) && !additional_headers(o, p->one->path))
> >               return;
>
> This does not feel quite right.  At least there needs a comment that
> says the _current_ callers that add additional_headers() would do so
> only for paths that the end-users cares about, even when there is no
> change in the contents.  It is quite plausible that future callers
> may want to add additional information to only paths that have some
> changes that need to be shown, no?  And at that point, they want to
> tweak this condition we place here, but without explanation they
> wouldn't know what they would be breaking if they did so.