Re: [PATCH v3 3/4] pathspec: add flag to indicate operation without repository

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/20/2025 8:13 AM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> A following change will add support for pathspecs to the git diff
>> --no-index command. This mode of git diff does not load any repository.
>>
>> Add a new PATHSPEC_NO_REPOSITORY flag indicating that we're parsing
>> pathspecs without a repository.
>>
>> Both PATHSPEC_ATTR and PATHSPEC_FROMTOP require a repository to
>> function. Thus, verify that both of these are set in magic_mask to
>> ensure they won't be accepted when PATHSPEC_NO_REPOSITORY is set.
>>
>> Check PATHSPEC_NO_REPOSITORY when warning about paths outside the
>> directory tree. When the flag is set, do not look for a git repository
>> when generating the warning message.
>>
>> Finally, add a BUG in match_pathspec_item if the istate is NULL but the
>> pathspec has PATHSPEC_ATTR set. Callers which support PATHSPEC_ATTR
>> should always pass a valid istate, and callers which don't pass a valid
>> istate should have set PATHSPEC_ATTR in the magic_mask field to disable
>> support for attribute-based pathspecs.
> 
> All very sensible considerations.
> 
>> diff --git a/dir.c b/dir.c
>> index 2f2b654b0252..45aac0bfacab 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -396,9 +396,12 @@ static int match_pathspec_item(struct index_state *istate,
>>  	    strncmp(item->match, name - prefix, item->prefix))
>>  		return 0;
>>  
>> -	if (item->attr_match_nr &&
>> -	    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))
>> -		return 0;
>> +	if (item->attr_match_nr) {
>> +		if (!istate)
>> +			BUG("magic PATHSPEC_ATTR requires an index");
>> +		if (!match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))
>> +			return 0;
>> +	}
> 
> It is a bit curious why we do not check PATHSPEC_NO_REPOSITORY here,
> but it is OK, because it is a BUG for istate to be NULL when we have
> a repository anyway.
> 
match_pathspec doesn't take the same flags as parse_pathspec anyways, so
we can't check it here :)