Re: [PATCH 03/18] notes.h/c: Clarify the handling of notes objects that are == null_sha1

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 5, 2010, 15:21 UTC
Message-ID: <20101005152120.GE12797@burratino>
In-Reply-To: <1285719811-10871-4-git-send-email-johan@herland.net>

Johan Herland wrote:

> The only functional changes in this patch concern the handling of null_sha1
> in notes_tree_insert(). Otherwise the patch consists solely of reordering
> functions in notes.c to avoid use-before-declaration

Would it makes sense to split off that no-op as a separate patch?

Show 14 quoted lines

> --- a/notes.c
> +++ b/notes.c
> @@ -175,7 +248,10 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
>  	switch (GET_PTR_TYPE(*p)) {
>  	case PTR_TYPE_NULL:
>  		assert(!*p);
> -		*p = SET_PTR_TYPE(entry, type);
> +		if (is_null_sha1(entry->val_sha1))
> +			free(entry);
> +		else
> +			*p = SET_PTR_TYPE(entry, type);
>  		return;
>  	case PTR_TYPE_NOTE:
>  		switch (type) {

No note present, but the node for one is. This skips insertion of empty notes, for consistency with:

Show 7 quoted lines

> @@ -191,6 +267,9 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
>  					    sha1_to_hex(l->val_sha1),
>  					    sha1_to_hex(entry->val_sha1),
>  					    sha1_to_hex(l->key_sha1));
> +
> +				if (is_null_sha1(l->val_sha1))
> +					note_tree_remove(t, tree, n, entry);

The note-present case, where the combine_notes() function can return a null sha1 to request that a note be removed.

Show 11 quoted lines

>  				free(entry);
>  				return;
>  			}
> @@ -222,6 +301,10 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
>  	/* non-matching leaf_node */
>  	assert(GET_PTR_TYPE(*p) == PTR_TYPE_NOTE ||
>  	       GET_PTR_TYPE(*p) == PTR_TYPE_SUBTREE);
> +	if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty note */
> +		free(entry);
> +		return;
> +	}

The more usual no-note-present case. Again, this skips insertion of empty notes.

Do I understand correctly that the point of the main point of this patch is to allow combine_notes() functions to request that a note be deleted? If so, it would be nice if the commit message said so.

Regardless, for what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>