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@xxxxxxxxx>
Thanks,
...Johan
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html