From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 21 Oct 2010 05:21:00 GMT
Subject: Re: [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond
Message-ID: <20101021052100.GB2413@burratino>
In-Reply-To: <1287626936-32232-7-git-send-email-johan@herland.net>

Johan Herland wrote:

> --- a/notes.c
> +++ b/notes.c
> @@ -303,13 +298,17 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
>  	       GET_PTR_TYPE(*p) == PTR_TYPE_SUBTREE);
>  	if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty note */
>  		free(entry);
> -		return;
> +		return 0;
>  	}
>  	new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
> -	note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> -			 combine_notes);
> -	*p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> -	note_tree_insert(t, new_node, n + 1, entry, type, combine_notes);
> +	ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> +			       combine_notes);
> +	if (!ret) {
> +		*p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> +		ret = note_tree_insert(t, new_node, n + 1, entry, type,
> +				       combine_notes);
> +	}
> +	return ret;

Micronit: it would probably be clearer to write

	if (ret)
		return ret;
	*p = SET_PTR_TYPE(...
	return note_tree_insert(...

to avoid a little nesting.

The patch still looks good to me.