Brandon Williams [off-list ref] writes:
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.
Am 11.05.2017 um 03:48 schrieb Junio C Hamano:
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.
Just a reminder: if core.ignoreCase is set, the variant of a path in the index takes precedence over the variant found in the working tree. Hence, pathspec must be matched against the index that corresponds to the working tree. I guess that's the_index.
-- Hannes
On 05/11, Junio C Hamano wrote:
Brandon Williams [off-list ref] writes:
quoted
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.
I didn't get the feeling you thought it was a bad change. I really
appreciate your thoughts since they are things which I also was
wondering about.
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.
I'll polish up the other version of the series which does the processing
(using an index) outside of parse_pathspec() and let you see how you
feel about those changes. Note that if we go with the route to not pass
in an index now, it doesn't necessarily mean that down the line we won't
have to pass a 'repository' instance into parse_pathspec(). Just food
for thought.
--
Brandon Williams