[PATCH v3 00/14] output improvements for git range-diff
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Jul 8, 2019, 16:33 UTC
- Message-ID
- <20190708163315.29912-1-t.gummerer@gmail.com>
- In-Reply-To
- <20190705170630.27500-1-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