Re: [PATCH 10/16] pack-objects: use bitmaps when packing objects

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2013, 23:06 UTC
Message-ID: <7v7ghhzt73.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1372116193-32762-11-git-send-email-tanoku@gmail.com>

Vicent Marti <tanoku@gmail.com> writes:

Show 6 quoted lines

> @@ -83,6 +84,9 @@ static struct progress *progress_state;
>  static int pack_compression_level = Z_DEFAULT_COMPRESSION;
>  static int pack_compression_seen;
>  
> +static int bitmap_support;
> +static int use_bitmap_index;

OK.

Show 8 quoted lines

> @@ -2131,6 +2135,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
>  		cache_max_small_delta_size = git_config_int(k, v);
>  		return 0;
>  	}
> +	if (!strcmp(k, "pack.usebitmaps")) {
> +		bitmap_support = git_config_bool(k, v);
> +		return 0;
> +	}

Hmph, so bitmap_support, not use_bitmap_index, keeps track of the user request? Somewhat confusing.

Show 12 quoted lines

>  	if (!strcmp(k, "pack.threads")) {
>  		delta_search_threads = git_config_int(k, v);
>  		if (delta_search_threads < 0)
> @@ -2366,8 +2374,24 @@ static void get_object_list(int ac, const char **av)
>  			die("bad revision '%s'", line);
>  	}
>  
> +	if (use_bitmap_index) {
> +		uint32_t size_hint;
> +
> +		if (!prepare_bitmap_walk(&revs, &size_hint)) {
> +			khint_t new_hash_size = (size_hint * (1.0 / __ac_HASH_UPPER)) + 0.5;

What is __ac_HASH_UPPER? That is a very unusual name for a variable or a constant. Also it is mildly annoying to see unnecessary use of float like this.

Show 22 quoted lines

> +			kh_resize_sha1(packed_objects, new_hash_size);
> +
> +			nr_alloc = (size_hint + 63) & ~63;
> +			objects = xrealloc(objects, nr_alloc * sizeof(struct object_entry *));
> +
> +			traverse_bitmap_commit_list(&add_object_entry_1);
> +			return;
> +		}
> +	}
> +
>  	if (prepare_revision_walk(&revs))
>  		die("revision walk setup failed");
> +
>  	mark_edges_uninteresting(revs.commits, &revs, show_edge);
>  	traverse_commit_list(&revs, show_commit, show_object, NULL);
>  
> @@ -2495,6 +2519,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			    N_("pack compression level")),
>  		OPT_SET_INT(0, "keep-true-parents", &grafts_replace_parents,
>  			    N_("do not hide commits by grafts"), 0),
> +		OPT_BOOL(0, "bitmaps", &bitmap_support,
> +			 N_("enable support for bitmap optimizations")),

Please match this with the name of configuration variable, i.e. --use-bitmaps

Show 10 quoted lines

>  		OPT_END(),
>  	};
>  
> @@ -2561,6 +2587,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	if (keep_unreachable && unpack_unreachable)
>  		die("--keep-unreachable and --unpack-unreachable are incompatible.");
>  
> +	if (bitmap_support) {
> +		if (use_internal_rev_list && pack_to_stdout)
> +			use_bitmap_index = 1;

OK, so only when some internal condition is met, the user request to use bitmap is honored and the deision is kept in use_bitmap_index.

It may be easier to read if you get rid of bitmap_support, set user_bitmap_index directly from the command line and config, and did this here instead:

	if (!(use_internal_rev_list && pack_to_stdout))
		use_bitmap_index = 0;