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