Re: [PATCH 06/14] format-patch: allow --interdiff to apply to a lone-patch

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 23, 2018, 19:46 UTC
Message-ID: <CAPig+cR=rcoLqtefWJMe0NY_7OX7WR6q1UoF-HnYrHZWtKuHAw@mail.gmail.com>
In-Reply-To: <CACsJy8Aw6R8-3kDfhCqunXziajCg9O_1WrEYc4rfKa+-=m1D5g@mail.gmail.com>

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

Show 8 quoted lines

> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > +       if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
>
> OK putting idiff stuff in rev_info is probably the right choice. But
> we all three fields prefixed with idiff_, maybe you could just add a
> new struct "idiff_options" to contain them and add a pointer to that
> struct in rev_info. Then "opt->idiff" is enough to know if idiff is
> requested instead of relying on idiff_oid1 (seems too random).

I have mixed feelings about this suggestion for the following reasons:

* 'struct rev_info' already contains a number of specialized fields
which apply in only certain use cases but not others, and those fields
often are grouped textually to show relationship rather than being
bundled in a struct.

* These new fields are very specific to this particular operation and
are unlikely to ever have wider use, so adding the extra level of
indirection/abstraction (whatever you'd call it) feels overkill and,
while nice theoretically, adds complexity without much practical
value.

* Bundling these fields in a struct might incorrectly imply to readers
that these items, collectively, can be used in some general-purpose
fashion, which isn't at all the case; they are very specific to this
operation and that struct would never be used elsewhere or for any
other purpose.

The upside of bundling them in a struct, as you mention, is that "opt->idiff" would be slightly more obvious than "opt->idiff_oid1" as a "should we print an interdiff?" conditional. On the other hand, this case is so specific and narrow that I'm not sure it warrants such generality.