Merge branch 'js/range-diff-one-side-only'

The "git range-diff" command learned "--(left|right)-only" option
to show only one side of the compared range.

* js/range-diff-one-side-only:
  range-diff: offer --left-only/--right-only options
  range-diff: move the diffopt initialization down one layer
  range-diff: combine all options in a single data structure
  range-diff: simplify code spawning `git log`
  range-diff: libify the read_patches() function again
  range-diff: avoid leaking memory in two error code paths

Author: gitster

Documentation/git-range-diff.txt

@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>]
 	[--no-dual-color] [--creation-factor=<factor>]
+	[--left-only | --right-only]
 	( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
 
 DESCRIPTION
@@ -68,6 +69,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 `<rev1>...<rev2>` format).
+
+--right-only::
+	Suppress commits that are missing from the second specified range
+	(or the "right range" when using the `<rev1>...<rev2>` format).
+
 --[no-]notes[=<ref>]::
 	This flag is passed to the `git log` program
 	(see linkgit:git-log[1]) that generates the patches.

builtin/log.c

@@ -1223,14 +1223,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);
 	}
 }

builtin/range-diff.c

@@ -14,18 +14,27 @@ 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;
-	int simple_color = -1;
+	struct range_diff_options range_diff_opts = {
+		.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,
+		.diffopt = &diffopt,
+		.other_arg = &other_arg
+	};
+	int simple_color = -1, left_only = 0, right_only = 0;
 	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")),
 		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;
@@ -82,8 +91,10 @@ 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;
+	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);
 	strbuf_release(&range1);

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));
 	}

range-diff.c

@@ -81,6 +81,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;
@@ -98,10 +100,10 @@ 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);
-				finish_command(&cp);
 				return -1;
 			}
 			util->matching = -1;
@@ -113,10 +115,10 @@ 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);
-			finish_command(&cp);
 			return -1;
 		}
 
@@ -134,9 +136,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);
@@ -219,9 +228,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;
 }
 
@@ -459,12 +465,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
@@ -485,79 +514,59 @@ 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,
+			if (!range_diff_opts->right_only)
+				output_pair_header(&opts, patch_no_width,
 					   &buf, &dashes, a_util, NULL);
 			i++;
 			continue;
 		}
 
 		/* Show unmatched RHS commits. */
 		while (j < b->nr && b_util->matching < 0) {
-			output_pair_header(diffopt, 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;
 		}
 
 		/* 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,
-		    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 (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, 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));
-		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.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, creation_factor);
-		output(&branch1, &branch2, &opts);
-
-		strbuf_release(&indent);
+		get_correspondences(&branch1, &branch2,
+				    range_diff_opts->creation_factor);
+		output(&branch1, &branch2, range_diff_opts);
 	}
 
 	string_list_clear(&branch1, 1);

range-diff.h

@@ -6,15 +6,20 @@
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
+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 */
+};
+
 /*
- * 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);
 
 /*
  * Determine whether the given argument is usable as a range argument of `git

t/t3206-range-diff.sh

@@ -733,4 +733,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