git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] range-diff: support reading mbox files

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 21, 2022, 10:08 UTC
Message-ID
<8d40f170-650f-800a-02d7-d279186d5883@dunelm.org.uk>
In-Reply-To
<pull.1420.v2.git.1668899471058.gitgitgadget@gmail.com>
Hi Dscho
On 19/11/2022 23:11, Johannes Schindelin via GitGitGadget wrote:
Show 11 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>      
>      Changes since v1:
>      
>       * We no longer leak allocated memory in the struct patch instance
>       * Made strtost() a bit more stringent
>       * Postel [https://en.wikipedia.org/wiki/Postel%27s_Law]ized the mbox
>         parser substantially, together with a couple more cosmetic fixes,
>         based on Phillip Wood's excellent review of v1.
>       * Extended the test case to cover mboxes containing CR/LF and in-body
>         From: lines

This addresses all of my comments on V1. I've left a couple of queries on the range-diff for a couple of changes I didn't quite understand.

Show 73 quoted lines
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1420%2Fdscho%2Frange-diff-from-mbox-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1420/dscho/range-diff-from-mbox-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1420
> 
> Range-diff vs v1:
> 
>   1:  354840fc57c ! 1:  485249ddfb3 range-diff: support reading mbox files
>       @@ range-diff.c: struct patch_util {
>        +
>        +	errno = 0;
>        +	/* negative values would be accepted by strtoul */
>       -+	if (*s == '-')
>       ++	if (!isdigit(*s))
>        +		return -1;
>        +	u = strtoul(s, &p, 10);
>        +	if (errno || p == s)
>       @@ range-diff.c: struct patch_util {
>        +
>        +	if (!skip_prefix(p, "@@ -", &p) ||
>        +	    strtost(p, NULL, &p) ||
>       -+	    (*p != ' ' && (*p != ',' || strtost(p + 1, &o, &p))) ||
>       ++	    /* The range is -<start>[,<count>], defaulting to count = 1 */
>       ++	    !(*p == ' ' || (*p == ',' && !strtost(p + 1, &o, &p))) ||
>        +	    !skip_prefix(p, " +", &p) ||
>        +	    strtost(p, NULL, &p) ||
>       -+	    (*p != ' ' && (*p != ',' || strtost(p + 1, &n, &p))) ||
>       ++	    /* The range is +<start>[,<count>], defaulting to count = 1 */
>       ++	    !(*p == ' ' || (*p == ',' && !strtost(p + 1, &n, &p))) ||
>        +	    !skip_prefix(p, " @@", &p))
>        +		return -1;
>        +
>       @@ range-diff.c: struct patch_util {
>        +	return 0;
>        +}
>        +
>       -+static inline int find_eol(const char *line, size_t size)
>       ++/*
>       ++ * This function finds the end of the line, replaces the newline character with
>       ++ * a NUL, and returns the offset of the start of the next line.
>       ++ *
>       ++ * If no newline character was found, it returns the offset of the trailing NUL
>       ++ * instead.
>       ++ */
>       ++static inline int find_next_line(const char *line, size_t size)
>        +{
>        +	char *eol;
>        +
>       @@ range-diff.c: struct patch_util {
>        +	if (!eol)
>        +		return size;
>        +
>       -+	if (eol != line && eol[-1] == '\r')
>       -+		eol[-1] = '\0';
>       -+	else
>       -+		*eol = '\0';
>       ++	*eol = '\0';
>        +
>        +	return eol + 1 - line;
>        +}
>       @@ range-diff.c: struct patch_util {
>        +
>        +	line = contents.buf;
>        +	size = contents.len;
>       -+	for (; size > 0; size -= len, line += len) {
>       ++	for (; size; size -= len, line += len) {
>        +		const char *p;
>        +
>       -+		len = find_eol(line, size);
>       ++		len = find_next_line(line, size);
>        +
>       -+		if (state == MBOX_BEFORE_HEADER) {
>       ++		if (state == MBOX_BEFORE_HEADER ||
>       ++		    (state == MBOX_IN_DIFF && line[0] == 'F')) {

If we see an 'F' when parsing a diff then it is probably a From: line indicating that we've reached the start of a new patch. I think we need to add the diff we've just parsed to the list of commits before continuing and change state to MBOX_BEFORE_HEADER otherwise we'll hit the continue just below this comment with state unchanged.

Show 31 quoted lines
>        +			if (!skip_prefix(line, "From ", &p))
>        +				continue;
>        +
>       @@ range-diff.c: struct patch_util {
>        +			author = subject = NULL;
>        +
>        +			state = MBOX_IN_HEADER;
>       ++			continue;
>        +		}
>        +
>        +		if (starts_with(line, "diff --git ")) {
>       @@ range-diff.c: struct patch_util {
>        +			strbuf_addch(&buf, '\n');
>        +			if (!util->diff_offset)
>        +				util->diff_offset = buf.len;
>       -+			line[len - 1] = '\n';
>       ++
>        +			orig_len = len;
>       -+			len = parse_git_diff_header(&root, &linenr, 1, line,
>       -+						    len, size, &patch);
>       ++			/* `find_next_line()`'s replaced the LF with a NUL */
>       ++			line[len - 1] = '\n';
>       ++			len = len > 1 && line[len - 2] == '\r' ?
>       ++				error(_("cannot handle diff headers with "
>       ++					"CR/LF line endings")) :
>       ++				parse_git_diff_header(&root, &linenr, 1, line,
>       ++						      len, size, &patch);
>        +			if (len < 0) {
>        +				error(_("could not parse git header '%.*s'"),
>        +				      orig_len, line);
>       ++				release_patch(&patch);

Thank you for using release_patch() rather than the "free random struct members" approach that was suggested.

Show 80 quoted lines
>        +				free(util);
>        +				free(current_filename);
>        +				string_list_clear(list, 1);
>       @@ range-diff.c: struct patch_util {
>        +					    (const char **)&patch.new_name);
>        +
>        +			strbuf_addstr(&buf, " ## ");
>       -+			if (patch.is_new > 0)
>       ++			if (patch.is_new)
>        +				strbuf_addf(&buf, "%s (new)", patch.new_name);
>       -+			else if (patch.is_delete > 0)
>       ++			else if (patch.is_delete)
>        +				strbuf_addf(&buf, "%s (deleted)", patch.old_name);
>        +			else if (patch.is_rename)
>        +				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
>       @@ range-diff.c: struct patch_util {
>        +				strbuf_addstr(&buf, patch.new_name);
>        +
>        +			free(current_filename);
>       -+			if (patch.is_delete > 0)
>       ++			if (patch.is_delete)
>        +				current_filename = xstrdup(patch.old_name);
>        +			else
>        +				current_filename = xstrdup(patch.new_name);
>       @@ range-diff.c: struct patch_util {
>        +
>        +			strbuf_addstr(&buf, " ##\n");
>        +			util->diffsize++;
>       ++			release_patch(&patch);
>        +		} else if (state == MBOX_IN_HEADER) {
>        +			if (!line[0]) {
>        +				state = MBOX_IN_COMMIT_MESSAGE;
>        +				/* Look for an in-body From: */
>       -+				if (size > 5 && skip_prefix(line + 1, "From: ", &p)) {
>       ++				if (skip_prefix(line + 1, "From: ", &p)) {
>        +					size -= p - line;
>        +					line += p - line;
>       -+					len = find_eol(line, size);
>       ++					len = find_next_line(line, size);
>        +
>        +					while (isspace(*p))
>        +						p++;
>       @@ range-diff.c: struct patch_util {
>        +					while (len < size && line[len] == ' ') {
>        +						line += len;
>        +						size -= len;
>       -+						len = find_eol(line, size);
>       ++						len = find_next_line(line, size);
>        +						strbuf_addstr(&long_subject, line);
>        +					}
>        +					subject = long_subject.buf;
>        +				}
>        +			}
>        +		} else if (state == MBOX_IN_COMMIT_MESSAGE) {
>       -+			if (!*line)
>       ++			if (!line[0]) {
>        +				strbuf_addch(&buf, '\n');
>       -+			else if (strcmp(line, "---")) {
>       ++			} else if (strcmp(line, "---")) {
>        +				int tabs = 0;
>        +
>        +				/* simulate tab expansion */
>       @@ range-diff.c: struct patch_util {
>        +			case '+':
>        +			case '-':
>        +			case ' ':
>       ++				/* A `-- ` line indicates the end of a diff */
>        +				if (!old_count && !new_count)
>        +					break;
>        +				if (old_count && line[0] != '+')
>       @@ t/t3206-range-diff.sh: test_expect_success 'ranges with pathspecs' '
>        +test_expect_success 'compare range vs mbox' '
>        +	git format-patch --stdout topic..mode-only-change >mbox &&
>        +	git range-diff topic...mode-only-change >expect &&
>       ++
>        +	git range-diff mode-only-change..topic mbox:./mbox >actual &&
>        +	test_cmp expect actual &&
>       -+	git range-diff mode-only-change..topic mbox:- <mbox >actual &&
>       -+	test_cmp expect actual
>       ++

I'm a bit confused by this sed command, I've annotated it with my probably flawed understanding.

>       ++	sed -e "/^From: .*/{
>       ++		h
This stores the From: header in the hold space
>       ++		s/.*/From: Bugs Bunny <bugs@bun.ny>/
This changes the From: header in the pattern space
>       ++		:1
>       ++		N
>       ++		/[ -z]$/b1

We loop until we find a line that does not end with a space, letter or number adding the lines to the hold space

>       ++		G

This appends the hold space to the pattern space, then the pattern space is printed. Doesn't this mean we end up with two From: headers? Is the in-body From: line already present?

Show 8 quoted lines
>       ++	}" <mbox >mbox.from &&
>       ++	git range-diff mode-only-change..topic mbox:./mbox.from >actual.from &&
>       ++	test_cmp expect actual.from &&
>       ++
>       ++	append_cr <mbox >mbox.cr &&
>       ++	test_must_fail git range-diff \
>       ++		mode-only-change..topic mbox:./mbox.cr 2>err &&
>       ++	grep CR/LF err &&
Thanks for adding that
Best Wishes
Phillip
Show 448 quoted lines
>       ++	git range-diff mode-only-change..topic mbox:- <mbox >actual.stdin &&
>       ++	test_cmp expect actual.stdin
>        +'
>        +
>         test_expect_success 'submodule changes are shown irrespective of diff.submodule' '
> 
> 
>   Documentation/git-range-diff.txt |   3 +-
>   range-diff.c                     | 333 ++++++++++++++++++++++++++++++-
>   t/t3206-range-diff.sh            |  27 +++
>   3 files changed, 361 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> index 0b393715d70..e2c4661acde 100644
> --- a/Documentation/git-range-diff.txt
> +++ b/Documentation/git-range-diff.txt
> @@ -37,7 +37,8 @@ There are three ways to specify the commit ranges:
>   
>   - `<range1> <range2>`: Either commit range can be of the form
>     `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
> -  in linkgit:gitrevisions[7] for more details.
> +  in linkgit:gitrevisions[7] for more details. Alternatively, the
> +  patches can be provided as an mbox-formatted file via `mbox:<path>`.
>   
>   - `<rev1>...<rev2>`. This is equivalent to
>     `<rev2>..<rev1> <rev1>..<rev2>`.
> diff --git a/range-diff.c b/range-diff.c
> index 124dd678c38..9dc1e3af3f8 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -12,6 +12,7 @@
>   #include "userdiff.h"
>   #include "apply.h"
>   #include "revision.h"
> +#include "dir.h"
>   
>   struct patch_util {
>   	/* For the search for an exact match */
> @@ -26,6 +27,309 @@ struct patch_util {
>   	struct object_id oid;
>   };
>   
> +static inline int strtost(char const *s, size_t *result, const char **end)
> +{
> +	unsigned long u;
> +	char *p;
> +
> +	errno = 0;
> +	/* negative values would be accepted by strtoul */
> +	if (!isdigit(*s))
> +		return -1;
> +	u = strtoul(s, &p, 10);
> +	if (errno || p == s)
> +		return -1;
> +	if (result)
> +		*result = u;
> +	*end = p;
> +
> +	return 0;
> +}
> +
> +static int parse_hunk_header(const char *p,
> +			     size_t *old_count, size_t *new_count,
> +			     const char **end)
> +{
> +	size_t o = 1, n = 1;
> +
> +	if (!skip_prefix(p, "@@ -", &p) ||
> +	    strtost(p, NULL, &p) ||
> +	    /* The range is -<start>[,<count>], defaulting to count = 1 */
> +	    !(*p == ' ' || (*p == ',' && !strtost(p + 1, &o, &p))) ||
> +	    !skip_prefix(p, " +", &p) ||
> +	    strtost(p, NULL, &p) ||
> +	    /* The range is +<start>[,<count>], defaulting to count = 1 */
> +	    !(*p == ' ' || (*p == ',' && !strtost(p + 1, &n, &p))) ||
> +	    !skip_prefix(p, " @@", &p))
> +		return -1;
> +
> +	*old_count = o;
> +	*new_count = n;
> +	*end = p;
> +
> +	return 0;
> +}
> +
> +/*
> + * This function finds the end of the line, replaces the newline character with
> + * a NUL, and returns the offset of the start of the next line.
> + *
> + * If no newline character was found, it returns the offset of the trailing NUL
> + * instead.
> + */
> +static inline int find_next_line(const char *line, size_t size)
> +{
> +	char *eol;
> +
> +	eol = memchr(line, '\n', size);
> +	if (!eol)
> +		return size;
> +
> +	*eol = '\0';
> +
> +	return eol + 1 - line;
> +}
> +
> +static int read_mbox(const char *path, struct string_list *list)
> +{
> +	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
> +	struct strbuf long_subject = STRBUF_INIT;
> +	struct patch_util *util = NULL;
> +	enum {
> +		MBOX_BEFORE_HEADER,
> +		MBOX_IN_HEADER,
> +		MBOX_IN_COMMIT_MESSAGE,
> +		MBOX_AFTER_TRIPLE_DASH,
> +		MBOX_IN_DIFF
> +	} state = MBOX_BEFORE_HEADER;
> +	char *line, *current_filename = NULL;
> +	int len;
> +	size_t size, old_count = 0, new_count = 0;
> +	const char *author = NULL, *subject = NULL;
> +
> +	if (!strcmp(path, "-")) {
> +		if (strbuf_read(&contents, STDIN_FILENO, 0) < 0)
> +			return error_errno(_("could not read stdin"));
> +	} else if (strbuf_read_file(&contents, path, 0) < 0)
> +		return error_errno(_("could not read '%s'"), path);
> +
> +	line = contents.buf;
> +	size = contents.len;
> +	for (; size; size -= len, line += len) {
> +		const char *p;
> +
> +		len = find_next_line(line, size);
> +
> +		if (state == MBOX_BEFORE_HEADER ||
> +		    (state == MBOX_IN_DIFF && line[0] == 'F')) {
> +			if (!skip_prefix(line, "From ", &p))
> +				continue;
> +
> +			util = xcalloc(1, sizeof(*util));
> +			if (get_oid_hex(p, &util->oid) < 0)
> +				oidcpy(&util->oid, null_oid());
> +			util->matching = -1;
> +			author = subject = NULL;
> +
> +			state = MBOX_IN_HEADER;
> +			continue;
> +		}
> +
> +		if (starts_with(line, "diff --git ")) {
> +			struct patch patch = { 0 };
> +			struct strbuf root = STRBUF_INIT;
> +			int linenr = 0;
> +			int orig_len;
> +
> +			state = MBOX_IN_DIFF;
> +			old_count = new_count = 0;
> +			strbuf_addch(&buf, '\n');
> +			if (!util->diff_offset)
> +				util->diff_offset = buf.len;
> +
> +			orig_len = len;
> +			/* `find_next_line()`'s replaced the LF with a NUL */
> +			line[len - 1] = '\n';
> +			len = len > 1 && line[len - 2] == '\r' ?
> +				error(_("cannot handle diff headers with "
> +					"CR/LF line endings")) :
> +				parse_git_diff_header(&root, &linenr, 1, line,
> +						      len, size, &patch);
> +			if (len < 0) {
> +				error(_("could not parse git header '%.*s'"),
> +				      orig_len, line);
> +				release_patch(&patch);
> +				free(util);
> +				free(current_filename);
> +				string_list_clear(list, 1);
> +				strbuf_release(&buf);
> +				strbuf_release(&contents);
> +				strbuf_release(&long_subject);
> +				return -1;
> +			}
> +
> +			if (patch.old_name)
> +				skip_prefix(patch.old_name, "a/",
> +					    (const char **)&patch.old_name);
> +			if (patch.new_name)
> +				skip_prefix(patch.new_name, "b/",
> +					    (const char **)&patch.new_name);
> +
> +			strbuf_addstr(&buf, " ## ");
> +			if (patch.is_new)
> +				strbuf_addf(&buf, "%s (new)", patch.new_name);
> +			else if (patch.is_delete)
> +				strbuf_addf(&buf, "%s (deleted)", patch.old_name);
> +			else if (patch.is_rename)
> +				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
> +			else
> +				strbuf_addstr(&buf, patch.new_name);
> +
> +			free(current_filename);
> +			if (patch.is_delete)
> +				current_filename = xstrdup(patch.old_name);
> +			else
> +				current_filename = xstrdup(patch.new_name);
> +
> +			if (patch.new_mode && patch.old_mode &&
> +			    patch.old_mode != patch.new_mode)
> +				strbuf_addf(&buf, " (mode change %06o => %06o)",
> +					    patch.old_mode, patch.new_mode);
> +
> +			strbuf_addstr(&buf, " ##\n");
> +			util->diffsize++;
> +			release_patch(&patch);
> +		} else if (state == MBOX_IN_HEADER) {
> +			if (!line[0]) {
> +				state = MBOX_IN_COMMIT_MESSAGE;
> +				/* Look for an in-body From: */
> +				if (skip_prefix(line + 1, "From: ", &p)) {
> +					size -= p - line;
> +					line += p - line;
> +					len = find_next_line(line, size);
> +
> +					while (isspace(*p))
> +						p++;
> +					author = p;
> +				}
> +				strbuf_addstr(&buf, " ## Metadata ##\n");
> +				if (author)
> +					strbuf_addf(&buf, "Author: %s\n", author);
> +				strbuf_addstr(&buf, "\n ## Commit message ##\n");
> +				if (subject)
> +					strbuf_addf(&buf, "    %s\n\n", subject);
> +			} else if (skip_prefix(line, "From: ", &p)) {
> +				while (isspace(*p))
> +					p++;
> +				author = p;
> +			} else if (skip_prefix(line, "Subject: ", &p)) {
> +				const char *q;
> +
> +				while (isspace(*p))
> +					p++;
> +				subject = p;
> +
> +				if (starts_with(p, "[PATCH") &&
> +				    (q = strchr(p, ']'))) {
> +					q++;
> +					while (isspace(*q))
> +						q++;
> +					subject = q;
> +				}
> +
> +				if (len < size && line[len] == ' ') {
> +					/* handle long subject */
> +					strbuf_reset(&long_subject);
> +					strbuf_addstr(&long_subject, subject);
> +					while (len < size && line[len] == ' ') {
> +						line += len;
> +						size -= len;
> +						len = find_next_line(line, size);
> +						strbuf_addstr(&long_subject, line);
> +					}
> +					subject = long_subject.buf;
> +				}
> +			}
> +		} else if (state == MBOX_IN_COMMIT_MESSAGE) {
> +			if (!line[0]) {
> +				strbuf_addch(&buf, '\n');
> +			} else if (strcmp(line, "---")) {
> +				int tabs = 0;
> +
> +				/* simulate tab expansion */
> +				while (line[tabs] == '\t')
> +					tabs++;
> +				strbuf_addf(&buf, "%*s%s\n",
> +					    4 + 8 * tabs, "", line + tabs);
> +			} else {
> +				/*
> +				 * Trim the trailing newline that is added
> +				 * by `format-patch`.
> +				 */
> +				strbuf_trim_trailing_newline(&buf);
> +				state = MBOX_AFTER_TRIPLE_DASH;
> +			}
> +		} else if (state == MBOX_IN_DIFF) {
> +			switch (line[0]) {
> +			case '\0':
> +				continue; /* ignore empty lines after diff */
> +			case '+':
> +			case '-':
> +			case ' ':
> +				/* A `-- ` line indicates the end of a diff */
> +				if (!old_count && !new_count)
> +					break;
> +				if (old_count && line[0] != '+')
> +					old_count--;
> +				if (new_count && line[0] != '-')
> +					new_count--;
> +				/* fallthrough */
> +			case '\\':
> +				strbuf_addstr(&buf, line);
> +				strbuf_addch(&buf, '\n');
> +				util->diffsize++;
> +				continue;
> +			case '@':
> +				if (parse_hunk_header(line, &old_count,
> +						      &new_count, &p))
> +					break;
> +
> +				strbuf_addstr(&buf, "@@");
> +				if (current_filename && *p)
> +					strbuf_addf(&buf, " %s:",
> +						    current_filename);
> +				strbuf_addstr(&buf, p);
> +				strbuf_addch(&buf, '\n');
> +				util->diffsize++;
> +				continue;
> +			}
> +
> +			if (util) {
> +				string_list_append(list, buf.buf)->util = util;
> +				strbuf_reset(&buf);
> +			}
> +			util = xcalloc(1, sizeof(*util));
> +			oidcpy(&util->oid, null_oid());
> +			util->matching = -1;
> +			author = subject = NULL;
> +			state = MBOX_BEFORE_HEADER;
> +		}
> +	}
> +	strbuf_release(&contents);
> +
> +	if (util) {
> +		if (state == MBOX_IN_DIFF)
> +			string_list_append(list, buf.buf)->util = util;
> +		else
> +			free(util);
> +	}
> +	strbuf_release(&buf);
> +	strbuf_release(&long_subject);
> +	free(current_filename);
> +
> +	return 0;
> +}
> +
>   /*
>    * Reads the patches into a string list, with the `util` field being populated
>    * as struct object_id (will need to be free()d).
> @@ -41,6 +345,10 @@ static int read_patches(const char *range, struct string_list *list,
>   	ssize_t len;
>   	size_t size;
>   	int ret = -1;
> +	const char *path;
> +
> +	if (skip_prefix(range, "mbox:", &path))
> +		return read_mbox(path, list);
>   
>   	strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
>   		     "--reverse", "--date-order", "--decorate=no",
> @@ -424,6 +732,19 @@ static void output_pair_header(struct diff_options *diffopt,
>   
>   		strbuf_addch(buf, ' ');
>   		pp_commit_easy(CMIT_FMT_ONELINE, commit, buf);
> +	} else {
> +		struct patch_util *util = b_util ? b_util : a_util;
> +		const char *needle = "\n ## Commit message ##\n";
> +		const char *p = !util || !util->patch ?
> +			NULL : strstr(util->patch, needle);
> +		if (p) {
> +			if (status == '!')
> +				strbuf_addf(buf, "%s%s", color_reset, color);
> +
> +			strbuf_addch(buf, ' ');
> +			p += strlen(needle);
> +			strbuf_add(buf, p, strchrnul(p, '\n') - p);
> +		}
>   	}
>   	strbuf_addf(buf, "%s\n", color_reset);
>   
> @@ -554,6 +875,9 @@ int show_range_diff(const char *range1, const char *range2,
>   	if (range_diff_opts->left_only && range_diff_opts->right_only)
>   		res = error(_("options '%s' and '%s' cannot be used together"), "--left-only", "--right-only");
>   
> +	if (!strcmp(range1, "mbox:-") && !strcmp(range2, "mbox:-"))
> +		res = error(_("only one mbox can be read from stdin"));
> +
>   	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))
> @@ -575,10 +899,17 @@ int show_range_diff(const char *range1, const char *range2,
>   int is_range_diff_range(const char *arg)
>   {
>   	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
> -	const char *argv[] = { "", copy, "--", NULL };
> +	const char *argv[] = { "", copy, "--", NULL }, *path;
>   	int i, positive = 0, negative = 0;
>   	struct rev_info revs;
>   
> +	if (skip_prefix(arg, "mbox:", &path)) {
> +		if (!strcmp(path, "-") || file_exists(path))
> +			return 1;
> +		error_errno(_("not an mbox: '%s'"), path);
> +		return 0;
> +	}
> +
>   	init_revisions(&revs, NULL);
>   	if (setup_revisions(3, argv, &revs, NULL) == 1) {
>   		for (i = 0; i < revs.pending.nr; i++)
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 459beaf7d9c..89ef9a5ffc4 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -783,6 +783,33 @@ test_expect_success 'ranges with pathspecs' '
>   	! grep "$topic_oid" actual
>   '
>   
> +test_expect_success 'compare range vs mbox' '
> +	git format-patch --stdout topic..mode-only-change >mbox &&
> +	git range-diff topic...mode-only-change >expect &&
> +
> +	git range-diff mode-only-change..topic mbox:./mbox >actual &&
> +	test_cmp expect actual &&
> +
> +	sed -e "/^From: .*/{
> +		h
> +		s/.*/From: Bugs Bunny <bugs@bun.ny>/
> +		:1
> +		N
> +		/[ -z]$/b1
> +		G
> +	}" <mbox >mbox.from &&
> +	git range-diff mode-only-change..topic mbox:./mbox.from >actual.from &&
> +	test_cmp expect actual.from &&
> +
> +	append_cr <mbox >mbox.cr &&
> +	test_must_fail git range-diff \
> +		mode-only-change..topic mbox:./mbox.cr 2>err &&
> +	grep CR/LF err &&
> +
> +	git range-diff mode-only-change..topic mbox:- <mbox >actual.stdin &&
> +	test_cmp expect actual.stdin
> +'
> +
>   test_expect_success 'submodule changes are shown irrespective of diff.submodule' '
>   	git init sub-repo &&
>   	test_commit -C sub-repo sub-first &&
> 
> base-commit: b75747829f4c277323c78b1c5973ad63ea038a2d
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin
Message 10 of 16 in “range-diff: support reading mbox files”
  1. range-diff: support reading mbox filesJohannes Schindelin via GitGitGadget, Nov 15, 2022
  2. Taylor BlauNov 16, 2022
  3. Phillip WoodNov 16, 2022
  4. Phillip WoodNov 17, 2022
  5. Johannes SchindelinNov 18, 2022
  6. Johannes SchindelinNov 18, 2022
  7. Ævar Arnfjörð BjarmasonNov 17, 2022
  8. Johannes SchindelinNov 18, 2022
  9. range-diff: support reading mbox filesJohannes Schindelin via GitGitGadget, Nov 19, 2022
  10. Phillip WoodNov 21, 2022
  11. Johannes SchindelinNov 22, 2022
  12. Phillip WoodNov 22, 2022
  13. range-diff: support reading mbox filesJohannes Schindelin via GitGitGadget, Nov 22, 2022
  14. Phillip WoodNov 22, 2022
  15. Junio C HamanoNov 22, 2022
  16. Junio C HamanoMar 3, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.