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

"Elijah Newren via GitGitGadget" [off-list ref] writes:

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);
+}

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?

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.

On Tue, Dec 21, 2021 at 4:24 PM Junio C Hamano [off-list ref] wrote:

"Elijah Newren via GitGitGadget" [off-list ref] writes:

quoted

From: Elijah Newren <redacted>

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 <redacted>
---
 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.

quoted

+                                      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().

quoted

+     if (*next)
+             strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
+}

quoted

@@ -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.

quoted

@@ -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.