git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Eli Schwartz" <eschwartz93@gmail.com>,
	"René Scharfe" <l.s.r@web.de>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Konstantin Ryabitsev" <konstantin@linuxfoundation.org>,
	"Michal Suchánek" <msuchanek@suse.de>,
	"Raymond E . Pasco" <ray@ameretat.dev>,
	demerphq <demerphq@gmail.com>, "Theodore Ts'o" <tytso@mit.edu>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH 4/9] archive: omit the shell for built-in "command" filters
Date: Thu,  2 Feb 2023 10:32:24 +0100	[thread overview]
Message-ID: <patch-4.9-8bc1bfd1fe2-20230202T093212Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.9-00000000000-20230202T093212Z-avarab@gmail.com>

Since the "tar.<format.command" interface was added in [1] we've
promised to invoke the shell to run if e.g. "gzip -cn" is
configured. That common format was then added as a default in [2].

But if we have no such configuration we can safely assume that the
user isn't expecting the "gzip" to be invoked via a shell, and we can
skip the "sh" process.

We are intentionally not treating a configured
"tar.<format>.command=<cmd>" where "<cmd>" is equivalent to our
hardcoded "<cmd>" the same as when the same "<cmd>" is specified in
the config. If the user has configured e.g. "gzip -cn" they may be
relying on what the shell gives them over a direct execve() of "gzip".

This makes us marginally faster, but the real point is to make the
error handling easier to deal with. When we're using the shell we
don't know if e.g. the "gzip" we spawned fails as easily,
i.e. "start_command()" won't fail, because we can find the "sh".

A subsequent commit will tweak the default that [3] introduced to be a
fallback instead, at which point we'll need this for correctness.

1. 767cf4579f0 (archive: implement configurable tar filters, 2011-06-21)
2. 0e804e09938 (archive: provide builtin .tar.gz filter, 2011-06-21)
3. 4f4be00d302 (archive-tar: use internal gzip by default, 2022-06-15)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/tar.txt |  3 +++
 archive-tar.c                | 17 +++++++++++++----
 archive.h                    |  1 +
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/tar.txt b/Documentation/config/tar.txt
index 894c1163bb9..5456fc617a2 100644
--- a/Documentation/config/tar.txt
+++ b/Documentation/config/tar.txt
@@ -18,6 +18,9 @@ tar.<format>.command::
 The `tar.gz` and `tgz` formats are defined automatically and use the
 magic command `git archive gzip` by default, which invokes an internal
 implementation of gzip.
++
+The automatically defined commands do not invoke the shell, avoiding
+the minor overhead of an extra sh(1) process.
 
 tar.<format>.remote::
 	If true, enable the format for use by remote clients via
diff --git a/archive-tar.c b/archive-tar.c
index f8fad2946ef..8c5de949c64 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -367,12 +367,13 @@ static struct archiver *find_tar_filter(const char *name, size_t len)
 }
 
 static int tar_filter_config(const char *var, const char *value,
-			     void *data UNUSED)
+			     void *data)
 {
 	struct archiver *ar;
 	const char *name;
 	const char *type;
 	size_t namelen;
+	int *configured = data;
 
 	if (parse_config_key(var, "tar", &name, &namelen, &type) < 0 || !name)
 		return 0;
@@ -388,6 +389,9 @@ static int tar_filter_config(const char *var, const char *value,
 		tar_filters[nr_tar_filters++] = ar;
 	}
 
+	if (configured && *configured)
+		ar->flags |= ARCHIVER_COMMAND_FROM_CONFIG;
+
 	if (!strcmp(type, "command")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -495,8 +499,12 @@ static int write_tar_filter_archive(const struct archiver *ar,
 	if (args->compression_level >= 0)
 		strbuf_addf(&cmd, " -%d", args->compression_level);
 
-	strvec_push(&filter.args, cmd.buf);
-	filter.use_shell = 1;
+	if (ar->flags & ARCHIVER_COMMAND_FROM_CONFIG) {
+		strvec_push(&filter.args, cmd.buf);
+		filter.use_shell = 1;
+	} else {
+		strvec_split(&filter.args, cmd.buf);
+	}
 	filter.in = -1;
 	filter.silent_exec_failure = 1;
 
@@ -526,13 +534,14 @@ static struct archiver tar_archiver = {
 void init_tar_archiver(void)
 {
 	int i;
+	int configured = 1;
 	register_archiver(&tar_archiver);
 
 	tar_filter_config("tar.tgz.command", internal_gzip_command, NULL);
 	tar_filter_config("tar.tgz.remote", "true", NULL);
 	tar_filter_config("tar.tar.gz.command", internal_gzip_command, NULL);
 	tar_filter_config("tar.tar.gz.remote", "true", NULL);
-	git_config(git_tar_config, NULL);
+	git_config(git_tar_config, &configured);
 	for (i = 0; i < nr_tar_filters; i++) {
 		/* omit any filters that never had a command configured */
 		if (tar_filters[i]->filter_command)
diff --git a/archive.h b/archive.h
index 6b51288c2ed..9686b3b5cc1 100644
--- a/archive.h
+++ b/archive.h
@@ -40,6 +40,7 @@ enum archiver_flags {
 	ARCHIVER_WANT_COMPRESSION_LEVELS = 1<<0,
 	ARCHIVER_REMOTE = 1<<1,
 	ARCHIVER_HIGH_COMPRESSION_LEVELS = 1<<2,
+	ARCHIVER_COMMAND_FROM_CONFIG = 1<<3,
 };
 struct archiver {
 	const char *name;
-- 
2.39.1.1392.g63e6d408230


  parent reply	other threads:[~2023-02-02  9:32 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-31  0:06 Stability of git-archive, breaking (?) the Github universe, and a possible solution Eli Schwartz
2023-01-31  7:49 ` Ævar Arnfjörð Bjarmason
2023-01-31  9:11   ` Eli Schwartz
2023-02-02  9:32   ` [PATCH 0/9] git archive: use gzip again by default, document output stabilty Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 1/9] archive & tar config docs: de-duplicate configuration section Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 2/9] git config docs: document "tar.<format>.{command,remote}" Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 3/9] archiver API: make the "flags" in "struct archiver" an enum Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` Ævar Arnfjörð Bjarmason [this message]
2023-02-02  9:32     ` [PATCH 5/9] archive-tar.c: move internal gzip implementation to a function Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 6/9] archive: use "gzip -cn" for stability, not "git archive gzip" Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 7/9] test-lib.sh: add a lazy GZIP prerequisite Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 8/9] archive tests: test for "gzip -cn" and "git archive gzip" stability Ævar Arnfjörð Bjarmason
2023-02-02  9:32     ` [PATCH 9/9] git archive docs: document output non-stability Ævar Arnfjörð Bjarmason
2023-02-02 10:25       ` brian m. carlson
2023-02-02 10:30         ` Ævar Arnfjörð Bjarmason
2023-02-02 16:34         ` Junio C Hamano
2023-02-04 17:46           ` brian m. carlson
2023-02-02 16:17     ` [PATCH 0/9] git archive: use gzip again by default, document output stabilty Phillip Wood
2023-02-02 16:40       ` Junio C Hamano
2023-02-03 13:49       ` Ævar Arnfjörð Bjarmason
2023-02-06 14:46         ` Phillip Wood
2023-02-03 15:47       ` Theodore Ts'o
2023-02-02 16:25     ` Junio C Hamano
2023-02-04 18:08       ` René Scharfe
2023-02-05 21:30         ` Ævar Arnfjörð Bjarmason
2023-02-12 17:41           ` René Scharfe
2023-02-02 19:23     ` Raymond E. Pasco
2023-02-03  8:06       ` [PATCH] archive: document output stability concerns Raymond E. Pasco
2023-01-31  9:54 ` Stability of git-archive, breaking (?) the Github universe, and a possible solution brian m. carlson
2023-01-31 11:31   ` Ævar Arnfjörð Bjarmason
2023-01-31 15:05   ` Konstantin Ryabitsev
2023-01-31 22:32     ` brian m. carlson
2023-02-01  9:40       ` Ævar Arnfjörð Bjarmason
2023-02-01 11:34         ` demerphq
2023-02-01 12:21           ` Michal Suchánek
2023-02-01 12:48             ` demerphq
2023-02-01 13:43               ` Ævar Arnfjörð Bjarmason
2023-02-01 15:21                 ` demerphq
2023-02-01 18:56                   ` Theodore Ts'o
2023-02-02 21:19                     ` Joey Hess
2023-02-03  4:02                       ` Theodore Ts'o
2023-02-03 13:32                         ` Ævar Arnfjörð Bjarmason
2023-02-01 23:16         ` brian m. carlson
2023-02-01 23:37           ` Junio C Hamano
2023-02-02 23:01             ` brian m. carlson
2023-02-02 23:47               ` rsbecker
2023-02-03 13:18                 ` Ævar Arnfjörð Bjarmason
2023-02-02  0:42           ` Ævar Arnfjörð Bjarmason
2023-02-01 12:17       ` Raymond E. Pasco
2023-01-31 15:56   ` Eli Schwartz
2023-01-31 16:20     ` Konstantin Ryabitsev
2023-01-31 16:34       ` Eli Schwartz
2023-01-31 20:34         ` Konstantin Ryabitsev
2023-01-31 20:45         ` Michal Suchánek
2023-02-01  1:33     ` brian m. carlson
2023-02-01 12:42   ` Ævar Arnfjörð Bjarmason
2023-02-01 23:18     ` brian m. carlson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-4.9-8bc1bfd1fe2-20230202T093212Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=demerphq@gmail.com \
    --cc=eschwartz93@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=konstantin@linuxfoundation.org \
    --cc=l.s.r@web.de \
    --cc=msuchanek@suse.de \
    --cc=ray@ameretat.dev \
    --cc=sandals@crustytoothpaste.net \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).