Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 17, 2013, 05:00 UTC
- Message-ID
- <7vehfetycj.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CACsJy8DUb5j0of=cuQje5cRWLwyi-MiH-d-RsyBiPOjaD1Tntg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
Show 29 quoted lines
> On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> @@ -232,11 +228,18 @@ static int path_exists(struct tree *tree, const char *path)
>>> static void parse_pathspec_arg(const char **pathspec,
>>> struct archiver_args *ar_args)
>>> {
>>> - ar_args->pathspec = pathspec = get_pathspec("", pathspec);
>>> + /*
>>> + * must be consistent with parse_pathspec in path_exists()
>>> + * Also if pathspec patterns are dependent, we're in big
>>> + * trouble as we test each one separately
>>> + */
>>> + parse_pathspec(&ar_args->pathspec, 0,
>>> + PATHSPEC_PREFER_FULL,
>>> + "", pathspec);
>>> if (pathspec) {
>>> while (*pathspec) {
>>> if (!path_exists(ar_args->tree, *pathspec))
>>> - die("path not found: %s", *pathspec);
>>> + die(_("pathspec '%s' did not match any files"), *pathspec);
>>> pathspec++;
>>> }
>>
>> You do not use ar_args->pathspec even though you used parse_pathspec()
>> to grok it? What's the point of this change?
>
> parse_pathspec() here is needed because write_archive_entries needs it
> later.That is not the issue I was pointing out. Even though you parse the pathspec into args->pathspec, the "if() { while () {} }" here still uses strings contained in **pathspec, as if they are literal strings and not ":(glob)Documentation" and such, and will not match the named directory.
Technically, erroring out saying "':(glob)Documentation' does not exist as a path in the tree" is correct, but it would be nicer to have the code inspect parse_pathspec() result and independently barf, saying "this command does not support magic pathspecs, give me leading paths and nothing else", until we do support magic pathspecs, no?