Re: [RFC PATCH 02/10] Move textconv_object to be with other textconv methods

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2017, 01:49 UTC
Message-ID: <xmqqziem6bq8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170508215525.gcyzzntqvm52mqcc@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> The specific helpers have less visibility, which is good. The public
> functions a() and b() were already public, so no change. But now the
> common helper is public, too, even though nobody except a() and b() care
> about it.
>
> So it's a tradeoff. And the important question is: is the bleed-over
> between a() and b() worse than the common bits being made public?

At this point you are saying the same thing as I said ;-)

I haven't touched diff.c for a while, but my impression was that it was already a smallest logical unit, especially since some bits like diff-lib.c (the interface for the front-end programs to drive the machinery via the three standard pairs of sources) and ws.c (diff subsystem to deal with whitespace errors) are already split out (and needless to say, the diffcore transformations are all in their own separate files). Over time people may have added what does not belong there merely for convenience, which may want to get ejected, but I do not think of many instances of them offhand.

    ... goes and looks ...

It appears that the textconv related helpers (which was where this discussion thread started from) could be first restructured so that they do not depend on diff_filespec and turned into a more generic "I have this path and a blob object name, please run textconv to it and..." interface and then moved out of the file into its own textconv.[ch]. It does not need access to quite a few fields in diff_filespec structure (e.g. it shouldn't care what filemode the thing has). The diff machinery wants the result in a contiguous piece of memory and that could be a good starting point, but I said "and..." above because I am not sure if it is the best generic thing the restructured interface can do to the result.