Re: [PATCH] merge-tree: sometimes, d/f conflict is not an issue

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 8, 2007, 13:16 UTC
Message-ID: <Pine.LNX.4.64.0707081353560.4248@racer.site>
In-Reply-To: <7vwsxb4e2q.fsf@assigned-by-dhcp.cox.net>

Hi,

On Sat, 7 Jul 2007, Junio C Hamano wrote:

Show 7 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The last time I looked at merge-recursive's D/F check, I found that it 
> > was not quite doing things right.  I may be able to dig up what I 
> > posted to the list...
> 
> It was from around April 7th-10th this year.

Unfortunately, this is way over my time budget. As well as over my intelligence budget, since I did not even succeed in understanding the code in threeway_merge _at all_.

Besides, IMHO there is a deeper issue. Since merge-recursive started out as a Python script, and grew there until it was usable, and grew the rename detection therein, too, until it was finally converted to C, it accumulated a lot of features that would have been nice to have independently.

Almost the same goes for unpack-trees, which (its name to the contrary) does quite a few things to merge entries, too. And it tries to detect d/f conflicts, too.

So there we are, with two really big and unwieldy chunks of code, each deserving an own GSoC project to clean them up. Or maybe not even a GSoC project, but a longer project.

What I would _like_ to see is something as clean as merge-tree. Which is clearly separated (code and file wise, too) into these stages:

- reading the trees

- determining renames

- determining true d/f conflicts

- threeway merge

- writing the tree object

- writing the work tree

- recursive

Ideally, merge-recursive would really have been as simple as

	case "$1" in
	--index_only)
		index_only=$1
		shift
	esac
	a="$1"
	b="$2"
	set $(git merge-base --all $a $b)
	temp=$1
	shift
	while case $# in 0) break;; esac
	do
		temp=$(git merge-recursive --index-only $temp $1)
		shift
	done
	git merge-non-recursive $index_only $temp -- "$a" "$b"

because _read-tree -m_ should have learnt about renames, _not_ merge-recursive.

As it is, both unpack_trees() and merge-recursive have a certain degree of not-quite duplicated yet wants-to-do-largely-the-same functionality. Which of course leads to much finger pointing: "it's unpack_trees() fault. no. it's merge-recursive's fault. no, vice versa."

Maybe the proper way out is really to start from merge-tree.c and do something which is easy to understand, and concise, and thus has a much lesser chance of being buggy.

Ciao, Dscho