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

[Johan Herland <johan@herland.net>]

On Tuesday 5. October 2010 17.21.20 Jonathan Nieder wrote:
> 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?

Yes. This will be done in the next iteration.

> > --- 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:
> 
> > @@ -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.
> 
> >  				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.

All correct.

> 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.

Indeed, that is the main point. I believe the paragraph following the
commit subject did indeed explain this, but I will try to clarify this
further in the next iteration.

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

Thanks,

...Johan


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