threads / discuss / 24934

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

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

## tl;dr

5 messages between Sep 1, 2010 and Sep 1, 2010.

replies: 4people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Sep 1, 2010, 14:17 UTC · lore

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.

Stephen Boyd· Sep 1, 2010, 14:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

 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.

Junio C Hamano· Sep 1, 2010, 17:31 UTC · re: Stephen Boyd · lore

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

Stephen Boyd <bebarino@gmail.com> writes:
Show 11 quoted lines
>  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.
Ævar Arnfjörð Bjarmason· Sep 1, 2010, 16:23 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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:
Show 11 quoted lines
>  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
Junio C Hamano· Sep 1, 2010, 17:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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.
Show 13 quoted lines
> 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.

← back to recent threads