Re: [PATCH v5 05/21] range-diff: also show the diff between patches

Hi Thomas,

On Sun, 12 Aug 2018, Thomas Gummerer wrote:

On 08/10, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

[..]

@@ -13,15 +14,38 @@ NULL
 int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = 60;
+	struct diff_options diffopt = { NULL };
 	struct option options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_END()
 	};
-	int res = 0;
+	int i, j, res = 0;
 	struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
 
+	git_config(git_diff_ui_config, NULL);
+
+	diff_setup(&diffopt);
+	diffopt.output_format = DIFF_FORMAT_PATCH;
+
 	argc = parse_options(argc, argv, NULL, options,
+			     builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
+
+	for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
+		int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+
+		if (!c)
+			argv[j++] = argv[i++];
+		else
+			i += c;
+	}

I don't think this handles "--" quite as would be expected.  Trying to
use "git range-diff -- js/range-diff-v4...HEAD" I get:

    $ ./git range-diff -- js/range-diff-v4...HEAD
    error: need two commit ranges
    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
       or: git range-diff [<options>] <old-tip>...<new-tip>
       or: git range-diff [<options>] <base> <old-tip> <new-tip>

        --creation-factor <n>
                              Percentage by which creation is weighted
        --no-dual-color       color both diff and diff-between-diffs

while what I would have expected is to actually get a range diff.
This happens because after we break out of the loop we don't add the
actual ranges to argv, but just skip them instead.

Ouch, good point.

I do not think that is correct. The original idea was for the first
`parse_options()` call to keep the dashdash, for the second one to keep
the dashdash, too, and for the final one to swallow it.

Also, if `i < argc` at this point, we already know that `argv[i]` refers
to the dashdash, otherwise the previous loop would not have exited early.

I went with this simple version instead:

	while (i < argc)
		argv[j++] = argv[i++];

Thanks!
Dscho

--->8---

quoted

+	argc = j;
+	diff_setup_done(&diffopt);
+
+	/* Make sure that there are no unparsed options */
+	argc = parse_options(argc, argv, NULL,
+			     options + ARRAY_SIZE(options) - 1, /* OPT_END */
 			     builtin_range_diff_usage, 0);
 
 	if (argc == 2) {

@@ -59,7 +83,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_range_diff_usage, options);
 	}
 
-	res = show_range_diff(range1.buf, range2.buf, creation_factor);
+	res = show_range_diff(range1.buf, range2.buf, creation_factor,
+			      &diffopt);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);

diff --git a/range-diff.c b/range-diff.c
index 2d94200d3..71883a4b7 100644
--- a/range-diff.c
+++ b/range-diff.c

@@ -6,6 +6,7 @@
 #include "hashmap.h"
 #include "xdiff-interface.h"
 #include "linear-assignment.h"
+#include "diffcore.h"
 
 struct patch_util {
 	/* For the search for an exact match */

@@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
 	return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
 }
 
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+	struct diff_filespec *spec = alloc_filespec(name);
+
+	fill_filespec(spec, &null_oid, 0, 0644);
+	spec->data = (char *)p;
+	spec->size = strlen(p);
+	spec->should_munmap = 0;
+	spec->is_stdin = 1;
+
+	return spec;
+}
+
+static void patch_diff(const char *a, const char *b,
+			      struct diff_options *diffopt)
+{
+	diff_queue(&diff_queued_diff,
+		   get_filespec("a", a), get_filespec("b", b));
+
+	diffcore_std(diffopt);
+	diff_flush(diffopt);
+}
+
+static void output(struct string_list *a, struct string_list *b,
+		   struct diff_options *diffopt)
 {
 	int i = 0, j = 0;
 

@@ -300,6 +325,9 @@ static void output(struct string_list *a, struct string_list *b)
 			printf("%d: %s ! %d: %s\n",
 			       b_util->matching + 1, short_oid(a_util),
 			       j + 1, short_oid(b_util));
+			if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
+				patch_diff(a->items[b_util->matching].string,
+					   b->items[j].string, diffopt);
 			a_util->shown = 1;
 			j++;
 		}

@@ -307,7 +335,7 @@ static void output(struct string_list *a, struct string_list *b)
 }
 
 int show_range_diff(const char *range1, const char *range2,
-		    int creation_factor)
+		    int creation_factor, struct diff_options *diffopt)
 {
 	int res = 0;
 

@@ -322,7 +350,7 @@ int show_range_diff(const char *range1, const char *range2,
 	if (!res) {
 		find_exact_matches(&branch1, &branch2);
 		get_correspondences(&branch1, &branch2, creation_factor);
-		output(&branch1, &branch2);
+		output(&branch1, &branch2, diffopt);
 	}
 
 	string_list_clear(&branch1, 1);

diff --git a/range-diff.h b/range-diff.h
index 7b6eef303..2407d46a3 100644
--- a/range-diff.h
+++ b/range-diff.h

@@ -1,7 +1,9 @@
 #ifndef RANGE_DIFF_H
 #define RANGE_DIFF_H
 
+#include "diff.h"
+
 int show_range_diff(const char *range1, const char *range2,
-		    int creation_factor);
+		    int creation_factor, struct diff_options *diffopt);
 
 #endif

-- 
gitgitgadget