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

[Junio C Hamano <gitster@pobox.com>]

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