Re: [PATCHv6 13/14] Allow flexible organization of notes trees, using both commit date and SHA1
- From
Johan Herland <johan@herland.net>
- Date
- Sep 12, 2009, 22:33 UTC
- Message-ID
- <200909130033.28666.johan@herland.net>
- In-Reply-To
- <7viqfof1kc.fsf@alter.siamese.dyndns.org>
On Saturday 12 September 2009, Junio C Hamano wrote:
Show 6 quoted lines
> 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:
Show 20 quoted lines
> @@ -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?
Show 12 quoted lines
> @@ -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
Show 34 quoted lines
> + 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.
Show 16 quoted lines
> @@ -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