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

[Junio C Hamano <gitster@pobox.com>]

Xiaolong Ye <xiaolong.ye@intel.com> writes:

Reviewing the patch out of order, caller first and then callee.

> +static void print_bases(struct base_tree_info *bases)
> +{
> +	int i;
> +
> +	/* Only do this once, either for the cover or for the first one */
> +	if (is_null_oid(&bases->base_commit))
> +		return;
> +
> +	printf("** base-commit-info **\n");

I am not sure if you want to have this line (an empty line might not
hurt), as the "base-commit: ..." that comes next is a clear enough
indication of what these lines are.

> +	if (base_commit) {
> +		struct commit *prerequisite_head = NULL;
> +		if (list[nr - 1]->parents)
> +			prerequisite_head = list[nr - 1]->parents->item;
> +		memset(&bases, 0, sizeof(bases));
> +		reset_revision_walk();
> +		prepare_bases(&bases, base_commit, prerequisite_head);
> +	}
> +

list[] holds the commits in reverse topological order, so the first
parent of the last element in the list[] would hopefully give you
the latest change your series depends on, and that is why you are
working on list[nr - 1] here.

I however think that is flawed.

What if your series A, B and C are on this topology?

    ---P---X---A---M---C
        \         /
         Y---Z---B

"git format-patch --base=P -3 C" would show A, B and C.  It may show
B, A and C, as A and B are independent (you would be flattening the
history into the shape you have in your documentation part of the
patch in order to adjust for their interactions before running
format-patch if that were not the case).  Depending on which one
happens to be chosen as prerequisite_head, you would miss either X
or Y & Z, wouldn't you?

A simpler and safer (but arguably less useful) approach may be to
use the logic and limitation of your patch as-is but add code to
check that the history is linear and refuse to do the "base" thing.
But just in case you want to handle a more general case like the
above, read on.

> +static void prepare_bases(struct base_tree_info *bases,
> +			  const char *base_commit,
> +			  struct commit *prerequisite_head)
> +{
> +	struct commit *base = NULL, *commit;
> +	struct rev_info revs;
> +	struct diff_options diffopt;
> +	struct object_id *patch_id;
> +	unsigned char sha1[20];
> +	int pos = 0;
> +
> +	if (!prerequisite_head)
> +		return;
> +	base = lookup_commit_reference_by_name(base_commit);
> +	if (!base)
> +		die(_("Unknown commit %s"), base_commit);
> +	oidcpy(&bases->base_commit, &base->object.oid);
> +
> +	if (base == prerequisite_head)
> +		return;
> +
> +	if (!in_merge_bases(base, prerequisite_head))
> +		die(_("base commit should be the ancestor of revs you specified"));
> +
> +	init_revisions(&revs, NULL);
> +	revs.max_parents = 1;
> +
> +	base->object.flags |= UNINTERESTING;
> +	add_pending_object(&revs, &base->object, "base");
> +	prerequisite_head->object.flags |= 0;
> +	add_pending_object(&revs, &prerequisite_head->object, "prerequisite-head");
> +
> +	diff_setup(&diffopt);
> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
> +	diff_setup_done(&diffopt);
> +
> +	if (prepare_revision_walk(&revs))
> +		die(_("revision walk setup failed"));
> +	/*
> +	 * Traverse the commits list between base and prerequisite head,
> +	 * get the patch ids and stuff them in bases structure.
> +	 */
> +	while ((commit = get_revision(&revs)) != NULL) {
> +		if (commit_patch_id(commit, &diffopt, sha1))
> +			return;
> +		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
> +		patch_id = bases->patch_id + pos;
> +		hashcpy(patch_id->hash, sha1);
> +		pos++;
> +		bases->nr_patch_id++;

Micronit.  Aren't pos and nr_patch_id always the same?

> +	}
> +}

I think the right thing to do is probably to start another revision
walk (which you do) but setting the starting points of the traversal
to all elements in the list[] (which you don't--you use either A^ or
B^).  And skip the ones in the list[] before grabbing its patch-id
in the loop.  That way, you do not have to worry about the topology
of the history tripping you up at all.

So I'd suggest to redo this function perhaps like so, if you do want
to handle the more general case:

static void prepare_bases(struct base_tree_info *bases,
			  const char *base_commit,
			  struct commit **list,
                          int total)
{
	... boilerplate ...

	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;
	add_pending_commit(&revs, base, UNINTERESTING);
	for (i = 0; i < total; i++)
        	add_pending_commit(&revs, list[i], 0);

	if (prepare_revision_walk(&revs))
		die(_("revision walk setup failed"));

	while ((commit = get_revision(&revs)) != NULL) {
        	if (COMMIT_IS_IN_LIST(commit))
                	continue;
		if (commit_patch_id(commit, &diffopt, sha1))
                	die("cannot get patch id");
		... do your ptach_id thing ...
	}
}

And COMMIT_IS_IN_LIST() can probably be implemented by using commit->util
field, e.g. change the part that sets up the traversal

	for (i = 0; i < total; i++) {
        	add_pending_commit(&revs, list[i], 0);
		list[i]->util = (void *)1;
	}

to mark those in list[] as such, and the test would be

		if (commit->util)
                	continue; /* on list[] */

or something like that.