On Fri, May 5, 2017 at 10:44 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jeff Smith <whydoubt@xxxxxxxxx> writes:
>
>> Signed-off-by: Jeff Smith <whydoubt@xxxxxxxxx>
>> ---
>> 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