Re: [PATCH] rev-list: make "struct rev_list_info" static to the only user

From: René Scharfe <l.s.r@web.de>
Date: Jul 19, 2025, 12:36 UTC
Message-ID: <fc4da804-37ed-4425-8dda-14cfcb607906@web.de>
In-Reply-To: <20250719063551.GA705356@coredump.intra.peff.net>

On 7/19/25 8:35 AM, Jeff King wrote:

Show 7 quoted lines

> On Fri, Jul 18, 2025 at 04:58:03PM -0700, Junio C Hamano wrote:
> 
>> The structure has nothing to do with what "git bisect" does; as
>> nobody other than "git rev-list" implementation uses it, move it
>> as a private data type to builtin/rev-list.c
> 
> Nice improvement.

*nod*

Show 12 quoted lines

>> @@ -652,7 +660,7 @@ int cmd_rev_list(int argc,
>>  	 */
>>  	/*
>>  	 * NEEDSWORK: These loops that attempt to find presence of
>> -	 * options without understanding that the options they are
>> +	 * options without understanding the options they are
>>  	 * skipping are broken (e.g., it would not know "--grep
>>  	 * --exclude-promisor-objects" is not triggering
>>  	 * "--exclude-promisor-objects" option).  We really need
> 
> This tacked-on bit seems funny to me. Isn't the original more correct?
> The loops do not understand that the options are broken.

No, the options are fine, but the loops are broken -- they cannot tell what they are looking at is an option or an argument of a preceding option, yet they ignore that latter possibility. So the word "that" is best left out. I also don't see a connection to the struct move, though.

René