Re: [PATCH] revision.h: add includes of "diff.h" and "commit.h"

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2009, 08:02 UTC
Message-ID: <7v3acnzfy6.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090405162841.6117@nanako3.lavabit.com>

Nanako Shiraishi <nanako3@lavabit.com> writes:

Show 14 quoted lines

> Quoting Christian Couder <chriscool@tuxfamily.org>:
>
>> Because they are needed by some features included in
>> "revision.h".
>>
>> This makes the following just work:
>>
>>         $ cat >1.c <<\EOF
>>         #include "cache.h"
>>         #include "revision.h"
>>         EOF
>>         $ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c
>
> I'm sorry if this is obvious to experienced people, but I don't understand what benefit there is to make such an empty program compilable.

I believe this was prompted by my earlier comment on the header clean-up ($gmane/115443), but I'd say it does not make much sense. I have already explained why it doesn't, and in addition, as the above example shows, you still have to include "cache.h" in your 1.c file anyway, so it is not making the header "usable standalone" either.

If you have a follow-up patch that removes the inclusion of diff.h and commit.h to millions of .c files that already include revision.h, it might start to make sense, but it goes against one of the rules Christian wanted to add, namely:

    a header file should be included in a C file only if it is needed to 
    compile the C file (it is not ok to include it only because it includes 
    many other headers that are needed)

in the sense that if somebody wants to run diff in his C code, he should explicitly include diff.h (or diffcore.h if necessary), instead of relying on the fact that revision.h happens to include it, and he happens to include revision.h because he uses setup_revisions() to parse the command line arguments (and I happen to think that guideline makes sense).

Even though including the same .h file twice is protected with the standard:

	#ifndef FROTZ_H
        #define FROTZ_H
        ...
        #endif

it does make C preprocessor do extra work to open the header twice (and skip the whole file in its second inclusion), so there is a slight performance issue.

You can argue revision.h is somewhat special---it are so central that almost all core-ish history inspection commands in git revolve around them, and it is not particularly a bad idea to say "you can rely on revision.h to include diff.h" in practice. That would give you an escape hatch to omit inclusion of diff.h from programs that include revision.h and avoid the performance issue.

But then that introduces new rules on which ones are special and which ones are not, and overall it does not help simplifying the life of the programmers.

So I do not feel strongly supportive about this patch.