Re: [RFC/PATCHv8 08/10] fast-import: Proper notes tree manipulation using the notes API
From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:47:46
Johan Herland [off-list ref] wrote:
This patch teaches 'git fast-import' to use the notes API to organize
...
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. 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 hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
@@ -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. -- Shawn.