> I am a bit torn on this, though. > > Because it uses lookup_commit_reference_by_name() to obtain base_commit > in addition to parent1 and parent2, and then > get_commit_tree() on them to get their trees, the real_merge() function in the > posted patch is incapable of accepting a single "pretend as if this tree object is > the common ancestor tree" and merging the two tree objects. But once that > flaw is fixed, using > merge_incore_nonrecursive() with an explicitly given "base", we can merge > totally trees regardless of how the commits they are found in are related in the > history, which is a very logical thing to do. > And while operating in that mode, there is no way to accept more than one > "base". > > So, I would be PERFECTLY HAPPY if this new mode of operation can take only > one "base" tree object, if it allows BASE, PARENT1, and PARENT2 to be all tree > objects, not necessarily commit objects. > > But if we insist that PARENT1 and PARENT2 must be commits, then I would find > it very unsatisfying if we only took a single BASE that also must be a commit. If > the merge-base has to be a tree or trees, then there is no way to perform > recursive merge (because you cannot compute common ancestors across tree > objects) , so it is perfectly justifiable to take only a single base (and error out > upon seeing a second --merge-base=X option). > > But it has to be a commit, then there is no justification to forbid recursive > operation, and I do not see a good reason to take only one COMMON thing. > > So, it is easy to say "let's take the current patch as a good first step", but it is > unclear what the good second step would be. Yeah, I think so. In fact, I planned to implement a version that specifies *only one* merge-base at the beginning and made the format of this option would be possible to extend to support multiple bases and octopus at the same time. > > We could correct the code to allow PARENT1, PARENT2 and BASE to be tree > objects, not necessarily commits, when only a single merge-base is specified. > That corrects the current flaw that tree objects cannot be used. And then when > more than one BASE is given (or no BASE is given---which is the original code), > we could correct the code to call merge_incore_recursive() instead. I prefer this solution. > > But the amount of the effort to get there from the current codebase (without > your patch) feels more or less the same ballpark as the patch in question, so... It means we need to support specifying multiple bases in the first version, which makes merge-tree command more complete. Since individual merge-tree support multiple bases, --stdin mode seems to need to support this too. However, I can't think of when the user needs to manually specify multiple bases for a merge ;). In other words, do we really need to support multiple bases in the first version? Thanks, Kyle
