Eric Sunshine <sunsh...@sunshineco.com> writes:
> On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen <pclo...@gmail.com> wrote:
>> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunsh...@sunshineco.com>
>> wrote:
>> > @@ -0,0 +1,17 @@
>> > +void show_interdiff(struct rev_info *rev)
>> > +{
>> > + struct diff_options opts;
>> > +
>> > + memcpy(&opts, &rev->diffopt, sizeof(opts));
>> > + opts.output_format = DIFF_FORMAT_PATCH;
>> > + diff_setup_done(&opts);
>> > +
>> > + diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
>> > + diffcore_std(&opts);
>> > + diff_flush(&opts);
>> > +}
>>
>> Is it worth adding a new file just for a single function? I haven't
>> read the rest of the series, but the cover letter's diffstat suggests
>> this is it. Is interdiff intended to become a lot more complicated in
>> the future? If not maybe just add this function in diff-lib.c
>
> Good question. The functionality originally lived in builtin/log.c but
> moved to log-tree.c when I added the ability to embed an interdiff in
> a single patch. However, it didn't "feel" right in log-tree.c, so I
> moved it to its own file to mirror how the range-diff engine resides
> in its own file.
> And, the function actually did several more things as originally
> implemented. For instance, it took care of not clobbering global
> diff-queue state, and consulting 'reroll_count' and printing the
> "Interdiff:" header, but those bits eventually moved to live at more
> "correct" locations, leaving this relatively minimal function behind.
> It does get a bit more complex in a later patch, but not significantly
> so.
>
> I wasn't aware of diff-lib.c, but it does seem like show_interdiff()
> could be at home there.
Yeah, diff-lib.c is meant to be a home for the implementation of low
level "diff-{files,tree,index}" plumbing that can be called from
other routines; if you are adding "take two things, compare them"
that can be reused from places, then it is a good match.
