Re: [PATCH 05/16] revision: add missing include

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2013, 18:11 UTC
Message-ID: <xmqqppql71l0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1383211547-9145-6-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 19 quoted lines

> Otherwise we might not have 'struct diff_options'.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  revision.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/revision.h b/revision.h
> index e7f1d21..89132df 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -5,6 +5,7 @@
>  #include "grep.h"
>  #include "notes.h"
>  #include "commit.h"
> +#include "diff.h"
>  
>  #define SEEN		(1u<<0)
>  #define UNINTERESTING   (1u<<1)

This is a step in the right direction to change the contract between this header file and its consumers, but I think it falls short of doing a good job at it.

The rule used to be that "if you use a declaration in revision.h, you must include diff.h before including it, even if you do not use any declaration made in diff.h yourself".

The new rule this patch introduces is "if you use a declaration in foo.h, include foo.h, period---foo.h should handle its requirement on its own internally and consumers should not have to care", which is much saner.

But the patch needs to also remove '#include "diff.h"' from existing consumers that themselves do not use any declaration from "diff.h" (e.g. bundle.c; there are others), while keeping the inclusion in those that do (e.g. builtin/commit.c). That can be a separate patch that immediately follow this one, or a part of the same patch.

Thanks.