Re: [PATCH v6 06/13] merge-index: don't fork if the requested program is `git-merge-one-file'

Hi Junio,

Le 10/01/2021 à 21:51, Junio C Hamano a écrit :

Alban Gruin [off-list ref] writes:

quoted

quoted

These days, there exists an optional installation option exists that
won't even install built-in commands in $GIT_EXEC_PATH, which
invalidates the assessment made in 2019 in the article you cited
above, so the code might still be OK, but the old justification no
longer would apply.

In any case, if two people who reviewed a patch found the same thing
in it fishy, it is an indication that the reason why the apparently
fishy code is OK needs to be better explained so that future readers
of the code do not have to be puzzled about the same thing.

Perhaps we could try to check if the provided command exists (with
locate_in_PATH()), if it does, run it through merge_one_file_spawn(),
else, use merge_one_file_func()?

So you think your current implementation will be broken if the "no
dashed git binary on disk" installation option is used?

I do not think "first check if an on-disk command exists and use it,
otherwise check its name" alone would work well in practice.  Both
the 'cat' example that appears in the manual page, and the typical
invocation of git-merge-one-file from merge-resolve:

	git merge-index cat MM
	git merge-index git-merge-one-file -a

would work just as well as before, but does not give you a way to
bypass fork() for the latter.  And changing the order of checks
would mean the users won't have a way to override a buggy builtin
implementation of merge_one_file function.  Besides, using the name
of the binary feels like a bad hack.  

As the invocation from merge-resolve is purely an internal matter,
it may make more sense to introduce a new option and explicitly tell
merge-index that the command line is not asking for an external
program to be spawned, e.g.

	git merge-index --use=merge-one-file -a

You'd prepare a table of internally implemented "take info on a
single path that is being merged and give an automated resolution"
functions, which begins with a single entry that maps the string
"merge-one-file" to your merge_one_file_func function.  Any value to
the "--use" option that names a function not in the table would
cause an error.

Note that in the above the "table of functions" is merely
conceptual.  It is perfectly OK to implement the single entry table
by codeflow (i.e. "if (!strcmp()) ... else error();").  But thinking
in terms of "a table of functions the user can choose from" helps to
form the right mental picture.

Hmm?

Yes, this should work.

To achieve this, I think I have to reorder this series a bit.
Currently, this is what it looks like:

 1. convert git-merge-one-file;
 2. libify git-merge-index, add the ability to call merge-one-file directly;
 3. convert the resolve strategy;
 4. convert the octopus strategy.

After the reorder, the series would look like this:

 1. libify git-merge-index, add `--use=merge-one-file', change
git-merge-resolve.sh, -octopus.sh, and t6060 to use this new parameter;
 2. convert git-merge-one-file, add the ability for merge-index to call
it directly;
 3. convert the resolve strategy;
 4. convert the octopus strategy.

Alban