Re: CFT: merge-recursive in C

From

Alex Riesen <raa.lkml@gmail.com>

Date

Jun 27, 2006, 08:58 UTC

Message-ID

<81b0412b0606270158i16ebee20me81ca2b9fa71db5c@mail.gmail.com>

In-Reply-To

<Pine.LNX.4.63.0606270936520.29667@wbgn013.biozentrum.uni-wuerzburg.de>

On 6/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

>
> > I finally got pis^Witched enough by my platform at work and decided
> > to start the effort of converting Fredriks git-merge-recursive to C.
>
> Darn. I was working on the same thing since a few days.

I didn't know :)

> - have you considered using run-command() instead of system()?

No. What run-program?

> - in setup_index(), you set GIT_INDEX_FILE, but I do not think that the
>   rest of git picks up on it. environment.cc:get_index_file() checks if
>   the variable was set already, but not if it changed.

Not even sure it's needed. Leftover from conversion

> - You work with linked lists all the time. This is slow, especially for
>   the checks, if a file/directory is already there. Sorted lists would be
>   way faster there. Since you encapsulated that, it is no problem to
>   change that later (before inclusion).

Right, that's why it is mostly encapsulated.

> - is not "struct commit_list" more appropriate than "struct graph"?

Not even properly considered it yet. It probably is.

> - I always wondered why merge-recursive did not call merge-base, but did
>   its own thing. Hmm?

No idea yet.

> > To my deep disappointment, it didn't work out as good as I hoped: one
> > program I see most often and for longest time in the process list
> > (git-diff-tree) is a too complex thing to be put directly into
> > merge-recursive.c, so any help in this direction will be greatly
> > appreciated.
>
> Maybe something like this (ripped from my fragment of merge-recursive.c):
>
> static struct container *get_renames(struct tree *tree,
>                 struct tree *o, struct tree *a, struct tree *b,
>                 struct container *cache_entries)

...

> It is not tested, evidently, since I did not get the merge-base code
> integrated yet. But it should give you an idea.
>