Re: [PATCH v8 05/14] merge-index: add a new way to invoke `git-merge-one-file'

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 10, 2022, 13:14 UTC
Message-ID: <0e1be1b9-0a69-0531-8b2c-0f0f618925b7@gmail.com>
In-Reply-To: <o759r3qn-nqn9-oq22-p90o-2nrn24085n80@tzk.qr>

On 09/08/2022 22:36, Johannes Schindelin wrote:

Show 11 quoted lines

> Hi Alban,
> 
> On Tue, 9 Aug 2022, Alban Gruin wrote:
> 
>> Since `git-merge-one-file' will be rewritten and libified, there may be
>> cases where there is no executable named this way (ie. when git is
>> compiled with `SKIP_DASHED_BUILT_INS' enabled).  This adds a new way to
>> invoke this particular program even if it does not exist, by passing
>> `--use=merge-one-file' to merge-index.  For now, it still forks.
> 
> I read up about Stolee's and Phillip's suggestion,

I thought I was in favor of special casing git-merge-one-file. Stolee seems to have been worried about someone passing "git merge-one-file" but I think we only accept a program name and not a program plus arguments so that shouldn't be a problem.

Show 20 quoted lines

> and about Junio chiming
> in, but I have to point out that all the objections against special-casing
> `!strcmp(pgm, "git-merge-one-file`") share one fundamental flaw: they fail
> to acknowledge that we will _have_ to special-case this value once we turn
> `merge-one-file` into a built-in.
> 
> And the reason is: there might be scripts out there that expect `git
> merge-index git-merge-one-file [...]` to continue to work even when
> building Git with `SKIP_DASHED_BUILT_INS`.
> 
> In light of that, I would like to point out that we really _must_ revert
> to `if (!strcmp(pgm, "git-merge-one-file"))`, ie. to what v6 did (see
> https://lore.kernel.org/git/20201124115315.13311-7-alban.gruin@gmail.com/).
> 
> And since we must do that anyway, I do not see any need for the `--use`
> option at all, it just complicates the usage and does not really provide
> any benefit that I can see.
> 
> On the upside: skipping the `--use` option will dramatically simplify this
> patch.

I'd be happy to see the '--use' option dropped as well.

Thanks for continuing to work on this Alban, I'm sorry I've not had time to look at it properly since v1, I'll try and take a good look at this version.

Best Wishes

Phillip

Show 19 quoted lines

> Sorry for not catching this earlier.
> 
>> diff --git a/Documentation/git-merge-index.txt b/Documentation/git-merge-index.txt
>> [...]
>> @@ -44,8 +44,9 @@ code.
>>   Typically this is run with a script calling Git's imitation of
>>   the 'merge' command from the RCS package.
>>
>> -A sample script called 'git merge-one-file' is included in the
>> -distribution.
>> +A sample script called 'git merge-one-file' used to be included in the
>> +distribution. This program must now be called with
>> +'--use=merge-one-file'.
> 
> It probably makes more sense to just drop this paragraph because we will
> no longer provide that sample script.
> 
> Thanks,
> Dscho