Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins

Stephan Beyer [off-list ref] writes:

Hmm, I have the long-run vision that we have a nice libgit some day,
with merge_recursive() being part of it.  And I'm a little unsure if
libified functions should rely on environment variables.

I think the environment variable is the least of your worries.  

I do not think anybody has vetted if it is safe to call merge_recursive()
more than once in a single run of a process.  Things to watch out for are
the use of static variables (e.g. current_{file,directory}_set that are
used for its (semi-broken) D/F conflict detection), its liberal use of
die(), leaking of "virtual commit", to name a few.  They are all perfectly
fine programming constructs when we assume this is a one-shot "run and
clean up with exit" program, but whoever wants to libify it needs to
arrange them to be cleaned up inside the "library" without making the code
too ugly nor one-shot use too expensive.

But such a clean-up may not be too bad as I initially feared, I suppose.
After a cursory look in builtin-merge-recursive.c, at least it does not
seem to mark objects with random object flags, expecting that nobody else
will look at them after it exits --- which would have been very expensive
to clean up after the fact.

Hi,

Junio C Hamano wrote:

Stephan Beyer [off-list ref] writes:

quoted

Hmm, I have the long-run vision that we have a nice libgit some day,
with merge_recursive() being part of it.  And I'm a little unsure if
libified functions should rely on environment variables.

I think the environment variable is the least of your worries.  

I do not think anybody has vetted if it is safe to call merge_recursive()
more than once in a single run of a process.

I use it in builtin-sequencer.c, without yet spending much effort in
libifying it. For the tests, it works. (Each "pick" calls a
merge_recursive(), each threeway-merge needing "patch" does.)
But I should either spend some effort in improving the libification
or, if this gets too time-consuming before Aug 17, I revert the commits
that make use of merge_recursive() instead of running git-merge-recursive.

leaking of "virtual commit",

Apropos, I think I have some tiny leak fixes lying around here.
Would such patches go into 1.6.0 or is it too dangerous?
(A free() that fixes a leak in one place could cause a segmentation
fault in another place. Of course I can try to explain in the commit
messages, why those are leaks in every case.)

But such a clean-up may not be too bad as I initially feared, I suppose.

I suppose, too.

Regards.

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F