Re: [PATCH v3 2/3] builtin: introduce diff-pairs command

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

> +static void flush_diff_queue(struct diff_options *options)
> +{
> +	/*
> +	 * If rename detection is not requested, use rename information from the
> +	 * raw diff formatted input. Setting found_follow ensures diffcore_std()
> +	 * does not mess with rename information already present in queued
> +	 * filepairs.
> +	 */
> +	if (!options->detect_rename)
> +		options->found_follow = 1;

An ugly design decision that may be suboptimal from maintainability
point of view.

The parts of diffcore_std() that --follow wants to bypass may happen
to be the same as the parts that this new caller wants to bypass,
but who guarantees that they will stay that way in the future?

> +	diffcore_std(options);
> +	diff_flush(options);
> +}
> +
> +int cmd_diff_pairs(int argc, const char **argv, const char *prefix,
> +		   struct repository *repo)
> +{
> +	struct strbuf path_dst = STRBUF_INIT;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf meta = STRBUF_INIT;
> +	struct rev_info revs;
> +	int ret;
> +
> +	const char * const usage[] = {
> +		N_("git diff-pairs -z [<diff-options>]"),
> +		NULL
> +	};
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	struct option *parseopts = add_diff_options(options, &revs.diffopt);
> +
> +	show_usage_with_options_if_asked(argc, argv, usage, parseopts);
> +
> +	repo_init_revisions(repo, &revs, prefix);
> +	repo_config(repo, git_diff_basic_config, NULL);
> +	revs.disable_stdin = 1;
> +	revs.abbrev = 0;
> +	revs.diff = 1;
> +
> +	if (setup_revisions(argc, argv, &revs, NULL) > 1)
> +		usage_with_options(usage, parseopts);
> +
> +	/*
> +	 * With the -z option, both command input and raw output are
> +	 * NUL-delimited (this mode does not effect patch output). At present

Probably "effect" -> "affect".

> +	 * only NUL-delimited raw diff formatted input is supported.
> +	 */
> +	if (revs.diffopt.line_termination) {
> +		error(_("working without -z is not supported"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (revs.prune_data.nr) {
> +		error(_("pathspec arguments not supported"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (revs.pending.nr || revs.max_count != -1 ||
> +	    revs.min_age != (timestamp_t)-1 ||
> +	    revs.max_age != (timestamp_t)-1) {
> +		error(_("revision arguments not allowed"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (!revs.diffopt.output_format)
> +		revs.diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> +	while (1) {
> +		struct object_id oid_a, oid_b;
> +		struct diff_filepair *pair;
> +		unsigned mode_a, mode_b;
> +		const char *p;
> +		char status;
> +
> +		if (strbuf_getline_nul(&meta, stdin) == EOF)
> +			break;

There should be a variant of this function that takes delimiter
parameter.  By declaring an int variable that is initialized to '\0'
(because you only deal with "-z" input) and passing that delimiter
variable to strbuf_getwholeline() would future-proof this code path.

How builtin/update-ref.c:update_refs_stdin() works may be inspiring.

> +		switch (status) {
> +		case DIFF_STATUS_ADDED:
> +			pair = diff_queue_addremove(&diff_queued_diff,
> +						    &revs.diffopt, '+', mode_b,
> +						    &oid_b, 1, path.buf, 0);
> +			if (pair)
> +				pair->status = status;
> +			break;
> + ...
> +		default:
> +			die(_("unknown diff status: %c"), status);
> +		}
> +	}

Amusing; looking good.

> +	flush_diff_queue(&revs.diffopt);
> +	ret = diff_result_code(&revs);
> +
> +	strbuf_release(&path_dst);
> +	strbuf_release(&path);
> +	strbuf_release(&meta);
> +	release_revisions(&revs);
> +	FREE_AND_NULL(parseopts);
> +
> +	return ret;
> +}

Nice.  It is surprisingly compact and had everything I expected it
to have ;-).

> +test_expect_success 'diff-pairs recreates --raw' '
> +	git diff-tree -r -M -C -C -z base new >expect &&
> +	git diff-tree -r -M -C -C -z base new |
> +	git diff-pairs --raw -z >actual &&
> +	test_cmp expect actual
> +'

Amusing ;-)  But a very obvious and important thing to test.
I would have fed <expect to diff-pairs for this test, though.

Other than that, nicely done.