Re: [PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Philip Oakley <philipoakley@iee.email>]

Sorry for delay, I had other family priorities to attend to.

On 06/06/2022 18:57, Junio C Hamano wrote:
> "Philip Oakley via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> The `--preserve-merges` option was removed by v2.34.0. However
>> users may not be aware that it is also a Pull configuration option,
>> which is still offered by major IDE vendors such as Visual Studio.
>>
>> Extend the `--preserve-merges` die message to also direct users to
>> the possible use of the `preserve` option in the `pull.rebase` config.
>> This is an additional 'belt and braces' information statement.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>> ---
>>  builtin/rebase.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 17cc776b4b1..5f8921551e1 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1205,7 +1205,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  			     builtin_rebase_usage, 0);
>>  
>>  	if (preserve_merges_selected)
>> -		die(_("--preserve-merges was replaced by --rebase-merges"));
>> +		die(_("--preserve-merges was replaced by --rebase-merges\n"
>> +			"Note: Your `pull.rebase` configuration may also be  set to 'preserve',\n"
>> +			"which is no longer supported; use 'merges' instead"));
> "be  set" -> "be set".
Noted. I see that the series is now in `next` [Thank you], so not worth
the churn of a patch, unless folks start noticing..

>
> I am not sure how this helps anybody, though.  

It's the Catch 22 problem for deleted capabilities, which we rarely see
because we normally have backward compatibility.
 
>
> When pull.rebase is parsed, rebase.c::rebase_parse_value() is called
> from builtin/pull.c::parse_config_rebase() and would trigger an
> error, whether it comes from the pull.rebase or the branch.*.rebase
> configuration variable.  An error() message already said that
> 'preserve' was removed and 'merges' would be a replacement when it
> happened.
>
> If the user has *not* reached this die() due to a configuration
> variable, then there is not much point giving this new message,
> either.

From my perspective, users should then be purging _all_ their `preserve`
configurations once they hit such errors. As the v2.34.0 change
propagates through the Git ecosystem, hopefully it'll be a sufficient
prompt for those who haven't realised that the option can be 'hidden' in
their configuration options.

Time will tell.

Thanks

Philip