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

From: Justin Tobler <jltobler@gmail.com>
Date: Feb 27, 2025, 23:00 UTC
Message-ID: <qnd5cou3mhareqvxhndgwkomn7t3kwdn3xd55wqn6vidly33uk@5yq4cv2yfnbh>
In-Reply-To: <Z8Bg5EAArZVGPaAc@pks.im>

On 25/02/27 01:56PM, Patrick Steinhardt wrote:

Show 13 quoted lines

> > +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;
> 
> It's a bit weird that we set this over here. Shouldn't we have set it up
> in the main function already?

Everytime diffcore_std() is invoked found_follow gets reset. This was included here to ensure the correct value is always set.

In the next version I am going to move away from using found_follow in favor of a new diff_options field to avoid some of this awkwardness altogether.

Show 26 quoted lines

> > +	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);
> 
> Don't we also have to call `parse_options()` even though we don't have
> our own options yet? Or is this all handled by `setup_revisions()`?

In the current implementation, the diff options that get appended are only really used so that the usage message prints with the diff option info. It still relies on setup_revisions() to actually parse the options. Since there are not any real options that need parsing, parse_options() was not invoked.

This is fairly confusing though. I plan to instead parse the diff options upfront with parse_options(). The diff options parsing through setup_revisions() becomes effectively a no-op. I think this makes more sense to read and still lets us print the common diff options is the usage message.

Show 16 quoted lines

> > +	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);
> 
> I think it's discouraged nowadays to use `usage_with_options()` as it
> generates a ton of noise while hiding the actual error message. It is
> instead recommended to directly call `usage()` with an error message.
> 
> In this case here we would say e.g. `usage(_("unrecognized argument:
> %s"), argv[0])`, in the cases below we'd use the error messages you
> already have.

Good to know. I'll avoid printing the usage options message in all these failure scenarios in favor of what you suggested.

Show 5 quoted lines

> > +	if (!revs.diffopt.output_format)
> > +		revs.diffopt.output_format = DIFF_FORMAT_PATCH;
> 
> Instead of setting this conditionally, can we already set it up as a
> default before calling `setup_revisions()`?

The diff output format is set via OPT_BITOP() and thus can have multiple values at the same time. For example:

  $ git diff-tree --raw --patch HEAD

will render both patch and raw output. If we unconditionally set DIFF_FORMAT_PATCH, it will always be included in the output which is not what we want. We only want to set DIFF_FORMAT_PATCH if there is still no value after all options parsing has occurred.

Show 24 quoted lines

> > +	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;
> > +
> > +		p = meta.buf;
> > +		if (*p != ':')
> > +			die(_("invalid raw diff input"));
> > +		p++;
> > +
> > +		mode_a = parse_mode_or_die(p, &p);
> > +		mode_b = parse_mode_or_die(p, &p);
> > +
> > +		if (S_ISDIR(mode_a) || S_ISDIR(mode_b))
> > +			die(_("tree objects not supported"));
> 
> I assume submodules aren't supported either, are they? If so, do we also
> have to check for `S_ISGITLINK()`? It would be nice to have a test for
> them.

Submodules should actually be supported as I believe all the info present in the raw formatted input should be enough to properly display patch output. I'll add a submodule to the existing test setup to validate.

Thanks -Justin