Re: [PATCH 2/4] reset: use "unpack_trees()" directly instead of "git read-tree"

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Sep 11, 2009, 06:32 UTC
Message-ID: <alpine.LNX.2.00.0909110136160.28290@iabervon.org>
In-Reply-To: <200909110734.13903.chriscool@tuxfamily.org>

On Fri, 11 Sep 2009, Christian Couder wrote:

Show 12 quoted lines

> On Friday 11 September 2009, Junio C Hamano wrote:
> > Christian Couder <chriscool@tuxfamily.org> writes:
> > > From: Stephan Beyer <s-beyer@gmx.net>
> > >
> > > This patch makes "reset_index_file()" call "unpack_trees()" directly
> > > instead of forking and execing "git read-tree".
> >
> > And the reason why it is a good idea is...?
> 
> ...that it should be faster.
> 
> Ok, I will add that to the commit message in the next version.

There's also the benefit (IMHO, more significant) that git-read-tree's command line parsing is complicated, and using it from git-reset makes it hard to see exactly what each option of "git reset" does in terms of operations on the index and the working tree.

For example, it's not obvious when reading the code to run read-tree that all of the branches lead to the use of the "merge" flag, because some branches use "-m" and some use "--reset".

Actually, there's a behavior difference between the old version and the new version. The old version gives an error for "git reset --merge" with unmerged entries (unlike any other option to "git reset", AFAICT), and the new version does not. There's no way you'd know this without a careful reading of cmd_read_tree() and cross-reference with reset_index_file(), since the documentation doesn't mention it, the code in reset_index_file() only replaces "--reset" with "-m", and it seems to be doing that for the effect of not ignoring differences in the working tree.

	-Daniel
*This .sig left intentionally blank*