Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

> Le 07/02/2022 à 15:10, Derrick Stolee a écrit :
>> On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:
>>> On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:
>>
>> Hi Jean-Noël. Thanks for your attention to the translatable messages
>> here:
>>
>>>> error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
>>>>        from_file, to_file);
>>
>>>> die(_("failed to copy worktree config from '%s' to '%s'"),
>>>>      from_file, to_file);
>>
>>>> error(_("failed to unset 'core.bare' in '%s'"), to_file);
>>
>>>> error(_("failed to unset 'core.worktree' in '%s'"), to_file);
>>
>>> In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?
>>
>> I would argue that "unable to set" is not appropriate for any of these
>> messages. Perhaps the "failed to copy" messages might be able to use
>> "unable to set", but the information that the config setting is coming
>> from settings the user controlled is valuable.
>>
>> The "failed to unset" means "we are trying to _remove_ this setting
>> from the config file", so "unable to set" does not seem to work here.
>>
>> I'm open to revisiting this if you disagree.
>>
>> Thanks,
>> -Stolee
>>
> 
> Hi Derrick,
> 
> Sorry for not being more precise. The first two errors were not the subject of this remark.
> 
> For the last two, this is quite surprising that the same function failing (git_config_set_in_file_gently) can lead to different error messages.
> 
> In any case, I would argue at least for  shifting to :
> 
>     error(_("failed to unset '%s' in '%s'"), 'core.bare", to_file);
> and
>     error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file);
> 
> in order to factorize the message and get the option name out of the way.