# [BUG] git archive formats and dashdash 11 messages from 2009-12-10 to 2009-12-30. Participants: Ilari Liusvaara, Junio C Hamano, Miklos Vajna, René Scharfe, Nanako Shiraishi. Thread: https://gitlist.dev/t/21931 ## Ilari Liusvaara, 2009-12-10 21:26 Subject: [BUG] git archive formats and dashdash Message-ID: <20091210212636.GA27722@Knoppix> URL: https://gitlist.dev/e/20091210212636.GA27722%40Knoppix ``` --format option of git archive stops working if -- is used: Shell session: ---------------- $ git archive -o test3.zip --format=zip master -- exec_cmd.c $ file test3.zip test3.zip: POSIX tar archive # Eh... $ git archive -o test4.zip --format=zip master exec_cmd.c $ file test4.zip test4.zip: Zip archive data, at least v1.0 to extract # That worked. ---------------- The bug seems to be in that archive appends the --format option to its arguments, not taking into account that there may be -- before that point, disabling option interpretation. Git version 1.6.6-rc2 -Ilari ``` ## Junio C Hamano, 2009-12-10 22:05 Subject: Re: [BUG] git archive formats and dashdash Message-ID: <7v1vj2a3ik.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vj2a3ik.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091210212636.GA27722@Knoppix> ``` Ilari Liusvaara writes: > --format option of git archive stops working if -- is used: Good catch. Is this a regression between 1.6.5 and the current code? builtin-archive.c | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 12351e9..8ef5ab3 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -106,13 +106,17 @@ int cmd_archive(int argc, const char **argv, const char *prefix) if (format) { sprintf(fmt_opt, "--format=%s", format); /* - * This is safe because either --format and/or --output must - * have been given on the original command line if we get to - * this point, and parse_options() must have eaten at least - * one argument, i.e. we have enough room to append to argv[]. + * We have enough room in argv[] to muck it in place, + * because either --format and/or --output must have + * been given on the original command line if we get + * to this point, and parse_options() must have eaten + * it, i.e. we can add back one element to the array. + * But argv[] may contain "--" so we should make this + * the first option. */ - argv[argc++] = fmt_opt; - argv[argc] = NULL; + memmove(argv + 2, argv + 1, sizeof(*argv) * argc); + argv[1] = fmt_opt; + argv[++argc] = NULL; } if (remote) ``` ## Miklos Vajna, 2009-12-10 22:20 Subject: [PATCH] builtin-archive: insert --format before double dash if necessary. Message-ID: <20091210222005.GP4114@genesis.frugalware.org> URL: https://gitlist.dev/e/20091210222005.GP4114%40genesis.frugalware.org In-Reply-To: <20091210212636.GA27722@Knoppix> ``` Reported-by: Ilari Liusvaara Signed-off-by: Miklos Vajna --- What about this fix? builtin-archive.c | 23 ++++++++++++++++++++++- 1 files changed, 22 insertions(+), 1 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 12351e9..ffbc9b0 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -104,14 +104,35 @@ int cmd_archive(int argc, const char **argv, const char *prefix) } if (format) { + int i, found = 0; + const char *ptr = NULL; + sprintf(fmt_opt, "--format=%s", format); /* * This is safe because either --format and/or --output must * have been given on the original command line if we get to * this point, and parse_options() must have eaten at least * one argument, i.e. we have enough room to append to argv[]. + * + * First check if we have to insert the argument before + * two dashes. */ - argv[argc++] = fmt_opt; + for (i = 0; i < argc; i++) { + if (found) { + const char *tmp = argv[i]; + argv[i] = ptr; + ptr = tmp; + } else if (!strcmp(argv[i], "--")) { + found = 1; + ptr = argv[i]; + argv[i] = fmt_opt; + argc++; + } + } + + /* No double dash? Then just append it to the end of the list. */ + if (!found) + argv[argc++] = fmt_opt; argv[argc] = NULL; } -- 1.6.5.2 ``` ## Miklos Vajna, 2009-12-10 22:22 Subject: Re: [BUG] git archive formats and dashdash Message-ID: <20091210222258.GQ4114@genesis.frugalware.org> URL: https://gitlist.dev/e/20091210222258.GQ4114%40genesis.frugalware.org In-Reply-To: <7v1vj2a3ik.fsf@alter.siamese.dyndns.org> ``` On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote: > Good catch. Is this a regression between 1.6.5 and the current code? Ah, you version is much shorter. :) I think it was introduced by 0f4b377c (git-archive: infer output format from filename when unspecified, 2009-09-14), so it was introduced before 1.6.5. ``` ## Ilari Liusvaara, 2009-12-10 22:25 Subject: Re: [BUG] git archive formats and dashdash Message-ID: <20091210222503.GA27840@Knoppix> URL: https://gitlist.dev/e/20091210222503.GA27840%40Knoppix In-Reply-To: <7v1vj2a3ik.fsf@alter.siamese.dyndns.org> ``` On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote: > Ilari Liusvaara writes: > > > --format option of git archive stops working if -- is used: > > Good catch. Is this a regression between 1.6.5 and the current code? Doesn't appear to be so: Based on quick look at source, it seems that this is indeed regression, and the commit which introduced it is: commit 0f4b377c20fb7d93f8bfeec39efb2b9392d6aebc Author: Dmitry Potapov Date: Mon Sep 14 00:17:01 2009 +0400 Describe: v1.6.5-rc1-7-g0f4b377 / v1.6.5-rc2~34 -Ilari ``` ## Junio C Hamano, 2009-12-10 22:42 Subject: Re: [BUG] git archive formats and dashdash Message-ID: <7vws0u8n99.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vws0u8n99.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091210222258.GQ4114@genesis.frugalware.org> ``` Miklos Vajna writes: > On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote: >> Good catch. Is this a regression between 1.6.5 and the current code? > > Ah, you version is much shorter. :) More importantly it would avoid running "archive path --format=zip". That may be reordered by the current parse-options implementation, and other command line parsers (e.g. "git diff HEAD path --stat") may accept command line in such an order, but I somehow find it utterly wrong. We should prepare ourselves for the (distant) day on which we start enforcing "options then rev then paths" order from the command line. ``` ## Junio C Hamano, 2009-12-10 23:27 Subject: [PATCH] Fix archive format with -- on the command line Message-ID: <7vhbry8l54.fsf_-_@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhbry8l54.fsf_-_%40alter.siamese.dyndns.org In-Reply-To: <7vws0u8n99.fsf@alter.siamese.dyndns.org> ``` Giving --format from the command line, or using output file extention to DWIM the output format, with a pathspec that is disambiguated with an explicit double-dash on the command line, e.g. git archive -o file --format=zip HEAD -- path git archive -o file.zip HEAD -- path didn't work correctly. This was because the code reordered (when one was given) or added (when the former was inferred) a --format argument at the end, effectively making it to "archive HEAD -- path --format=zip", i.e. an extra pathspec that is unlikely to match anything. The command line argument list should always be "options, revs and then paths", and we should set a good example by inserting the --format at the beginning instead. Reported-by: Ilari Liusvaara Signed-off-by: Junio C Hamano --- * So here is one with a proper commit log message. builtin-archive.c | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin-archive.c b/builtin-archive.c index 12351e9..8ef5ab3 100644 --- a/builtin-archive.c +++ b/builtin-archive.c @@ -106,13 +106,17 @@ int cmd_archive(int argc, const char **argv, const char *prefix) if (format) { sprintf(fmt_opt, "--format=%s", format); /* - * This is safe because either --format and/or --output must - * have been given on the original command line if we get to - * this point, and parse_options() must have eaten at least - * one argument, i.e. we have enough room to append to argv[]. + * We have enough room in argv[] to muck it in place, + * because either --format and/or --output must have + * been given on the original command line if we get + * to this point, and parse_options() must have eaten + * it, i.e. we can add back one element to the array. + * But argv[] may contain "--"; we should make it the + * first option. */ - argv[argc++] = fmt_opt; - argv[argc] = NULL; + memmove(argv + 2, argv + 1, sizeof(*argv) * argc); + argv[1] = fmt_opt; + argv[++argc] = NULL; } if (remote) -- 1.6.6.rc2 ``` ## Junio C Hamano, 2009-12-10 23:31 Subject: Re: [PATCH] Fix archive format with -- on the command line Message-ID: <7vd42m8kyr.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd42m8kyr.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vhbry8l54.fsf_-_@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Giving --format from the command line, or using output file extention to > DWIM the output format, with a pathspec that is disambiguated with an > explicit double-dash on the command line, e.g. > > git archive -o file --format=zip HEAD -- path > git archive -o file.zip HEAD -- path > > didn't work correctly. > > This was because the code reordered (when one was given) or added (when > the former was inferred) a --format argument at the end, effectively > making it to "archive HEAD -- path --format=zip", i.e. an extra pathspec > that is unlikely to match anything. A side note to this issue is that $ git add non-existing-path complains but $ git archive HEAD non-existing-path doesn't. Is this something we should consider a bug, or a feature? ``` ## René Scharfe, 2009-12-12 15:00 Subject: Re: [PATCH] Fix archive format with -- on the command line Message-ID: <4B23B019.40106@lsrfire.ath.cx> URL: https://gitlist.dev/e/4B23B019.40106%40lsrfire.ath.cx In-Reply-To: <7vd42m8kyr.fsf@alter.siamese.dyndns.org> ``` Am 11.12.2009 00:31, schrieb Junio C Hamano: > Junio C Hamano writes: > >> Giving --format from the command line, or using output file extention to >> DWIM the output format, with a pathspec that is disambiguated with an >> explicit double-dash on the command line, e.g. >> >> git archive -o file --format=zip HEAD -- path >> git archive -o file.zip HEAD -- path >> >> didn't work correctly. >> >> This was because the code reordered (when one was given) or added (when >> the former was inferred) a --format argument at the end, effectively >> making it to "archive HEAD -- path --format=zip", i.e. an extra pathspec >> that is unlikely to match anything. > > A side note to this issue is that > > $ git add non-existing-path > > complains but > > $ git archive HEAD non-existing-path > > doesn't. Is this something we should consider a bug, or a feature? I wouldn't go so far as to call it a bug, but it's certainly a missing feature. If the user asks for something we can't give him or her, it's better to report that fact. We didn't do that because it doesn't fall out naturally from the archive streaming loop. ls-tree doesn't do it, either, by the way. Something like this? -- >8 -- Subject: archive: complain about path specs that don't match anything Verify that all path specs match at least one path in the specified tree and reject those that don't. This would have made the bug fixed by 782a0005 easier to find. This implementation is simple to the point of being stupid. It walks the full tree for each path spec until it matches something. It's short and seems to be fast enough, though. Signed-off-by: Rene Scharfe --- archive.c | 25 ++++++++++++++++++++++++- 1 files changed, 24 insertions(+), 1 deletions(-) diff --git a/archive.c b/archive.c index 55b2732..5b88507 100644 --- a/archive.c +++ b/archive.c @@ -211,10 +211,33 @@ static const struct archiver *lookup_archiver(const char *name) return NULL; } +static int reject_entry(const unsigned char *sha1, const char *base, + int baselen, const char *filename, unsigned mode, + int stage, void *context) +{ + return -1; +} + +static int path_exists(struct tree *tree, const char *path) +{ + const char *pathspec[] = { path, NULL }; + + if (read_tree_recursive(tree, "", 0, 0, pathspec, reject_entry, NULL)) + return 1; + return 0; +} + static void parse_pathspec_arg(const char **pathspec, struct archiver_args *ar_args) { - ar_args->pathspec = get_pathspec("", pathspec); + ar_args->pathspec = pathspec = get_pathspec("", pathspec); + if (pathspec) { + while (*pathspec) { + if (!path_exists(ar_args->tree, *pathspec)) + die("path not found: %s", *pathspec); + pathspec++; + } + } } static void parse_treeish_arg(const char **argv, ``` ## Nanako Shiraishi, 2009-12-30 03:13 Subject: Re: [PATCH] Fix archive format with -- on the command line Message-ID: <20091230121309.6117@nanako3.lavabit.com> URL: https://gitlist.dev/e/20091230121309.6117%40nanako3.lavabit.com In-Reply-To: <4B23B019.40106@lsrfire.ath.cx> ``` Junio, could you tell us what happened to this thread? "git archive HEAD non-existing-path" doesn't complain like "git add" does, and the patch is to fix it. No discussion. ``` ## Junio C Hamano, 2009-12-30 08:11 Subject: Re: [PATCH] Fix archive format with -- on the command line Message-ID: <7vhbr8519z.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhbr8519z.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091230121309.6117@nanako3.lavabit.com> ``` Nanako Shiraishi writes: > Junio, could you tell us what happened to this thread? > > "git archive HEAD non-existing-path" doesn't complain like "git > add" does, and the patch is to fix it. No discussion. It walks the tree twice, once for the checking and then another for doing its real work. Doing that way obviously looks stupid and inefficient but on the other hand it can fail before doing anything, which may be a big plus. I couldn't decide the pros-and-cons at the moment. Probably it is worth queuing the patch as is, and if there are motivated people who want to, let them "optimize" it by rolling that check into the main loop. I dunno. ```