Re: [PATCH 2/2] teach git-archive to auto detect the output format

[Junio C Hamano <gitster@pobox.com>]

Dmitry Potapov <dpotapov@gmail.com> writes:

> diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
> index f7a3b95..c6fb21c 100644
> --- a/Documentation/git-archive.txt
> +++ b/Documentation/git-archive.txt
> @@ -35,7 +35,9 @@ OPTIONS
>  
>  --format=<fmt>::
>  	Format of the resulting archive: 'tar' or 'zip'.  The default
> -	is 'tar'.
> +	is 'tar', unless the output file is specified, and it has a known
> +	extension (such as '.zip') then the default for the output format
> +	will be determined by this extension.

Once it is _determined_, then it is not the default anymore.

	If this option is not given, and the output file is specified, the
	format is inferred from the filename if possible (e.g. writing to
	"foo.zip" makes the output to be in the zip format).  Otherwise
	the output format is `tar`.

> @@ -130,6 +132,12 @@ git archive --format=zip
>  	Put everything in the current head's Documentation/ directory
>  	into 'git-1.4.0-docs.zip', with the prefix 'git-docs/'.
>  
> +git archive -o latest.zip HEAD::
> +
> +	Create a Zip archive that contains the contents of the latest
> +	commit on the current branch. Note that the output format is
> +	specified implicitly by the extension of the output file.
> +

Perhaps "s/specified implicitly/inferred/" but that is a minor point.

> diff --git a/builtin-archive.c b/builtin-archive.c
> index 565314b..878c6b2 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -77,14 +89,27 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>  			"retrieve the archive from remote repository <repo>"),
>  		OPT_STRING(0, "exec", &exec, "cmd",
>  			"path to the remote git-upload-archive command"),
> +		OPT_STRING(0, "format", &format, "fmt", "archive format"),
>  		OPT_END()
>  	};
> +	char fmt_opt[32];
>  
>  	argc = parse_options(argc, argv, prefix, local_opts, NULL,
>  			     PARSE_OPT_KEEP_ALL);
>  
>  	if (output)
> +	{

On the same line, i.e. "if (output) {".

>  		create_output_file(output);
> +		if (!format)
> +			format = format_from_name(output);
> +	}
> +
> +	if (format)
> +	{

On the same line, i.e. "if (format) {".

> +		sprintf(fmt_opt, "--format=%s", format);
> +		argv[argc++] = fmt_opt;
> +		argv[argc] = NULL;

Did you make sure you are allowed to write into argv[] and the array is
large enough?  You probably need to make a copy of the array.

Otherwise, the idea feels sound.