Why isn't builtin.h used by all builtin/*.c files?

Why isn't builtin.h used by all builtin/*.c files?

[Ævar Arnfjörð Bjarmason]

I'm working on gettextizing the 'mainporcelain common' commands now,
and I ran into this:

    $ perl -MArray::Diff -E 'chomp(my @hb = qx[ack -l builtin.h *.c]);
my @c = glob "*.c"; my $d = Array::Diff->diff( \@c, \@hb ); say for
@{$d->deleted}'
    fetch-pack.c
    hash-object.c
    index-pack.c
    merge-index.c
    merge-recursive.c
    merge-tree.c
    mktag.c
    pack-redundant.c
    pack-refs.c
    patch-id.c
    receive-pack.c
    remote-ext.c
    remote-fd.c
    remote.c
    reset.c
    send-pack.c
    skew.c
    unpack-file.c
    var.c

There seems to be no reason not to do this:

    --- a/builtin/clone.c
    +++ b/builtin/clone.c
    @@ -10,3 +10,3 @@

    -#include "cache.h"
    +#include "builtin.h"
     #include "parse-options.h"
    @@ -18,3 +18,2 @@
     #include "transport.h"
    -#include "strbuf.h"
     #include "dir.h"

builtin.h even includes the cmd_clone() prototype. I'm only asking
since clone.c was added *after* builtin.h, so this seems like an
omission.

Anyway, I'll convert these to builtin.h where appropriate as part of
the series unless someone can find a good reason not to.

Re: Why isn't builtin.h used by all builtin/*.c files?

[Stephen Boyd]

 On 09/01/2010 07:17 AM, Ævar Arnfjörð Bjarmason wrote:
> I'm working on gettextizing the 'mainporcelain common' commands now,
> and I ran into this:
>
[snip]
>
> Anyway, I'll convert these to builtin.h where appropriate as part of
> the series unless someone can find a good reason not to.

Sparse warns about this too. "Prototype not found, should it be static?"
for a few cmd_*'s. So sounds good to me.

[PATCH] builtin: use builtin.h for all builtin/ commands

[Ævar Arnfjörð Bjarmason]

Some builtin/*.c commands weren't using the builtin.h header, but
instead manually imported headers like cache.h and commit.h which
builtin.h would include for them.

This impeded my efforts to gettextize git, since I'd otherwise have to
add gettext.h to all of these, and using builtin.h is a good idea in
any case, since it's defining the prototypes for the cmd_* functions
that these files define.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Sep 1, 2010 at 14:56, Stephen Boyd <bebarino@gmail.com> wrote:
>  On 09/01/2010 07:17 AM, Ęvar Arnfjörš Bjarmason wrote:
>> I'm working on gettextizing the 'mainporcelain common' commands now,
>> and I ran into this:
>>
> [snip]
>>
>> Anyway, I'll convert these to builtin.h where appropriate as part of
>> the series unless someone can find a good reason not to.
>
> Sparse warns about this too. "Prototype not found, should it be static?"
> for a few cmd_*'s. So sounds good to me.

This is a patch against master for this issue. It also affects the
skew.c and commands from the remote-ext series which are not on master.

I made this with:

    for i in $(perl -MArray::Diff -E 'chomp(my @hb = qx[ack -l builtin.h *.c]); my @c = glob "*.c"; my $d = Array::Diff->diff( \@c, \@hb ); say for @{$d->deleted}'); do perl /tmp/builtin-munger.pl $i; done

Where builtin-munger.pl is:
    
    #!/usr/bin/env perl
    use 5.12.0;
    use strict;
    use autodie;
    
    my $f = shift;
    
    open my $out, ">", "$f.tmp";
    open my $in, "<", $f;
    
    while (<$in>) {
        chomp;
        say $out qq[#include "builtin.h"] if 1 .. /^#include/ and /^#include/;
        say $out $_ unless $_ ~~ [
            '#include "git-compat-util.h"',
            '#include "strbuf.h"',
            '#include "cache.h"',
            '#include "commit.h"',
            '#include "notes.h"',
            '#include "gettext.h"',
        ];
    }
    close $_ for $in, $out;
    rename("$f.tmp", $f);

 builtin/clone.c           |    3 +--
 builtin/fetch-pack.c      |    3 +--
 builtin/hash-object.c     |    2 +-
 builtin/index-pack.c      |    3 +--
 builtin/merge-index.c     |    2 +-
 builtin/merge-recursive.c |    3 +--
 builtin/merge-tree.c      |    2 +-
 builtin/mktag.c           |    2 +-
 builtin/pack-redundant.c  |    2 +-
 builtin/pack-refs.c       |    2 +-
 builtin/patch-id.c        |    2 +-
 builtin/receive-pack.c    |    3 +--
 builtin/remote.c          |    3 +--
 builtin/reset.c           |    3 +--
 builtin/send-pack.c       |    3 +--
 builtin/unpack-file.c     |    2 +-
 builtin/var.c             |    2 +-
 17 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index efb1e6f..f2a7a54 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -8,7 +8,7 @@
  * Clone a repository into a different directory that does not yet exist.
  */
 
-#include "cache.h"
+#include "builtin.h"
 #include "parse-options.h"
 #include "fetch-pack.h"
 #include "refs.h"
@@ -16,7 +16,6 @@
 #include "tree-walk.h"
 #include "unpack-trees.h"
 #include "transport.h"
-#include "strbuf.h"
 #include "dir.h"
 #include "pack-refs.h"
 #include "sigchain.h"
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index dbd8b7b..03abc30 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1,7 +1,6 @@
-#include "cache.h"
+#include "builtin.h"
 #include "refs.h"
 #include "pkt-line.h"
-#include "commit.h"
 #include "tag.h"
 #include "exec_cmd.h"
 #include "pack.h"
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 080af1a..8a5670f 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -4,7 +4,7 @@
  * Copyright (C) Linus Torvalds, 2005
  * Copyright (C) Junio C Hamano, 2005
  */
-#include "cache.h"
+#include "builtin.h"
 #include "blob.h"
 #include "quote.h"
 #include "parse-options.h"
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index fad76bf..e073bdb 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1,9 +1,8 @@
-#include "cache.h"
+#include "builtin.h"
 #include "delta.h"
 #include "pack.h"
 #include "csum-file.h"
 #include "blob.h"
-#include "commit.h"
 #include "tag.h"
 #include "tree.h"
 #include "progress.h"
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 2c4cf5e..adc2a6d 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "run-command.h"
 #include "exec_cmd.h"
 
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index d8875d5..455fd28 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -1,5 +1,4 @@
-#include "cache.h"
-#include "commit.h"
+#include "builtin.h"
 #include "tag.h"
 #include "merge-recursive.h"
 
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9b25ddc..1991742 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "blob.h"
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 1cb0f3f..9148cc0 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "tag.h"
 #include "exec_cmd.h"
 
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 41e1615..a15e366 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -6,7 +6,7 @@
 *
 */
 
-#include "cache.h"
+#include "builtin.h"
 #include "exec_cmd.h"
 
 #define BLKSIZE 512
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 091860b..39a9d89 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "parse-options.h"
 #include "pack-refs.h"
 
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 5125300..33e9725 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "exec_cmd.h"
 
 static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 760817d..f07b021 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1,11 +1,10 @@
-#include "cache.h"
+#include "builtin.h"
 #include "pack.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
 #include "exec_cmd.h"
-#include "commit.h"
 #include "object.h"
 #include "remote.h"
 #include "transport.h"
diff --git a/builtin/remote.c b/builtin/remote.c
index 48e0a6b..3cf3c6b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1,9 +1,8 @@
-#include "cache.h"
+#include "builtin.h"
 #include "parse-options.h"
 #include "transport.h"
 #include "remote.h"
 #include "string-list.h"
-#include "strbuf.h"
 #include "run-command.h"
 #include "refs.h"
 
diff --git a/builtin/reset.c b/builtin/reset.c
index 1283068..3cbdf94 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -7,10 +7,9 @@
  *
  * Copyright (c) 2005, 2006 Linus Torvalds and Junio C Hamano
  */
-#include "cache.h"
+#include "builtin.h"
 #include "tag.h"
 #include "object.h"
-#include "commit.h"
 #include "run-command.h"
 #include "refs.h"
 #include "diff.h"
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..80b063b 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -1,5 +1,4 @@
-#include "cache.h"
-#include "commit.h"
+#include "builtin.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "sideband.h"
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 608590a..c905d80 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -1,4 +1,4 @@
-#include "cache.h"
+#include "builtin.h"
 #include "blob.h"
 #include "exec_cmd.h"
 
diff --git a/builtin/var.c b/builtin/var.c
index 70fdb4d..0c18196 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) Eric Biederman, 2005
  */
-#include "cache.h"
+#include "builtin.h"
 #include "exec_cmd.h"
 
 static const char var_usage[] = "git var (-l | <variable>)";
-- 
1.7.2.2.178.gd8a94.dirty

Re: Why isn't builtin.h used by all builtin/*.c files?

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

>  On 09/01/2010 07:17 AM, Ævar Arnfjörð Bjarmason wrote:
>> I'm working on gettextizing the 'mainporcelain common' commands now,
>> and I ran into this:
>>
> [snip]
>>
>> Anyway, I'll convert these to builtin.h where appropriate as part of
>> the series unless someone can find a good reason not to.
>
> Sparse warns about this too. "Prototype not found, should it be static?"
> for a few cmd_*'s. So sounds good to me.

Sounds good to me, too.

Re: [PATCH] builtin: use builtin.h for all builtin/ commands

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Some builtin/*.c commands weren't using the builtin.h header, but
> instead manually imported headers like cache.h and commit.h which
> builtin.h would include for them.

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index dbd8b7b..03abc30 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -1,7 +1,6 @@
> -#include "cache.h"
> +#include "builtin.h"
>  #include "refs.h"
>  #include "pkt-line.h"
> -#include "commit.h"
>  #include "tag.h"
>  #include "exec_cmd.h"
>  #include "pack.h"

Leveraging the fact that builtin.h includes git-compat-util.h and cache.h
is fine and I think strbuf is also generic and pervasive enough to be
included in the mix, but in the longer term we would probably want to fix
builtin.h not to include it itself does not need, notably commit.h and
notes.h.

For example, the header currently defines notes related stuff, such as
notes_rewrite_cfg parse_combine_notes_fn, etc.  Shouldn't they belong to
notes.h?  And if we lose them, do we still need to include commit.h and
notes.h in builtin.h?

And when that happens, the inclusion of commit.h needs to come back to
this file, I think.