Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 23, 2018, 19:18 UTC
Message-ID: <CAPig+cRsPV+PGtkEM_c1b4P-NF7Dk7A8DvPVuvdbx4ESr3pmqg@mail.gmail.com>
In-Reply-To: <CACsJy8C8RK6HkfoEYJGZg=sgtJS0WksHD3=7Souw3jYebRo=Sg@mail.gmail.com>

On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen <pclouds@gmail.com> wrote:

Show 19 quoted lines

> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@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.