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