On Mon, Jul 23, 2018 at 12:22 PM Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
