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

>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.