Re: [PATCHv6 13/14] Allow flexible organization of notes trees, using both commit date and SHA1

[Johan Herland <johan@herland.net>]

On Saturday 12 September 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > This is a major expansion of the notes lookup code to allow for
> > variations in the notes tree organization. The variations allowed
> > include mixing fanout schemes based on the commit dates of the
> > annotated commits (aka. date-based fanout) with fanout schemes based on
> > the SHA1 of the annotated commits (aka. SHA1-based fanout).

Note that this patch is about to be removed from this series, cf. today's 
discussion with Shawn elsewhere in this thread.

> Will squash this in.

I agree with the attempt, but not all of the hunks are good:

> @@ -281,20 +281,20 @@ static int note_tree_insert(struct int_node *tree,
>  unsigned char n, /* Free the entire notes data contained in the given
>  tree */
>  static void note_tree_free(struct int_node *tree)
>  {
> -	if (tree->magic == (void *) ~0) {
> -		if (tree->prev) {
> -			note_tree_free(tree->prev);
> -			free(tree->prev);
> +	if (tree->u.s.magic == (void *) ~0) {
> +		if (tree->u.s.prev) {
> +			note_tree_free(tree->u.s.prev);
> +			free(tree->u.s.prev);
>  		}
> -		if (tree->child) {
> -			note_tree_free(tree->child);
> -			free(tree->child);
> +		if (tree->u.s.magic) {
> +			note_tree_free(tree->u.s.magic);
> +			free(tree->u.s.magic);

Here, you are replacing tree->child with tree->u.s.magic. Shouldn't that be 
tree->u.s.child instead?

> @@ -439,12 +439,12 @@ static void load_date_subtree(struct tree_desc
>  *tree_desc, else  /* this is the last entry, store directly into node */
>  			new_node = node;
> 
> -		new_node->magic = (void *) ~0;
> -		new_node->child = NULL;
> -		new_node->prev = cur_node;
> -		new_node->parent = parent;
> -		hashcpy(new_node->tree_sha1, entry.sha1);
> -		strcpy(new_node->period, period);
> +		new_node->u.s.magic = (void *) ~0;
> +		new_node->u.s.magic = NULL;

Same as above: new_node->u.s.child

> +		new_node->u.s.prev = cur_node;
> +		new_node->u.s.parent = parent;
> +		hashcpy(new_node->u.s.tree_sha1, entry.sha1);
> +		strcpy(new_node->u.s.period, period);
>  		cur_node = new_node;
>  	}
>  	assert(!cur_node || cur_node == node);
> @@ -552,38 +552,38 @@ static unsigned char *lookup_notes(const struct
>  commit *commit) /* Convert commit->date to YYYY-MM-DD format */
>  	short_date = show_date(commit->date, 0, DATE_SHORT);
> 
> -	while (node->magic == (void *) ~0) {  /* date-based node */
> -		int cmp = SUBTREE_DATE_PREFIXCMP(short_date, node->period);
> +	while (node->u.s.magic == (void *) ~0) {  /* date-based node */
> +		int cmp = SUBTREE_DATE_PREFIXCMP(short_date, node->u.s.period);
>  		if (cmp == 0) {
>  			/* Search inside child node */
> -			if (!node->child) {
> +			if (!node->u.s.magic) {
>  				/* Must unpack child node first */
> -				node->child = (struct int_node *)
> +				node->u.s.magic = (struct int_node *)
>  					xcalloc(sizeof(struct int_node), 1);
> -				load_subtree(node->tree_sha1,
> -					(const unsigned char *) node->period,
> -					strlen(node->period), node->child,
> +				load_subtree(node->u.s.tree_sha1,
> +					(const unsigned char *) node->u.s.period,
> +					strlen(node->u.s.period), node->u.s.magic,
>  					node, -1);
>  			}
>  			seen_node = node;
> -			node = node->child;
> +			node = node->u.s.magic;

Same again, 4 times in the above hunk.

> @@ -591,15 +591,15 @@ static unsigned char *lookup_notes(const struct
>  commit *commit) }
>  	}
>  	while (cur_node &&
> -	       SUBTREE_DATE_PREFIXCMP(cur_node->period, seen_node->period) < 0)
> +	       SUBTREE_DATE_PREFIXCMP(cur_node->u.s.period,
>  seen_node->u.s.period) < 0) {
>  		/*
>  		 * We're about to move cur_node backwards in history. We are
>  		 * unlikely to need this cur_node in the future, so free() it.
>  		 */
> -		note_tree_free(cur_node->child);
> -		cur_node->child = NULL;
> -		cur_node = cur_node->parent;
> +		note_tree_free(cur_node->u.s.magic);
> +		cur_node->u.s.magic = NULL;

...and another one here.

> +		cur_node = cur_node->u.s.parent;
>  	}
>  	cur_node = seen_node;
> 


But as I said above, you may want to drop 13/14 and 14/14 completely, 
instead.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net