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

From: Justin Tobler <jltobler@gmail.com>
Date: Feb 27, 2025, 22:15 UTC
Message-ID: <wxr2memiox7l4ljz4wozxlxtkd2qms7crzsztb2vim6f5jjw4s@jm5retwyxhib>
In-Reply-To: <xmqqjz9cd0nl.fsf@gitster.g>

On 25/02/26 10:24AM, Junio C Hamano wrote:

Show 19 quoted lines

> 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?

Good point. When invoking diffcore_std(), we really just need to be able to skip diff_resolve_rename_copy() as that is what is updating the diff filepair statuses. In the next version, instead of relying on `found_follow`, I think I'll introduce a new diff_options field, `skip_resolving_statuses` for this specific purpose.

Show 16 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;
> 
> 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.

Makes sense, I'll swap to using strbuf_getwholeline() with a defined line terminator variable in the next version. This way it can help make supporting the "normal" raw diff format as input easier in the future.

Show 9 quoted lines

> > +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.

Will adjust in the next version.

> Other than that, nicely done.

Thanks for the review! -Justin