Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[Derrick Stolee <derrickstolee@github.com>]

On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>  {
>> +	int force = 0;
>>  	struct option options[] = {
>> +		OPT_BOOL(0, "force", &force,
>> +			 N_("return success even if repository was not registered")),
> 
> This could be shortened a bit by using OPT__FORCE() instead of
> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:

Looks like I can do both like this:

		OPT__FORCE(&force,
			   N_("return success even if repository was not registered"),
			   PARSE_OPT_NOCOMPLETE),

>> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>  		usage_with_options(builtin_maintenance_unregister_usage,
>>  				   options);
>>  
>> -	config_unset.git_cmd = 1;
>> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
>> +	for_each_string_list_item(item, list) {
>> +		if (!strcmp(maintpath, item->string)) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (found) {
>> +		config_unset.git_cmd = 1;
>> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> +			     "--fixed-value", key, maintpath, NULL);
>> +
>> +		rc = run_command(&config_unset);
> 
> It seems a bit heavy-handed to fork() an extra git process just to
> unset a config variable.  Wouldn't a properly parametrized
> git_config_set_multivar_in_file_gently() call be sufficient?  Though
> TBH I don't offhand know what those parameters should be, in
> particular whether there is a convenient way to find out the path of
> the global config file in order to pass it to this function.
> 
> (Not an issue with this patch, as the context shows that this has been
> done with the extra process before; it just caught my eye.)

Thanks. I'll look into it.

-Stolee