Re: [RFC/PATCHv8 08/10] fast-import: Proper notes tree manipulation using the notes API

On Thursday 26 November 2009, Shawn O. Pearce wrote:

Johan Herland [off-list ref] wrote:

quoted

This patch teaches 'git fast-import' to use the notes API to
organize

...

quoted

This patch is substantially different from the previous iteration.
Unloading (and reloading) the notes tree along with its
corresponding branch was relatively straightforward to fix, but
avoiding the destroying and re-adding of all the notes in every
commit was much harder. After 3-4 attempts at a simpler (but
fundamentally broken) approach, I finally landed on this. I'm not
satisfied with the amount of code introduced by this patch, and
would be happy if someone found a better/shorter/more elegant way
to solve this problem.

Yea, I agree, I'm not happy with the amount of complex code added
to implement this.  But I can't say there's a better way to do it
and still reuse the notes code.  Maybe its just worth breaking away
from the notes code altogether?  fast-import also implements its
own pack formatting functions because reusing them from pack-objects
was just too ugly.

Ok, I will attempt to redo the patch without reusing the notes code. I 
have couple of ideas on how to get it done, but probably won't have the 
time to implement them until next week...

Aside from a few minor nits below though, I could ACK this, it at
least avoids the nasty corners that can arise when there are a lot
of branches and tries to minimize the cost when there are many notes.

quoted

diff --git a/fast-import.c b/fast-import.c
+
+static void add_to_replace_list(
+		struct tree_entry_replace **replace_list,
+		const char *old_path, const char *new_path)
+{
+	struct tree_entry_replace *r = (struct tree_entry_replace *)
+		xmalloc(sizeof(struct tree_entry_replace));
+	r->next = (*replace_list)->next;
+	r->old_path = xstrdup(old_path);
+	r->new_path = xstrdup(new_path);
+	(*replace_list)->next = r;
+	*replace_list = r;

Really?  I don't get why you are replacing the head's next with r
only to then replace head itself with r.

quoted

@@ -2265,6 +2540,18 @@ static void parse_new_commit(void)
 			break;
 	}

+	if (notes) {
+		/* reconcile diffs between b->branch_tree and the notes tree */
+		struct reconcile_notes_tree_helper_data d;
+		struct tree_entry_replace *replace_list =
+			xcalloc(1, sizeof(struct tree_entry_replace));

Oh, I see.  The issue I had with understanding add_to_replace_list()
is due to this spot allocating a blank header node.  Normally we do
this with a pointer to a pointer and initialize NULL:

	struct tree_entry_replace *list = NULL;
	struct tree_entry_replace **replace_list = &list;

Can we avoid this blank header node?  I think it comlicates the code,
e.g. in process_replace_list() you have to skip over the blank node
by testing for both paths being NULL.

The main problem is that I need to grow a linked list at the far end, 
while keeping a reference to the first element, so that 
process_replace_list() traverse the list in the correct order (FIFO). 
However, I agree that my approach to doing so may have been somewhat 
ham-fisted... Will redo (unless the alternative approach above renders 
this code obsolete).

BTW, while we're on the topic, this whole code is only present because I 
assume it's not possible to edit the fast-import tree structure _while_ 
traversing it. Is this assumption correct, or are there ways to get 
around maintaining a separate edit list that is applied to the tree 
structure afterwards?


Thanks for the review! :)

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net