Re: [PATCH 4/6 (v2)] administrative functions for rev-cache, and start of integration into git

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2009, 18:36 UTC
Message-ID: <7vtz0g7s1p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <op.uyb1vcvytdk399@sirnot.private>

"Nick Edelen" <sirnot@gmail.com> writes:

Note that I didn't read the entire patch, so the parts I did not comment on in this message may or may not have problematic parts.

>  - extra 'size' field added to commit, tag and blob objects, initialized in 
>    parse_* functions and cache traversal

It is unclear which "size" this field refers to without comment.

I do not know if this change is justifiable. We (mostly Linus) spent considerble effort to shrink the memory footprint of struct commit (and struct object in general) exactly because revision traversal needs to inspect tons of them.

Show 21 quoted lines

> +static unsigned int parse_size(const char *name)
> +{
> +	unsigned int size;
> +	char *p;
> +	
> +	size = strtol(name, &p, 10);
> +	switch (*p) {
> +	case 'k' : 
> +	case 'K' : 
> +		size *= 0x400;
> +		break;
> +	case 'm' : 
> +	case 'M' : 
> +		size *= 0x100000;
> +		break;
> +	default : 
> +		break;
> +	}
> +	
> +	return size;
> +}

Looks vaguely familiar.

The configuration parser already knows about these size suffixes when told to read "int". Can't that code, e.g. git_parse_ulong(), be reused here?

Show 7 quoted lines

> @@ -219,7 +221,6 @@ unsigned char *get_cache_slice(struct commit *commit)
>  		return 0;
>  	
>  	ie = search_index(commit->object.sha1);
> -	
>  	if (ie && ie->cache_index < idx_head.cache_nr)
>  		return idx_caches + ie->cache_index * 20;

"Oops, fix an earlier mistake"...

Show 5 quoted lines

> @@ -262,7 +282,6 @@ static int setup_traversal(struct cache_slice_header *head, unsigned char *map,
>  	struct commit_list *prev, *wp, **wpp;
>  	int retval;
>  	
> -	/* printf("adding %s\n", sha1_to_hex(commit->object.sha1)); */

Likewise.