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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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.

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?