From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 21 Feb 2023 16:29:59 GMT
Subject: Re: [PATCH v2] rebase -i: check labels and refs when parsing todo list
Message-ID: <09ba35c3-1434-ac23-7ac0-1295dd8aca60@dunelm.org.uk>
In-Reply-To: <f99d7043-7757-2e19-d1e1-4ee9b887af1f@github.com>

On 21/02/2023 14:24, Derrick Stolee wrote:
> On 2/20/2023 9:19 AM, Phillip Wood via GitGitGadget wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
>> +static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>> +{
>> +	switch (command) {
>> +	case TODO_LABEL:
>> +	  /*
>> +	   * '#' is not a valid label as the merge command uses it to
>> +	   * separate merge parents from the commit subject.
>> +	   */
>> +	  if (!strcmp(arg, "#") ||
>> +		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
> 
> Tabbing is strange here. Within the case there seems to be "\t  " to
> the left of each line. Then the conditional has strange spacing. I
> think it should be:
> 
> 		if (!strcmp(arg, "#") ||
> 		    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))
> 
> (The "check_refname_format()" line is correct in your patch, but the
> lines above it are not, for some reason.)

Yes you're right, I'm not sure what happened there. I'll re-roll

Thanks

Phillip

> The rest of the switch statement is correctly tabbed.
> 
>> @@ -2525,10 +2553,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   
>>   	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
>>   	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
>> +		int ret = 0;
>> +
>>   		item->commit = NULL;
>>   		item->arg_offset = bol - buf;
>>   		item->arg_len = (int)(eol - bol);
>> -		return 0;
>> +		if (item->command == TODO_LABEL ||
>> +		    item->command == TODO_UPDATE_REF) {
>> +			saved = *eol;
>> +			*eol = '\0';
>> +			ret = check_label_or_ref_arg(item->command, bol);
>> +			*eol = saved;
>> +		}
>> +		return ret;
>>   	}
> 
> This diff is much simpler to understand for the purpose of this
> patch. I saw your comment about splitting out TODO_EXEC for a
> future change, and that would be fine when it happens, too.
> 
> Thanks for the updates. Outside of that strange whitespace issue,
> this patch LGTM.
> 
> Thanks,
> -Stolee