[PATCH v3 00/14] output improvements for git range-diff

[Thomas Gummerer <t.gummerer@gmail.com>]

Thanks Dscho for the review of the previous round [1].  This rounds
addresses all the comments from that review.

In particular

- update commit messages where necessary.
- rename the function in apply.c to 'parse_git_diff_header()'
- code cleanups in 09/14
- fix a memory leak introduced in 09/14
- be less strict about parsing hunk headers, so the new code isn't
  more strict than it was before
- give more information when we are unable to parse the git diff
  header

[1]: https://public-inbox.org/git/20190705170630.27500-1-t.gummerer@gmail.com/

Thomas Gummerer (14):
  apply: replace marc.info link with public-inbox
  apply: only pass required data to skip_tree_prefix
  apply: only pass required data to git_header_name
  apply: only pass required data to check_header_line
  apply: only pass required data to find_name_*
  apply: only pass required data to gitdiff_* functions
  apply: make parse_git_header public
  range-diff: fix function parameter indentation
  range-diff: split lines manually
  range-diff: don't remove funcname from inner diff
  range-diff: suppress line count in outer diff
  range-diff: add section header instead of diff header
  range-diff: add filename to inner diff
  range-diff: add headers to the outer hunk header

 apply.c                | 186 ++++++++++++++++++-----------------------
 apply.h                |  48 +++++++++++
 diff.c                 |   5 +-
 diff.h                 |   1 +
 range-diff.c           | 124 +++++++++++++++++++--------
 t/t3206-range-diff.sh  | 124 ++++++++++++++++++++++-----
 t/t3206/history.export |  84 ++++++++++++++++++-
 7 files changed, 409 insertions(+), 163 deletions(-)

Range-diff against v2:
 1:  ef2245edda =  1:  ef2245edda apply: replace marc.info link with public-inbox
 2:  94578fa45c =  2:  94578fa45c apply: only pass required data to skip_tree_prefix
 3:  988269a68e =  3:  988269a68e apply: only pass required data to git_header_name
 4:  a2c1ef3f5f =  4:  a2c1ef3f5f apply: only pass required data to check_header_line
 5:  0f4cfe21cb =  5:  0f4cfe21cb apply: only pass required data to find_name_*
 6:  7f1d7a4569 !  6:  07a271518d apply: only pass required data to gitdiff_* functions
    @@ Commit message
         we want functions in the callchain of 'parse_git_header()' to only
         take arguments they really need.
     
    +    As these functions are called in a loop using their function pointers,
    +    each function needs to be passed all the parameters even if only one
    +    of the functions actually needs it.  We therefore pass this data along
    +    in a struct to avoid adding too many unused parameters to each
    +    function and making the code very verbose in the process.
    +
         Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
     
      ## apply.c ##
 7:  a5af8b0845 !  7:  9cb6732a5f apply: make parse_git_header public
    @@ apply.c: struct fragment {
      {
      	while (list) {
     @@ apply.c: static int check_header_line(int linenr, struct patch *patch)
    + 	return 0;
      }
      
    - /* Verify that we recognize the lines following a git header */
    +-/* Verify that we recognize the lines following a git header */
     -static int parse_git_header(struct apply_state *state,
     -			    const char *line,
     -			    int len,
     -			    unsigned int size,
     -			    struct patch *patch)
    -+int parse_git_header(struct strbuf *root,
    -+		     int *linenr,
    -+		     int p_value,
    -+		     const char *line,
    -+		     int len,
    -+		     unsigned int size,
    -+		     struct patch *patch)
    ++int parse_git_diff_header(struct strbuf *root,
    ++			  int *linenr,
    ++			  int p_value,
    ++			  const char *line,
    ++			  int len,
    ++			  unsigned int size,
    ++			  struct patch *patch)
      {
      	unsigned long offset;
      	struct parse_git_header_state parse_hdr_state;
    @@ apply.c: static int find_header(struct apply_state *state,
      		 */
      		if (!memcmp("diff --git ", line, 11)) {
     -			int git_hdr_len = parse_git_header(state, line, len, size, patch);
    -+			int git_hdr_len = parse_git_header(&state->root, &state->linenr,
    -+							   state->p_value, line, len,
    -+							   size, patch);
    ++			int git_hdr_len = parse_git_diff_header(&state->root, &state->linenr,
    ++								state->p_value, line, len,
    ++								size, patch);
      			if (git_hdr_len < 0)
      				return -128;
      			if (git_hdr_len <= len)
    @@ apply.h: int init_apply_state(struct apply_state *state,
      int check_apply_state(struct apply_state *state, int force_apply);
      
     +/*
    -+ * Parse a get header, starting at line.  Fills the relevant metadata
    -+ * information in 'struct patch'.
    ++ * Parse a git diff header, starting at line.  Fills the relevant
    ++ * metadata information in 'struct patch'.
     + *
     + * Returns -1 on failure, the length of the parsed header otherwise.
     + */
    -+int parse_git_header(struct strbuf *root,
    -+		     int *linenr,
    -+		     int p_value,
    -+		     const char *line,
    -+		     int len,
    -+		     unsigned int size,
    -+		     struct patch *patch);
    ++int parse_git_diff_header(struct strbuf *root,
    ++			  int *linenr,
    ++			  int p_value,
    ++			  const char *line,
    ++			  int len,
    ++			  unsigned int size,
    ++			  struct patch *patch);
     +
      /*
       * Some aspects of the apply behavior are controlled by the following
 8:  1f25bb1002 =  8:  76a11ce995 range-diff: fix function parameter indentation
 9:  01ed0f2a6a !  9:  6f70e7faa6 range-diff: split lines manually
    @@ Commit message
     
         Currently range-diff uses the 'strbuf_getline()' function for doing
         its line by line processing.  In a future patch we want to do parts of
    -    that parsing using the 'parse_git_header()' function, which does
    -    requires reading parts of the input from that function, which doesn't
    -    use strbufs.
    +    that parsing using the 'parse_git_diff_header()' function.  That
    +    function does its own line by line reading of the input, and doesn't
    +    use strbufs.  This doesn't match with how we do the line-by-line
    +    processing in range-diff currently.
     
         Switch range-diff to do our own line by line parsing, so we can re-use
    -    the parse_git_header function later.
    +    the 'parse_git_diff_header()' function later.
     
         Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
     
    @@ range-diff.c: struct patch_util {
      	struct object_id oid;
      };
      
    -+static unsigned long linelen(const char *buffer, unsigned long size)
    ++static size_t find_end_of_line(char *buffer, unsigned long size)
     +{
    -+	unsigned long len = 0;
    -+	while (size--) {
    -+		len++;
    -+		if (*buffer++ == '\n')
    -+			break;
    -+	}
    -+	return len;
    ++	char *eol = memchr(buffer, '\n', size);
    ++
    ++	if (!eol)
    ++		return size;
    ++
    ++	*eol = '\0';
    ++	return eol + 1 - buffer;
     +}
     +
      /*
    @@ range-diff.c: struct patch_util {
      	struct child_process cp = CHILD_PROCESS_INIT;
     -	FILE *in;
     -	struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
    -+	struct strbuf buf = STRBUF_INIT, file = STRBUF_INIT;
    ++	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
      	struct patch_util *util = NULL;
      	int in_header = 1;
     +	char *line;
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      		return error_errno(_("could not start `log`"));
     -	in = fdopen(cp.out, "r");
     -	if (!in) {
    --		error_errno(_("could not read `log` output"));
    --		finish_command(&cp);
    --		return -1;
    --	}
    -+	strbuf_read(&file, cp.out, 0);
    ++	if (strbuf_read(&contents, cp.out, 0) < 0) {
    + 		error_errno(_("could not read `log` output"));
    + 		finish_command(&cp);
    + 		return -1;
    + 	}
      
     -	while (strbuf_getline(&line, in) != EOF) {
    -+	line = strbuf_detach(&file, &size);
    ++	line = contents.buf;
    ++	size = contents.len;
     +	for (offset = 0; size > 0; offset += len, size -= len, line += len) {
      		const char *p;
      
     -		if (skip_prefix(line.buf, "commit ", &p)) {
    -+		len = linelen(line, size);
    ++		len = find_end_of_line(line, size);
     +		line[len - 1] = '\0';
     +		if (skip_prefix(line, "commit ", &p)) {
      			if (util) {
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      				strbuf_release(&buf);
     -				strbuf_release(&line);
     -				fclose(in);
    ++				strbuf_release(&contents);
      				finish_command(&cp);
      				return -1;
      			}
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      	}
     -	fclose(in);
     -	strbuf_release(&line);
    ++	strbuf_release(&contents);
      
      	if (util)
      		string_list_append(list, buf.buf)->util = util;
10:  044a79868b ! 10:  6618cdff2c range-diff: don't remove funcname from inner diff
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
     -			strbuf_addstr(&buf, "@@");
     -		else if (!line[0] || starts_with(line, "index "))
     +		} else if (skip_prefix(line, "@@ ", &p)) {
    -+			if (!(p = strstr(p, "@@")))
    -+				die(_("invalid hunk header in inner diff"));
    -+			strbuf_addstr(&buf, p);
    ++			p = strstr(p, "@@");
    ++			strbuf_addstr(&buf, p ? p : "@@");
     +		} else if (!line[0] || starts_with(line, "index "))
      			/*
      			 * A completely blank (not ' \n', which is context)
11:  69654fe76d = 11:  2667df4fa5 range-diff: suppress line count in outer diff
12:  c38f929b9a ! 12:  47cd8c6733 range-diff: add section header instead of diff header
    @@ Commit message
         noisy.  However the diff of a single line is concise and should be
         easier to understand.
     
    -    Additionaly, this allows us to add these range diff section headers to
    +    Additionally, this allows us to add these range diff section headers to
         the outer diffs hunk headers using a custom userdiff pattern, which
         should help making the range-diff more readable.
     
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      		}
      
      		if (starts_with(line, "diff --git")) {
    -+			struct patch patch;
    ++			struct patch patch = { 0 };
     +			struct strbuf root = STRBUF_INIT;
     +			int linenr = 0;
     +
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      				util->diff_offset = buf.len;
     -			strbuf_addch(&buf, ' ');
     -			strbuf_addstr(&buf, line);
    -+			memset(&patch, 0, sizeof(patch));
     +			line[len - 1] = '\n';
    -+			len = parse_git_header(&root, &linenr, 1, line,
    -+					       len, size, &patch);
    ++			len = parse_git_diff_header(&root, &linenr, 1, line,
    ++						    len, size, &patch);
     +			if (len < 0)
    -+				die(_("could not parse git header"));
    ++				die(_("could not parse git header '%.*s'"), (int)len, line);
     +			strbuf_addstr(&buf, " ## ");
     +			if (patch.is_new > 0)
     +				strbuf_addf(&buf, "%s (new)", patch.new_name);
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      			if (starts_with(line, "Author: ")) {
      				strbuf_addstr(&buf, line);
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *list)
    - 			if (!(p = strstr(p, "@@")))
    - 				die(_("invalid hunk header in inner diff"));
    - 			strbuf_addstr(&buf, p);
    + 		} else if (skip_prefix(line, "@@ ", &p)) {
    + 			p = strstr(p, "@@");
    + 			strbuf_addstr(&buf, p ? p : "@@");
     -		} else if (!line[0] || starts_with(line, "index "))
     +		} else if (!line[0])
      			/*
13:  6df03ecdcf ! 13:  f67fd5dd9a range-diff: add filename to inner diff
    @@ Commit message
     
      ## range-diff.c ##
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *list)
    - 	struct strbuf buf = STRBUF_INIT, file = STRBUF_INIT;
    + 	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
      	struct patch_util *util = NULL;
      	int in_header = 1;
     -	char *line;
    @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
      			    patch.old_mode != patch.new_mode)
      				strbuf_addf(&buf, " (mode change %06o => %06o)",
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *list)
    + 			continue;
      		} else if (skip_prefix(line, "@@ ", &p)) {
    - 			if (!(p = strstr(p, "@@")))
    - 				die(_("invalid hunk header in inner diff"));
    --			strbuf_addstr(&buf, p);
    + 			p = strstr(p, "@@");
    +-			strbuf_addstr(&buf, p ? p : "@@");
     +			strbuf_addstr(&buf, "@@");
     +			if (current_filename && p[2])
     +				strbuf_addf(&buf, " %s:", current_filename);
    -+			strbuf_addstr(&buf, p + 2);
    ++			if (p)
    ++				strbuf_addstr(&buf, p + 2);
      		} else if (!line[0])
      			/*
      			 * A completely blank (not ' \n', which is context)
14:  5ceef49035 = 14:  812893a5dc range-diff: add headers to the outer hunk header

-- 
2.22.0.510.g264f2c817a