On 25/02/26 10:24AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> 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.
> > + 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.
> > +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
