git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCHv2 5/9] archive: move file extension format-guessing lower

From
Jeff King <peff@peff.net>
Date
Jun 22, 2011, 01:24 UTC
Message-ID
<20110622012448.GE30604@sigill.intra.peff.net>
In-Reply-To
<20110622011923.GA30370@sigill.intra.peff.net>

The process for guessing an archive output format based on the filename is something like this:

  a. parse --output in cmd_archive; check the filename
     against a static set of mapping heuristics (right now
     it just matches ".zip" for zip files).
  b. if found, stick a fake "--format=zip" at the beginning
     of the arguments list (if the user did specify a
     --format manually, the later option will override our
     fake one)
  c. if it's a remote call, ship the arguments to the remote
     (including the fake), which will call write_archive on
     their end
  d. if it's local, ship the arguments to write_archive
     locally
There are two problems:
  1. The set of mappings is static and at too high a level.
     The write_archive level is going to check config for
     user-defined formats, some of which will specify
     extensions. We need to delay lookup until those are
     parsed, so we can match against them.
  2. For a remote archive call, our set of mappings (or
     formats) may not match the remote side's. This is OK in
     practice right now, because all versions of git
     understand "zip" and "tar". But as new formats are
     added, there is going to be a mismatch between what the
     client can do and what the remote server can do.

To fix (1), this patch refactors the location guessing to happen at the write_archive level, instead of the cmd_archive level. So instead of sticking a fake --format field in the argv list, we actually pass a "name hint" down the callchain; this hint is used at the appropriate time to guess the format (if one hasn't been given already).

This patch leaves (2) unfixed. The name_hint is converted to a "--format" option as before, and passed to the remote. This means the local side's idea of how extensions map to formats will take precedence.

Another option would be to pass the name hint to the remote side and let the remote choose. This isn't a good idea for two reasons:

  1. There's no room in the protocol for passing that
     information. We can pass a new argument, but older
     versions of git on the server will choke on it.
  2. Letting the remote side decide creates a silent
     inconsistency in user experience. Consider the case
     that the locally installed git knows about the "tar.gz"
     format, but a remote server doesn't.
     Running "git archive -o foo.tar.gz" will use the tar.gz
     format. If we use --remote, and the local side chooses
     the format, then we send "--format=tar.gz" to the
     remote, which will complain about the unknown format.
     But if we let the remote side choose the format, then
     it will realize that it doesn't know about "tar.gz" and
     output uncompressed tar without even issuing a warning.
Signed-off-by: Jeff King <peff@peff.net>
---
Code same as v1, but I cleaned up the commit message to be a little less
rambling.
 archive.c                |   25 +++++++++++++++++++---
 archive.h                |    4 ++-
 builtin/archive.c        |   51 ++++++++++++++-------------------------------
 builtin/upload-archive.c |    2 +-
 4 files changed, 41 insertions(+), 41 deletions(-)
diff --git a/archive.c b/archive.c
index a0a5beb..7d0ca32 100644
--- a/archive.c
+++ b/archive.c
@@ -298,9 +298,10 @@ static void parse_treeish_arg(const char **argv,
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
 
 static int parse_archive_args(int argc, const char **argv,
-		const struct archiver **ar, struct archiver_args *args)
+		const struct archiver **ar, struct archiver_args *args,
+		const char *name_hint)
 {
-	const char *format = "tar";
+	const char *format = NULL;
 	const char *base = NULL;
 	const char *remote = NULL;
 	const char *exec = NULL;
@@ -359,6 +360,11 @@ static int parse_archive_args(int argc, const char **argv,
 		exit(0);
 	}
 
+	if (!format && name_hint)
+		format = archive_format_from_filename(name_hint);
+	if (!format)
+		format = "tar";
+
 	/* We need at least one parameter -- tree-ish */
 	if (argc < 1)
 		usage_with_options(archive_usage, opts);
@@ -384,7 +390,7 @@ static int parse_archive_args(int argc, const char **argv,
 }
 
 int write_archive(int argc, const char **argv, const char *prefix,
-		int setup_prefix)
+		  int setup_prefix, const char *name_hint)
 {
 	int nongit = 0;
 	const struct archiver *ar = NULL;
@@ -397,7 +403,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	init_tar_archiver();
 	init_zip_archiver();
 
-	argc = parse_archive_args(argc, argv, &ar, &args);
+	argc = parse_archive_args(argc, argv, &ar, &args, name_hint);
 	if (nongit) {
 		/*
 		 * We know this will die() with an error, so we could just
@@ -412,3 +418,14 @@ int write_archive(int argc, const char **argv, const char *prefix,
 
 	return ar->write_archive(ar, &args);
 }
+
+const char *archive_format_from_filename(const char *filename)
+{
+	const char *ext = strrchr(filename, '.');
+	if (!ext)
+		return NULL;
+	ext++;
+	if (!strcasecmp(ext, "zip"))
+		return "zip";
+	return NULL;
+}
diff --git a/archive.h b/archive.h
index b3cf219..202d528 100644
--- a/archive.h
+++ b/archive.h
@@ -29,6 +29,8 @@ extern void init_zip_archiver(void);
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args, const unsigned char *sha1, const char *path, size_t pathlen, unsigned int mode, void *buffer, unsigned long size);
 
 extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
-extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix);
+extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix, const char *name_hint);
+
+const char *archive_format_from_filename(const char *filename);
 
 #endif	/* ARCHIVE_H */
diff --git a/builtin/archive.c b/builtin/archive.c
index b14eaba..2578cf5 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -24,7 +24,8 @@ static void create_output_file(const char *output_file)
 }
 
 static int run_remote_archiver(int argc, const char **argv,
-			       const char *remote, const char *exec)
+			       const char *remote, const char *exec,
+			       const char *name_hint)
 {
 	char buf[LARGE_PACKET_MAX];
 	int fd[2], i, len, rv;
@@ -37,6 +38,17 @@ static int run_remote_archiver(int argc, const char **argv,
 	transport = transport_get(_remote, _remote->url[0]);
 	transport_connect(transport, "git-upload-archive", exec, fd);
 
+	/*
+	 * Inject a fake --format field at the beginning of the
+	 * arguments, with the format inferred from our output
+	 * filename. This way explicit --format options can override
+	 * it.
+	 */
+	if (name_hint) {
+		const char *format = archive_format_from_filename(name_hint);
+		if (format)
+			packet_write(fd[1], "argument --format=%s\n", format);
+	}
 	for (i = 1; i < argc; i++)
 		packet_write(fd[1], "argument %s\n", argv[i]);
 	packet_flush(fd[1]);
@@ -63,17 +75,6 @@ static int run_remote_archiver(int argc, const char **argv,
 	return !!rv;
 }
 
-static const char *format_from_name(const char *filename)
-{
-	const char *ext = strrchr(filename, '.');
-	if (!ext)
-		return NULL;
-	ext++;
-	if (!strcasecmp(ext, "zip"))
-		return "--format=zip";
-	return NULL;
-}
-
 #define PARSE_OPT_KEEP_ALL ( PARSE_OPT_KEEP_DASHDASH | 	\
 			     PARSE_OPT_KEEP_ARGV0 | 	\
 			     PARSE_OPT_KEEP_UNKNOWN |	\
@@ -84,7 +85,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	const char *exec = "git-upload-archive";
 	const char *output = NULL;
 	const char *remote = NULL;
-	const char *format_option = NULL;
 	struct option local_opts[] = {
 		OPT_STRING('o', "output", &output, "file",
 			"write the archive to this file"),
@@ -98,32 +98,13 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, local_opts, NULL,
 			     PARSE_OPT_KEEP_ALL);
 
-	if (output) {
+	if (output)
 		create_output_file(output);
-		format_option = format_from_name(output);
-	}
-
-	/*
-	 * We have enough room in argv[] to muck it in place, because
-	 * --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.
-	 *
-	 * We add a fake --format option at the beginning, with the
-	 * format inferred from our output filename.  This way explicit
-	 * --format options can override it, and the fake option is
-	 * inserted before any "--" that might have been given.
-	 */
-	if (format_option) {
-		memmove(argv + 2, argv + 1, sizeof(*argv) * argc);
-		argv[1] = format_option;
-		argv[++argc] = NULL;
-	}
 
 	if (remote)
-		return run_remote_archiver(argc, argv, remote, exec);
+		return run_remote_archiver(argc, argv, remote, exec, output);
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	return write_archive(argc, argv, prefix, 1);
+	return write_archive(argc, argv, prefix, 1, output);
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 73f788e..e6bb97d 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -64,7 +64,7 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 	sent_argv[sent_argc] = NULL;
 
 	/* parse all options sent by the client */
-	return write_archive(sent_argc, sent_argv, prefix, 0);
+	return write_archive(sent_argc, sent_argv, prefix, 0, NULL);
 }
 
 __attribute__((format (printf, 1, 2)))

-- 
1.7.5.4.44.g4b107
Previous: Jeff KingNext: Jeff King
Message 42 of 56 in “archive: factor out write phase of tar format”
  1. 1/2 archive: factor out write phase of tar formatJeff King, Jun 14, 2011
  2. 2/2 archive: support gzipped tar filesJeff King, Jun 14, 2011
  3. J.H.Jun 14, 2011
  4. Jeff KingJun 14, 2011
  5. René ScharfeJun 14, 2011
  6. Jeff KingJun 14, 2011
  7. Jeff KingJun 14, 2011
  8. 0/7 user-configurable git-archive output formatsJeff King, Jun 15, 2011
  9. 1/7 archive: reorder option parsing and config readingJeff King, Jun 15, 2011
  10. 2/7 archive: add user-configurable tar-filter infrastructureJeff King, Jun 15, 2011
  11. Junio C HamanoJun 15, 2011
  12. Jeff KingJun 16, 2011
  13. 3/7 archive: support user tar-filters via --formatJeff King, Jun 15, 2011
  14. 4/7 archive: advertise user tar-filters in --listJeff King, Jun 15, 2011
  15. 5/7 archive: refactor format-guessing from filenameJeff King, Jun 15, 2011
  16. Junio C HamanoJun 15, 2011
  17. Jeff KingJun 16, 2011
  18. 6/7 archive: match extensions from user-configured formatsJeff King, Jun 15, 2011
  19. 7/7 archive: provide builtin .tar.gz filterJeff King, Jun 15, 2011
  20. Junio C HamanoJun 15, 2011
  21. Junio C HamanoJun 15, 2011
  22. Jeff KingJun 16, 2011
  23. Junio C HamanoJun 16, 2011
  24. Jeff KingJun 16, 2011
  25. Chris WebbJun 16, 2011
  26. Jeff KingJun 16, 2011
  27. Junio C HamanoJun 16, 2011
  28. Jeff KingJun 16, 2011
  29. John SzakmeisterJun 16, 2011
  30. Junio C HamanoJun 16, 2011
  31. Jeff KingJun 16, 2011
  32. René ScharfeJun 18, 2011
  33. Jakub NarebskiJun 18, 2011
  34. Junio C HamanoJun 20, 2011
  35. 0/9 configurable tar compressorsJeff King, Jun 22, 2011
  36. 1/9 archive: reorder option parsing and config readingJeff King, Jun 22, 2011
  37. 2/9 archive-tar: don't reload default config optionsJeff King, Jun 22, 2011
  38. 3/9 archive: refactor list of archive formatsJeff King, Jun 22, 2011
  39. Thiago FarinaJun 23, 2011
  40. Jeff KingJun 23, 2011
  41. 4/9 archive: pass archiver struct to write_archive callbackJeff King, Jun 22, 2011
  42. 5/9 archive: move file extension format-guessing lowerJeff King, Jun 22, 2011
  43. 6/9 archive: refactor file extension format-guessingJeff King, Jun 22, 2011
  44. 7/9 archive: implement configurable tar filtersJeff King, Jun 22, 2011
  45. Jeff KingJun 22, 2011
  46. René ScharfeJun 22, 2011
  47. Jeff KingJun 22, 2011
  48. 8/9 archive: provide builtin .tar.gz filterJeff King, Jun 22, 2011
  49. 9/9 upload-archive: allow user to turn off filtersJeff King, Jun 22, 2011
  50. Jeff KingJun 22, 2011
  51. Jeff KingJun 21, 2011
  52. René ScharfeJun 18, 2011
  53. Junio C HamanoJun 14, 2011
  54. Jeff KingJun 14, 2011
  55. Miles BaderJun 14, 2011
  56. Jeff KingJun 15, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.