Re: [PATCH v1 22/45] archive: convert to use parse_pathspec

Duy Nguyen [off-list ref] writes:

On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano [off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

@@ -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?

On Sun, Mar 17, 2013 at 12:00 PM, Junio C Hamano [off-list ref] wrote:

Duy Nguyen [off-list ref] writes:

quoted

On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano [off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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