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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2017, 01:48 UTC
Message-ID: <xmqqh90sw4dr.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170510170226.GB41649@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 12 quoted lines

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

Oh, don't get me wrong---I do not think passing an index_state instance throughout the callchain (and perhaps later we may pass an instance of "repository" instead) is a wrong move in the longer term for various APIs. I was just wondering if we have callers that can benefit from this change immediately---manipulators like "commit" do already use multiple instances of index_state structure.

But perhaps you are right---it may be wrong for the contents of the current index (or any index) to affect how a pathspec element is parsed in the first place. If the current code (before this series) uses the_index only for error checking, we may want to separate that out of the parse_pathspec() callchain, so that it does not even look at any index (not just the_index). And that may be a better change overall.

Thanks.