Re: [PATCH v3 2/4] format-patch: add '--base' option to record base tree info

On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:

quoted

diff --git a/builtin/log.c b/builtin/log.c
index 0d738d6..03cbab0 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -1185,6 +1185,82 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+struct base_tree_info {
+	struct object_id base_commit;
+	int nr_patch_id, alloc_patch_id;
+	struct object_id *patch_id;
+};
+
+static void prepare_bases(struct base_tree_info *bases,
+			  const char *base_commit,
+			  struct commit **list,
+			  int total)
+{
+	struct commit *base = NULL, *commit;
+	struct rev_info revs;
+	struct diff_options diffopt;
+	struct object_id *patch_id;
+	unsigned char sha1[20];
+	int i;
+
+	diff_setup(&diffopt);
+	DIFF_OPT_SET(&diffopt, RECURSIVE);
+	diff_setup_done(&diffopt);
+
+	base = lookup_commit_reference_by_name(base_commit);
+	if (!base)
+		die(_("Unknown commit %s"), base_commit);
+	oidcpy(&bases->base_commit, &base->object.oid);
+
+	init_revisions(&revs, NULL);
+	revs.max_parents = 1;
+	base->object.flags |= UNINTERESTING;
+	add_pending_object(&revs, &base->object, "base");
+	for (i = 0; i < total; i++) {
+		list[i]->object.flags |= 0;

What does this statement do, exactly?  Are you clearing some bits
but not others, and if so which ones?

quoted

+		add_pending_object(&revs, &list[i]->object, "rev_list");
+		list[i]->util = (void *)1;

Are we sure commit objects not on the list have their ->util cleared?
The while() loop below seems to rely on that to correctly filter out
the ones that are on the list.

After some investigation and according to my understanding, the commit
object is allocated through alloc_commit_node->alloc_node, 

void *alloc_commit_node(void)
{
	struct commit *c = alloc_node(&commit_state, sizeof(struct commit));
	c->object.type = OBJ_COMMIT;
	c->index = alloc_commit_index();
	return c;
}

static inline void *alloc_node(struct alloc_state *s, size_t node_size)
{
	void *ret;

	if (!s->nr) {
		s->nr = BLOCKING;
		s->p = xmalloc(BLOCKING * node_size);
	}
	s->nr--;
	s->count++;
	ret = s->p;
	s->p = (char *)s->p + node_size;
	memset(ret, 0, node_size);
	return ret;
}

So the commit->util should be cleared after initialization, and it has
not been touched except above "for" loop in our code execution path, I think
it is safe to rely on it to filter out commits that are on the rev list.

Thanks,
Xiaolong.

quoted

+	}
+
+	if (prepare_revision_walk(&revs))
+		die(_("revision walk setup failed"));
+	/*
+	 * Traverse the prerequisite commits list,
+	 * get the patch ids and stuff them in bases structure.
+	 */
+	while ((commit = get_revision(&revs)) != NULL) {
+		if (commit->util)
+			continue;
+		if (commit_patch_id(commit, &diffopt, sha1))
+			die(_("cannot get patch id"));
+		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
+		patch_id = bases->patch_id + bases->nr_patch_id;
+		hashcpy(patch_id->hash, sha1);