From 8c29b497946fde2a6ef597d960a05d3dd36dcbf0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:49 +0000 Subject: range-diff: avoid leaking memory in two error code paths In the code paths in question, we already release a lot of memory, but the `current_filename` variable was missed. Fix that. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index b9950f10c8..a4d7a90dde 100644 --- a/range-diff.c +++ b/range-diff.c @@ -97,6 +97,7 @@ static int read_patches(const char *range, struct string_list *list, if (get_oid(p, &util->oid)) { error(_("could not parse commit '%s'"), p); free(util); + free(current_filename); string_list_clear(list, 1); strbuf_release(&buf); strbuf_release(&contents); @@ -112,6 +113,7 @@ static int read_patches(const char *range, struct string_list *list, error(_("could not parse first line of `log` output: " "did not start with 'commit ': '%s'"), line); + free(current_filename); string_list_clear(list, 1); strbuf_release(&buf); strbuf_release(&contents); -- cgit v1.2.3 From a2d474adf32c4ea2585b2c6109c356523124e1f9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:50 +0000 Subject: range-diff: libify the read_patches() function again In library functions, we do want to avoid the (simple, but rather final) `die()` calls, instead returning with a value indicating an error. Let's do exactly that in the code introduced in b66885a30cb8 (range-diff: add section header instead of diff header, 2019-07-11) that wants to error out if a diff header could not be parsed. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index a4d7a90dde..a83c386ffc 100644 --- a/range-diff.c +++ b/range-diff.c @@ -135,9 +135,16 @@ static int read_patches(const char *range, struct string_list *list, orig_len = len; len = parse_git_diff_header(&root, &linenr, 0, line, len, size, &patch); - if (len < 0) - die(_("could not parse git header '%.*s'"), - orig_len, line); + if (len < 0) { + error(_("could not parse git header '%.*s'"), + orig_len, line); + free(util); + free(current_filename); + string_list_clear(list, 1); + strbuf_release(&buf); + strbuf_release(&contents); + return -1; + } strbuf_addstr(&buf, " ## "); if (patch.is_new > 0) strbuf_addf(&buf, "%s (new)", patch.new_name); -- cgit v1.2.3 From 5189bb87249434fba3a82f17b2bc6c93025ba88d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:51 +0000 Subject: range-diff: simplify code spawning `git log` Previously, we waited for the child process to be finished in every failing code path as well as at the end of the function `show_range_diff()`. However, we do not need to wait that long. Directly after reading the output of the child process, we can wrap up the child process. This also has the advantage that we don't do a bunch of unnecessary work in case `finish_command()` returns with an error anyway. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index a83c386ffc..48d6e26f1a 100644 --- a/range-diff.c +++ b/range-diff.c @@ -80,6 +80,8 @@ static int read_patches(const char *range, struct string_list *list, finish_command(&cp); return -1; } + if (finish_command(&cp)) + return -1; line = contents.buf; size = contents.len; @@ -101,7 +103,6 @@ static int read_patches(const char *range, struct string_list *list, string_list_clear(list, 1); strbuf_release(&buf); strbuf_release(&contents); - finish_command(&cp); return -1; } util->matching = -1; @@ -117,7 +118,6 @@ static int read_patches(const char *range, struct string_list *list, string_list_clear(list, 1); strbuf_release(&buf); strbuf_release(&contents); - finish_command(&cp); return -1; } @@ -227,9 +227,6 @@ static int read_patches(const char *range, struct string_list *list, strbuf_release(&buf); free(current_filename); - if (finish_command(&cp)) - return -1; - return 0; } -- cgit v1.2.3 From f1ce6c191e9d15ce78041d8b6496c246b10d9b2d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:11 +0000 Subject: range-diff: combine all options in a single data structure This will make it easier to implement the `--left-only` and `--right-only` options. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index 48d6e26f1a..bc32ef6c34 100644 --- a/range-diff.c +++ b/range-diff.c @@ -525,33 +525,32 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data) } int show_range_diff(const char *range1, const char *range2, - int creation_factor, int dual_color, - const struct diff_options *diffopt, - const struct strvec *other_arg) + struct range_diff_options *range_diff_opts) { int res = 0; struct string_list branch1 = STRING_LIST_INIT_DUP; struct string_list branch2 = STRING_LIST_INIT_DUP; - if (read_patches(range1, &branch1, other_arg)) + if (read_patches(range1, &branch1, range_diff_opts->other_arg)) res = error(_("could not parse log for '%s'"), range1); - if (!res && read_patches(range2, &branch2, other_arg)) + if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg)) res = error(_("could not parse log for '%s'"), range2); if (!res) { struct diff_options opts; struct strbuf indent = STRBUF_INIT; - if (diffopt) - memcpy(&opts, diffopt, sizeof(opts)); + if (range_diff_opts->diffopt) + memcpy(&opts, range_diff_opts->diffopt, sizeof(opts)); else diff_setup(&opts); if (!opts.output_format) opts.output_format = DIFF_FORMAT_PATCH; opts.flags.suppress_diff_headers = 1; - opts.flags.dual_color_diffed_diffs = dual_color; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; opts.flags.suppress_hunk_header_line_count = 1; opts.output_prefix = output_prefix_cb; strbuf_addstr(&indent, " "); @@ -559,7 +558,8 @@ int show_range_diff(const char *range1, const char *range2, diff_setup_done(&opts); find_exact_matches(&branch1, &branch2); - get_correspondences(&branch1, &branch2, creation_factor); + get_correspondences(&branch1, &branch2, + range_diff_opts->creation_factor); output(&branch1, &branch2, &opts); strbuf_release(&indent); -- cgit v1.2.3 From 3e6046edadf409537cc9e991d1df628fa96953ba Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:12 +0000 Subject: range-diff: move the diffopt initialization down one layer It is actually only the `output()` function that uses those diffopts. By moving the diffopt initialization down into that function, it is encapsulated better. Incidentally, it will also make it easier to implement the `--left-only` and `--right-only` options in `git range-diff` because the `output()` function is now receiving all range-diff options as a parameter, not just the diffopts. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 64 +++++++++++++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 33 deletions(-) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index bc32ef6c34..514125b90c 100644 --- a/range-diff.c +++ b/range-diff.c @@ -464,12 +464,35 @@ static void patch_diff(const char *a, const char *b, diff_flush(diffopt); } +static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data) +{ + return data; +} + static void output(struct string_list *a, struct string_list *b, - struct diff_options *diffopt) + struct range_diff_options *range_diff_opts) { struct strbuf buf = STRBUF_INIT, dashes = STRBUF_INIT; int patch_no_width = decimal_width(1 + (a->nr > b->nr ? a->nr : b->nr)); int i = 0, j = 0; + struct diff_options opts; + struct strbuf indent = STRBUF_INIT; + + if (range_diff_opts->diffopt) + memcpy(&opts, range_diff_opts->diffopt, sizeof(opts)); + else + diff_setup(&opts); + + if (!opts.output_format) + opts.output_format = DIFF_FORMAT_PATCH; + opts.flags.suppress_diff_headers = 1; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; + opts.flags.suppress_hunk_header_line_count = 1; + opts.output_prefix = output_prefix_cb; + strbuf_addstr(&indent, " "); + opts.output_prefix_data = &indent; + diff_setup_done(&opts); /* * We assume the user is really more interested in the second argument @@ -490,7 +513,7 @@ static void output(struct string_list *a, struct string_list *b, /* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching < 0) { - output_pair_header(diffopt, patch_no_width, + output_pair_header(&opts, patch_no_width, &buf, &dashes, a_util, NULL); i++; continue; @@ -498,7 +521,7 @@ static void output(struct string_list *a, struct string_list *b, /* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching < 0) { - output_pair_header(diffopt, patch_no_width, + output_pair_header(&opts, patch_no_width, &buf, &dashes, NULL, b_util); b_util = ++j < b->nr ? b->items[j].util : NULL; } @@ -506,22 +529,18 @@ static void output(struct string_list *a, struct string_list *b, /* Show matching LHS/RHS pair. */ if (j < b->nr) { a_util = a->items[b_util->matching].util; - output_pair_header(diffopt, patch_no_width, + output_pair_header(&opts, patch_no_width, &buf, &dashes, a_util, b_util); - if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT)) + if (!(opts.output_format & DIFF_FORMAT_NO_OUTPUT)) patch_diff(a->items[b_util->matching].string, - b->items[j].string, diffopt); + b->items[j].string, &opts); a_util->shown = 1; j++; } } strbuf_release(&buf); strbuf_release(&dashes); -} - -static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data) -{ - return data; + strbuf_release(&indent); } int show_range_diff(const char *range1, const char *range2, @@ -538,31 +557,10 @@ int show_range_diff(const char *range1, const char *range2, res = error(_("could not parse log for '%s'"), range2); if (!res) { - struct diff_options opts; - struct strbuf indent = STRBUF_INIT; - - if (range_diff_opts->diffopt) - memcpy(&opts, range_diff_opts->diffopt, sizeof(opts)); - else - diff_setup(&opts); - - if (!opts.output_format) - opts.output_format = DIFF_FORMAT_PATCH; - opts.flags.suppress_diff_headers = 1; - opts.flags.dual_color_diffed_diffs = - range_diff_opts->dual_color; - opts.flags.suppress_hunk_header_line_count = 1; - opts.output_prefix = output_prefix_cb; - strbuf_addstr(&indent, " "); - opts.output_prefix_data = &indent; - diff_setup_done(&opts); - find_exact_matches(&branch1, &branch2); get_correspondences(&branch1, &branch2, range_diff_opts->creation_factor); - output(&branch1, &branch2, &opts); - - strbuf_release(&indent); + output(&branch1, &branch2, range_diff_opts); } string_list_clear(&branch1, 1); -- cgit v1.2.3 From 1e79f973266cfe0e3bab0e26e869b682078e457d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:13 +0000 Subject: range-diff: offer --left-only/--right-only options When comparing commit ranges, one is frequently interested only in one side, such as asking the question "Has this patch that I submitted to the Git mailing list been applied?": one would only care about the part of the output that corresponds to the commits in a local branch. To make that possible, imitate the `git rev-list` options `--left-only` and `--right-only`. This addresses https://github.com/gitgitgadget/git/issues/206 Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- range-diff.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'range-diff.c') diff --git a/range-diff.c b/range-diff.c index 514125b90c..0a0d9ed2f1 100644 --- a/range-diff.c +++ b/range-diff.c @@ -513,7 +513,8 @@ static void output(struct string_list *a, struct string_list *b, /* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching < 0) { - output_pair_header(&opts, patch_no_width, + if (!range_diff_opts->right_only) + output_pair_header(&opts, patch_no_width, &buf, &dashes, a_util, NULL); i++; continue; @@ -521,7 +522,8 @@ static void output(struct string_list *a, struct string_list *b, /* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching < 0) { - output_pair_header(&opts, patch_no_width, + if (!range_diff_opts->left_only) + output_pair_header(&opts, patch_no_width, &buf, &dashes, NULL, b_util); b_util = ++j < b->nr ? b->items[j].util : NULL; } @@ -551,7 +553,10 @@ int show_range_diff(const char *range1, const char *range2, struct string_list branch1 = STRING_LIST_INIT_DUP; struct string_list branch2 = STRING_LIST_INIT_DUP; - if (read_patches(range1, &branch1, range_diff_opts->other_arg)) + if (range_diff_opts->left_only && range_diff_opts->right_only) + res = error(_("--left-only and --right-only are mutually exclusive")); + + if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg)) res = error(_("could not parse log for '%s'"), range1); if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg)) res = error(_("could not parse log for '%s'"), range2); -- cgit v1.2.3