Re: [PATCH 2/2] merge-recursive: move make_virtual_commit()'s virtual_id to merge_options

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

I do not think this one is a good idea.

What does it mean for a two "virtual commits" that are returned from
separate calls to make_virtual_commit() to have the same virtual_id?

I do not know offhand if the existing code does something like:

	commit = lookup_commit(commit->object.sha1)

or

	if (hashcmp(commit1->object.sha1, commit2->object.sha1))
        	...

but if there is such a code, I think this change makes the problem
"virtual id" has even worse.  With the current single function local
static "virtual_id", at least during a program's lifetime it is guaranteed
that there won't be any two instances of virtual commits created for
different purposes that share the same (fake) sha1 value.  If you call
merge_recursive more than once in your process, using a fresh "struct
merge_options" each time, that guarantee is lost with your change.

The only purpose of a "virtual commit", as I understand it, is to allow
you to pass a tree resulting from an internal merge to a function that
expects you to call with three commit objects to come up with a new tree
that is the result of the merge.  The code stores a "virtual_id" as a
phoney sha1 value in the object, but I do not think the actual value is
used for anything but debugging purposes (Alex, Dscho, please correct me
as necessary).

Does it hurt if we get rid of virtual_id and always leave the
object->sha1 field of virtual commits 0{40} as it is initialized?

I further suspect we _could_ fix the API that requires you to pass three
commits to accept three trees instead and get rid of virtual commits
altogether, but then we would lose an easy access to the message of
commits that are being merged, and we would need to pass these strings as
separate parameters (or part of merge_options) --- which might be a good
clean-up in a longer run, but I do not think it is absolutely necessary
during this round.