Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.

[Stefan Zager]

I uploaded a new patch; a few comments inline below...

On Wed, Feb 12, 2014 at 1:19 PM, Junio C Hamano gits...@pobox.com wrote:
 sza...@chromium.org writes:

 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.

Fixed this everywhere.

  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?

No, just need packed_git declared before use.  I moved all the static
variables up, for clarity.

 + foreach_packed_git(find_pack_fn, NULL, fpd);
 + if (fpd.found_pack  !exclude 
 + (incremental ||
 +  (local  fpd.found_non_local_pack) ||
 +  (ignore_packed_keep  fpd.found_pack_keep)))
 + return 0;

 When told to do --incremental, we used to return 0 from this
 function immediately once we find the object in one pack, without
 going thru the list of packs.  Now we let foreach to loop thru all
 of them and then return 0.  Does this difference matter?  A similar
 difference may exist for local/keep but I did not think it through.

Fixed.



Thanks,

Stefan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Make the global packed_git variable static to sha1_file.c.

[szager]

From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@chromium.org
Date: Mon, 10 Feb 2014 16:55:12 -0800
Subject: [PATCH] Make the global packed_git variable static to sha1_file.c.

This is a first step in making the codebase thread-safe.  By and
large, the operations which might benefit from threading are those
that work with pack files (e.g., checkout, blame), so the focus of
this patch is stop leaking the global list of pack files outside of
sha1_file.c.

The next step will be to control access to the list of pack files
with a mutex.  However, that alone is not enough to make pack file
access thread safe.  Even in a read-only operation, the window list
associated with each pack file will need to be controlled.
Additionally, the global counters in sha1_file.c will need to be
controlled.

This patch is a pure refactor with no functional changes, so it
shouldn't require any additional tests.  Adding the actual locks
will be a functional change, and will require additional tests.

Signed-off-by: Stefan Zager sza...@chromium.org
---
 builtin/count-objects.c  |  44 ++-
 builtin/fsck.c   |  46 +++-
 builtin/gc.c |  26 +++
 builtin/pack-objects.c   | 188 ---
 builtin/pack-redundant.c |  37 +++---
 cache.h  |  16 +++-
 fast-import.c|   4 +-
 http-backend.c   |  28 ---
 http-push.c  |   4 +-
 http-walker.c|   2 +-
 pack-revindex.c  |  20 ++---
 server-info.c|  35 +
 sha1_file.c  |  35 -
 sha1_name.c  |  18 -
 14 files changed, 315 insertions(+), 188 deletions(-)

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)
+{
+   struct pack_data *pd = (struct pack_data *) data;
+   if (p-pack_local  !open_pack_index(p)) {
+   pd-packed += p-num_objects;
+   pd-size_pack += p-pack_size + p-index_size;
+   pd-num_pack++;
+   }
+   return 0;
+}
+
 int cmd_count_objects(int argc, const char **argv, const char *prefix)
 {
int i, verbose = 0, human_readable = 0;
const char *objdir = get_object_directory();
int len = strlen(objdir);
char *path = xmalloc(len + 50);
-   unsigned long loose = 0, packed = 0, packed_loose = 0;
+   unsigned long loose = 0, packed_loose = 0;
off_t loose_size = 0;
+   struct pack_data pd = {0,0,0};
struct option opts[] = {
OPT__VERBOSE(verbose, N_(be verbose)),
OPT_BOOL('H', human-readable, human_readable,
@@ -118,41 +136,29 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
closedir(d);
}
if (verbose) {
-   struct packed_git *p;
-   unsigned long num_pack = 0;
-   off_t size_pack = 0;
struct strbuf loose_buf = STRBUF_INIT;
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
-   if (!packed_git)
-   prepare_packed_git();
-   for (p = packed_git; p; p = p-next) {
-   if (!p-pack_local)
-   continue;
-   if (open_pack_index(p))
-   continue;
-   packed += p-num_objects;
-   size_pack += p-pack_size + p-index_size;
-   num_pack++;
-   }
+   prepare_packed_git();
+   foreach_packed_git(pack_data_fn, NULL, pd);
 
if (human_readable) {
strbuf_humanise_bytes(loose_buf, loose_size);
-   strbuf_humanise_bytes(pack_buf, size_pack);
+   strbuf_humanise_bytes(pack_buf, pd.size_pack);
strbuf_humanise_bytes(garbage_buf, size_garbage);
} else {
strbuf_addf(loose_buf, %lu,
(unsigned long)(loose_size / 1024));
strbuf_addf(pack_buf, %lu,
-   (unsigned long)(size_pack / 1024));
+   (unsigned long)(pd.size_pack / 1024));
strbuf_addf(garbage_buf, %lu,
(unsigned long)(size_garbage / 1024));
}
 
printf(count: %lu\n, loose);
printf(size: %s\n, loose_buf.buf);
-   printf(in-pack: %lu\n, packed);
- 

Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.

[Junio C Hamano]

I'll locally fix up these style issues before commenting on the patch.

Thanks.

ERROR: space required after that ',' (ctx:VxV)
#78: FILE: builtin/count-objects.c:111:
+   struct pack_data pd = {0,0,0};
^

ERROR: space required after that ',' (ctx:VxV)
#78: FILE: builtin/count-objects.c:111:
+   struct pack_data pd = {0,0,0};
  ^

ERROR: space required after that ',' (ctx:VxV)
#171: FILE: builtin/fsck.c:683:
+   struct verify_packs_data vpd = {0,0,0};
 ^

ERROR: space required after that ',' (ctx:VxV)
#171: FILE: builtin/fsck.c:683:
+   struct verify_packs_data vpd = {0,0,0};
   ^

ERROR: space required after that ',' (ctx:VxV)
#541: FILE: builtin/pack-redundant.c:591:
+   struct add_pack_data apd = {filename,0,NULL};
^

ERROR: space required after that ',' (ctx:VxV)
#541: FILE: builtin/pack-redundant.c:591:
+   struct add_pack_data apd = {filename,0,NULL};
  ^

ERROR: space required after that ',' (ctx:VxV)
#565: FILE: builtin/pack-redundant.c:604:
+   struct add_pack_data apd = {NULL,0,NULL};
^

ERROR: space required after that ',' (ctx:VxV)
#565: FILE: builtin/pack-redundant.c:604:
+   struct add_pack_data apd = {NULL,0,NULL};
  ^

ERROR: space prohibited after that '-' (ctx:WxW)
#726: FILE: pack-revindex.c:47:
+   num = - 1 - num;
  ^

ERROR: space required after that ',' (ctx:VxV)
#901: FILE: sha1_name.c:195:
+   struct disambiguate_data d = {len,bin_pfx,ds};
 ^

ERROR: space required after that ',' (ctx:VxV)
#901: FILE: sha1_name.c:195:
+   struct disambiguate_data d = {len,bin_pfx,ds};
 ^

total: 11 errors, 0 warnings, 781 lines checked
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.

[Junio C Hamano]

sza...@chromium.org writes:

 From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001

Please drop this line.

 From: Stefan Zager sza...@chromium.org

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;
 + if (!p-pack_local)
 + fpd-found_non_local_pack = 1;
 + else if