sza...@chromium.org writes:
> From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001
Please drop this line.
> From: Stefan Zager
Please drop this line and instead have it in your e-mail header.
> Date: Mon, 10 Feb 2014 16:55:12 -0800
The date in your e-mail header, which is the first time general
public saw this particular version of the patch, is used by default
as the author time. Unless there is a compelling reason to override
that with an in-body header, please drop this line.
> Subject: [PATCH] Make the global packed_git variable static to sha1_file.c.
Please drop this line and instead have it in your e-mail header.
> This patch is a pure refactor with no functional changes,...
>
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index a7f70cb..6554dfe 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -83,14 +83,32 @@ static char const * const count_objects_usage[] = {
> NULL
> };
>
> +struct pack_data {
> + unsigned long packed;
> + off_t size_pack;
> + unsigned long num_pack;
> +};
> +
> +int pack_data_fn(struct packed_git *p, void *data)
Can't/shouldn't this be static?
Also I'd suggest s/pack_data_fn/collect_pack_data/ or something.
"_fn" may be a good suffix for typedef'ed typename used in a
callback function, but for a concrete function, it only adds noise
without giving us anything new.
Yes, I know there are a few existing violators, but we
shouldn't make the codebase worse, using their existence due
to past carelessness as an excuse.
> diff --git a/cache.h b/cache.h
> index dc040fb..542a9d9 100644
> --- a/cache.h
> +++ b/cache.h
> ...
> +/* The 'hint' argument is for the commonly-used 'last found pack'
> optimization.
> + * It can be NULL.
> + */
/*
* Please try to have opening slash-asterisk and closing
* asterisk-slash in a multi-line comment block on their
* own lines by themselves.
*/
> +extern void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git
> *hint, void *data);
> +
> +extern size_t packed_git_count();
> +extern size_t packed_git_local_count();
extern size_t packed_git_count(void);
extern size_t packed_git_local_count(void);
> diff --git a/sha1_file.c b/sha1_file.c
> index 6e8c05d..aeeb7e6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -60,6 +60,7 @@ static struct cached_object empty_tree = {
> 0
> };
>
> +static struct packed_git *packed_git;
> static struct packed_git *last_found_pack;
>
> static struct cached_object *find_cached_object(const unsigned char *sha1)
> @@ -468,7 +469,6 @@ static unsigned int pack_open_fds;
> static unsigned int pack_max_fds;
> static size_t peak_pack_mapped;
> static size_t pack_mapped;
> -struct packed_git *packed_git;
Hmm, any particular reason why only this variable and not others are
moved up?
> @@ -1091,6 +1091,37 @@ struct packed_git *add_packed_git(const char *path,
> int path_len, int local)
> return p;
> }
>
> +void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint,
> void *data)
> +{
> + struct packed_git *p;
> + if (hint && ((*fn)(hint, data)))
> + return;
> + for (p = packed_git; p; p = p->next)
> + if (p != hint && (*fn)(p, data))
> + return;
(mental note) In the new API, a non-zero return signals an early
return/break from the loop.
> +}
> +
> +size_t packed_git_count()
size_t packed_git_count(void)
> +{
> + size_t res = 0;
> + struct packed_git *p;
> +
> + for (p = packed_git; p; p = p->next)
> + ++res;
When pre- or post- increment does not make any difference (i.e. you
do not use its value), please stick to post-increment. pre-increment
looks unusual in our codebase and becomes distracting while reading
it through.
Same comments for packed-git-local-count.
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 541667f..bc3074b 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -900,14 +900,45 @@ static int no_try_delta(const char *path)
> return 0;
> }
>
> +struct find_pack_data {
> + const unsigned char *sha1;
> + off_t offset;
> + struct packed_git *found_pack;
> + int exclude;
> + int found_non_local_pack;
> + int found_pack_keep;
> +};
> +
> +static int find_pack_fn(struct packed_git *p, void *data)
> +{
> + struct find_pack_data *fpd = (struct find_pack_data *) data;
> + off_t offset = find_pack_entry_one(fpd->sha1, p);
> + if (offset) {
> + if (!fpd->found_pack) {
> + if (!is_pack_valid(p)) {
> + warning("packfile %s cannot be accessed",
> p->pack_name);
> + return 0;
> + }
> + fpd->offset = offset;
> + fpd->found_pack = p;
> + }
> + if (fpd->exclude)
> + return 1;
> +
