On Tue, Dec 21, 2021 at 4:24 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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?
That's a fair question. Most of the involved messages are of the form
CONFLICT (<reason>): more details
and "CONFLICT" seems like a pretty identifiable string. There are
some others, which made me wonder if we wanted some kind of additional
prefix, but I was having a hard time coming up with a meaningful
prefix; most that I thought of didn't seem like they'd help.
I've provided some testcases in the next re-roll so you can see some
examples; maybe that will help others judge if a prefix is needed and
spur creative juices for coming up with a good once since I seem to be
unable to.
> > @@ -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.
I've added a comment.
