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

From: Brandon Williams <bmwill@google.com>
Date: May 10, 2017, 17:02 UTC
Message-ID: <20170510170226.GB41649@google.com>
In-Reply-To: <xmqq1srxxn72.fsf@gitster.mtv.corp.google.com>

On 05/10, Junio C Hamano wrote:

Show 30 quoted lines

> 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