[BUG] git archive formats and dashdash

[BUG] git archive formats and dashdash

[Ilari Liusvaara]

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

Re: [BUG] git archive formats and dashdash

[Junio C Hamano]

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)

[PATCH] builtin-archive: insert --format before double dash if necessary.

[Miklos Vajna]

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

Re: [BUG] git archive formats and dashdash

[Miklos Vajna]

[-- 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 --]

Re: [BUG] git archive formats and dashdash

[Ilari Liusvaara]

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

Re: [BUG] git archive formats and dashdash

[Junio C Hamano]

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.

[PATCH] Fix archive format with -- on the command line

[Junio C Hamano]

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

Re: [PATCH] Fix archive format with -- on the command line

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

Re: [PATCH] Fix archive format with -- on the command line

[René Scharfe]

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,

Re: [PATCH] Fix archive format with -- on the command line

[Nanako Shiraishi]

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.

Re: [PATCH] Fix archive format with -- on the command line

[Junio C Hamano]

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.