{"thread":{"id":"21931","subject":"[BUG] git archive formats and dashdash","startedAt":"2009-12-10T21:26:36Z","lastAt":"2009-12-30T08:11:52Z","messageCount":11,"participants":["Ilari Liusvaara","Junio C Hamano","Miklos Vajna","René Scharfe","Nanako Shiraishi"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"129704","messageId":"20091210212636.GA27722@Knoppix","threadId":"21931","inReplyTo":null,"subject":"[BUG] git archive formats and dashdash","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-10T21:26:36Z","receivedAt":"2009-12-10T21:26:36Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"--format option of git archive stops working if -- is used:\n\nShell session:\n----------------\n$ git archive -o test3.zip --format=zip master -- exec_cmd.c\n$ file test3.zip\ntest3.zip: POSIX tar archive\n\n# Eh...\n\n$ git archive -o test4.zip --format=zip master exec_cmd.c\n$ file test4.zip\ntest4.zip: Zip archive data, at least v1.0 to extract\n\n# That worked.\n\n----------------\n\nThe bug seems to be in that archive appends the --format\noption to its arguments, not taking into account that there\nmay be -- before that point, disabling option interpretation.\n\nGit version 1.6.6-rc2\n\n-Ilari\n"},{"id":"129705","messageId":"7v1vj2a3ik.fsf@alter.siamese.dyndns.org","threadId":"21931","inReplyTo":"20091210212636.GA27722@Knoppix","subject":"Re: [BUG] git archive formats and dashdash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-10T22:05:39Z","receivedAt":"2009-12-10T22:05:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> --format option of git archive stops working if -- is used:\n\nGood catch. Is this a regression between 1.6.5 and the current code?\n\n builtin-archive.c | 16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 12351e9..8ef5ab3 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -106,13 +106,17 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \tif (format) {\n \t\tsprintf(fmt_opt, \"--format=%s\", format);\n \t\t/*\n-\t\t * This is safe because either --format and/or --output must\n-\t\t * have been given on the original command line if we get to\n-\t\t * this point, and parse_options() must have eaten at least\n-\t\t * one argument, i.e. we have enough room to append to argv[].\n+\t\t * We have enough room in argv[] to muck it in place,\n+\t\t * because either --format and/or --output must have\n+\t\t * been given on the original command line if we get\n+\t\t * to this point, and parse_options() must have eaten\n+\t\t * it, i.e. we can add back one element to the array.\n+\t\t * But argv[] may contain \"--\" so we should make this\n+\t\t * the first option.\n \t\t */\n-\t\targv[argc++] = fmt_opt;\n-\t\targv[argc] = NULL;\n+\t\tmemmove(argv + 2, argv + 1, sizeof(*argv) * argc);\n+\t\targv[1] = fmt_opt;\n+\t\targv[++argc] = NULL;\n \t}\n \n \tif (remote)\n"},{"id":"129706","messageId":"20091210222005.GP4114@genesis.frugalware.org","threadId":"21931","inReplyTo":"20091210212636.GA27722@Knoppix","subject":"[PATCH] builtin-archive: insert --format before double dash if necessary.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-12-10T22:20:05Z","receivedAt":"2009-12-10T22:20:05Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Reported-by: Ilari Liusvaara \nSigned-off-by: Miklos Vajna \n---\n\nWhat about this fix?\n\n builtin-archive.c | 23 ++++++++++++++++++++++-\n 1 files changed, 22 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 12351e9..ffbc9b0 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -104,14 +104,35 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (format) {\n+\t\tint i, found = 0;\n+\t\tconst char *ptr = NULL;\n+\n \t\tsprintf(fmt_opt, \"--format=%s\", format);\n \t\t/*\n \t\t * This is safe because either --format and/or --output must\n \t\t * have been given on the original command line if we get to\n \t\t * this point, and parse_options() must have eaten at least\n \t\t * one argument, i.e. we have enough room to append to argv[].\n+\t\t *\n+\t\t * First check if we have to insert the argument before\n+\t\t * two dashes.\n \t\t */\n-\t\targv[argc++] = fmt_opt;\n+\t\tfor (i = 0; i < argc; i++) {\n+\t\t\tif (found) {\n+\t\t\t\tconst char *tmp = argv[i];\n+\t\t\t\targv[i] = ptr;\n+\t\t\t\tptr = tmp;\n+\t\t\t} else if (!strcmp(argv[i], \"--\")) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tptr = argv[i];\n+\t\t\t\targv[i] = fmt_opt;\n+\t\t\t\targc++;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/* No double dash? Then just append it to the end of the list. */\n+\t\tif (!found)\n+\t\t\targv[argc++] = fmt_opt;\n \t\targv[argc] = NULL;\n \t}\n \n-- \n1.6.5.2\n"},{"id":"129707","messageId":"20091210222258.GQ4114@genesis.frugalware.org","threadId":"21931","inReplyTo":"7v1vj2a3ik.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git archive formats and dashdash","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-12-10T22:22:58Z","receivedAt":"2009-12-10T22:22:58Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote:\n> Good catch. Is this a regression between 1.6.5 and the current code?\n\nAh, you version is much shorter. :)\n\nI think it was introduced by 0f4b377c (git-archive: infer output format\nfrom filename when unspecified, 2009-09-14), so it was introduced\nbefore 1.6.5.\n"},{"id":"129708","messageId":"20091210222503.GA27840@Knoppix","threadId":"21931","inReplyTo":"7v1vj2a3ik.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git archive formats and dashdash","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-10T22:25:03Z","receivedAt":"2009-12-10T22:25:03Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> > --format option of git archive stops working if -- is used:\n> \n> Good catch. Is this a regression between 1.6.5 and the current code?\n\nDoesn't appear to be so:\n\nBased on quick look at source, it seems that this is indeed regression,\nand the commit which introduced it is:\n\ncommit 0f4b377c20fb7d93f8bfeec39efb2b9392d6aebc\nAuthor: Dmitry Potapov \nDate: Mon Sep 14 00:17:01 2009 +0400\n\nDescribe: v1.6.5-rc1-7-g0f4b377 / v1.6.5-rc2~34\n\n-Ilari\n"},{"id":"129709","messageId":"7vws0u8n99.fsf@alter.siamese.dyndns.org","threadId":"21931","inReplyTo":"20091210222258.GQ4114@genesis.frugalware.org","subject":"Re: [BUG] git archive formats and dashdash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-10T22:42:10Z","receivedAt":"2009-12-10T22:42:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote:\n>> Good catch. Is this a regression between 1.6.5 and the current code?\n>\n> Ah, you version is much shorter. :)\n\nMore importantly it would avoid running \"archive path --format=zip\".\n\nThat may be reordered by the current parse-options implementation, and\nother command line parsers (e.g. \"git diff HEAD path --stat\") may accept\ncommand line in such an order, but I somehow find it utterly wrong. We\nshould prepare ourselves for the (distant) day on which we start enforcing\n\"options then rev then paths\" order from the command line.\n"},{"id":"129712","messageId":"7vhbry8l54.fsf_-_@alter.siamese.dyndns.org","threadId":"21931","inReplyTo":"7vws0u8n99.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Fix archive format with -- on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-10T23:27:51Z","receivedAt":"2009-12-10T23:27:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giving --format from the command line, or using output file extention to\nDWIM the output format, with a pathspec that is disambiguated with an\nexplicit double-dash on the command line, e.g.\n\n git archive -o file --format=zip HEAD -- path\n git archive -o file.zip HEAD -- path\n\ndidn't work correctly.\n\nThis was because the code reordered (when one was given) or added (when\nthe former was inferred) a --format argument at the end, effectively\nmaking it to \"archive HEAD -- path --format=zip\", i.e. an extra pathspec\nthat is unlikely to match anything.\n\nThe command line argument list should always be \"options, revs and then\npaths\", and we should set a good example by inserting the --format at the\nbeginning instead.\n\nReported-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \n---\n\n * So here is one with a proper commit log message. \n\n builtin-archive.c | 16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 12351e9..8ef5ab3 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -106,13 +106,17 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \tif (format) {\n \t\tsprintf(fmt_opt, \"--format=%s\", format);\n \t\t/*\n-\t\t * This is safe because either --format and/or --output must\n-\t\t * have been given on the original command line if we get to\n-\t\t * this point, and parse_options() must have eaten at least\n-\t\t * one argument, i.e. we have enough room to append to argv[].\n+\t\t * We have enough room in argv[] to muck it in place,\n+\t\t * because either --format and/or --output must have\n+\t\t * been given on the original command line if we get\n+\t\t * to this point, and parse_options() must have eaten\n+\t\t * it, i.e. we can add back one element to the array.\n+\t\t * But argv[] may contain \"--\"; we should make it the\n+\t\t * first option.\n \t\t */\n-\t\targv[argc++] = fmt_opt;\n-\t\targv[argc] = NULL;\n+\t\tmemmove(argv + 2, argv + 1, sizeof(*argv) * argc);\n+\t\targv[1] = fmt_opt;\n+\t\targv[++argc] = NULL;\n \t}\n \n \tif (remote)\n-- \n1.6.6.rc2\n"},{"id":"129713","messageId":"7vd42m8kyr.fsf@alter.siamese.dyndns.org","threadId":"21931","inReplyTo":"7vhbry8l54.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix archive format with -- on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-10T23:31:40Z","receivedAt":"2009-12-10T23:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Giving --format from the command line, or using output file extention to\n> DWIM the output format, with a pathspec that is disambiguated with an\n> explicit double-dash on the command line, e.g.\n>\n> git archive -o file --format=zip HEAD -- path\n> git archive -o file.zip HEAD -- path\n>\n> didn't work correctly.\n>\n> This was because the code reordered (when one was given) or added (when\n> the former was inferred) a --format argument at the end, effectively\n> making it to \"archive HEAD -- path --format=zip\", i.e. an extra pathspec\n> that is unlikely to match anything.\n\nA side note to this issue is that\n\n $ git add non-existing-path\n\ncomplains but\n\n $ git archive HEAD non-existing-path\n\ndoesn't. Is this something we should consider a bug, or a feature?\n"},{"id":"129810","messageId":"4B23B019.40106@lsrfire.ath.cx","threadId":"21931","inReplyTo":"7vd42m8kyr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix archive format with -- on the command line","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-12-12T15:00:41Z","receivedAt":"2009-12-12T15:00:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.12.2009 00:31, schrieb Junio C Hamano:\n> Junio C Hamano writes:\n> \n>> Giving --format from the command line, or using output file extention to\n>> DWIM the output format, with a pathspec that is disambiguated with an\n>> explicit double-dash on the command line, e.g.\n>>\n>> git archive -o file --format=zip HEAD -- path\n>> git archive -o file.zip HEAD -- path\n>>\n>> didn't work correctly.\n>>\n>> This was because the code reordered (when one was given) or added (when\n>> the former was inferred) a --format argument at the end, effectively\n>> making it to \"archive HEAD -- path --format=zip\", i.e. an extra pathspec\n>> that is unlikely to match anything.\n> \n> A side note to this issue is that\n> \n> $ git add non-existing-path\n> \n> complains but\n> \n> $ git archive HEAD non-existing-path\n> \n> doesn't. Is this something we should consider a bug, or a feature?\n\nI wouldn't go so far as to call it a bug, but it's certainly a missing\nfeature. If the user asks for something we can't give him or her, it's\nbetter to report that fact. We didn't do that because it doesn't fall\nout naturally from the archive streaming loop. ls-tree doesn't do it,\neither, by the way.\n\nSomething like this?\n\n-- >8 --\nSubject: archive: complain about path specs that don't match anything\n\nVerify that all path specs match at least one path in the specified\ntree and reject those that don't.\n\nThis would have made the bug fixed by 782a0005 easier to find.\n\nThis implementation is simple to the point of being stupid. It walks\nthe full tree for each path spec until it matches something. It's short\nand seems to be fast enough, though.\n\nSigned-off-by: Rene Scharfe \n---\n archive.c | 25 ++++++++++++++++++++++++-\n 1 files changed, 24 insertions(+), 1 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 55b2732..5b88507 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -211,10 +211,33 @@ static const struct archiver *lookup_archiver(const char *name)\n \treturn NULL;\n }\n \n+static int reject_entry(const unsigned char *sha1, const char *base,\n+\t\t\tint baselen, const char *filename, unsigned mode,\n+\t\t\tint stage, void *context)\n+{\n+\treturn -1;\n+}\n+\n+static int path_exists(struct tree *tree, const char *path)\n+{\n+\tconst char *pathspec[] = { path, NULL };\n+\n+\tif (read_tree_recursive(tree, \"\", 0, 0, pathspec, reject_entry, NULL))\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static void parse_pathspec_arg(const char **pathspec,\n \t\tstruct archiver_args *ar_args)\n {\n-\tar_args->pathspec = get_pathspec(\"\", pathspec);\n+\tar_args->pathspec = pathspec = get_pathspec(\"\", pathspec);\n+\tif (pathspec) {\n+\t\twhile (*pathspec) {\n+\t\t\tif (!path_exists(ar_args->tree, *pathspec))\n+\t\t\t\tdie(\"path not found: %s\", *pathspec);\n+\t\t\tpathspec++;\n+\t\t}\n+\t}\n }\n \n static void parse_treeish_arg(const char **argv,\n"},{"id":"130508","messageId":"20091230121309.6117@nanako3.lavabit.com","threadId":"21931","inReplyTo":"4B23B019.40106@lsrfire.ath.cx","subject":"Re: [PATCH] Fix archive format with -- on the command line","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-12-30T03:13:09Z","receivedAt":"2009-12-30T03:13:09Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Junio, could you tell us what happened to this thread?\n\n\"git archive HEAD non-existing-path\" doesn't complain like \"git\nadd\" does, and the patch is to fix it. No discussion.\n"},{"id":"130527","messageId":"7vhbr8519z.fsf@alter.siamese.dyndns.org","threadId":"21931","inReplyTo":"20091230121309.6117@nanako3.lavabit.com","subject":"Re: [PATCH] Fix archive format with -- on the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-30T08:11:52Z","receivedAt":"2009-12-30T08:11:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi writes:\n\n> Junio, could you tell us what happened to this thread?\n>\n> \"git archive HEAD non-existing-path\" doesn't complain like \"git\n> add\" does, and the patch is to fix it. No discussion.\n\nIt walks the tree twice, once for the checking and then another for doing\nits real work. Doing that way obviously looks stupid and inefficient but\non the other hand it can fail before doing anything, which may be a big\nplus. I couldn't decide the pros-and-cons at the moment.\n\nProbably it is worth queuing the patch as is, and if there are motivated\npeople who want to, let them \"optimize\" it by rolling that check into the\nmain loop.\n\nI dunno.\n"}]}