{"thread":{"id":"24934","subject":"Why isn't builtin.h used by all builtin/*.c files?","startedAt":"2010-09-01T14:17:29Z","lastAt":"2010-09-01T17:43:19Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Stephen Boyd","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"149558","messageId":"AANLkTinHtvHt0q=dK9KHtCX6JzymB5dx1NuaAZN_uu2R@mail.gmail.com","threadId":"24934","inReplyTo":null,"subject":"Why isn't builtin.h used by all builtin/*.c files?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-01T14:17:29Z","receivedAt":"2010-09-01T14:17:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I'm working on gettextizing the 'mainporcelain common' commands now,\nand I ran into this:\n\n    $ perl -MArray::Diff -E 'chomp(my @hb = qx[ack -l builtin.h *.c]);\nmy @c = glob \"*.c\"; my $d = Array::Diff->diff( \\@c, \\@hb ); say for\n@{$d->deleted}'\n    fetch-pack.c\n    hash-object.c\n    index-pack.c\n    merge-index.c\n    merge-recursive.c\n    merge-tree.c\n    mktag.c\n    pack-redundant.c\n    pack-refs.c\n    patch-id.c\n    receive-pack.c\n    remote-ext.c\n    remote-fd.c\n    remote.c\n    reset.c\n    send-pack.c\n    skew.c\n    unpack-file.c\n    var.c\n\nThere seems to be no reason not to do this:\n\n    --- a/builtin/clone.c\n    +++ b/builtin/clone.c\n    @@ -10,3 +10,3 @@\n\n    -#include \"cache.h\"\n    +#include \"builtin.h\"\n     #include \"parse-options.h\"\n    @@ -18,3 +18,2 @@\n     #include \"transport.h\"\n    -#include \"strbuf.h\"\n     #include \"dir.h\"\n\nbuiltin.h even includes the cmd_clone() prototype. I'm only asking\nsince clone.c was added *after* builtin.h, so this seems like an\nomission.\n\nAnyway, I'll convert these to builtin.h where appropriate as part of\nthe series unless someone can find a good reason not to.\n"},{"id":"149562","messageId":"4C7E69A5.4070008@gmail.com","threadId":"24934","inReplyTo":"AANLkTinHtvHt0q=dK9KHtCX6JzymB5dx1NuaAZN_uu2R@mail.gmail.com","subject":"Re: Why isn't builtin.h used by all builtin/*.c files?","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-09-01T14:56:37Z","receivedAt":"2010-09-01T14:56:37Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":" On 09/01/2010 07:17 AM, Ævar Arnfjörð Bjarmason wrote:\n> I'm working on gettextizing the 'mainporcelain common' commands now,\n> and I ran into this:\n>\n[snip]\n>\n> Anyway, I'll convert these to builtin.h where appropriate as part of\n> the series unless someone can find a good reason not to.\n\nSparse warns about this too. \"Prototype not found, should it be static?\"\nfor a few cmd_*'s. So sounds good to me.\n"},{"id":"149571","messageId":"1283358207-3165-1-git-send-email-avarab@gmail.com","threadId":"24934","inReplyTo":"AANLkTinHtvHt0q=dK9KHtCX6JzymB5dx1NuaAZN_uu2R@mail.gmail.com","subject":"[PATCH] builtin: use builtin.h for all builtin/ commands","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-01T16:23:27Z","receivedAt":"2010-09-01T16:23:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some builtin/*.c commands weren't using the builtin.h header, but\ninstead manually imported headers like cache.h and commit.h which\nbuiltin.h would include for them.\n\nThis impeded my efforts to gettextize git, since I'd otherwise have to\nadd gettext.h to all of these, and using builtin.h is a good idea in\nany case, since it's defining the prototypes for the cmd_* functions\nthat these files define.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Wed, Sep 1, 2010 at 14:56, Stephen Boyd <bebarino@gmail.com> wrote:\n>  On 09/01/2010 07:17 AM, Ęvar Arnfjörš Bjarmason wrote:\n>> I'm working on gettextizing the 'mainporcelain common' commands now,\n>> and I ran into this:\n>>\n> [snip]\n>>\n>> Anyway, I'll convert these to builtin.h where appropriate as part of\n>> the series unless someone can find a good reason not to.\n>\n> Sparse warns about this too. \"Prototype not found, should it be static?\"\n> for a few cmd_*'s. So sounds good to me.\n\nThis is a patch against master for this issue. It also affects the\nskew.c and commands from the remote-ext series which are not on master.\n\nI made this with:\n\n    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\n\nWhere builtin-munger.pl is:\n    \n    #!/usr/bin/env perl\n    use 5.12.0;\n    use strict;\n    use autodie;\n    \n    my $f = shift;\n    \n    open my $out, \">\", \"$f.tmp\";\n    open my $in, \"<\", $f;\n    \n    while (<$in>) {\n        chomp;\n        say $out qq[#include \"builtin.h\"] if 1 .. /^#include/ and /^#include/;\n        say $out $_ unless $_ ~~ [\n            '#include \"git-compat-util.h\"',\n            '#include \"strbuf.h\"',\n            '#include \"cache.h\"',\n            '#include \"commit.h\"',\n            '#include \"notes.h\"',\n            '#include \"gettext.h\"',\n        ];\n    }\n    close $_ for $in, $out;\n    rename(\"$f.tmp\", $f);\n\n builtin/clone.c           |    3 +--\n builtin/fetch-pack.c      |    3 +--\n builtin/hash-object.c     |    2 +-\n builtin/index-pack.c      |    3 +--\n builtin/merge-index.c     |    2 +-\n builtin/merge-recursive.c |    3 +--\n builtin/merge-tree.c      |    2 +-\n builtin/mktag.c           |    2 +-\n builtin/pack-redundant.c  |    2 +-\n builtin/pack-refs.c       |    2 +-\n builtin/patch-id.c        |    2 +-\n builtin/receive-pack.c    |    3 +--\n builtin/remote.c          |    3 +--\n builtin/reset.c           |    3 +--\n builtin/send-pack.c       |    3 +--\n builtin/unpack-file.c     |    2 +-\n builtin/var.c             |    2 +-\n 17 files changed, 17 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex efb1e6f..f2a7a54 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -8,7 +8,7 @@\n  * Clone a repository into a different directory that does not yet exist.\n  */\n \n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"fetch-pack.h\"\n #include \"refs.h\"\n@@ -16,7 +16,6 @@\n #include \"tree-walk.h\"\n #include \"unpack-trees.h\"\n #include \"transport.h\"\n-#include \"strbuf.h\"\n #include \"dir.h\"\n #include \"pack-refs.h\"\n #include \"sigchain.h\"\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex dbd8b7b..03abc30 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1,7 +1,6 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n-#include \"commit.h\"\n #include \"tag.h\"\n #include \"exec_cmd.h\"\n #include \"pack.h\"\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 080af1a..8a5670f 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -4,7 +4,7 @@\n  * Copyright (C) Linus Torvalds, 2005\n  * Copyright (C) Junio C Hamano, 2005\n  */\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"blob.h\"\n #include \"quote.h\"\n #include \"parse-options.h\"\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex fad76bf..e073bdb 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1,9 +1,8 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"delta.h\"\n #include \"pack.h\"\n #include \"csum-file.h\"\n #include \"blob.h\"\n-#include \"commit.h\"\n #include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex 2c4cf5e..adc2a6d 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n \ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex d8875d5..455fd28 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -1,5 +1,4 @@\n-#include \"cache.h\"\n-#include \"commit.h\"\n+#include \"builtin.h\"\n #include \"tag.h\"\n #include \"merge-recursive.h\"\n \ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 9b25ddc..1991742 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"tree-walk.h\"\n #include \"xdiff-interface.h\"\n #include \"blob.h\"\ndiff --git a/builtin/mktag.c b/builtin/mktag.c\nindex 1cb0f3f..9148cc0 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"tag.h\"\n #include \"exec_cmd.h\"\n \ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 41e1615..a15e366 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -6,7 +6,7 @@\n *\n */\n \n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"exec_cmd.h\"\n \n #define BLKSIZE 512\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 091860b..39a9d89 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"pack-refs.h\"\n \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 5125300..33e9725 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"exec_cmd.h\"\n \n static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 760817d..f07b021 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1,11 +1,10 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"pack.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n-#include \"commit.h\"\n #include \"object.h\"\n #include \"remote.h\"\n #include \"transport.h\"\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 48e0a6b..3cf3c6b 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1,9 +1,8 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"transport.h\"\n #include \"remote.h\"\n #include \"string-list.h\"\n-#include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"refs.h\"\n \ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 1283068..3cbdf94 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -7,10 +7,9 @@\n  *\n  * Copyright (c) 2005, 2006 Linus Torvalds and Junio C Hamano\n  */\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"tag.h\"\n #include \"object.h\"\n-#include \"commit.h\"\n #include \"run-command.h\"\n #include \"refs.h\"\n #include \"diff.h\"\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..80b063b 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -1,5 +1,4 @@\n-#include \"cache.h\"\n-#include \"commit.h\"\n+#include \"builtin.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 608590a..c905d80 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -1,4 +1,4 @@\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"blob.h\"\n #include \"exec_cmd.h\"\n \ndiff --git a/builtin/var.c b/builtin/var.c\nindex 70fdb4d..0c18196 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -3,7 +3,7 @@\n  *\n  * Copyright (C) Eric Biederman, 2005\n  */\n-#include \"cache.h\"\n+#include \"builtin.h\"\n #include \"exec_cmd.h\"\n \n static const char var_usage[] = \"git var (-l | <variable>)\";\n-- \n1.7.2.2.178.gd8a94.dirty\n"},{"id":"149578","messageId":"7vvd6p5q0u.fsf@alter.siamese.dyndns.org","threadId":"24934","inReplyTo":"4C7E69A5.4070008@gmail.com","subject":"Re: Why isn't builtin.h used by all builtin/*.c files?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-01T17:31:45Z","receivedAt":"2010-09-01T17:31:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd <bebarino@gmail.com> writes:\n\n>  On 09/01/2010 07:17 AM, Ævar Arnfjörð Bjarmason wrote:\n>> I'm working on gettextizing the 'mainporcelain common' commands now,\n>> and I ran into this:\n>>\n> [snip]\n>>\n>> Anyway, I'll convert these to builtin.h where appropriate as part of\n>> the series unless someone can find a good reason not to.\n>\n> Sparse warns about this too. \"Prototype not found, should it be static?\"\n> for a few cmd_*'s. So sounds good to me.\n\nSounds good to me, too.\n"},{"id":"149580","messageId":"7vmxs15phk.fsf@alter.siamese.dyndns.org","threadId":"24934","inReplyTo":"1283358207-3165-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] builtin: use builtin.h for all builtin/ commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-01T17:43:19Z","receivedAt":"2010-09-01T17:43:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Some builtin/*.c commands weren't using the builtin.h header, but\n> instead manually imported headers like cache.h and commit.h which\n> builtin.h would include for them.\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index dbd8b7b..03abc30 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -1,7 +1,6 @@\n> -#include \"cache.h\"\n> +#include \"builtin.h\"\n>  #include \"refs.h\"\n>  #include \"pkt-line.h\"\n> -#include \"commit.h\"\n>  #include \"tag.h\"\n>  #include \"exec_cmd.h\"\n>  #include \"pack.h\"\n\nLeveraging the fact that builtin.h includes git-compat-util.h and cache.h\nis fine and I think strbuf is also generic and pervasive enough to be\nincluded in the mix, but in the longer term we would probably want to fix\nbuiltin.h not to include it itself does not need, notably commit.h and\nnotes.h.\n\nFor example, the header currently defines notes related stuff, such as\nnotes_rewrite_cfg parse_combine_notes_fn, etc.  Shouldn't they belong to\nnotes.h?  And if we lose them, do we still need to include commit.h and\nnotes.h in builtin.h?\n\nAnd when that happens, the inclusion of commit.h needs to come back to\nthis file, I think.\n"}]}