Add missing includes and forward declares
cover Add missing includes and forward declares
I was bit yet again by compilation errors when adding a #include of some header file to some new place, and decided to determine which header files were missing their own necessary #include's and forward declarations. This patch series is the result. A few notes:
* Patch 1: This patch is the bulk of the work; I included everything here except for special cases that need extra attention and the unrelated change in patch 2.
* Patch 2: This patch is more of a while-at-it independent header fixup; it only changes two lines so I lumped it into the series.
* Patch 3: I could have just added a #include of cache.h in branch.h instead, but opted to move things around slightly. I'm unsure which is better.
* Patches 4-5: Fix include guard issues
* Patches 6-8: These patches might need to be submitted to separate projects elsewhere. Let me know if so.
* Patch 9: This patch includes the changes that conflict with other topics in next and pu. I included these changes for completeness, but kept it separate because I wanted to make it easy to drop the stuff that conflicted with other things in flight (since I can just resubmit it later).
Elijah Newren (9):
Add missing includes and forward declares
alloc: make allocate_alloc_state and clear_alloc_state more consistent Move definition of enum branch_track from cache.h to branch.h urlmatch.h: fix include guard
compat/precompose_utf8.h: use more common include guard style ewah/ewok.h: add missing include
sha1dc/sha1.h: add missing include
xdiff/xdiff.h: add missing include
Add missing includes and forward declares
alloc.c | 2 +- alloc.h | 4 +++- apply.h | 3 +++ archive.h | 4 ++++ argv-array.h | 2 ++ attr.h | 1 + bisect.h | 2 ++ branch.h | 13 +++++++++++++ bulk-checkin.h | 2 ++ cache.h | 10 ---------- color.h | 2 ++ column.h | 1 + commit-graph.h | 1 + compat/precompose_utf8.h | 3 ++- config.c | 1 + config.h | 6 ++++++ connected.h | 1 + convert.h | 3 +++ credential.h | 1 + csum-file.h | 2 ++ delta.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ environment.c | 1 + ewah/ewok.h | 2 ++ exec-cmd.h | 2 ++ fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 4 ++++ grep.h | 1 + hashmap.h | 2 ++ khash.h | 4 ++++ kwset.h | 2 ++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 4 ++++ mem-pool.h | 2 ++ merge-recursive.h | 4 +++- notes-cache.h | 1 + notes-merge.h | 4 ++++ notes-utils.h | 4 ++++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ pack-objects.h | 1 + pack-revindex.h | 2 ++ parse-options.h | 2 ++ patch-ids.h | 6 ++++++ path.h | 3 +++ pathspec.h | 5 +++++ pretty.h | 4 ++++ progress.h | 2 ++ quote.h | 2 ++ reachable.h | 4 ++++ reflog-walk.h | 1 + refs.h | 4 ++++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ sha1-lookup.h | 2 ++ sha1dc/sha1.h | 1 + shortlog.h | 3 +++ sideband.h | 2 ++ strbuf.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 3 +++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ urlmatch.h | 3 +++ utf8.h | 4 ++++ varint.h | 2 ++ worktree.h | 1 + xdiff/xdiff.h | 2 ++ 82 files changed, 218 insertions(+), 17 deletions(-)
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 1/9 Add missing includes and forward declares
--- alloc.h | 2 ++ apply.h | 3 +++ archive.h | 4 ++++ argv-array.h | 2 ++ attr.h | 1 + branch.h | 2 ++ bulk-checkin.h | 2 ++ color.h | 2 ++ column.h | 1 + commit-graph.h | 1 + config.h | 6 ++++++ connected.h | 1 + convert.h | 3 +++ credential.h | 1 + csum-file.h | 2 ++ delta.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ exec-cmd.h | 2 ++ fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 4 ++++ grep.h | 1 + hashmap.h | 2 ++ khash.h | 4 ++++ kwset.h | 2 ++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 4 ++++ mem-pool.h | 2 ++ merge-recursive.h | 4 +++- notes-cache.h | 1 + notes-merge.h | 4 ++++ notes-utils.h | 4 ++++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ pack-revindex.h | 2 ++ parse-options.h | 2 ++ patch-ids.h | 6 ++++++ path.h | 3 +++ pathspec.h | 5 +++++ pretty.h | 4 ++++ progress.h | 2 ++ quote.h | 2 ++ reachable.h | 4 ++++ reflog-walk.h | 1 + refs.h | 4 ++++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ sha1-lookup.h | 2 ++ shortlog.h | 3 +++ sideband.h | 2 ++ strbuf.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 3 +++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ urlmatch.h | 1 + utf8.h | 4 ++++ varint.h | 2 ++ worktree.h | 1 + 72 files changed, 191 insertions(+), 4 deletions(-)
alloc.h +2 -0
@@ -1,9 +1,11 @@#ifndef ALLOC_H #define ALLOC_H +struct alloc_state ;struct tree ;struct commit ;struct tag ; +struct repository ;void *alloc_blob_node (struct repository *r);void *alloc_tree_node (struct repository *r);
apply.h +3 -0
@@ -1,6 +1,9 @@#ifndef APPLY_H #define APPLY_H +#include "lockfile.h" +#include "string-list.h" +enum apply_ws_error_action { nowarn_ws_error, warn_on_ws_error,
archive.h +4 -0
@@ -1,8 +1,12 @@ #ifndef ARCHIVE_H #define ARCHIVE_H +#include "git-compat-util.h" /* For size_t, timestamp_t, etc. */ #include "pathspec.h" +struct object_id; +enum object_type; + struct archiver_args { const char *base; size_t baselen;
argv-array.h +2 -0
@@ -1,6 +1,8 @@#ifndef ARGV_ARRAY_H #define ARGV_ARRAY_H +#include "git-compat-util.h" /* for LAST_ARG_MUST_BE_NULL */ +extern const char *empty_argv[]; struct argv_array {
attr.h +1 -0
@@ -7,6 +7,7 @@ struct git_attr;/* opaque structures used internally for attribute collection */ struct all_attrs_item ;struct attr_stack ; +struct index_state ;/* * Given a string, return the gitattribute object that
branch.h +2 -0
@@ -1,6 +1,8 @@#ifndef BRANCH_H #define BRANCH_H +struct strbuf ; +/* Functions for acting on the information about branches. */ /*
bulk-checkin.h +2 -0
@@ -4,6 +4,8 @@#ifndef BULK_CHECKIN_H #define BULK_CHECKIN_H +#include "cache.h" /* for object_type */ +extern int index_bulk_checkin (struct object_id *oid,int fd,size_t size,enum object_type type,const char *path,unsigned flags);
color.h +2 -0
@@ -1,6 +1,8 @@#ifndef COLOR_H #define COLOR_H +#include <stdio.h> +struct strbuf ;/*
column.h +1 -0
@@ -36,6 +36,7 @@ static inline int column_active(unsigned int colopts)return (colopts &COL_ENABLE_MASK ) ==COL_ENABLED ; } +struct string_list ;extern void print_columns (const struct string_list *list,unsigned int colopts,const struct column_options *opts);
commit-graph.h +1 -0
@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" /* for object_id */ struct commit ;
config.h +6 -0
@@ -1,6 +1,12 @@#ifndef CONFIG_H #define CONFIG_H +#include "git-compat-util.h" /* for size_t, off_t, timestamp_t, etc. */ +#include "hashmap.h" +#include "string-list.h" + +struct object_id ; +/* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1#define CONFIG_NO_SECTION_OR_NAME 2
connected.h +1 -0
@@ -1,6 +1,7 @@#ifndef CONNECTED_H #define CONNECTED_H +struct object_id ;struct transport ;/*
convert.h +3 -0
@@ -4,9 +4,12 @@#ifndef CONVERT_H #define CONVERT_H +#include <stdlib.h> /* for size_t */ #include "string-list.h" struct index_state ; +struct object_id ; +struct strbuf ;#define CONV_EOL_RNDTRP_DIE (1<<0)/* Die if CRLF to LF to CRLF is different */ #define CONV_EOL_RNDTRP_WARN (1<<1)/* Warn if CRLF to LF to CRLF is different */
credential.h +1 -0
@@ -1,6 +1,7 @@#ifndef CREDENTIAL_H #define CREDENTIAL_H +#include <stdio.h> #include "string-list.h" struct credential {
csum-file.h +2 -0
@@ -1,6 +1,8 @@#ifndef CSUM_FILE_H #define CSUM_FILE_H +#include "hash.h" +struct progress ;/* A SHA1-protected file */
delta.h +2 -0
@@ -1,6 +1,8 @@#ifndef DELTA_H #define DELTA_H +#include <stdlib.h> /* for NULL */ +/* opaque object for delta index */ struct delta_index ;
diffcore.h +4 -0
@@ -4,6 +4,10 @@#ifndef DIFFCORE_H #define DIFFCORE_H +#include "cache.h" /* for object_id */ + +struct diff_options ; +/* This header file is internal between diff.c and its diff transformers * (e.g. diffcore-rename, diffcore-pickaxe). Never include this header * in anything else.
dir-iterator.h +2 -0
@@ -1,6 +1,8 @@#ifndef DIR_ITERATOR_H #define DIR_ITERATOR_H +#include "strbuf.h" +/* * Iterate over a directory tree. *
exec-cmd.h +2 -0
@@ -1,6 +1,8 @@#ifndef GIT_EXEC_CMD_H #define GIT_EXEC_CMD_H +#include "git-compat-util.h" /* for LAST_ARG_MUST_BE_NULL */ +struct argv_array ;extern void git_set_exec_path (const char *exec_path);
fsck.h +1 -0
@@ -6,6 +6,7 @@#define FSCK_IGNORE 3struct fsck_options ; +struct object ;void fsck_set_msg_type (struct fsck_options *options,const char *msg_id,const char *msg_type);
fsmonitor.h +7 -0
@@ -1,6 +1,13 @@#ifndef FSMONITOR_H #define FSMONITOR_H +#include "cache.h" /* for core_fsmonitor */ +#include "dir.h" /* for untracked_cache_invalidate_path */ + +struct cache_entry ; +struct index_state ; +struct strbuf ; +extern struct trace_key trace_fsmonitor;/*
gpg-interface.h +4 -0
@@ -1,6 +1,10 @@#ifndef GPG_INTERFACE_H #define GPG_INTERFACE_H +#include <stdlib.h> + +struct strbuf ; +#define GPG_VERIFY_VERBOSE 1#define GPG_VERIFY_RAW 2#define GPG_VERIFY_OMIT_STATUS 4
grep.h +1 -0
@@ -35,6 +35,7 @@ typedef int pcre2_jit_stack;#include "kwset.h" #include "thread-utils.h" #include "userdiff.h" +#include "git-compat-util.h" /* for regex_t and uint32_t */ enum grep_pat_token {GREP_PATTERN ,
hashmap.h +2 -0
@@ -1,6 +1,8 @@#ifndef HASHMAP_H #define HASHMAP_H +#include "git-compat-util.h" /* for BUG, size_t, memcpy, etc. */ +/* * Generic implementation of hash-based key-value mappings. *
khash.h +4 -0
@@ -26,6 +26,10 @@#ifndef __AC_KHASH_H#define __AC_KHASH_H +#include "git-compat-util.h" /* for xcalloc, etc. */ +#include "hashmap.h" /* for sha1hash */ +#include "cache.h" /* for hashcmp */ +#define AC_VERSION_KHASH_H "0.2.8"typedef uint32_t khint32_t ;
kwset.h +2 -0
@@ -3,6 +3,8 @@* Git. */ +#include <stdlib.h> +/* kwset.h - header declaring the keyword set library. Copyright (C) 1989, 1998, 2005 Free Software Foundation, Inc.
list-objects-filter.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_FILTER_H #define LIST_OBJECTS_FILTER_H +struct list_objects_filter_options ; +struct object ; +struct oidset ; +/* * During list-object traversal we allow certain objects to be * filtered (omitted) from the result. The active filter uses
list-objects.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_H #define LIST_OBJECTS_H +struct commit ; +struct object ; +struct rev_info ; +typedef void (*show_commit_fn )(struct commit *,void *);typedef void (*show_object_fn )(struct object *,const char *,void *);void traverse_commit_list (struct rev_info *,show_commit_fn ,show_object_fn ,void *);
ll-merge.h +2 -0
@@ -5,6 +5,8 @@ #ifndef LL_MERGE_H #define LL_MERGE_H +#include "xdiff/xdiff.h" /* for mmbuffer_t, mmfile_t */ + struct ll_merge_options { unsigned virtual_ancestor : 1; unsigned variant : 2; /* favor ours, favor theirs, or union merge */
mailinfo.h +2 -0
@@ -1,6 +1,8 @@#ifndef MAILINFO_H #define MAILINFO_H +#include "strbuf.h" +#define MAX_BOUNDARIES 5 struct mailinfo {
mailmap.h +4 -0
@@ -1,6 +1,10 @@#ifndef MAILMAP_H #define MAILMAP_H +#include <stdlib.h> + +struct string_list ; +int read_mailmap (struct string_list *map,char **repo_abbrev);void clear_mailmap (struct string_list *map);
mem-pool.h +2 -0
@@ -1,6 +1,8 @@ #ifndef MEM_POOL_H #define MEM_POOL_H +#include "git-compat-util.h" /* for FLEX_ARRAY, etc. */ + struct mp_block { struct mp_block *next_block; char *next_free;
merge-recursive.h +3 -1
@@ -1,8 +1,10 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H -#include "unpack-trees.h" #include "string-list.h" +#include "unpack-trees.h" + +struct commit; struct merge_options { const char *ancestor;
notes-cache.h +1 -0
@@ -2,6 +2,7 @@#define NOTES_CACHE_H #include "notes.h" +#include <stdlib.h> struct notes_cache {struct notes_tree tree;
notes-merge.h +4 -0
@@ -2,6 +2,10 @@#define NOTES_MERGE_H #include "notes-utils.h" +#include "strbuf.h" + +struct commit ; +struct object_id ;#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
notes-utils.h +4 -0
@@ -2,6 +2,10 @@#define NOTES_UTILS_H #include "notes.h" +#include <stdlib.h> + +struct commit_list ; +struct object_id ;/* * Create new notes commit from the given notes tree
notes.h +3 -0
@@ -3,6 +3,9 @@#include "string-list.h" +struct object_id ; +struct strbuf ; +/* * Function type for combining two notes annotating the same object. *
object-store.h +1 -0
@@ -1,6 +1,7 @@#ifndef OBJECT_STORE_H #define OBJECT_STORE_H +#include "cache.h" /* for object_id */ #include "oidmap.h" #include "list.h" #include "sha1-array.h"
object.h +2 -0
@@ -1,6 +1,8 @@#ifndef OBJECT_H #define OBJECT_H +#include "cache.h" /* For TYPE_BITS */ +struct buffer_slab ; struct parsed_object_pool {
oidmap.h +1 -0
@@ -1,6 +1,7 @@#ifndef OIDMAP_H #define OIDMAP_H +#include "cache.h" /* for object_id */ #include "hashmap.h" /*
pack-bitmap.h +3 -0
@@ -5,6 +5,9 @@#include "khash.h" #include "pack-objects.h" +struct commit ; +struct rev_info ; +struct bitmap_disk_header {char magic[4 ];uint16_t version;
pack-revindex.h +2 -0
@@ -1,6 +1,8 @@#ifndef PACK_REVINDEX_H #define PACK_REVINDEX_H +#include "git-compat-util.h" /* for off_t */ +struct packed_git ; struct revindex_entry {
parse-options.h +2 -0
@@ -1,6 +1,8 @@#ifndef PARSE_OPTIONS_H #define PARSE_OPTIONS_H +#include "git-compat-util.h" /* for NORETURN, intptr_t */ +enum parse_opt_type {/* special types */ OPTION_END,
patch-ids.h +6 -0
@@ -1,6 +1,12 @@ #ifndef PATCH_IDS_H #define PATCH_IDS_H +#include "diff.h" +#include "hashmap.h" + +struct commit; +struct object_id; + struct patch_id { struct hashmap_entry ent; struct object_id patch_id;
path.h +3 -0
@@ -1,7 +1,10 @@#ifndef PATH_H #define PATH_H +#include <stdlib.h> +struct repository ; +struct strbuf ;/* * The result to all functions which return statically allocated memory may be
pathspec.h +5 -0
@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" /* For str[n]cmp */ +#include "strings.h" /* For str[n]casecmp */ + +struct index_state ; +/* Pathspec magic */ #define PATHSPEC_FROMTOP (1<<0)#define PATHSPEC_MAXDEPTH (1<<1)
pretty.h +4 -0
@@ -1,7 +1,11 @@#ifndef PRETTY_H #define PRETTY_H +#include "cache.h" /* for date_mode */ +#include "string-list.h" +struct commit ; +struct strbuf ;/* Commit formats */ enum cmit_fmt {
progress.h +2 -0
@@ -1,6 +1,8 @@#ifndef PROGRESS_H #define PROGRESS_H +#include "git-compat-util.h" /* for uint64_t */ +struct progress ;void display_throughput (struct progress *progress,uint64_t total);
quote.h +2 -0
@@ -1,6 +1,8 @@#ifndef QUOTE_H #define QUOTE_H +#include <stdio.h> +struct strbuf ;/* Help to copy the thing properly quoted for the shell safety.
reachable.h +4 -0
@@ -1,7 +1,11 @@#ifndef REACHEABLE_H #define REACHEABLE_H +#include "git-compat-util.h" /* for timestamp_t */ +struct progress ; +struct rev_info ; +extern int add_unseen_recent_objects_to_traversal (struct rev_info *revs,timestamp_t timestamp); extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
reflog-walk.h +1 -0
@@ -3,6 +3,7 @@#include "cache.h" +struct commit ;struct reflog_walk_info ;extern void init_reflog_walk (struct reflog_walk_info **info);
refs.h +4 -0
@@ -1,10 +1,14 @@#ifndef REFS_H #define REFS_H +#include "git-compat-util.h" /* for timestamp_t, strpbrk from string.h, etc. */ +struct object_id ;struct ref_store ; +struct repository ;struct strbuf ;struct string_list ; +struct string_list_item ;struct worktree ;/*
remote.h +1 -0
@@ -1,6 +1,7 @@#ifndef REMOTE_H #define REMOTE_H +#include "cache.h" /* for object_id */ #include "parse-options.h" #include "hashmap.h" #include "refspec.h"
repository.h +2 -0
@@ -1,6 +1,8 @@#ifndef REPOSITORY_H #define REPOSITORY_H +#include "path.h" /* for path_cache */ +struct config_set ;struct git_hash_algo ;struct index_state ;
resolve-undo.h +2 -0
@@ -1,6 +1,8 @@ #ifndef RESOLVE_UNDO_H #define RESOLVE_UNDO_H +#include "cache.h" /* for object_id...and a bunch of other stuff */ + struct resolve_undo_info { unsigned int mode[3]; struct object_id oid[3];
revision.h +1 -0
@@ -1,6 +1,7 @@#ifndef REVISION_H #define REVISION_H +#include "commit.h" /* for rev_sort_order */ #include "parse-options.h" #include "grep.h" #include "notes.h"
send-pack.h +4 -0
@@ -3,6 +3,10 @@#include "string-list.h" +struct child_process ; +struct oid_array ; +struct ref ; +/* Possible values for push_cert field in send_pack_args. */ #define SEND_PACK_PUSH_CERT_NEVER 0#define SEND_PACK_PUSH_CERT_IF_ASKED 1
sequencer.h +5 -0
@@ -1,6 +1,11 @@#ifndef SEQUENCER_H #define SEQUENCER_H +#include "cache.h" /* for object_id */ +#include "strbuf.h" + +struct commit ; +const char *git_path_commit_editmsg (void );const char *git_path_seq_dir (void );
sha1-lookup.h +2 -0
@@ -1,6 +1,8 @@#ifndef SHA1_LOOKUP_H #define SHA1_LOOKUP_H +#include "git-compat-util.h" /* for uint32_t, etc. */ +typedef const unsigned char *sha1_access_fn (size_t index,void *table); extern int sha1_pos(const unsigned char *sha1,
shortlog.h +3 -0
@@ -2,6 +2,9 @@#define SHORTLOG_H #include "string-list.h" +#include <stdio.h> + +struct commit ;struct shortlog {struct string_list list;
sideband.h +2 -0
@@ -1,6 +1,8 @@#ifndef SIDEBAND_H #define SIDEBAND_H +#include "git-compat-util.h" /* for ssize_t */ +#define SIDEBAND_PROTOCOL_ERROR -2#define SIDEBAND_REMOTE_ERROR -1
strbuf.h +2 -0
@@ -1,6 +1,8 @@#ifndef STRBUF_H #define STRBUF_H +#include "git-compat-util.h" /* for size_t, SWAP, strlen, off_t, etc. */ +struct string_list ;/**
submodule.h +8 -2
@@ -1,11 +1,17 @@#ifndef SUBMODULE_H #define SUBMODULE_H -struct repository ; -struct diff_options ;struct argv_array ; +struct cache_entry ; +struct diff_options ; +struct index_state ; +struct object_id ;struct oid_array ; +struct pathspec ;struct remote ; +struct repository ; +struct string_list ; +struct strbuf ;enum { RECURSE_SUBMODULES_ONLY = -5,
tempfile.h +1 -0
@@ -2,6 +2,7 @@#define TEMPFILE_H #include "list.h" +#include "strbuf.h" /* * Handle temporary files.
trailer.h +3 -0
@@ -2,6 +2,9 @@#define TRAILER_H #include "list.h" +#include <stdlib.h> + +struct strbuf ;enum trailer_where {WHERE_DEFAULT ,
tree-walk.h +2 -0
@@ -1,6 +1,8 @@ #ifndef TREE_WALK_H #define TREE_WALK_H +struct strbuf; + struct name_entry { const struct object_id *oid; const char *path;
unpack-trees.h +4 -1
@@ -1,11 +1,14 @@#ifndef UNPACK_TREES_H #define UNPACK_TREES_H -#include "tree-walk.h" #include "argv-array.h" +#include "cache.h" /* for index_state */ +#include "string-list.h" +#include "tree-walk.h" #define MAX_UNPACK_TREES 8 +struct cache_entry ;struct unpack_trees_options ;struct exclude_list ;
url.h +2 -0
@@ -1,6 +1,8 @@#ifndef URL_H #define URL_H +struct strbuf ; +extern int is_url (const char *url);extern int is_urlschemechar (int first_flag,int ch);extern char *url_decode (const char *url);
urlmatch.h +1 -0
@@ -1,5 +1,6 @@#ifndef URL_MATCH_H #include "string-list.h" +#include <stdlib.h> struct url_info {/* normalized url on success, must be freed, otherwise NULL */
utf8.h +4 -0
@@ -1,6 +1,10 @@#ifndef GIT_UTF8_H #define GIT_UTF8_H +#include "git-compat-util.h" /* for iconv_t */ + +struct strbuf ; +typedef unsigned int ucs_char_t ;/* assuming 32bit int */ size_t display_mode_esc_sequence_len (const char *s);
varint.h +2 -0
@@ -1,6 +1,8 @@#ifndef VARINT_H #define VARINT_H +#include "git-compat-util.h" /* for uintmax_t */ +extern int encode_varint (uintmax_t ,unsigned char *);extern uintmax_t decode_varint (const unsigned char **);
worktree.h +1 -0
@@ -1,6 +1,7 @@#ifndef WORKTREE_H #define WORKTREE_H +#include "cache.h" /* for object_id */ #include "refs.h" struct strbuf ;
Signature · 1 line
2.18.0.549.gd4454f3f9b
argv-array.h line 4
Quoted 9 lines · Elijah Newren
argv-array.h +1
-0@@ -1,6 +1,8 @@#ifndef ARGV_ARRAY_H #define ARGV_ARRAY_H +#include "git-compat-util.h" /* for LAST_ARG_MUST_BE_NULL */
Following up on what I said in the other message, I think we should omit this. All header files should assume the environment provided by git-compat-util.h is already loaded (and all non-compat C files should make sure they load it first).
color.h line 4
Quoted 9 lines
color.h +1
-0@@ -1,6 +1,8 @@#ifndef COLOR_H #define COLOR_H +#include <stdio.h>
This one is more actively wrong than the git-compat-util.h one. Anybody who already included git-compat-util.h would not be helped by this. And anybody who _didn't_ may be hurt, because now git-compat-util.h might run into ordering problems caused by headers already loaded as part of stdio.h (and keep in mind that stdio.h often brings in other system-dependent definitions, too).
bulk-checkin.h line 7
Quoted 9 lines
bulk-checkin.h +1
-0@@ -4,6 +4,8 @@#ifndef BULK_CHECKIN_H #define BULK_CHECKIN_H +#include "cache.h" /* for object_type */
As I said earlier, I don't mind includes like these that make everybody's lives a little easier, and shouldn't introduce any funny ordering constraints.
IMHO, though, comments like "for X" are probably a bad idea. That may be accurate _now_, but it is very likely to grow stale without anybody noticing. You only care about that "X" if you are thinking about removing the include, and then the right answer is not to trust the comment, but to remove the include and make sure there is no other breakage.
The one exception is if there is some subtlety, like including a header file that uses macros to replace a function, and removing it wouldn't break compilation but produce a different result. But then:
- I'd hope we don't have anything like that, because it sounds horrible ;)
- you'd probably want a much more explanatory comment
Quoted 1 line
[...]
The rest of them are all variants on these three (or forward declarations, which I put in the same boat as this third recursive include; i.e., it's a good change).
-Peff
PATCH 2/9 alloc: make allocate_alloc_state and clear_alloc_state more consistent
In reply to Elijah NewrenSince both functions are using the same data type, they should either both refer to it as void *, or both use the real type (struct alloc_state *). Opt for the latter.
--- alloc.c | 2 +- alloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
alloc.c +1 -1
@@ -36,7 +36,7 @@ struct alloc_state {int slab_nr, slab_alloc; }; -void *allocate_alloc_state (void ) +struct alloc_state *allocate_alloc_state (void ) {return xcalloc (1 ,sizeof (struct alloc_state )); }
alloc.h +1 -1
@@ -15,7 +15,7 @@ void *alloc_object_node(struct repository *r);void alloc_report (struct repository *r);unsigned int alloc_commit_index (struct repository *r); -void *allocate_alloc_state (void ); +struct alloc_state *allocate_alloc_state (void );void clear_alloc_state (struct alloc_state *s);#endif
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 3/9 Move definition of enum branch_track from cache.h to branch.h
In reply to Elijah Newren'branch_track' feels more closely related to branching, and it is needed later in branch.h; rather than #include'ing cache.h in branch.h for this small enum, just move the enum and the external declaration for git_branch_track to branch.h.
--- branch.h | 11 +++++++++++ cache.h | 10 ---------- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-)
branch.h +11 -0
@@ -3,6 +3,17 @@struct strbuf ; +enum branch_track { +BRANCH_TRACK_UNSPECIFIED =-1 , +BRANCH_TRACK_NEVER =0 , +BRANCH_TRACK_REMOTE , +BRANCH_TRACK_ALWAYS , +BRANCH_TRACK_EXPLICIT , +BRANCH_TRACK_OVERRIDE +}; + +extern enum branch_track git_branch_track; +/* Functions for acting on the information about branches. */ /*
cache.h +0 -10
@@ -919,15 +919,6 @@ enum log_refs_config { };extern enum log_refs_config log_all_ref_updates; -enum branch_track { -BRANCH_TRACK_UNSPECIFIED =-1 , -BRANCH_TRACK_NEVER =0 , -BRANCH_TRACK_REMOTE , -BRANCH_TRACK_ALWAYS , -BRANCH_TRACK_EXPLICIT , -BRANCH_TRACK_OVERRIDE -}; -enum rebase_setup_type {AUTOREBASE_NEVER =0 ,AUTOREBASE_LOCAL ,
@@ -944,7 +935,6 @@ enum push_default_type { PUSH_DEFAULT_UNSPECIFIED }; -extern enum branch_track git_branch_track;extern enum rebase_setup_type autorebase;extern enum push_default_type push_default;
config.c +1 -0
@@ -5,6 +5,7 @@* Copyright (C) Johannes Schindelin, 2005 * */ +#include "branch.h" #include "cache.h" #include "config.h" #include "repository.h"
environment.c +1 -0
@@ -7,6 +7,7 @@* even if you might want to know where the git directory etc * are. */ +#include "branch.h" #include "cache.h" #include "repository.h" #include "config.h"
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 4/9 urlmatch.h: fix include guard
In reply to Elijah Newren--- urlmatch.h | 2 ++ 1 file changed, 2 insertions(+)
urlmatch.h +2 -0
@@ -1,4 +1,6 @@#ifndef URL_MATCH_H +#define URL_MATCH_H +#include "string-list.h" #include <stdlib.h>
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 5/9 compat/precompose_utf8.h: use more common include guard style
In reply to Elijah Newren--- compat/precompose_utf8.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
compat/precompose_utf8.h +2 -1
@@ -1,4 +1,6 @@#ifndef PRECOMPOSE_UNICODE_H +#define PRECOMPOSE_UNICODE_H +#include <sys/stat.h> #include <sys/types.h> #include <dirent.h>
@@ -41,5 +43,4 @@ int precompose_utf8_closedir(PREC_DIR *dirp);#define DIR PREC_DIR#endif /* PRECOMPOSE_UNICODE_C */ -#define PRECOMPOSE_UNICODE_H #endif /* PRECOMPOSE_UNICODE_H */
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 6/9 ewah/ewok.h: add missing include
In reply to Elijah Newren--- ewah/ewok.h | 2 ++ 1 file changed, 2 insertions(+)
ewah/ewok.h +2 -0
@@ -19,6 +19,8 @@#ifndef __EWOK_BITMAP_H__#define __EWOK_BITMAP_H__ +#include "../git-compat-util.h" /* for uint64_t, uint32_t, etc. */ +struct strbuf ;typedef uint64_t eword_t ;#define BITS_IN_EWORD (sizeof(eword_t) * 8)
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 7/9 sha1dc/sha1.h: add missing include
In reply to Elijah Newren--- sha1dc/sha1.h | 1 + 1 file changed, 1 insertion(+)
sha1dc/sha1.h +1 -0
@@ -14,6 +14,7 @@ extern "C" {#ifndef SHA1DC_NO_STANDARD_INCLUDES #include <stdint.h> +#include <stdlib.h> #endif /* sha-1 compression function that takes an already expanded message, and additionally store intermediate states */
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 8/9 xdiff/xdiff.h: add missing include
In reply to Elijah Newren--- xdiff/xdiff.h | 2 ++ 1 file changed, 2 insertions(+)
xdiff/xdiff.h +2 -0
@@ -27,6 +27,8 @@extern "C" {#endif /* #ifdef __cplusplus */ +#include <stdlib.h> /* for size_t */ +/* xpparm_t.flags */ #define XDF_NEED_MINIMAL (1 << 0)
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH 9/9 Add missing includes and forward declares
In reply to Elijah Newren--- bisect.h | 2 ++ pack-objects.h | 1 + 2 files changed, 3 insertions(+)
bisect.h +2 -0
@@ -1,6 +1,8 @@#ifndef BISECT_H #define BISECT_H +struct commit_list ; +/* * Find bisection. If something is found, `reaches` will be the number of * commits that the best commit reaches. `all` will be the count of
pack-objects.h +1 -0
@@ -2,6 +2,7 @@#define PACK_OBJECTS_H #include "object-store.h" +#include "pack.h" /* for pack_idx_entry */ #define DEFAULT_DELTA_CACHE_SIZE (256 * 1024 * 1024)
Signature · 1 line
2.18.0.549.gd4454f3f9b
On Sat, Aug 11 2018, Elijah Newren wrote:
[CC'd sha1dc maintainers, for context the relevent patch is
https://public-inbox.org/git/[email protected]/T/#u]
Quoted 2 lines
* Patches 6-8: These patches might need to be submitted to separate projects elsewhere. Let me know if so.
Yes, for sha1dc (your 7/9) it's much better if we submit patches to upstream and then submit a patch to git.git to update from upstream, as my 23e37f8e9d ("sha1dc: update from upstream", 2018-08-02) sitting in 'next' does.
When we build that library we define SHA1DC_NO_STANDARD_INCLUDES, so we don't use the codepath you patched, so your description of "I was bit yet again by compilation errors[...]" doesn't add up in that case. I assume you just started adding stdlib.h where you could grep for stdint.h.
When I check out sha1collisiondetection.git stand-alone (which will use that path) and compile it, it works fine for me. This is on GCC 8.1.0 on Debian, so perhaps that patch isn't needed in that case.
On Sat, Aug 11, 2018 at 1:30 AM Ævar Arnfjörð Bjarmason <[email protected]> wrote
Quoted 13 lines · Elijah Newren
[CC'd sha1dc maintainers, for context the relevent patch is
https://public-inbox.org/git/[email protected]/T/#u]Quoted 2 lines
* Patches 6-8: These patches might need to be submitted to separate projects elsewhere. Let me know if so.
Yes, for sha1dc (your 7/9) it's much better if we submit patches to upstream and then submit a patch to git.git to update from upstream, as my
23e37f8e9d("sha1dc: update from upstream", 2018-08-02) sitting in 'next' does.
Quoted 5 lines
When we build that library we define
SHA1DC_NO_STANDARD_INCLUDES, so we don't use the codepath you patched, so your description of "I was bit yet again by compilation errors[...]" doesn't add up in that case. I assume you just started addingstdlib.hwhere you could grep forstdint.h.
The part of my story you snipped in the ellipsis is kind of important,
though: "...and decided to determine which header files were missing their own necessary #include's and forward declarations." The way I did so was making a simple one-line .c file that included exactly one header, and checked to see if I could compile it (without any special defines), fixed it up as necessary, then repeated that process for every toplevel header.
Most of the stdlib.h additions were for the definition of size_t, sha1dc/sha1.h was no different; without my patch:
In file included from nukeme.c:1:0:sha1dc/sha1.h:96:43: error: unknown type name ‘size_t’
void SHA1DCUpdate(SHA1_CTX*, const char*, size_t);
Your point that we use SHA1DC_NO_STANDARD_INCLUDES normally when compiling within git, though, does mean my patch #7 is kind of useless to git.
Quoted 3 lines
When I check out sha1collisiondetection.git stand-alone (which will use that path) and compile it, it works fine for me. This is on GCC 8.1.0 on Debian, so perhaps that patch isn't needed in that case.
I never said git or its subprojects failed to compile. I said when I added another #include of some header to some existing or new files, I sometimes saw unknown symbol errors. It's happened before, it happened again today, and I got annoyed, so I set out to address it. Sorry if that wasn't clear.
Quoted 7 lines · Elijah Newren
The part of my story you snipped in the ellipsis is kind of important,
though: "...and decided to determine which header files were missing their own necessary #include's and forward declarations." The way I did so was making a simple one-line .c file that included exactly one header, and checked to see if I could compile it (without any special defines), fixed it up as necessary, then repeated that process for every toplevel header.
The rule in Git has always been that your very first include must always be "git-compat-util.h" or an equivalent header that includes it first (like "cache.h"). This is mentioned in CodingGuidelines.
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_check
And I'd be fine with fixing any compilation failures there, either with forward declarations or recursive includes. I think the in the early days there was some resistance to having a lot of recursive includes, because it _can_ lead to confusion, but IMHO it mostly helps people. And I don't recall much discussion on it in recent years.
Including "git-compat-util.h" in many more headers probably doesn't hurt (after all, it's a noop for every .c file which is following the existing rule). But I'd just as soon not sprinkle it everywhere, nor imply that that people don't need to be including it. It's really important that it comes first because it wants a clean slate and have subtle effects on other header files due to macros. Worse still, some of the effects are platform dependent, so we might not realize an ordering problem until somebody on AIX complains.
I wonder if there's an easy way to check. I guess adding '-Dint=foo' to the command line, and then putting '#undef int' at the top of git-compat-util would catch just about any code the compiler sees that doesn't have git-compat-util included. :)
-Peff
Quoted 3 lines · Jeff King
The rule in Git has always been that your very first include must always be "
git-compat-util.h" or an equivalent header that includes it first (like "cache.h"). This is mentioned in CodingGuidelines.
Glad to see that you already have written the above so I don't have to ;-)
As things are slowly moving out of the so-far kitchen-sink "cache.h" into more specific subsystem headers (like object-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grep cache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.
Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_check
But until that tightening happens, I do not actually mind the two-line .c file started with inclusion of cache.h instead of git-compat-util.h. That would limit the scope of this series further.
Quoted 4 lines
I wonder if there's an easy way to check. I guess adding '-Dint=foo' to the command line, and then putting '#undef int' at the top of git-compat-util would catch just about any code the compiler sees that doesn't have git-compat-util included. :)
;-).
Quoted 11 lines · Junio C Hamano
Jeff King <[email protected]> writes:
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.Quoted 2 lines
So I think the better test is a two-line .c file with:
Quoted 7 lines
Quoted 2 lines
#include "git-compat-util.h" #include $header_to_checkBut until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.
Yes, this removes about 2/3 of patch #1. But it makes things kind of odd to me; do we change the relevant paragraph of Documentation/CodingGuidelines to add an additional sentence so it reads:
- The first #include in C files, except in platform specific compat/ implementations, must be either "git-compat-util.h", "cache.h" or "builtin.h". You do not have to include more than one of these. However, note that including any of a few dozen header files may result in compilation failures if cache.h is not included first.
That seems annoying to state, but also annoying to leave undocumented. I'd rather have the various structs (object_id, strbuf, index_state, cache_entry, etc.) forward declared and/or relevant headers (e.g. "cache.h", "strbuf.h", or "string-list.h") #include'd. Are you sure you really want me to remove these fixes? Let me know, and if so I'll do it, even if I'd prefer to leave them in. Let me know how/if you'd like that CodingGuidline to be updated too.
Also sha1dc_git.h fails to compile if cache.h is #included before it (various redefinition errors, such as for "platform_SHA_CTX"); it's possible that's expected, but it does make things a little weirder to add implicit assumptions that cache.h is included first.
Quoted 15 lines · Elijah Newren
Quoted 14 lines · Junio C Hamano
Jeff King <[email protected]> writes:
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_check
Quoted 6 lines
Quoted 4 lines
But until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.Yes, this removes about 2/3 of patch #1.
Sorry for making a misleading comment. I should have phrased "I would not have minded if the series were looser by assuming cache.h", implying that "but now the actual patch went extra mile to be more complete, what we have is even better ;-)".
Quoted 22 lines · Junio C Hamano
Quoted 20 lines · Elijah Newren
Quoted 18 lines · Junio C Hamano
Jeff King <[email protected]> writes:
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_checkBut until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.
Quoted 6 lines
Quoted 1 line
Yes, this removes about 2/3 of patch #1.
Sorry for making a misleading comment. I should have phrased "I would not have minded if the series were looser by assuming
cache.h", implying that "but now the actual patch went extra mile to be more complete, what we have is even better ;-)".
Ah, gotcha. Thanks for the clarification.
Quoted 23 lines · Elijah Newren
Quoted 22 lines · Junio C Hamano
Quoted 20 lines · Elijah Newren
Quoted 18 lines · Junio C Hamano
Jeff King <[email protected]> writes:
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_checkBut until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Yes, this removes about 2/3 of patch #1.
Sorry for making a misleading comment. I should have phrased "I would not have minded if the series were looser by assuming
cache.h", implying that "but now the actual patch went extra mile to be more complete, what we have is even better ;-)".Ah, gotcha. Thanks for the clarification.
But please remind me not to merge this round down to 'next', for the "enum" forward decl gotcha.
Quoted 32 lines · Junio C Hamano
Quoted 30 lines · Elijah Newren
Quoted 28 lines · Junio C Hamano
Quoted 15 lines · Elijah Newren
Quoted 14 lines · Junio C Hamano
Jeff King <[email protected]> writes:
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_checkQuoted 6 lines
Quoted 4 lines
But until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.Yes, this removes about 2/3 of patch #1.
Sorry for making a misleading comment. I should have phrased "I would not have minded if the series were looser by assuming
cache.h", implying that "but now the actual patch went extra mile to be more complete, what we have is even better ;-)".
Quoted 4 lines
Quoted 1 line
Ah, gotcha. Thanks for the clarification.
But please remind me not to merge this round down to 'next', for the "enum" forward decl gotcha.
I'll send out a new round shortly. Would you like me to squash the last patch (the one that had two hunks with minor conflicts with other topics in next and pu) into the first patch, or would you rather I dropped that patch and waited to submit it until later?
Quoted 8 lines · Elijah Newren
Quoted 3 lines
But please remind me not to merge this round down to 'next', for the "enum" forward decl gotcha.
I'll send out a new round shortly. Would you like me to squash the last patch (the one that had two hunks with minor conflicts with other topics in next and pu) into the first patch, or would you rather I dropped that patch and waited to submit it until later?
Either way is fine, especially if you looked at the intergration result from yesterday and resolution of these conflicts looked reasonable to you.
Quoted 6 lines · Junio C Hamano
As things are slowly moving out of the so-far kitchen-sink "
cache.h" into more specific subsystem headers (likeobject-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grepcache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around.
Yeah, I agree that's a good long-term goal. I think "builtin.h" is reasonable to remain as a magic header for builtins.
Quoted 9 lines
Quoted 4 lines
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_checkBut until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.
I can go either way on this. Using cache.h makes Elijah's current series a bit more focused. But I think we'd eventually want to go there anyway. I don't have a strong opinion on doing it now or later.
Quoted 6 lines
Quoted 4 lines
I wonder if there's an easy way to check. I guess adding '-Dint=foo' to the command line, and then putting '#undef int' at the top of git-compat-util would catch just about any code the compiler sees that doesn't have git-compat-util included. :)
;-).
So much to my amazement, my off-the-cuff suggestion actually worked pretty well. The only failures were xdiff (expected, since it does its own system-header stuff, though IMHO we might think about changing that) and the programs in t/helper. Here's a patch to address the latter (and you can see now why I said the above thing about "builtin.h"):
-- >8 --
Subject: [PATCH] test-tool.h: include git-compat-util.h
The test-tool programs include "test-tool.h" as their first include, which breaks our CodingGuideline of "the first include must be git-compat-util.h or an equivalent". This isn't actually a problem, as test-tool.h doesn't define anything tricky, but we should probably follow our own rule.
Rather than change them all, let's instead make test-tool.h one of those equivalents, just like we do for builtin.h (which many of the actual git builtins include first).
--- t/helper/test-tool.h | 2 ++ 1 file changed, 2 insertions(+)
t/helper/test-tool.h +2 -0
@@ -1,6 +1,8 @@#ifndef __TEST_TOOL_H__#define __TEST_TOOL_H__ +#include "git-compat-util.h" +int cmd__chmtime (int argc,const char **argv);int cmd__config (int argc,const char **argv);int cmd__ctype (int argc,const char **argv);
Signature · 1 line
2.18.0.1070.g4763fa3c01
Quoted 8 lines · Jeff King
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Jeff King
So I think the better test is a two-line .c file with:
#include "git-compat-util.h" #include $header_to_check
Quoted 8 lines
Quoted 4 lines
But until that tightening happens, I do not actually mind the two-line .c file started with inclusion of
cache.hinstead ofgit-compat-util.h. That would limit the scope of this series further.I can go either way on this. Using
cache.hmakes Elijah's current series a bit more focused. But I think we'd eventually want to go there anyway. I don't have a strong opinion on doing it now or later.
For what it's worth, I'd been assuming that any header that is not self-contained after a #include of "git-compat-util.h" is a bug.
It's true that many .c files include git-compat-util.h via cache.h, making such a bug harder to notice. But the moment you write a source file that doesn't include cache.h you'd run into that problem, so it's worth fixing ahead-of-time anyway.
Thanks,
Jonathan
Jonathan Nieder
Reviewed-byQuoted 11 lines · Jeff King
Subject: [PATCH]
test-tool.h: includegit-compat-util.hThe test-tool programs include "
test-tool.h" as their first include, which breaks our CodingGuideline of "the first include must begit-compat-util.hor an equivalent". This isn't actually a problem, astest-tool.hdoesn't define anything tricky, but we should probably follow our own rule.Rather than change them all, let's instead make
test-tool.hone of those equivalents, just like we do forbuiltin.h(which many of the actual git builtins include first).
I wonder if it would not be simpler to change them all. It would be one less special case.
That said,
Quoted 4 lines
Signed-off-by Jeff King <[email protected]>--- t/helper/test-tool.h | 2 ++ 1 file changed, 2 insertions(+)
Thanks.
v2 cover Add missing includes and forward declares
In reply to Elijah NewrenThis series fixes compilation errors when using a simple test.c file that includes git-compat-util.h and then exactly one other header (and repeating this for different headers of git).
Changes since v1:
- Followed Peff's suggestion to make my simple test .c file first #include git-compat-util.h; removes the need for inclusions of this header or stdlib.h or stdio.h throughout my series. (Patches 6-8 of v1 no longer necessary at all.)
- Removed the explanatory comments about why various #include's were needed from v1.
Note: the final patch has minor conflicts with topics in flight in next and pu; it can just be dropped and I can resubmit it later if that is easier.
Elijah Newren (6):
Add missing includes and forward declares
alloc: make allocate_alloc_state and clear_alloc_state more consistent Move definition of enum branch_track from cache.h to branch.h urlmatch.h: fix include guard
compat/precompose_utf8.h: use more common include guard style Add missing includes and forward declares
alloc.c | 2 +- alloc.h | 4 +++- apply.h | 3 +++ archive.h | 3 +++ attr.h | 1 + bisect.h | 2 ++ branch.h | 13 +++++++++++++ bulk-checkin.h | 2 ++ cache.h | 10 ---------- column.h | 1 + commit-graph.h | 1 + compat/precompose_utf8.h | 3 ++- config.c | 1 + config.h | 5 +++++ connected.h | 1 + convert.h | 2 ++ csum-file.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ environment.c | 1 + fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 2 ++ merge-recursive.h | 4 +++- notes-merge.h | 4 ++++ notes-utils.h | 3 +++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ pack-objects.h | 1 + patch-ids.h | 6 ++++++ path.h | 1 + pathspec.h | 5 +++++ pretty.h | 4 ++++ reachable.h | 2 ++ reflog-walk.h | 1 + refs.h | 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ shortlog.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 2 ++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ urlmatch.h | 2 ++ utf8.h | 2 ++ worktree.h | 1 + 61 files changed, 160 insertions(+), 17 deletions(-)
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v2 1/6 Add missing includes and forward declares
--- alloc.h | 2 ++ apply.h | 3 +++ archive.h | 3 +++ attr.h | 1 + branch.h | 2 ++ bulk-checkin.h | 2 ++ column.h | 1 + commit-graph.h | 1 + config.h | 5 +++++ connected.h | 1 + convert.h | 2 ++ csum-file.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 2 ++ merge-recursive.h | 4 +++- notes-merge.h | 4 ++++ notes-utils.h | 3 +++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ patch-ids.h | 6 ++++++ path.h | 1 + pathspec.h | 5 +++++ pretty.h | 4 ++++ reachable.h | 2 ++ reflog-walk.h | 1 + refs.h | 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ shortlog.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 2 ++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ utf8.h | 2 ++ worktree.h | 1 + 53 files changed, 138 insertions(+), 4 deletions(-)
alloc.h +2 -0
@@ -1,9 +1,11 @@#ifndef ALLOC_H #define ALLOC_H +struct alloc_state ;struct tree ;struct commit ;struct tag ; +struct repository ;void *alloc_blob_node (struct repository *r);void *alloc_tree_node (struct repository *r);
apply.h +3 -0
@@ -1,6 +1,9 @@#ifndef APPLY_H #define APPLY_H +#include "lockfile.h" +#include "string-list.h" +enum apply_ws_error_action { nowarn_ws_error, warn_on_ws_error,
archive.h +3 -0
@@ -3,6 +3,9 @@#include "pathspec.h" +struct object_id ; +enum object_type ; +struct archiver_args {const char *base;size_t baselen;
attr.h +1 -0
@@ -7,6 +7,7 @@ struct git_attr;/* opaque structures used internally for attribute collection */ struct all_attrs_item ;struct attr_stack ; +struct index_state ;/* * Given a string, return the gitattribute object that
branch.h +2 -0
@@ -1,6 +1,8 @@#ifndef BRANCH_H #define BRANCH_H +struct strbuf ; +/* Functions for acting on the information about branches. */ /*
bulk-checkin.h +2 -0
@@ -4,6 +4,8 @@#ifndef BULK_CHECKIN_H #define BULK_CHECKIN_H +#include "cache.h" +extern int index_bulk_checkin (struct object_id *oid,int fd,size_t size,enum object_type type,const char *path,unsigned flags);
column.h +1 -0
@@ -36,6 +36,7 @@ static inline int column_active(unsigned int colopts)return (colopts &COL_ENABLE_MASK ) ==COL_ENABLED ; } +struct string_list ;extern void print_columns (const struct string_list *list,unsigned int colopts,const struct column_options *opts);
commit-graph.h +1 -0
@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" struct commit ;
config.h +5 -0
@@ -1,6 +1,11 @@#ifndef CONFIG_H #define CONFIG_H +#include "hashmap.h" +#include "string-list.h" + +struct object_id ; +/* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1#define CONFIG_NO_SECTION_OR_NAME 2
connected.h +1 -0
@@ -1,6 +1,7 @@#ifndef CONNECTED_H #define CONNECTED_H +struct object_id ;struct transport ;/*
convert.h +2 -0
@@ -7,6 +7,8 @@#include "string-list.h" struct index_state ; +struct object_id ; +struct strbuf ;#define CONV_EOL_RNDTRP_DIE (1<<0)/* Die if CRLF to LF to CRLF is different */ #define CONV_EOL_RNDTRP_WARN (1<<1)/* Warn if CRLF to LF to CRLF is different */
csum-file.h +2 -0
@@ -1,6 +1,8 @@#ifndef CSUM_FILE_H #define CSUM_FILE_H +#include "hash.h" +struct progress ;/* A SHA1-protected file */
diffcore.h +4 -0
@@ -4,6 +4,10 @@#ifndef DIFFCORE_H #define DIFFCORE_H +#include "cache.h" + +struct diff_options ; +/* This header file is internal between diff.c and its diff transformers * (e.g. diffcore-rename, diffcore-pickaxe). Never include this header * in anything else.
dir-iterator.h +2 -0
@@ -1,6 +1,8 @@#ifndef DIR_ITERATOR_H #define DIR_ITERATOR_H +#include "strbuf.h" +/* * Iterate over a directory tree. *
fsck.h +1 -0
@@ -6,6 +6,7 @@#define FSCK_IGNORE 3struct fsck_options ; +struct object ;void fsck_set_msg_type (struct fsck_options *options,const char *msg_id,const char *msg_type);
fsmonitor.h +7 -0
@@ -1,6 +1,13 @@#ifndef FSMONITOR_H #define FSMONITOR_H +#include "cache.h" +#include "dir.h" + +struct cache_entry ; +struct index_state ; +struct strbuf ; +extern struct trace_key trace_fsmonitor;/*
gpg-interface.h +2 -0
@@ -1,6 +1,8 @@#ifndef GPG_INTERFACE_H #define GPG_INTERFACE_H +struct strbuf ; +#define GPG_VERIFY_VERBOSE 1#define GPG_VERIFY_RAW 2#define GPG_VERIFY_OMIT_STATUS 4
khash.h +3 -0
@@ -26,6 +26,9 @@#ifndef __AC_KHASH_H#define __AC_KHASH_H +#include "hashmap.h" +#include "cache.h" +#define AC_VERSION_KHASH_H "0.2.8"typedef uint32_t khint32_t ;
list-objects-filter.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_FILTER_H #define LIST_OBJECTS_FILTER_H +struct list_objects_filter_options ; +struct object ; +struct oidset ; +/* * During list-object traversal we allow certain objects to be * filtered (omitted) from the result. The active filter uses
list-objects.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_H #define LIST_OBJECTS_H +struct commit ; +struct object ; +struct rev_info ; +typedef void (*show_commit_fn )(struct commit *,void *);typedef void (*show_object_fn )(struct object *,const char *,void *);void traverse_commit_list (struct rev_info *,show_commit_fn ,show_object_fn ,void *);
ll-merge.h +2 -0
@@ -5,6 +5,8 @@ #ifndef LL_MERGE_H #define LL_MERGE_H +#include "xdiff/xdiff.h" + struct ll_merge_options { unsigned virtual_ancestor : 1; unsigned variant : 2; /* favor ours, favor theirs, or union merge */
mailinfo.h +2 -0
@@ -1,6 +1,8 @@#ifndef MAILINFO_H #define MAILINFO_H +#include "strbuf.h" +#define MAX_BOUNDARIES 5 struct mailinfo {
mailmap.h +2 -0
@@ -1,6 +1,8 @@#ifndef MAILMAP_H #define MAILMAP_H +struct string_list ; +int read_mailmap (struct string_list *map,char **repo_abbrev);void clear_mailmap (struct string_list *map);
merge-recursive.h +3 -1
@@ -1,8 +1,10 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H -#include "unpack-trees.h" #include "string-list.h" +#include "unpack-trees.h" + +struct commit; struct merge_options { const char *ancestor;
notes-merge.h +4 -0
@@ -2,6 +2,10 @@#define NOTES_MERGE_H #include "notes-utils.h" +#include "strbuf.h" + +struct commit ; +struct object_id ;#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
notes-utils.h +3 -0
@@ -3,6 +3,9 @@#include "notes.h" +struct commit_list ; +struct object_id ; +/* * Create new notes commit from the given notes tree *
notes.h +3 -0
@@ -3,6 +3,9 @@#include "string-list.h" +struct object_id ; +struct strbuf ; +/* * Function type for combining two notes annotating the same object. *
object-store.h +1 -0
@@ -1,6 +1,7 @@#ifndef OBJECT_STORE_H #define OBJECT_STORE_H +#include "cache.h" #include "oidmap.h" #include "list.h" #include "sha1-array.h"
object.h +2 -0
@@ -1,6 +1,8 @@#ifndef OBJECT_H #define OBJECT_H +#include "cache.h" +struct buffer_slab ; struct parsed_object_pool {
oidmap.h +1 -0
@@ -1,6 +1,7 @@#ifndef OIDMAP_H #define OIDMAP_H +#include "cache.h" #include "hashmap.h" /*
pack-bitmap.h +3 -0
@@ -5,6 +5,9 @@#include "khash.h" #include "pack-objects.h" +struct commit ; +struct rev_info ; +struct bitmap_disk_header {char magic[4 ];uint16_t version;
patch-ids.h +6 -0
@@ -1,6 +1,12 @@ #ifndef PATCH_IDS_H #define PATCH_IDS_H +#include "diff.h" +#include "hashmap.h" + +struct commit; +struct object_id; + struct patch_id { struct hashmap_entry ent; struct object_id patch_id;
path.h +1 -0
@@ -2,6 +2,7 @@#define PATH_H struct repository ; +struct strbuf ;/* * The result to all functions which return statically allocated memory may be
pathspec.h +5 -0
@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h" + +struct index_state ; +/* Pathspec magic */ #define PATHSPEC_FROMTOP (1<<0)#define PATHSPEC_MAXDEPTH (1<<1)
pretty.h +4 -0
@@ -1,7 +1,11 @@#ifndef PRETTY_H #define PRETTY_H +#include "cache.h" +#include "string-list.h" +struct commit ; +struct strbuf ;/* Commit formats */ enum cmit_fmt {
reachable.h +2 -0
@@ -2,6 +2,8 @@#define REACHEABLE_H struct progress ; +struct rev_info ; +extern int add_unseen_recent_objects_to_traversal (struct rev_info *revs,timestamp_t timestamp); extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
reflog-walk.h +1 -0
@@ -3,6 +3,7 @@#include "cache.h" +struct commit ;struct reflog_walk_info ;extern void init_reflog_walk (struct reflog_walk_info **info);
refs.h +2 -0
@@ -3,8 +3,10 @@struct object_id ;struct ref_store ; +struct repository ;struct strbuf ;struct string_list ; +struct string_list_item ;struct worktree ;/*
remote.h +1 -0
@@ -1,6 +1,7 @@#ifndef REMOTE_H #define REMOTE_H +#include "cache.h" #include "parse-options.h" #include "hashmap.h" #include "refspec.h"
repository.h +2 -0
@@ -1,6 +1,8 @@#ifndef REPOSITORY_H #define REPOSITORY_H +#include "path.h" +struct config_set ;struct git_hash_algo ;struct index_state ;
resolve-undo.h +2 -0
@@ -1,6 +1,8 @@ #ifndef RESOLVE_UNDO_H #define RESOLVE_UNDO_H +#include "cache.h" + struct resolve_undo_info { unsigned int mode[3]; struct object_id oid[3];
revision.h +1 -0
@@ -1,6 +1,7 @@#ifndef REVISION_H #define REVISION_H +#include "commit.h" #include "parse-options.h" #include "grep.h" #include "notes.h"
send-pack.h +4 -0
@@ -3,6 +3,10 @@#include "string-list.h" +struct child_process ; +struct oid_array ; +struct ref ; +/* Possible values for push_cert field in send_pack_args. */ #define SEND_PACK_PUSH_CERT_NEVER 0#define SEND_PACK_PUSH_CERT_IF_ASKED 1
sequencer.h +5 -0
@@ -1,6 +1,11 @@#ifndef SEQUENCER_H #define SEQUENCER_H +#include "cache.h" +#include "strbuf.h" + +struct commit ; +const char *git_path_commit_editmsg (void );const char *git_path_seq_dir (void );
shortlog.h +2 -0
@@ -3,6 +3,8 @@#include "string-list.h" +struct commit ; +struct shortlog {struct string_list list;int summary;
submodule.h +8 -2
@@ -1,11 +1,17 @@#ifndef SUBMODULE_H #define SUBMODULE_H -struct repository ; -struct diff_options ;struct argv_array ; +struct cache_entry ; +struct diff_options ; +struct index_state ; +struct object_id ;struct oid_array ; +struct pathspec ;struct remote ; +struct repository ; +struct string_list ; +struct strbuf ;enum { RECURSE_SUBMODULES_ONLY = -5,
tempfile.h +1 -0
@@ -2,6 +2,7 @@#define TEMPFILE_H #include "list.h" +#include "strbuf.h" /* * Handle temporary files.
trailer.h +2 -0
@@ -3,6 +3,8 @@#include "list.h" +struct strbuf ; +enum trailer_where {WHERE_DEFAULT ,WHERE_END ,
tree-walk.h +2 -0
@@ -1,6 +1,8 @@ #ifndef TREE_WALK_H #define TREE_WALK_H +struct strbuf; + struct name_entry { const struct object_id *oid; const char *path;
unpack-trees.h +4 -1
@@ -1,11 +1,14 @@#ifndef UNPACK_TREES_H #define UNPACK_TREES_H -#include "tree-walk.h" #include "argv-array.h" +#include "cache.h" +#include "string-list.h" +#include "tree-walk.h" #define MAX_UNPACK_TREES 8 +struct cache_entry ;struct unpack_trees_options ;struct exclude_list ;
url.h +2 -0
@@ -1,6 +1,8 @@#ifndef URL_H #define URL_H +struct strbuf ; +extern int is_url (const char *url);extern int is_urlschemechar (int first_flag,int ch);extern char *url_decode (const char *url);
utf8.h +2 -0
@@ -1,6 +1,8 @@#ifndef GIT_UTF8_H #define GIT_UTF8_H +struct strbuf ; +typedef unsigned int ucs_char_t ;/* assuming 32bit int */ size_t display_mode_esc_sequence_len (const char *s);
worktree.h +1 -0
@@ -1,6 +1,7 @@#ifndef WORKTREE_H #define WORKTREE_H +#include "cache.h" #include "refs.h" struct strbuf ;
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v2 2/6 alloc: make allocate_alloc_state and clear_alloc_state more consistent
In reply to Elijah NewrenSince both functions are using the same data type, they should either both refer to it as void *, or both use the real type (struct alloc_state *). Opt for the latter.
--- alloc.c | 2 +- alloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
alloc.c +1 -1
@@ -36,7 +36,7 @@ struct alloc_state {int slab_nr, slab_alloc; }; -void *allocate_alloc_state (void ) +struct alloc_state *allocate_alloc_state (void ) {return xcalloc (1 ,sizeof (struct alloc_state )); }
alloc.h +1 -1
@@ -15,7 +15,7 @@ void *alloc_object_node(struct repository *r);void alloc_report (struct repository *r);unsigned int alloc_commit_index (struct repository *r); -void *allocate_alloc_state (void ); +struct alloc_state *allocate_alloc_state (void );void clear_alloc_state (struct alloc_state *s);#endif
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v2 3/6 Move definition of enum branch_track from cache.h to branch.h
In reply to Elijah Newren'branch_track' feels more closely related to branching, and it is needed later in branch.h; rather than #include'ing cache.h in branch.h for this small enum, just move the enum and the external declaration for git_branch_track to branch.h.
--- branch.h | 11 +++++++++++ cache.h | 10 ---------- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-)
branch.h +11 -0
@@ -3,6 +3,17 @@struct strbuf ; +enum branch_track { +BRANCH_TRACK_UNSPECIFIED =-1 , +BRANCH_TRACK_NEVER =0 , +BRANCH_TRACK_REMOTE , +BRANCH_TRACK_ALWAYS , +BRANCH_TRACK_EXPLICIT , +BRANCH_TRACK_OVERRIDE +}; + +extern enum branch_track git_branch_track; +/* Functions for acting on the information about branches. */ /*
cache.h +0 -10
@@ -919,15 +919,6 @@ enum log_refs_config { };extern enum log_refs_config log_all_ref_updates; -enum branch_track { -BRANCH_TRACK_UNSPECIFIED =-1 , -BRANCH_TRACK_NEVER =0 , -BRANCH_TRACK_REMOTE , -BRANCH_TRACK_ALWAYS , -BRANCH_TRACK_EXPLICIT , -BRANCH_TRACK_OVERRIDE -}; -enum rebase_setup_type {AUTOREBASE_NEVER =0 ,AUTOREBASE_LOCAL ,
@@ -944,7 +935,6 @@ enum push_default_type { PUSH_DEFAULT_UNSPECIFIED }; -extern enum branch_track git_branch_track;extern enum rebase_setup_type autorebase;extern enum push_default_type push_default;
config.c +1 -0
@@ -5,6 +5,7 @@* Copyright (C) Johannes Schindelin, 2005 * */ +#include "branch.h" #include "cache.h" #include "config.h" #include "repository.h"
environment.c +1 -0
@@ -7,6 +7,7 @@* even if you might want to know where the git directory etc * are. */ +#include "branch.h" #include "cache.h" #include "repository.h" #include "config.h"
Signature · 1 line
2.18.0.549.gd4454f3f9b
Re: [PATCHv2 3/6] Move definition of enum branch_track from cache.h to branch.h
Quoted 68 lines · Elijah Newren
'
branch_track' feels more closely related to branching, and it is needed later inbranch.h; rather than #include'ingcache.hinbranch.hfor this small enum, just move the enum and the external declaration forgit_branch_tracktobranch.h.Signed-off-by Elijah Newren <[email protected]>--- branch.h | 11 +++++++++++ cache.h | 10 ---------- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-)branch.h +11
-0@@ -3,6 +3,17 @@struct strbuf ; +enum branch_track { +BRANCH_TRACK_UNSPECIFIED =-1 , +BRANCH_TRACK_NEVER =0 , +BRANCH_TRACK_REMOTE , +BRANCH_TRACK_ALWAYS , +BRANCH_TRACK_EXPLICIT , +BRANCH_TRACK_OVERRIDE +}; + +extern enum branch_track git_branch_track; +/* Functions for acting on the information about branches. */ /* cache.h +0
-10@@ -919,15 +919,6 @@ enum log_refs_config { };extern enum log_refs_config log_all_ref_updates; -enum branch_track { -BRANCH_TRACK_UNSPECIFIED =-1 , -BRANCH_TRACK_NEVER =0 , -BRANCH_TRACK_REMOTE , -BRANCH_TRACK_ALWAYS , -BRANCH_TRACK_EXPLICIT , -BRANCH_TRACK_OVERRIDE -}; -enum rebase_setup_type {AUTOREBASE_NEVER =0 ,AUTOREBASE_LOCAL ,@@ -944,7 +935,6 @@ enum push_default_type { PUSH_DEFAULT_UNSPECIFIED }; -extern enum branch_track git_branch_track;extern enum rebase_setup_type autorebase;extern enum push_default_type push_default;config.c +0
-0
config.c line 8
Quoted 4 lines
config.c +1
-0@@ -5,6 +5,7 @@* Copyright (C) Johannes Schindelin, 2005 * */ +#include "branch.h"
git-compat-util.h, cache.h or builtin.h _must_ be the first header file #included in a C file, so this new #include of the "branch.h" header should be moved ...
config.c line 9
Quoted 1 line
config.c +0
-0#include "cache.h"
... after here.
environment.c line 10
Quoted 11 lines
config.c +0
-0#include "config.h" #include "repository.h" environment.c +1
-0@@ -7,6 +7,7 @@* even if you might want to know where the git directory etc * are. */ +#include "branch.h"
ditto
environment.c line 13
Quoted 4 lines
environment.c +0
-0#include "cache.h" #include "repository.h" #include "config.h"
ATB,
Ramsay Jones
PATCH v2 4/6 urlmatch.h: fix include guard
In reply to Elijah Newren--- urlmatch.h | 2 ++ 1 file changed, 2 insertions(+)
urlmatch.h +2 -0
@@ -1,4 +1,6 @@#ifndef URL_MATCH_H +#define URL_MATCH_H +#include "string-list.h" struct url_info {
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v2 5/6 compat/precompose_utf8.h: use more common include guard style
In reply to Elijah Newren--- compat/precompose_utf8.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
compat/precompose_utf8.h +2 -1
@@ -1,4 +1,6 @@#ifndef PRECOMPOSE_UNICODE_H +#define PRECOMPOSE_UNICODE_H +#include <sys/stat.h> #include <sys/types.h> #include <dirent.h>
@@ -41,5 +43,4 @@ int precompose_utf8_closedir(PREC_DIR *dirp);#define DIR PREC_DIR#endif /* PRECOMPOSE_UNICODE_C */ -#define PRECOMPOSE_UNICODE_H #endif /* PRECOMPOSE_UNICODE_H */
Signature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v2 6/6 Add missing includes and forward declares
In reply to Elijah Newren--- bisect.h | 2 ++ pack-objects.h | 1 + 2 files changed, 3 insertions(+)
bisect.h +2 -0
@@ -1,6 +1,8 @@#ifndef BISECT_H #define BISECT_H +struct commit_list ; +/* * Find bisection. If something is found, `reaches` will be the number of * commits that the best commit reaches. `all` will be the count of
pack-objects.h +1 -0
@@ -2,6 +2,7 @@#define PACK_OBJECTS_H #include "object-store.h" +#include "pack.h" #define DEFAULT_DELTA_CACHE_SIZE (256 * 1024 * 1024)
Signature · 1 line
2.18.0.549.gd4454f3f9b
v3 cover Add missing includes and forward declares
In reply to Elijah NewrenThis series fixes compilation errors when using a simple test.c file that includes git-compat-util.h and then exactly one other header (and repeating this for different headers of git).
Changes since v2 (full range-diff below):
- Ensure the first #include in C files is either git-compat-util.h, cache.h, or builtin.h (pointed out by Ramsay). Also, although not important, also tweaked a couple other #include orders to put cache.h first in some .h files.
Elijah Newren (6):
Add missing includes and forward declares
alloc: make allocate_alloc_state and clear_alloc_state more consistent Move definition of enum branch_track from cache.h to branch.h urlmatch.h: fix include guard
compat/precompose_utf8.h: use more common include guard style Add missing includes and forward declares
alloc.c | 2 +- alloc.h | 4 +++- apply.h | 3 +++ archive.h | 3 +++ attr.h | 1 + bisect.h | 2 ++ branch.h | 13 +++++++++++++ bulk-checkin.h | 2 ++ cache.h | 10 ---------- column.h | 1 + commit-graph.h | 1 + compat/precompose_utf8.h | 3 ++- config.c | 1 + config.h | 5 +++++ connected.h | 1 + convert.h | 2 ++ csum-file.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ environment.c | 1 + fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 2 ++ merge-recursive.h | 4 +++- notes-merge.h | 4 ++++ notes-utils.h | 3 +++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ pack-objects.h | 1 + patch-ids.h | 6 ++++++ path.h | 1 + pathspec.h | 5 +++++ pretty.h | 4 ++++ reachable.h | 2 ++ reflog-walk.h | 1 + refs.h | 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ shortlog.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 2 ++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ urlmatch.h | 2 ++ utf8.h | 2 ++ worktree.h | 1 + 61 files changed, 160 insertions(+), 17 deletions(-)
1: 2bd09172ef ! 1: f7d50cef3b Add missing includes and forward declares
@@ -242,8 +242,8 @@
#ifndef __AC_KHASH_H
#define __AC_KHASH_H
-+#include "hashmap.h"
+#include "cache.h"
++#include "hashmap.h"
+
#define AC_VERSION_KHASH_H "0.2.8"
@@ -693,8 +693,8 @@
#define UNPACK_TREES_H
-#include "tree-walk.h"
- #include "argv-array.h"
+#include "cache.h"
+ #include "argv-array.h"
+#include "string-list.h"
+#include "tree-walk.h"2: 854860e967 = 2: e46bf7d601 alloc: make allocate_alloc_state and clear_alloc_state more consistent
3: c91c4eaa38 ! 3: aec8ddda59 Move definition of enum branch_track from cache.h to branch.h
@@ -63,22 +63,22 @@
--- a/config.c
+++ b/config.c
@@
- * Copyright (C) Johannes Schindelin, 2005
*
*/
-+#include "branch.h"
#include "cache.h"
++#include "branch.h"
#include "config.h"
#include "repository.h"
+ #include "lockfile.h"
diff --git a/environment.c b/environment.c
--- a/environment.c
+++ b/environment.c
@@
- * even if you might want to know where the git directory etc
* are.
*/
-+#include "branch.h"
#include "cache.h"
++#include "branch.h"
#include "repository.h"
#include "config.h"
+ #include "refs.h"
4: 550568912b = 4: 14e33fb5ff urlmatch.h: fix include guard
5: 1186b37a3c = 5: 3e02a381af compat/precompose_utf8.h: use more common include guard style
6: 0642f8ecf0 = 6: aca61bade1 Add missing includes and forward declaresSignature · 1 line
2.18.0.549.gd4454f3f9b
PATCH v3 1/6 Add missing includes and forward declares
--- alloc.h | 2 ++ apply.h | 3 +++ archive.h | 3 +++ attr.h | 1 + branch.h | 2 ++ bulk-checkin.h | 2 ++ column.h | 1 + commit-graph.h | 1 + config.h | 5 +++++ connected.h | 1 + convert.h | 2 ++ csum-file.h | 2 ++ diffcore.h | 4 ++++ dir-iterator.h | 2 ++ fsck.h | 1 + fsmonitor.h | 7 +++++++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h | 4 ++++ list-objects.h | 4 ++++ ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h | 2 ++ merge-recursive.h | 4 +++- notes-merge.h | 4 ++++ notes-utils.h | 3 +++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ patch-ids.h | 6 ++++++ path.h | 1 + pathspec.h | 5 +++++ pretty.h | 4 ++++ reachable.h | 2 ++ reflog-walk.h | 1 + refs.h | 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 ++++ sequencer.h | 5 +++++ shortlog.h | 2 ++ submodule.h | 10 ++++++++-- tempfile.h | 1 + trailer.h | 2 ++ tree-walk.h | 2 ++ unpack-trees.h | 5 ++++- url.h | 2 ++ utf8.h | 2 ++ worktree.h | 1 + 53 files changed, 138 insertions(+), 4 deletions(-)
alloc.h +2 -0
@@ -1,9 +1,11 @@#ifndef ALLOC_H #define ALLOC_H +struct alloc_state ;struct tree ;struct commit ;struct tag ; +struct repository ;void *alloc_blob_node (struct repository *r);void *alloc_tree_node (struct repository *r);
apply.h +3 -0
@@ -1,6 +1,9 @@#ifndef APPLY_H #define APPLY_H +#include "lockfile.h" +#include "string-list.h" +enum apply_ws_error_action { nowarn_ws_error, warn_on_ws_error,
archive.h +3 -0
@@ -3,6 +3,9 @@#include "pathspec.h" +struct object_id ; +enum object_type ; +struct archiver_args {const char *base;size_t baselen;
attr.h +1 -0
@@ -7,6 +7,7 @@ struct git_attr;/* opaque structures used internally for attribute collection */ struct all_attrs_item ;struct attr_stack ; +struct index_state ;/* * Given a string, return the gitattribute object that
branch.h +2 -0
@@ -1,6 +1,8 @@#ifndef BRANCH_H #define BRANCH_H +struct strbuf ; +/* Functions for acting on the information about branches. */ /*
bulk-checkin.h +2 -0
@@ -4,6 +4,8 @@#ifndef BULK_CHECKIN_H #define BULK_CHECKIN_H +#include "cache.h" +extern int index_bulk_checkin (struct object_id *oid,int fd,size_t size,enum object_type type,const char *path,unsigned flags);
column.h +1 -0
@@ -36,6 +36,7 @@ static inline int column_active(unsigned int colopts)return (colopts &COL_ENABLE_MASK ) ==COL_ENABLED ; } +struct string_list ;extern void print_columns (const struct string_list *list,unsigned int colopts,const struct column_options *opts);
commit-graph.h +1 -0
@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" struct commit ;
config.h +5 -0
@@ -1,6 +1,11 @@#ifndef CONFIG_H #define CONFIG_H +#include "hashmap.h" +#include "string-list.h" + +struct object_id ; +/* git_config_parse_key() returns these negated: */ #define CONFIG_INVALID_KEY 1#define CONFIG_NO_SECTION_OR_NAME 2
connected.h +1 -0
@@ -1,6 +1,7 @@#ifndef CONNECTED_H #define CONNECTED_H +struct object_id ;struct transport ;/*
convert.h +2 -0
@@ -7,6 +7,8 @@#include "string-list.h" struct index_state ; +struct object_id ; +struct strbuf ;#define CONV_EOL_RNDTRP_DIE (1<<0)/* Die if CRLF to LF to CRLF is different */ #define CONV_EOL_RNDTRP_WARN (1<<1)/* Warn if CRLF to LF to CRLF is different */
csum-file.h +2 -0
@@ -1,6 +1,8 @@#ifndef CSUM_FILE_H #define CSUM_FILE_H +#include "hash.h" +struct progress ;/* A SHA1-protected file */
diffcore.h +4 -0
@@ -4,6 +4,10 @@#ifndef DIFFCORE_H #define DIFFCORE_H +#include "cache.h" + +struct diff_options ; +/* This header file is internal between diff.c and its diff transformers * (e.g. diffcore-rename, diffcore-pickaxe). Never include this header * in anything else.
dir-iterator.h +2 -0
@@ -1,6 +1,8 @@#ifndef DIR_ITERATOR_H #define DIR_ITERATOR_H +#include "strbuf.h" +/* * Iterate over a directory tree. *
fsck.h +1 -0
@@ -6,6 +6,7 @@#define FSCK_IGNORE 3struct fsck_options ; +struct object ;void fsck_set_msg_type (struct fsck_options *options,const char *msg_id,const char *msg_type);
fsmonitor.h +7 -0
@@ -1,6 +1,13 @@#ifndef FSMONITOR_H #define FSMONITOR_H +#include "cache.h" +#include "dir.h" + +struct cache_entry ; +struct index_state ; +struct strbuf ; +extern struct trace_key trace_fsmonitor;/*
gpg-interface.h +2 -0
@@ -1,6 +1,8 @@#ifndef GPG_INTERFACE_H #define GPG_INTERFACE_H +struct strbuf ; +#define GPG_VERIFY_VERBOSE 1#define GPG_VERIFY_RAW 2#define GPG_VERIFY_OMIT_STATUS 4
khash.h +3 -0
@@ -26,6 +26,9 @@#ifndef __AC_KHASH_H#define __AC_KHASH_H +#include "cache.h" +#include "hashmap.h" +#define AC_VERSION_KHASH_H "0.2.8"typedef uint32_t khint32_t ;
list-objects-filter.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_FILTER_H #define LIST_OBJECTS_FILTER_H +struct list_objects_filter_options ; +struct object ; +struct oidset ; +/* * During list-object traversal we allow certain objects to be * filtered (omitted) from the result. The active filter uses
list-objects.h +4 -0
@@ -1,6 +1,10 @@#ifndef LIST_OBJECTS_H #define LIST_OBJECTS_H +struct commit ; +struct object ; +struct rev_info ; +typedef void (*show_commit_fn )(struct commit *,void *);typedef void (*show_object_fn )(struct object *,const char *,void *);void traverse_commit_list (struct rev_info *,show_commit_fn ,show_object_fn ,void *);
ll-merge.h +2 -0
@@ -5,6 +5,8 @@ #ifndef LL_MERGE_H #define LL_MERGE_H +#include "xdiff/xdiff.h" + struct ll_merge_options { unsigned virtual_ancestor : 1; unsigned variant : 2; /* favor ours, favor theirs, or union merge */
mailinfo.h +2 -0
@@ -1,6 +1,8 @@#ifndef MAILINFO_H #define MAILINFO_H +#include "strbuf.h" +#define MAX_BOUNDARIES 5 struct mailinfo {
mailmap.h +2 -0
@@ -1,6 +1,8 @@#ifndef MAILMAP_H #define MAILMAP_H +struct string_list ; +int read_mailmap (struct string_list *map,char **repo_abbrev);void clear_mailmap (struct string_list *map);
merge-recursive.h +3 -1
@@ -1,8 +1,10 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H -#include "unpack-trees.h" #include "string-list.h" +#include "unpack-trees.h" + +struct commit; struct merge_options { const char *ancestor;
notes-merge.h +4 -0
@@ -2,6 +2,10 @@#define NOTES_MERGE_H #include "notes-utils.h" +#include "strbuf.h" + +struct commit ; +struct object_id ;#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
notes-utils.h +3 -0
@@ -3,6 +3,9 @@#include "notes.h" +struct commit_list ; +struct object_id ; +/* * Create new notes commit from the given notes tree *
notes.h +3 -0
@@ -3,6 +3,9 @@#include "string-list.h" +struct object_id ; +struct strbuf ; +/* * Function type for combining two notes annotating the same object. *
object-store.h +1 -0
@@ -1,6 +1,7 @@#ifndef OBJECT_STORE_H #define OBJECT_STORE_H +#include "cache.h" #include "oidmap.h" #include "list.h" #include "sha1-array.h"
object.h +2 -0
@@ -1,6 +1,8 @@#ifndef OBJECT_H #define OBJECT_H +#include "cache.h" +struct buffer_slab ; struct parsed_object_pool {
oidmap.h +1 -0
@@ -1,6 +1,7 @@#ifndef OIDMAP_H #define OIDMAP_H +#include "cache.h" #include "hashmap.h" /*
pack-bitmap.h +3 -0
@@ -5,6 +5,9 @@#include "khash.h" #include "pack-objects.h" +struct commit ; +struct rev_info ; +struct bitmap_disk_header {char magic[4 ];uint16_t version;
patch-ids.h +6 -0
@@ -1,6 +1,12 @@ #ifndef PATCH_IDS_H #define PATCH_IDS_H +#include "diff.h" +#include "hashmap.h" + +struct commit; +struct object_id; + struct patch_id { struct hashmap_entry ent; struct object_id patch_id;
path.h +1 -0
@@ -2,6 +2,7 @@#define PATH_H struct repository ; +struct strbuf ;/* * The result to all functions which return statically allocated memory may be
pathspec.h +5 -0
@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h" + +struct index_state ; +/* Pathspec magic */ #define PATHSPEC_FROMTOP (1<<0)#define PATHSPEC_MAXDEPTH (1<<1)
pretty.h +4 -0
@@ -1,7 +1,11 @@#ifndef PRETTY_H #define PRETTY_H +#include "cache.h" +#include "string-list.h" +struct commit ; +struct strbuf ;/* Commit formats */ enum cmit_fmt {
reachable.h +2 -0
@@ -2,6 +2,8 @@#define REACHEABLE_H struct progress ; +struct rev_info ; +extern int add_unseen_recent_objects_to_traversal (struct rev_info *revs,timestamp_t timestamp); extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
reflog-walk.h +1 -0
@@ -3,6 +3,7 @@#include "cache.h" +struct commit ;struct reflog_walk_info ;extern void init_reflog_walk (struct reflog_walk_info **info);
refs.h +2 -0
@@ -3,8 +3,10 @@struct object_id ;struct ref_store ; +struct repository ;struct strbuf ;struct string_list ; +struct string_list_item ;struct worktree ;/*
remote.h +1 -0
@@ -1,6 +1,7 @@#ifndef REMOTE_H #define REMOTE_H +#include "cache.h" #include "parse-options.h" #include "hashmap.h" #include "refspec.h"
repository.h +2 -0
@@ -1,6 +1,8 @@#ifndef REPOSITORY_H #define REPOSITORY_H +#include "path.h" +struct config_set ;struct git_hash_algo ;struct index_state ;
resolve-undo.h +2 -0
@@ -1,6 +1,8 @@ #ifndef RESOLVE_UNDO_H #define RESOLVE_UNDO_H +#include "cache.h" + struct resolve_undo_info { unsigned int mode[3]; struct object_id oid[3];
revision.h +1 -0
@@ -1,6 +1,7 @@#ifndef REVISION_H #define REVISION_H +#include "commit.h" #include "parse-options.h" #include "grep.h" #include "notes.h"
send-pack.h +4 -0
@@ -3,6 +3,10 @@#include "string-list.h" +struct child_process ; +struct oid_array ; +struct ref ; +/* Possible values for push_cert field in send_pack_args. */ #define SEND_PACK_PUSH_CERT_NEVER 0#define SEND_PACK_PUSH_CERT_IF_ASKED 1
sequencer.h +5 -0
@@ -1,6 +1,11 @@#ifndef SEQUENCER_H #define SEQUENCER_H +#include "cache.h" +#include "strbuf.h" + +struct commit ; +const char *git_path_commit_editmsg (void );const char *git_path_seq_dir (void );
shortlog.h +2 -0
@@ -3,6 +3,8 @@#include "string-list.h" +struct commit ; +struct shortlog {struct string_list list;int summary;
submodule.h +8 -2
@@ -1,11 +1,17 @@#ifndef SUBMODULE_H #define SUBMODULE_H -struct repository ; -struct diff_options ;struct argv_array ; +struct cache_entry ; +struct diff_options ; +struct index_state ; +struct object_id ;struct oid_array ; +struct pathspec ;struct remote ; +struct repository ; +struct string_list ; +struct strbuf ;enum { RECURSE_SUBMODULES_ONLY = -5,
tempfile.h +1 -0
@@ -2,6 +2,7 @@#define TEMPFILE_H #include "list.h" +#include "strbuf.h" /* * Handle temporary files.
trailer.h +2 -0
@@ -3,6 +3,8 @@#include "list.h" +struct strbuf ; +enum trailer_where {WHERE_DEFAULT ,WHERE_END ,
tree-walk.h +2 -0
@@ -1,6 +1,8 @@ #ifndef TREE_WALK_H #define TREE_WALK_H +struct strbuf; + struct name_entry { const struct object_id *oid; const char *path;
unpack-trees.h +4 -1
@@ -1,11 +1,14 @@#ifndef UNPACK_TREES_H #define UNPACK_TREES_H -#include "tree-walk.h" +#include "cache.h" #include "argv-array.h" +#include "string-list.h" +#include "tree-walk.h" #define MAX_UNPACK_TREES 8 +struct cache_entry ;struct unpack_trees_options ;struct exclude_list ;
url.h +2 -0
@@ -1,6 +1,8 @@#ifndef URL_H #define URL_H +struct strbuf ; +extern int is_url (const char *url);extern int is_urlschemechar (int first_flag,int ch);extern char *url_decode (const char *url);
utf8.h +2 -0
@@ -1,6 +1,8 @@#ifndef GIT_UTF8_H #define GIT_UTF8_H +struct strbuf ; +typedef unsigned int ucs_char_t ;/* assuming 32bit int */ size_t display_mode_esc_sequence_len (const char *s);
worktree.h +1 -0
@@ -1,6 +1,7 @@#ifndef WORKTREE_H #define WORKTREE_H +#include "cache.h" #include "refs.h" struct strbuf ;
Signature · 1 line
2.18.0.549.gd4454f3f9b
Quoted 1 line · Elijah Newren
Subject: Add missing includes and forward declares
nit: s/declares/declarations/
This is a huge patch. Was it autogenerated or generated manually? Can the commit message say something about methodology?
Is there an easy way to review it? (Keep in mind that I'm super lazy. ;-))
Quoted 2 lines
Signed-off-by Elijah Newren <[email protected]>---
[...]
alloc.h line 10
Quoted 13 lines
alloc.h +2
-0@@ -1,9 +1,11 @@#ifndef ALLOC_H #define ALLOC_H +struct alloc_state ;struct tree ;struct commit ;struct tag ; +struct repository ;void *alloc_blob_node (struct repository *r);
That's reasonable. Going forward, is there a way to tell if some of these forward declarations are no longer needed at some point in the future (e.g. can clang be convinced to warn us about it)?
[...]
apply.h line 7
Quoted 10 lines
apply.h +3
-0@@ -1,6 +1,9 @@#ifndef APPLY_H #define APPLY_H +#include "lockfile.h" +#include "string-list.h" + enum apply_ws_error_action {
Here, to avoid strange behavior, we have to be careful to make sure the headers don't #include apply.h back. It's a pretty high-level header so there's no risk of that *phew*.
[...]
archive.h line 7
Quoted 8 lines
archive.h +2
-0@@ -3,6 +3,9 @@#include "pathspec.h" +struct object_id ; +enum object_type ;
enums are of unknown size, so forward declarations don't work for them. See bb/pedantic for some examples.
enum object_type is defined in cache.h, so should this #include that?
[...]
commit-graph.h line 7
Quoted 7 lines
commit-graph.h +1
-0@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h"
We can skip the #include of git-compat-util.h since all .c files include it.
[...]
fsmonitor.h line 9
Quoted 12 lines
fsmonitor.h +6
-0@@ -1,6 +1,13 @@#ifndef FSMONITOR_H #define FSMONITOR_H +#include "cache.h" +#include "dir.h" + +struct cache_entry ; +struct index_state ; +struct strbuf ;
cache_entry et al are defined in cache.h, right? Are these forward decls needed?
[...]
merge-recursive.h line 5
Quoted 9 lines
merge-recursive.h +1
-1@@ -1,8 +1,10 @@#ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H -#include "unpack-trees.h" #include "string-list.h" +#include "unpack-trees.h"
just curious, no need to change: where does this reordering come from?
[...]
pathspec.h line 5
Quoted 8 lines
pathspec.h +2
-0@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h"
What are these headers?
The rest looks good.
Thanks and hope that helps,
Jonathan
Quoted 6 lines · Jonathan Nieder
Quoted 1 line · Elijah Newren
Subject: Add missing includes and forward declares
nit: s/declares/declarations/
Thanks.
Quoted 2 lines
This is a huge patch. Was it autogenerated or generated manually? Can the commit message say something about methodology?
Mostly manually. I had a simple program that would create a dummy.c file that included git-compat-util.h then exactly one header, compile it, and spit any compile errors at me. I repeated that through the top-level headers.
I didn't want to repeat that description in all 6 patches, since all six came from that, so I put it in the cover letter. Since patch #1 has most that changes though, I guess it makes sense to include it at least in that one?
Quoted 2 lines
Is there an easy way to review it? (Keep in mind that I'm super lazy. ;-))
I guess I could send you my hacky python script that loops through the top-level header files and creates the dummy two-line c file, and you could inspect it and run it. But that only verifies that it compiles, not that the changes I choose are "correct".
Quoted 16 lines
Quoted 3 lines
Signed-off-by Elijah Newren <[email protected]>---
[...]
Quoted 12 lines
alloc.h +2
-0@@ -1,9 +1,11 @@#ifndef ALLOC_H #define ALLOC_H +struct alloc_state ;struct tree ;struct commit ;struct tag ; +struct repository ;
Quoted 5 lines
alloc.h line 10
Quoted 1 line
alloc.h +0
-0void *alloc_blob_node (struct repository *r);That's reasonable. Going forward, is there a way to tell if some of these forward declarations are no longer needed at some point in the future (e.g. can clang be convinced to warn us about it)?
I'm not aware of anything currently; while I could have easily missed things, projects like
https://github.com/include-what-you-use/include-what-you-use (which look active and have a July 2018 date on them) make me suspect there isn't a good answer currently.
Quoted 7 lines
[...]
Quoted 6 lines
apply.h +0
-0@@ -1,6 +1,9 @@#ifndef APPLY_H #define APPLY_H
Quoted 8 lines
apply.h line 7
Quoted 4 lines
apply.h +3
-0+#include "lockfile.h" +#include "string-list.h" +enum apply_ws_error_action {Here, to avoid strange behavior, we have to be careful to make sure the headers don't #include
apply.hback. It's a pretty high-level header so there's no risk of that *phew*.
:-)
Quoted 5 lines
[...]
Quoted 4 lines
archive.h +0
-0@@ -3,6 +3,9 @@
Quoted 7 lines
archive.h line 7
Quoted 4 lines
archive.h +2
-0#include "pathspec.h" +struct object_id ; +enum object_type ;enums are of unknown size, so forward declarations don't work for them. See bb/pedantic for some examples.
structs are also of unknown size; the size is irrelevant when the function signature merely uses a pointer to the struct or enum. The enum forward declaration fixes a compilation bug.
Quoted 1 line
enum
object_typeis defined incache.h, so should this #include that?
We could, but we don't need the definition; a forward declaration is sufficient.
Quoted 11 lines
[...]
commit-graph.h line 7
Quoted 7 lines
commit-graph.h +1
-0@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" We can skip the #include of
git-compat-util.hsince all .c files include it.
Good point. Should I go through and remove all the inclusions of git-compat-util.h in header files?
Quoted 7 lines
[...]
Quoted 6 lines
fsmonitor.h +0
-0@@ -1,6 +1,13 @@#ifndef FSMONITOR_H #define FSMONITOR_H
Quoted 9 lines
fsmonitor.h line 9
Quoted 6 lines
fsmonitor.h +6
-0+#include "cache.h" +#include "dir.h" + +struct cache_entry ; +struct index_state ; +struct strbuf ;
cache_entryet al are defined incache.h, right? Are these forward decls needed?
Good catch; they can be removed. I'm pretty sure I added the forward declarations first, then noticed it wasn't enough, added the cache.h include, and forgot to clean up.
Quoted 7 lines
[...]
Quoted 6 lines
merge-recursive.h +0
-0@@ -1,8 +1,10 @@#ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H
Quoted 5 lines
merge-recursive.h line 5
Quoted 3 lines
merge-recursive.h +1
-1-#include "unpack-trees.h" #include "string-list.h" +#include "unpack-trees.h" just curious, no need to change: where does this reordering come from?
Well, since I was manually editing anyway, I saw these and decided to alphabetize it since it's a file I deal with a lot. *shrug*
Quoted 11 lines
[...]
pathspec.h line 5
Quoted 8 lines
pathspec.h +2
-0@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h" What are these headers?
The original patch[1] had explanations of why I added them:
+#include "string.h" /* For str[n]cmp */
+#include "strings.h" /* For str[n]casecmp */
But Peff requested that I remove the comments.
[1] https://public-inbox.org/git/[email protected]/
Quoted 4 lines
The rest looks good.
Thanks and hope that helps,
Jonathan
Thanks for taking a look!
Quoted 4 lines · Elijah Newren
I didn't want to repeat that description in all 6 patches, since all six came from that, so I put it in the cover letter. Since patch #1 has most that changes though, I guess it makes sense to include it at least in that one?
Yes, that sounds sensible to me.
[...]
Quoted 9 lines · Jonathan Nieder
Quoted 3 lines
Is there an easy way to review it? (Keep in mind that I'm super lazy. ;-))
I guess I could send you my hacky python script that loops through the top-level header files and creates the dummy two-line c file, and you could inspect it and run it. But that only verifies that it compiles, not that the changes I choose are "correct".
I suppose leaving it as an exercise to the reviewer is not terrible. Maybe some reviewer will be inspired to make a test tool we can check in, or configuration for an existing tool like iwyu.
[...]
Quoted 6 lines
Quoted 2 lines
enums are of unknown size, so forward declarations don't work for them. See bb/pedantic for some examples.
structs are also of unknown size; the size is irrelevant when the function signature merely uses a pointer to the struct or enum. The enum forward declaration fixes a compilation bug.
My rationale may miss the point but the standard and some real compilers don't like this, unfortunately.
For structs, having an incomplete type is fine, but for enums we need the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags")
A type specifier of the form
enum identifier
without an enumerator list shall only appear after the type it specifies is complete.
[...]
Quoted 11 lines
Quoted 10 lines
commit-graph.h line 7
Quoted 7 lines
commit-graph.h +1
-0@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" We can skip the #include of
git-compat-util.hsince all .c files include it.
Quoted 2 lines
Good point. Should I go through and remove all the inclusions of
git-compat-util.hin header files?
It's orthogonal to this series but might be a good change.
[...]
Quoted 13 lines
Quoted 10 lines
pathspec.h line 5
Quoted 8 lines
pathspec.h +2
-0@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h" What are these headers?
The original patch[1] had explanations of why I added them:
Review
Quoted 2 lines
(quoted diff) +2
-0+#include "string.h" /* For str[n]cmp */ +#include "strings.h" /* For str[n]casecmp */
Ah. Please remove these #includes: they're part of the standard library that we get implicitly via git-compat-util.h.
I was tripped up because they were in quotes instead of angle brackets.
Thanks,
Jonathan
Quoted 9 lines · Jonathan Nieder
Quoted 4 lines · Elijah Newren
I didn't want to repeat that description in all 6 patches, since all six came from that, so I put it in the cover letter. Since patch #1 has most that changes though, I guess it makes sense to include it at least in that one?
Yes, that sounds sensible to me.
Will do.
Quoted 14 lines
[...]
Quoted 6 lines
Quoted 2 lines
enums are of unknown size, so forward declarations don't work for them. See bb/pedantic for some examples.
structs are also of unknown size; the size is irrelevant when the function signature merely uses a pointer to the struct or enum. The enum forward declaration fixes a compilation bug.
My rationale may miss the point but the standard and some real compilers don't like this, unfortunately.
For structs, having an incomplete type is fine, but for enums we need the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags")
Quoted 6 lines
A type specifier of the form
enum identifier
without an enumerator list shall only appear after the type it specifies is complete.
What about a type specifier of the form
enum identifier *
? Can that kind of type specifier appear before the full definition of the enum? (Or, alternatively, if the standard doesn't say, are there any compilers that have a problem with that?)
If so, we can include cache.h instead. We'll probably also have to fix up packfile.h for the exact same issue (even the same enum name) if that's the case.
Quoted 12 lines
[...]
Quoted 11 lines
Quoted 10 lines
commit-graph.h line 7
Quoted 7 lines
commit-graph.h +1
-0@@ -4,6 +4,7 @@#include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" We can skip the #include of
git-compat-util.hsince all .c files include it.
Quoted 4 lines
Quoted 2 lines
Good point. Should I go through and remove all the inclusions of
git-compat-util.hin header files?It's orthogonal to this series but might be a good change.
I think I'll leave it as #leftoverbits for someone else interested. :-)
Quoted 14 lines
[...]
Quoted 13 lines
Quoted 10 lines
pathspec.h line 5
Quoted 8 lines
pathspec.h +2
-0@@ -1,6 +1,11 @@#ifndef PATHSPEC_H #define PATHSPEC_H +#include "string.h" +#include "strings.h" What are these headers?
The original patch[1] had explanations of why I added them:
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#include "string.h" /* For str[n]cmp */ +#include "strings.h" /* For str[n]casecmp */Ah. Please remove these #includes: they're part of the standard library that we get implicitly via
git-compat-util.h.I was tripped up because they were in quotes instead of angle brackets.
Indeed; will do.
Quoted 27 lines · Elijah Newren
Quoted 14 lines
[...]
Quoted 6 lines
Quoted 2 lines
enums are of unknown size, so forward declarations don't work for them. See bb/pedantic for some examples.
structs are also of unknown size; the size is irrelevant when the function signature merely uses a pointer to the struct or enum. The enum forward declaration fixes a compilation bug.
My rationale may miss the point but the standard and some real compilers don't like this, unfortunately.
For structs, having an incomplete type is fine, but for enums we need the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags")
Quoted 6 lines
A type specifier of the form
enum identifier
without an enumerator list shall only appear after the type it specifies is complete.
What about a type specifier of the form
enum identifier *
? Can that kind of type specifier appear before the full definition of the enum? (Or, alternatively, if the standard doesn't say, are there any compilers that have a problem with that?)
Quoted 3 lines
If so, we can include
cache.hinstead. We'll probably also have to fix uppackfile.hfor the exact same issue (even the same enum name) if that's the case.
Digging a little further this morning, apparently C++ has defined a forward declaration of an enum to either be useless (because it was already defined), require an explicit size specifier, or be a compilation error.
That seemed stupid to me, but a little more digging turned up
http://c-faq.com/null/machexamp.html , which states that sizeof(char*) != sizeof(int*) on some platforms. That was a big surprise to me. Since an enum could be a char or int (or long or...), knowing the size of the enum thus is important to knowing the size of a pointer to an enum, so we actually do need the full enum definition (or a C++ style explicit size specifier).
What a crazy world.
So, I'll go with the inclusion of cache.h and also fix up packfile.h the same way. Thanks for pointing this out, Jonathan.
PATCH v3 2/6 alloc: make allocate_alloc_state and clear_alloc_state more consistent
In reply to Elijah NewrenSince both functions are using the same data type, they should either both refer to it as void *, or both use the real type (struct alloc_state *). Opt for the latter.
--- alloc.c | 2 +- alloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
alloc.c +1 -1
@@ -36,7 +36,7 @@ struct alloc_state {int slab_nr, slab_alloc; }; -void *allocate_alloc_state (void ) +struct alloc_state *allocate_alloc_state (void ) {return xcalloc (1 ,sizeof (struct alloc_state )); }
alloc.h +1 -1
@@ -15,7 +15,7 @@ void *alloc_object_node(struct repository *r);void alloc_report (struct repository *r);unsigned int alloc_commit_index (struct repository *r); -void *allocate_alloc_state (void ); +struct alloc_state *allocate_alloc_state (void );void clear_alloc_state (struct alloc_state *s);#endif
Signature · 1 line
2.18.0.549.gd4454f3f9b
Re: [PATCHv3 2/6] alloc: make allocate_alloc_state and clear_alloc_state more consistent
Quoted 9 lines · Elijah Newren
Since both functions are using the same data type, they should either both refer to it as void *, or both use the real type (struct
alloc_state*). Opt for the latter.Signed-off-by Elijah Newren <[email protected]>--- alloc.c | 2 +- alloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
This API comes from 14ba97f8 (alloc: allow arbitrary repositories for alloc functions, 2018-05-15). The pointer returned by allocate_alloc_state points to an initialized, usable object so returning a struct instead of a void pointer is the right thing to do. Thanks for noticing and fixing it.
That commit in turn was probably inspired by v1.5.2-rc0~16^2 (Clean up object creation to use more common code, 2007-04-16), which was following the same convention (void * for raw memory, struct blob * for initialized object).
PATCH v3 3/6 Move definition of enum branch_track from cache.h to branch.h
In reply to Elijah Newren'branch_track' feels more closely related to branching, and it is needed later in branch.h; rather than #include'ing cache.h in branch.h for this small enum, just move the enum and the external declaration for git_branch_track to branch.h.
--- branch.h | 11 +++++++++++ cache.h | 10 ---------- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-)
branch.h +11 -0
@@ -3,6 +3,17 @@struct strbuf ; +enum branch_track { +BRANCH_TRACK_UNSPECIFIED =-1 , +BRANCH_TRACK_NEVER =0 , +BRANCH_TRACK_REMOTE , +BRANCH_TRACK_ALWAYS , +BRANCH_TRACK_EXPLICIT , +BRANCH_TRACK_OVERRIDE +}; + +extern enum branch_track git_branch_track; +/* Functions for acting on the information about branches. */ /*
cache.h +0 -10
@@ -919,15 +919,6 @@ enum log_refs_config { };extern enum log_refs_config log_all_ref_updates; -enum branch_track { -BRANCH_TRACK_UNSPECIFIED =-1 , -BRANCH_TRACK_NEVER =0 , -BRANCH_TRACK_REMOTE , -BRANCH_TRACK_ALWAYS , -BRANCH_TRACK_EXPLICIT , -BRANCH_TRACK_OVERRIDE -}; -enum rebase_setup_type {AUTOREBASE_NEVER =0 ,AUTOREBASE_LOCAL ,
@@ -944,7 +935,6 @@ enum push_default_type { PUSH_DEFAULT_UNSPECIFIED }; -extern enum branch_track git_branch_track;extern enum rebase_setup_type autorebase;extern enum push_default_type push_default;
config.c +1 -0
@@ -6,6 +6,7 @@* */ #include "cache.h" +#include "branch.h" #include "config.h" #include "repository.h" #include "lockfile.h"
environment.c +1 -0
@@ -8,6 +8,7 @@* are. */ #include "cache.h" +#include "branch.h" #include "repository.h" #include "config.h" #include "refs.h"
Signature · 1 line
2.18.0.549.gd4454f3f9b
Re: [PATCHv3 3/6] Move definition of enum branch_track from cache.h to branch.h
Quoted 5 lines · Elijah Newren
'
branch_track' feels more closely related to branching, and it is needed later inbranch.h; rather than #include'ingcache.hinbranch.hfor this small enum, just move the enum and the external declaration forgit_branch_tracktobranch.h.
Quoted 7 lines
Signed-off-by Elijah Newren <[email protected]>--- branch.h | 11 +++++++++++ cache.h | 10 ---------- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-)
\o/
PATCH v3 4/6 urlmatch.h: fix include guard
In reply to Elijah Newren--- urlmatch.h | 2 ++ 1 file changed, 2 insertions(+)
urlmatch.h +2 -0
@@ -1,4 +1,6 @@#ifndef URL_MATCH_H +#define URL_MATCH_H +#include "string-list.h" struct url_info {
Signature · 1 line
2.18.0.549.gd4454f3f9b
Re: [PATCHv3 4/6] urlmatch.h: fix include guard
Quoted 4 lines · Elijah Newren
Signed-off-by Elijah Newren <[email protected]>--- urlmatch.h | 2 ++ 1 file changed, 2 insertions(+)
This has two benefits:
- avoids the definitions in this header from being evaluated twice
- many compilers recognize the #include guard idiom and also are able to avoid re-reading the text of the header twice, too
The header guard had been broken since day one. Is there a tool that can detect this class of problem?
PATCH v3 5/6 compat/precompose_utf8.h: use more common include guard style
In reply to Elijah Newren--- compat/precompose_utf8.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
compat/precompose_utf8.h +2 -1
@@ -1,4 +1,6 @@#ifndef PRECOMPOSE_UNICODE_H +#define PRECOMPOSE_UNICODE_H +#include <sys/stat.h> #include <sys/types.h> #include <dirent.h>
@@ -41,5 +43,4 @@ int precompose_utf8_closedir(PREC_DIR *dirp);#define DIR PREC_DIR#endif /* PRECOMPOSE_UNICODE_C */ -#define PRECOMPOSE_UNICODE_H #endif /* PRECOMPOSE_UNICODE_H */
Signature · 1 line
2.18.0.549.gd4454f3f9b
Re: [PATCHv3 5/6] compat/precompose_utf8.h: use more common include guard style
Quoted 4 lines · Elijah Newren
Signed-off-by Elijah Newren <[email protected]>--- compat/precompose_utf8.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
The more unusual style is less likely to be recognized by compilers, so we can waste some I/O re-reading the header at compile time.
compat/precompose_utf8.h line 6
Quoted 9 lines
compat/precompose_utf8.h +2
-0@@ -1,4 +1,6 @@#ifndef PRECOMPOSE_UNICODE_H +#define PRECOMPOSE_UNICODE_H +#include <sys/stat.h> #include <sys/types.h> #include <dirent.h>
Not about this patch: these headers are #include-d in git-compat-util.h which we assume to be already included first before anything else. Can we remove these redundant #includes?
PATCH v3 6/6 Add missing includes and forward declares
In reply to Elijah Newren--- bisect.h | 2 ++ pack-objects.h | 1 + 2 files changed, 3 insertions(+)
bisect.h +2 -0
@@ -1,6 +1,8 @@#ifndef BISECT_H #define BISECT_H +struct commit_list ; +/* * Find bisection. If something is found, `reaches` will be the number of * commits that the best commit reaches. `all` will be the count of
pack-objects.h +1 -0
@@ -2,6 +2,7 @@#define PACK_OBJECTS_H #include "object-store.h" +#include "pack.h" #define DEFAULT_DELTA_CACHE_SIZE (256 * 1024 * 1024)
Signature · 1 line
2.18.0.549.gd4454f3f9b
Hi,
Quoted 5 lines · Elijah Newren
Signed-off-by Elijah Newren <[email protected]>--- bisect.h | 2 ++ pack-objects.h | 1 + 2 files changed, 3 insertions(+)
Do you have more context about why these are in a separate commit?
For pack-objects.h, I think you might be making a fix to the cc/delta-islands topic. But for bisect.h, I don't see any recent relevant changes.
Curious,
Jonathan