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

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

[Christian Couder]

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 revision.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/revision.h b/revision.h
index 5adfc91..495d7eb 100644
--- a/revision.h
+++ b/revision.h
@@ -3,6 +3,8 @@
 
 #include "parse-options.h"
 #include "grep.h"
+#include "diff.h"
+#include "commit.h"
 
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)
-- 
1.6.2.1.530.g516a7.dirty

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

[Nanako Shiraishi]

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.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

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

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> 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.