On 5/20/2025 8:13 AM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
>
>> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>>
>> 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.
>
Right. We could check it here, but I actually had added this BUG first
before I added PATHSPEC_NO_REPOSITORY.
>> diff --git a/pathspec.c b/pathspec.c
>> index 2b4e434bc0aa..a3ddd701c740 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -492,7 +492,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
>> if (!match) {
>> const char *hint_path;
>>
>> - if (!have_git_dir())
>> + if ((flags & PATHSPEC_NO_REPOSITORY) || !have_git_dir())
>> die(_("'%s' is outside the directory tree"),
>> copyfrom);
>> hint_path = repo_get_work_tree(the_repository);
>
> This is a part of generating an error message. We die early to
> avoid having to call get-work-tree when we know we are not even in
> any working tree, which makes sense.
>
>> @@ -614,6 +614,10 @@ void parse_pathspec(struct pathspec *pathspec,
>> (flags & PATHSPEC_PREFER_FULL))
>> BUG("PATHSPEC_PREFER_CWD and PATHSPEC_PREFER_FULL are incompatible");
>>
>> + if ((flags & PATHSPEC_NO_REPOSITORY) &&
>> + (~magic_mask & (PATHSPEC_ATTR | PATHSPEC_FROMTOP)))
>> + BUG("PATHSPEC_NO_REPOSITORY is incompatible with PATHSPEC_ATTR and PATHSPEC_FROMTOP");
>
> Hmph, I am not sure if this change is correct. The magic_mask
> parameter is passed by a caller to say "even if parsr_pathspec()
> parses a pathspec using a certain set of features properly, the
> caller is not prepared to handle the parsed result". If magic_mask
> lacks PATHSPEC_ATTR, that does not necessarily mean that the given
> pathspec contains any pathspec items that do use the attr magic. It
> merely says that the caller is not prepared to handle a pathspec
> item that uses the attr magic feature.
>
Right. The magic_mask is a "these magic types are not allowed". I'm
checking to make sure that if you set PATHSPEC_NO_REPOSITORY, you must
also set PATHSPEC_ATTR and PATHSPEC_FROMTOP, because you cannot possibly
handle these pathspecs without a repository.
> If we are going to add a call to parse_pathspec() in a code path
> that is specific to diff-no-index, isn't it sufficient to pass
> PATHSPEC_ATTR and PATHSPEC_FROMTOP as magic_mask without this
> change?
>
Strictly speaking, yes. This part is really just a "this would be a
programmer error we should catch early".
