Re: [PATCH v3 5/5] repo-settings: create feature.experimental setting

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>, "Jakub Narebski" <jnareb@gmail.com>,
	"Nguyễn Thái Ngọc" <pclouds@gmail.com>,
	carenas@gmail.com, "Ævar Arnfjörð" <avarab@gmail.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v3 5/5] repo-settings: create feature.experimental setting
Date: Thu, 8 Aug 2019 16:49:54 -0400	[thread overview]
Message-ID: <1a2bf8e1-5cc1-c626-9dfc-dd6dc58fa0ad@gmail.com> (raw)
In-Reply-To: <CABPp-BGo5spDbk34YwAPGqT1Xgsr1dHm6OhxY1xKFQxQaotBig@mail.gmail.com>

On 8/8/2019 4:31 PM, Elijah Newren wrote:
> On Thu, Aug 8, 2019 at 12:12 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 8/8/2019 2:59 PM, Junio C Hamano wrote:
>>> Elijah Newren <newren@gmail.com> writes:
>>>
>>>>> --- a/Documentation/config/merge.txt
>>>>> +++ b/Documentation/config/merge.txt
>>>>> @@ -54,7 +54,8 @@ merge.directoryRenames::
>>>>>         moved into the new directory.  If set to "conflict", a conflict
>>>>>         will be reported for such paths.  If merge.renames is false,
>>>>>         merge.directoryRenames is ignored and treated as false.  Defaults
>>>>> -       to "conflict".
>>>>> +       to "conflict" unless `feature.experimental` is enabled and the
>>>>> +       default is "true".
>>>>
>>>> I have a hard time parsing that changed sentence.  Perhaps something like:
>>>>    ...unless `feature.experimental` is enabled in which case the
>>>> default is "true".
>>>> ?
>>>
>>> That reads better.
>>>
>>> But I am not sure about the wisdom of controlling between conflict
>>> and true with this feature macro in the first place.
>>>
>>> Between "conflict" and "true", the former forces the end user to
>>> verify (or allows the end user to veto) the auto resolution by the
>>> heuristics and is always a safer if more cumbersome option.  It's
>>> not like blindly trusting the directory rename heuristics is the
>>> bright future for all users, is it?
>>>
>>> I would not set rerere.autoUpdate to true when feature.experimental
>>> is set; for exactly the same reason, I do not find it reasonable to
>>> set this to true with feature.experimental macro.
>>
>> OK. I can remove it from the feature.experimental variable.
>>
>> Shall I keep the enum logic and the use of repo-settings.c? I can split
>> them out into a separate patch.
> 
> Good question.  In part, I was looking at this ds/feature-macros
> series because my cleanup-merge-api series[1] has some minor conflicts
> with it.  I'm a little unsure what route I should take with my series
> now.  Some choices:
>   * keep this logic in your series in a separate patch, and have me
> rebase my series on yours.
>   * drop this logic from your series since it may not be needed
> anymore, making our two series independent again.
>   * move this logic into a separate patch of yours but making that
> patch part of my series instead; that'd be easy with the enum logic,
> but the repo-settings.c stuff appears to depend on your other
> patches...
> 
> Thoughts or preferences?

I've done the work in my GGG PR [1] to split the commit into two,
with the merge.directoryRenames on top. I created a tag [2] at that
commit and I'll send my v4 without changes to merge.directoryRenames.

I'll let you decide if you like placing the config in repo-settings
or not (or want to adapt the enum logic in a different place).

Thanks,
-Stolee

[1] https://github.com/gitgitgadget/git/pull/292
[2] https://github.com/derrickstolee/git/releases/tag/merge.directoryRenames