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

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.