From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Tue, 09 Sep 2025 08:02:11 GMT
Subject: Re: [PATCH v3 1/8] git: add `deprecated` category to --list-cmds
Message-ID: <a56d716f-8598-46d0-9b63-8d44b482e1e4@app.fastmail.com>
In-Reply-To: <aL_MdiBLx9sssBwc@pks.im>

On Tue, Sep 9, 2025, at 08:43, Patrick Steinhardt wrote:
> On Mon, Sep 08, 2025 at 05:36:12PM +0200,
> kristofferhaugsbakk@fastmail.com wrote:
>> Notes (series):
>>     v3 (new):
>>
>>     This is something I wanted to submit independently until the point about
>>     aliasing builtins was brought up.[1]  It will help (in a small way) with
>>     the upcoming patch “git: allow alias-shadowing deprecated builtins”.
>>
>>     By the way: should `command-list.txt` be updated in some way (I didn’t
>>     know what way?)
>
> I think it would be a good idea to introduce a new category for
> deprecated commands. After all, next to planning for the removal we
> should also actively discourage the use of such commands. But if they
> are still prominently featured in git(1) then people might be misled and
> use them, only to get a deprecation warning thrown at them.

Thanks.  I’ll add your Ack if that’s okay.

>>[snip]
>> +static void list_builtins(struct string_list *list, unsigned int include_option, unsigned int exclude_option);
>
> Nit: let's wrap this overly long line.

I’ll do both of these, thanks.  In the same commit/patch I think.

>[snip]
> Same here.
>[snip]
>>  {
>> -	for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
>> -		if (exclude_option &&
>> -		    (commands[i].option & exclude_option))
>> -			continue;
>> -		string_list_append(out, commands[i].cmd);
>> +	if (include_option && exclude_option)
>> +		BUG("'include_option' and 'exclude_option' are mutually exclusive");
>> +	if (include_option) {
>> +		for (size_t i = 0; i < ARRAY_SIZE(commands); i++)
>> +			if (commands[i].option & include_option)
>> +				string_list_append(out, commands[i].cmd);
>> +	} else {
>> +		for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
>> +			if (commands[i].option & exclude_option)
>> +				continue;
>> +			string_list_append(out, commands[i].cmd);
>> +		}
>>  	}
>>  }
>
> We could combine these two loops into one:
>
>     for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
>         if (include_option && !(commands[i].option & include_option))
>             continue;
>         if (exclude_option && (commands[i].option & exclude_option))
>             continue;
>         string_list_append(out, commands[i].cmd);
>     }
>
> Which results in a bit less code duplication.

That’s more elegant.  I’ll use that.  And I’ll remove the function doc
now that it won’t be relevant.

Cheers

-- 
Kristoffer