From: Jeff King <peff@peff.net>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, sbeller@google.com
Subject: Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable
Date: Sat, 9 Jun 2018 02:04:38 -0400 [thread overview]
Message-ID: <20180609060437.GA30224@sigill.intra.peff.net> (raw)
In-Reply-To: <d4d4fec265c1c53e14da465904d93e559ff2c17d.1528397984.git.jonathantanmy@google.com>
On Thu, Jun 07, 2018 at 12:04:13PM -0700, Jonathan Tan wrote:
> Remove the bitmap_git global variable. Instead, generate on demand an
> instance of struct bitmap_index for code that needs to access it.
>
> This allows us significant control over the lifetime of instances of
> struct bitmap_index. In particular, packs can now be closed without
> worrying if an unnecessarily long-lived "pack" field in struct
> bitmap_index still points to it.
>
> The bitmap API is also clearer in that we need to first obtain a struct
> bitmap_index, then we use it.
I think this is the right direction, and overall it looks pretty good.
There's one call that gave me pause:
> -int prepare_bitmap_git(void)
> +struct bitmap_index *prepare_bitmap_git(void)
> {
> - if (bitmap_git.loaded)
> - return 0;
> + struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
This function used to be idempotent, so any code which wanted to use the
global bitmap_git could call it "just in case". After your patch, it's
not. I think it's probably OK, since such functions would generally now
take a bitmap_git argument and use that (e.g., rebuild_existing_bitmaps
works that way after your patch).
> - if (!open_pack_bitmap())
> - return load_pack_bitmap();
> + if (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))
> + return bitmap_git;
>
> - return -1;
> + return NULL;
> }
We probably need to free(bitmap_git) before returning NULL here (this is
still in prepare_bitmap_git()).
> @@ -662,12 +686,11 @@ int prepare_bitmap_walk(struct rev_info *revs)
> struct bitmap *wants_bitmap = NULL;
> struct bitmap *haves_bitmap = NULL;
>
> - if (!bitmap_git.loaded) {
> - /* try to open a bitmapped pack, but don't parse it yet
> - * because we may not need to use it */
> - if (open_pack_bitmap() < 0)
> - return -1;
> - }
> + struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
> + /* try to open a bitmapped pack, but don't parse it yet
> + * because we may not need to use it */
> + if (open_pack_bitmap(bitmap_git) < 0)
> + return NULL;
Ditto here (and probably other error returns lower in the function, but
I didn't go through it carefully).
-Peff
next prev parent reply other threads:[~2018-06-09 6:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-07 19:04 [PATCH 0/2] Object store refactoring: make bitmap_git not global Jonathan Tan
2018-06-07 19:04 ` [PATCH 1/2] pack-bitmap: remove bitmap_git global variable Jonathan Tan
2018-06-09 6:04 ` Jeff King [this message]
2018-06-11 18:50 ` Jonathan Tan
2018-06-11 21:10 ` Jeff King
2018-06-07 19:04 ` [PATCH 2/2] pack-bitmap: add free function Jonathan Tan
2018-06-09 6:06 ` Jeff King
2018-06-12 0:00 ` [PATCH 0/2] Object store refactoring: make bitmap_git not global Stefan Beller
2018-06-25 18:22 ` Brandon Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180609060437.GA30224@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).