* [BUG] git archive formats and dashdash @ 2009-12-10 21:26 Ilari Liusvaara 2009-12-10 22:05 ` Junio C Hamano 2009-12-10 22:20 ` [PATCH] builtin-archive: insert --format before double dash if necessary Miklos Vajna 0 siblings, 2 replies; 11+ messages in thread From: Ilari Liusvaara @ 2009-12-10 21:26 UTC (permalink / raw) To: git --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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] git archive formats and dashdash 2009-12-10 21:26 [BUG] git archive formats and dashdash Ilari Liusvaara @ 2009-12-10 22:05 ` Junio C Hamano 2009-12-10 22:22 ` Miklos Vajna 2009-12-10 22:25 ` [BUG] git archive formats and dashdash Ilari Liusvaara 2009-12-10 22:20 ` [PATCH] builtin-archive: insert --format before double dash if necessary Miklos Vajna 1 sibling, 2 replies; 11+ messages in thread From: Junio C Hamano @ 2009-12-10 22:05 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git Ilari Liusvaara <ilari.liusvaara@elisanet•fi> 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) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [BUG] git archive formats and dashdash 2009-12-10 22:05 ` Junio C Hamano @ 2009-12-10 22:22 ` Miklos Vajna 2009-12-10 22:42 ` Junio C Hamano 2009-12-10 22:25 ` [BUG] git archive formats and dashdash Ilari Liusvaara 1 sibling, 1 reply; 11+ messages in thread From: Miklos Vajna @ 2009-12-10 22:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ilari Liusvaara, git [-- Attachment #1: Type: text/plain, Size: 354 bytes --] On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano <gitster@pobox•com> 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. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] git archive formats and dashdash 2009-12-10 22:22 ` Miklos Vajna @ 2009-12-10 22:42 ` Junio C Hamano 2009-12-10 23:27 ` [PATCH] Fix archive format with -- on the command line Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2009-12-10 22:42 UTC (permalink / raw) To: Miklos Vajna; +Cc: Ilari Liusvaara, git Miklos Vajna <vmiklos@frugalware•org> writes: > On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano <gitster@pobox•com> 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Fix archive format with -- on the command line 2009-12-10 22:42 ` Junio C Hamano @ 2009-12-10 23:27 ` Junio C Hamano 2009-12-10 23:31 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2009-12-10 23:27 UTC (permalink / raw) To: Miklos Vajna, Ilari Liusvaara; +Cc: git 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 <ilari.liusvaara@elisanet•fi> Signed-off-by: Junio C Hamano <gitster@pobox•com> --- * 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix archive format with -- on the command line 2009-12-10 23:27 ` [PATCH] Fix archive format with -- on the command line Junio C Hamano @ 2009-12-10 23:31 ` Junio C Hamano 2009-12-12 15:00 ` René Scharfe 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2009-12-10 23:31 UTC (permalink / raw) To: René Scharfe; +Cc: Miklos Vajna, Ilari Liusvaara, git Junio C Hamano <gitster@pobox•com> 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix archive format with -- on the command line 2009-12-10 23:31 ` Junio C Hamano @ 2009-12-12 15:00 ` René Scharfe 2009-12-30 3:13 ` Nanako Shiraishi 0 siblings, 1 reply; 11+ messages in thread From: René Scharfe @ 2009-12-12 15:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, Ilari Liusvaara, git Am 11.12.2009 00:31, schrieb Junio C Hamano: > Junio C Hamano <gitster@pobox•com> 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 <rene.scharfe@lsrfire•ath.cx> --- 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, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix archive format with -- on the command line 2009-12-12 15:00 ` René Scharfe @ 2009-12-30 3:13 ` Nanako Shiraishi 2009-12-30 8:11 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Nanako Shiraishi @ 2009-12-30 3:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: René Scharfe, Miklos Vajna, Ilari Liusvaara, git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix archive format with -- on the command line 2009-12-30 3:13 ` Nanako Shiraishi @ 2009-12-30 8:11 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2009-12-30 8:11 UTC (permalink / raw) To: Nanako Shiraishi; +Cc: René Scharfe, Miklos Vajna, Ilari Liusvaara, git Nanako Shiraishi <nanako3@lavabit•com> 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] git archive formats and dashdash 2009-12-10 22:05 ` Junio C Hamano 2009-12-10 22:22 ` Miklos Vajna @ 2009-12-10 22:25 ` Ilari Liusvaara 1 sibling, 0 replies; 11+ messages in thread From: Ilari Liusvaara @ 2009-12-10 22:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Dec 10, 2009 at 02:05:39PM -0800, Junio C Hamano wrote: > Ilari Liusvaara <ilari.liusvaara@elisanet•fi> 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 <dpotapov@gmail•com> Date: Mon Sep 14 00:17:01 2009 +0400 Describe: v1.6.5-rc1-7-g0f4b377 / v1.6.5-rc2~34 -Ilari ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] builtin-archive: insert --format before double dash if necessary. 2009-12-10 21:26 [BUG] git archive formats and dashdash Ilari Liusvaara 2009-12-10 22:05 ` Junio C Hamano @ 2009-12-10 22:20 ` Miklos Vajna 1 sibling, 0 replies; 11+ messages in thread From: Miklos Vajna @ 2009-12-10 22:20 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git Reported-by: Ilari Liusvaara <ilari.liusvaara@elisanet•fi> Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-12-30 8:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-10 21:26 [BUG] git archive formats and dashdash Ilari Liusvaara 2009-12-10 22:05 ` Junio C Hamano 2009-12-10 22:22 ` Miklos Vajna 2009-12-10 22:42 ` Junio C Hamano 2009-12-10 23:27 ` [PATCH] Fix archive format with -- on the command line Junio C Hamano 2009-12-10 23:31 ` Junio C Hamano 2009-12-12 15:00 ` René Scharfe 2009-12-30 3:13 ` Nanako Shiraishi 2009-12-30 8:11 ` Junio C Hamano 2009-12-10 22:25 ` [BUG] git archive formats and dashdash Ilari Liusvaara 2009-12-10 22:20 ` [PATCH] builtin-archive: insert --format before double dash if necessary Miklos Vajna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox