From: Fredrik Kuivinen <freku045@student.liu.se>
Date: Wed, 28 Sep 2005 21:19:26 GMT
Subject: Re: [PATCH] Use git-update-ref in scripts.
Message-ID: <20050928211926.GA3378@c165.ib.student.liu.se>
In-Reply-To: <7vek79q8jj.fsf@assigned-by-dhcp.cox.net>

On Wed, Sep 28, 2005 at 12:47:12PM -0700, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Linus Torvalds <torvalds@osdl.org> writes:
> >
> >> On Wed, 28 Sep 2005, Junio C Hamano wrote:
> >>> 
> >> I don't like doing the diff before-hand, but it looked like the default 
> >> was to try just one strategy, and avoid the diff in that case.
> >
> > By 'diff before-hand' I take it to mean the savestate for later
> > rounds to keep the pre-merge state.  You are correct that it is
> > not done in a single strategy case, and 'git pull' by default
> > would use only one of Daniel's git-merge-resolve or in the
> > multi-remote case git-merge-octopus, depending on the number of
> > heads being merged.  BTW, I decided not to use diff, just in
> > case somebody has binary blob we cannot reproduce with diff and
> > patch.
> 
> I see two more diffs that turns out to be problematic in
> git-merge.sh code.
> 
>  (1) As a safety measure I have a check to make sure the index
>      is in sync with $head.  This visibly hurts; on my slow disk
>      and CPU with a couple of locally modified paths in the
>      working tree, this check takes about a second in the kernel
>      tree with hot cache.
> 
>      git-merge-resolve uses "git-read-tree -u -m O A B" form, so
>      this is totally unnecessary.  I am not so sure about
>      Fredrik's git-merge-recursive (I haven't looked at it for a
>      while).
> 

git-merge-recursive also uses 'git-read-tree -u -m O A B' so it
shouldn't have any problems with this change.


>  (2) savestate uses "git diff $head" to find out the list of
>      paths that have local modifications, but the current code
>      calls it after the check described above, so "git-ls-files
>      -m" is enough.  On a kernel tree with hot cache and index
>      in sync with HEAD, "git-ls-files -m" is about 3-4 times as
>      fast with a couple of locally modified files.
> 
> Keeping (1) and using 'git-ls-files -m' in (2) is optimizing for
> a wrong path.  On the other hand, (1) is a safety measure, and
> if it is kept (2) becomes a quite cheap operation.
> 
> I am inclined to just remove the check in (1), and make it the
> responsibility of merge strategies to make sure it does not
> commit unrelated changes.
> 
> Comments?
> 

Looks good to me.

- Fredrik