From 8c29b497946fde2a6ef597d960a05d3dd36dcbf0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:49 +0000 Subject: [PATCH 1/6] 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(+) diff --git a/range-diff.c b/range-diff.c index b9950f10c8c..a4d7a90ddea 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); From a2d474adf32c4ea2585b2c6109c356523124e1f9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:50 +0000 Subject: [PATCH 2/6] 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(-) diff --git a/range-diff.c b/range-diff.c index a4d7a90ddea..a83c386ffc5 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); From 5189bb87249434fba3a82f17b2bc6c93025ba88d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Feb 2021 20:07:51 +0000 Subject: [PATCH 3/6] 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(-) diff --git a/range-diff.c b/range-diff.c index a83c386ffc5..48d6e26f1a4 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; } From f1ce6c191e9d15ce78041d8b6496c246b10d9b2d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:11 +0000 Subject: [PATCH 4/6] 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 --- builtin/log.c | 10 ++++++++-- builtin/range-diff.c | 13 +++++++++---- log-tree.c | 8 ++++++-- range-diff.c | 18 +++++++++--------- range-diff.h | 16 ++++++++++------ 5 files changed, 42 insertions(+), 23 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index f23ccdbec32..300b1648d7f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1231,14 +1231,20 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, */ struct diff_options opts; struct strvec other_arg = STRVEC_INIT; + struct range_diff_options range_diff_opts = { + .creation_factor = rev->creation_factor, + .dual_color = 1, + .diffopt = &opts, + .other_arg = &other_arg + }; + diff_setup(&opts); opts.file = rev->diffopt.file; opts.use_color = rev->diffopt.use_color; diff_setup_done(&opts); fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); get_notes_args(&other_arg, rev); - show_range_diff(rev->rdiff1, rev->rdiff2, - rev->creation_factor, 1, &opts, &other_arg); + show_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts); strvec_clear(&other_arg); } } diff --git a/builtin/range-diff.c b/builtin/range-diff.c index 24c4162f744..73fea796016 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -13,12 +13,17 @@ NULL int cmd_range_diff(int argc, const char **argv, const char *prefix) { - int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT; struct diff_options diffopt = { NULL }; struct strvec other_arg = STRVEC_INIT; + struct range_diff_options range_diff_opts = { + .creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT, + .diffopt = &diffopt, + .other_arg = &other_arg + }; int simple_color = -1; struct option range_diff_options[] = { - OPT_INTEGER(0, "creation-factor", &creation_factor, + OPT_INTEGER(0, "creation-factor", + &range_diff_opts.creation_factor, N_("Percentage by which creation is weighted")), OPT_BOOL(0, "no-dual-color", &simple_color, N_("use simple diff colors")), @@ -81,8 +86,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) } FREE_AND_NULL(options); - res = show_range_diff(range1.buf, range2.buf, creation_factor, - simple_color < 1, &diffopt, &other_arg); + range_diff_opts.dual_color = simple_color < 1; + res = show_range_diff(range1.buf, range2.buf, &range_diff_opts); strvec_clear(&other_arg); strbuf_release(&range1); diff --git a/log-tree.c b/log-tree.c index fd0dde97ec3..eeacba15dc9 100644 --- a/log-tree.c +++ b/log-tree.c @@ -808,6 +808,11 @@ void show_log(struct rev_info *opt) if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) { struct diff_queue_struct dq; struct diff_options opts; + struct range_diff_options range_diff_opts = { + .creation_factor = opt->creation_factor, + .dual_color = 1, + .diffopt = &opts + }; memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(&diff_queued_diff); @@ -822,8 +827,7 @@ void show_log(struct rev_info *opt) opts.file = opt->diffopt.file; opts.use_color = opt->diffopt.use_color; diff_setup_done(&opts); - show_range_diff(opt->rdiff1, opt->rdiff2, - opt->creation_factor, 1, &opts, NULL); + show_range_diff(opt->rdiff1, opt->rdiff2, &range_diff_opts); memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff)); } diff --git a/range-diff.c b/range-diff.c index 48d6e26f1a4..bc32ef6c342 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); diff --git a/range-diff.h b/range-diff.h index 583ced2e8e7..0bae6b0cb15 100644 --- a/range-diff.h +++ b/range-diff.h @@ -6,14 +6,18 @@ #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +struct range_diff_options { + int creation_factor; + unsigned dual_color:1; + const struct diff_options *diffopt; /* may be NULL */ + const struct strvec *other_arg; /* may be NULL */ +}; + /* - * Compare series of commits in RANGE1 and RANGE2, and emit to the - * standard output. NULL can be passed to DIFFOPT to use the built-in - * default. + * Compare series of commits in `range1` and `range2`, and emit to the + * standard output. */ 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 *opts); #endif From 3e6046edadf409537cc9e991d1df628fa96953ba Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:12 +0000 Subject: [PATCH 5/6] 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(-) diff --git a/range-diff.c b/range-diff.c index bc32ef6c342..514125b90cc 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); From 1e79f973266cfe0e3bab0e26e869b682078e457d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 5 Feb 2021 14:46:13 +0000 Subject: [PATCH 6/6] 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 --- Documentation/git-range-diff.txt | 9 +++++++++ builtin/range-diff.c | 8 +++++++- range-diff.c | 11 ++++++++--- range-diff.h | 1 + t/t3206-range-diff.sh | 15 +++++++++++++++ 5 files changed, 40 insertions(+), 4 deletions(-) diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt index 9701c1e5fdd..dd46bb7f506 100644 --- a/Documentation/git-range-diff.txt +++ b/Documentation/git-range-diff.txt @@ -10,6 +10,7 @@ SYNOPSIS [verse] 'git range-diff' [--color=[]] [--no-color] [] [--no-dual-color] [--creation-factor=] + [--left-only | --right-only] ( | ... | ) DESCRIPTION @@ -57,6 +58,14 @@ to revert to color all lines according to the outer diff markers See the ``Algorithm`` section below for an explanation why this is needed. +--left-only:: + Suppress commits that are missing from the first specified range + (or the "left range" when using the `...` format). + +--right-only:: + Suppress commits that are missing from the second specified range + (or the "right range" when using the `...` format). + --[no-]notes[=]:: This flag is passed to the `git log` program (see linkgit:git-log[1]) that generates the patches. diff --git a/builtin/range-diff.c b/builtin/range-diff.c index 73fea796016..930f273b85a 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -20,7 +20,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) .diffopt = &diffopt, .other_arg = &other_arg }; - int simple_color = -1; + int simple_color = -1, left_only = 0, right_only = 0; struct option range_diff_options[] = { OPT_INTEGER(0, "creation-factor", &range_diff_opts.creation_factor, @@ -30,6 +30,10 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) OPT_PASSTHRU_ARGV(0, "notes", &other_arg, N_("notes"), N_("passed to 'git log'"), PARSE_OPT_OPTARG), + OPT_BOOL(0, "left-only", &left_only, + N_("only emit output related to the first range")), + OPT_BOOL(0, "right-only", &right_only, + N_("only emit output related to the second range")), OPT_END() }; struct option *options; @@ -87,6 +91,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) FREE_AND_NULL(options); range_diff_opts.dual_color = simple_color < 1; + range_diff_opts.left_only = left_only; + range_diff_opts.right_only = right_only; res = show_range_diff(range1.buf, range2.buf, &range_diff_opts); strvec_clear(&other_arg); diff --git a/range-diff.c b/range-diff.c index 514125b90cc..0a0d9ed2f12 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); diff --git a/range-diff.h b/range-diff.h index 0bae6b0cb15..27c9adfd2be 100644 --- a/range-diff.h +++ b/range-diff.h @@ -9,6 +9,7 @@ struct range_diff_options { int creation_factor; unsigned dual_color:1; + unsigned left_only:1, right_only:1; const struct diff_options *diffopt; /* may be NULL */ const struct strvec *other_arg; /* may be NULL */ }; diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 6eb344be031..323439d2111 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -717,4 +717,19 @@ test_expect_success 'format-patch --range-diff with multiple notes' ' test_cmp expect actual ' +test_expect_success '--left-only/--right-only' ' + git switch --orphan left-right && + test_commit first && + test_commit unmatched && + test_commit common && + git switch -C left-right first && + git cherry-pick common && + + git range-diff -s --left-only ...common >actual && + head_oid=$(git rev-parse --short HEAD) && + common_oid=$(git rev-parse --short common) && + echo "1: $head_oid = 2: $common_oid common" >expect && + test_cmp expect actual +' + test_done