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

[Chris Packham <judge.packham@gmail.com>]

Hi,

On 12/02/14 14:57, Stefan Zager wrote:
> From b4796d9d99c03b0b7cddd50808a41413e45f1129 Mon Sep 17 00:00:00 2001
> From: Stefan Zager <szager@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 <szager@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(-)

I'm not really qualified to comment on substance but there are some
basic style issues w.r.t. whitespace namely using 4 spaces for indent
and mixing tabs/spaces. This might seem pedantic for the first round of
a patch but it does put off reviewers.

>From Documentation/CodingGuidelines:

 - We use tabs to indent, and interpret tabs as taking up to
   8 spaces.