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

[Alban Gruin <alban.gruin@gmail.com>]

Hi Junio,

Le 10/01/2021 à 21:51, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>>> 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