On Thursday 04 September 2014 20:37:52 Nguyễn Thái Ngọc Duy wrote: > This patch fixes two problems with using :(glob) (or even "*.c" > without ":(glob)"). > > The first one is we forgot to turn on the 'recursive' flag in struct > pathspec. Without that, tree_entry_interesting() will not mark > potential directories "interesting" so that it can confirm whether > those directories have anything matching the pathspec. > > The marking directories interesting has a side effect that we need to > walk inside a directory to realize that there's nothing interested in > there. By that time, 'archive' code has already written the (empty) > directory down. That means lots of empty directories in the result > archive. > > This problem is fixed by lazily writing directories down when we know > they are actually needed. There is a theoretical bug in this > implementation: we can't write empty trees/directories that match that > pathspec. > > Noticed-by: Peter Wu <[email protected]> > Signed-off-by: Nguyễn Thái Ngọc Duy <[email protected]>
Ah, ignore my last mail, I just noticed this one. This patch works fine. By the way, the glob pattern is treated as a 'nullglob'. If you specify a glob pattern that matches nothing, an empty archive is created. Perhaps an error message should be raised for that as it is unlikely that a user wants that? Tested-by: Peter Wu <[email protected]> > --- > Similar approach could probably be used for teaching ls-tree about pathspec.. > > archive.c | 82 > ++++++++++++++++++++++++++++++++++++++++++++++++++++- > t/t5000-tar-tree.sh | 10 +++++++ > 2 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/archive.c b/archive.c > index 3fc0fb2..9e62bf4 100644 > --- a/archive.c > +++ b/archive.c > @@ -98,9 +98,19 @@ static void setup_archive_check(struct git_attr_check > *check) > check[1].attr = attr_export_subst; > } > > +struct directory { > + struct directory *up; > + unsigned char sha1[20]; > + int baselen, len; > + unsigned mode; > + int stage; > + char path[FLEX_ARRAY]; > +}; > + > struct archiver_context { > struct archiver_args *args; > write_archive_entry_fn_t write_entry; > + struct directory *bottom; > }; > > static int write_archive_entry(const unsigned char *sha1, const char *base, > @@ -146,6 +156,65 @@ static int write_archive_entry(const unsigned char > *sha1, const char *base, > return write_entry(args, sha1, path.buf, path.len, mode); > } > > +static void queue_directory(const unsigned char *sha1, > + const char *base, int baselen, const char *filename, > + unsigned mode, int stage, struct archiver_context *c) > +{ > + struct directory *d; > + d = xmallocz(sizeof(*d) + baselen + 1 + strlen(filename)); > + d->up = c->bottom; > + d->baselen = baselen; > + d->mode = mode; > + d->stage = stage; > + c->bottom = d; > + d->len = sprintf(d->path, "%.*s%s/", baselen, base, filename); > + hashcpy(d->sha1, sha1); > +} > + > +static int write_directory(struct archiver_context *c) > +{ > + struct directory *d = c->bottom; > + int ret; > + > + if (!d) > + return 0; > + c->bottom = d->up; > + d->path[d->len - 1] = '\0'; /* no trailing slash */ > + ret = > + write_directory(c) || > + write_archive_entry(d->sha1, d->path, d->baselen, > + d->path + d->baselen, d->mode, > + d->stage, c) != READ_TREE_RECURSIVE; > + free(d); > + return ret ? -1 : 0; > +} > + > +static int queue_or_write_archive_entry(const unsigned char *sha1, > + const char *base, int baselen, const char *filename, > + unsigned mode, int stage, void *context) > +{ > + struct archiver_context *c = context; > + > + while (c->bottom && > + !(baselen >= c->bottom->len && > + !strncmp(base, c->bottom->path, c->bottom->len))) { > + struct directory *next = c->bottom->up; > + free(c->bottom); > + c->bottom = next; > + } > + > + if (S_ISDIR(mode)) { > + queue_directory(sha1, base, baselen, filename, > + mode, stage, c); > + return READ_TREE_RECURSIVE; > + } > + > + if (write_directory(c)) > + return -1; > + return write_archive_entry(sha1, base, baselen, filename, mode, > + stage, context); > +} > + > int write_archive_entries(struct archiver_args *args, > write_archive_entry_fn_t write_entry) > { > @@ -167,6 +236,7 @@ int write_archive_entries(struct archiver_args *args, > return err; > } > > + memset(&context, 0, sizeof(context)); > context.args = args; > context.write_entry = write_entry; > > @@ -187,9 +257,17 @@ int write_archive_entries(struct archiver_args *args, > } > > err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec, > - write_archive_entry, &context); > + args->pathspec.has_wildcard ? > + queue_or_write_archive_entry : > + write_archive_entry, > + &context); > if (err == READ_TREE_RECURSIVE) > err = 0; > + while (context.bottom) { > + struct directory *next = context.bottom->up; > + free(context.bottom); > + context.bottom = next; > + } > return err; > } > > @@ -221,6 +299,7 @@ static int path_exists(struct tree *tree, const char > *path) > int ret; > > parse_pathspec(&pathspec, 0, 0, "", paths); > + pathspec.recursive = 1; > ret = read_tree_recursive(tree, "", 0, 0, &pathspec, reject_entry, > NULL); > free_pathspec(&pathspec); > return ret != 0; > @@ -237,6 +316,7 @@ static void parse_pathspec_arg(const char **pathspec, > parse_pathspec(&ar_args->pathspec, 0, > PATHSPEC_PREFER_FULL, > "", pathspec); > + ar_args->pathspec.recursive = 1; > if (pathspec) { > while (*pathspec) { > if (**pathspec && !path_exists(ar_args->tree, > *pathspec)) > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh > index 7b8babd..ade7a7f 100755 > --- a/t/t5000-tar-tree.sh > +++ b/t/t5000-tar-tree.sh > @@ -305,4 +305,14 @@ test_expect_success GZIP 'remote tar.gz can be disabled' > ' > >remote.tar.gz > ' > > +test_expect_success 'archive and :(glob)' ' > + git archive -v HEAD -- ":(glob)**/sh" >/dev/null 2>actual && > + cat >expect <<EOF && > +a/ > +a/bin/ > +a/bin/sh > +EOF > + test_cmp expect actual > +' > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
