Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams <bmwill@google.com>]

On 05/10, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Convert 'parse_pathspec()' to take an index parameter.
> >
> > Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
> > and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
> > requiring a non-NULL index when either of these flags are given.
> > Convert callers which use these two flags to pass in an index while
> > having other callers pass in NULL.
> >
> > Now that pathspec.c does not use any cache macros and has no references
> > to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> The same comment as 5/8 applies to this change, but it is a bit
> easier to judge, because it has so many callers, and for some
> builtins, especially manipulator commands like add, checkout, and
> commit, there may be a good reason why they want to keep the primary
> index while playing with an additional in-core index in a distant
> future.
> 
> Does a pathspec parsed using one instance of index_state expected to
> work when matching against a path in another instance of index_state?
> Otherwise, passing a non-NULL istate to parse_pathspec() would tie
> the resulting pathspec to a particular index_state in some way and
> there may need a mechanism to catch an attempt to match paths in
> another index_state with such a pathspec as an error.  Just
> speculating out loud...
> 

Currently I don't believe this links a pathspec with a particular
index_state since an index is really just used to do some pre-processing
on the paths (check if the path goes into a submodule and die, or strip
a slash if the path matches a submodule), though I can see a future where
this is true.

I did have another version of this series where I completely removed the
two flags related to submodules.  Since builtin/add is the only caller
which needs to die if a path descends into a submodule, I had a function
which did this after the parse_pathspec() call.  Also, since (ae8d08242
pathspec: pass directory indicator to match_pathspec_item()) stripping
off the slash from a submodule path really is no longer needed for the
path matching logic, this means that we could potentially just drop the
strip slash flag.  The only caveat is ls-files.

ls-files is the only command (that I know of) which does cache pruning
based on the common prefix of all pathspecs given.  If there is a
submodule named 'sub' and you provided a pathspec 'sub/', the matching
logic can handle this but the cache pruning logic would prune all
entries from the index which don't have a leading 'sub/' which is the
common prefix of all pathspecs (if you didn't strip off the slash).
Meaning you'd prune the submodule 'sub' when  you really didn't want to.
This could probably be fixed to have the cache pruning logic to prune by
ignoring the trailing slash always.

So there's another option here if you don't feel quite right about
piping through an index into parse_pathspec just yet.

-- 
Brandon Williams