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

From: Stefan Beller <sbeller@google.com>
Date: May 5, 2017, 17:50 UTC
Message-ID: <CAGZ79kZ4k7qVKE7OpSEELorwSx5hUgJksLAeNgVGs0w1JC_Ffg@mail.gmail.com>
In-Reply-To: <xmqqwp9v6vw2.fsf@gitster.mtv.corp.google.com>

On Fri, May 5, 2017 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Jeff Smith <whydoubt@gmail.com> writes:
>
>> Signed-off-by: Jeff Smith <whydoubt@gmail.com>
>> ---
>>  builtin.h          |  2 --
>>  builtin/blame.c    | 28 ----------------------------
>>  builtin/cat-file.c |  1 +
>>  diff.c             | 23 +++++++++++++++++++++++
>>  diff.h             |  7 +++++++
>>  5 files changed, 31 insertions(+), 30 deletions(-)
>
> This change makes sense regardless of your primary goal of the
> series.  It was not good that one builtin borrowing a helper from
> another.  The common helper should live outside builtin/ as a common
> code, and in this case, diff.[ch] may be an OK place for it.
>

My kneejerk reaction to this is would be:
    Please don't grow diff.[ch] even more. It has
    5k lines which is a lot IMHO. Although it might be
    ok for the compiler and from a logical point of view,
    I'd rather prefer to deal with lots of small files, than
    with such large files. I am undecided if this hints at
    bad tooling on my side or at how I think of code
    and their location.

I guess it is ok for now and in this series, but we may want to split up diff.[ch] in the future into multiple finer grained files.

Thanks, Stefan