Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
Duy Nguyen writes: > No, the literal strings are reparsed in path_exists() before being fed > to read_tree_recursive. Yuck. OK. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
On Sun, Mar 17, 2013 at 12:00 PM, Junio C Hamano wrote:
> Duy Nguyen writes:
>
>> On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano wrote:
>>> Nguyễn Thái Ngọc Duy 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.
No, the literal strings are reparsed in path_exists() before being fed
to read_tree_recursive. So ":(glob)Documentation" should match the
tree "Documentation".
> 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?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
Duy Nguyen writes:
> On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano wrote:
>> Nguyễn Thái Ngọc Duy 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy 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. tree_entry_interesting() has not supported "seen" feature like
match_pathspec_depth() to detect unused pathspecs. For simplicity,
just check each pathspec individually. We can revisit this when we add
"seen" feature to tree_entry_interesting.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
Nguyễn Thái Ngọc Duy 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?
> }
> diff --git a/archive.h b/archive.h
> index 895afcd..4a791e1 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -1,6 +1,8 @@
> #ifndef ARCHIVE_H
> #define ARCHIVE_H
>
> +#include "pathspec.h"
> +
> struct archiver_args {
> const char *base;
> size_t baselen;
> @@ -8,7 +10,7 @@ struct archiver_args {
> const unsigned char *commit_sha1;
> const struct commit *commit;
> time_t time;
> - const char **pathspec;
> + struct pathspec pathspec;
> unsigned int verbose : 1;
> unsigned int worktree_attributes : 1;
> unsigned int convert : 1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
