All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Michael Henry <git@drmikehenry.com>, git@vger.kernel.org
Subject: [PATCH 1/5] bundle: let "-" mean stdin for reading operations
Date: Sat, 4 Mar 2023 05:26:14 -0500	[thread overview]
Message-ID: <ZAMcxlaQ1h2bN1fm@coredump.intra.peff.net> (raw)
In-Reply-To: <ZAMb8LSpm2gOrpeY@coredump.intra.peff.net>

For writing, "bundle create -" indicates that the bundle should be
written to stdout. But there's no matching handling of "-" for reading
operations. This is inconsistent, and a little inflexible (though one
can always use "/dev/stdin" on systems that support it).

However, it's easy to change. Once upon a time, the bundle-reading code
required a seekable descriptor, but that was fixed long ago in
e9ee84cf28b (bundle: allowing to read from an unseekable fd,
2011-10-13). So we just need to handle "-" explicitly when opening the
file.

We _could_ do this by handling "-" in read_bundle_header(), which the
reading functions all call already. But that is probably a bad idea.
It's also used by low-level code like the transport functions, and we
may want to be more careful there. We do not know that stdin is even
available to us, and certainly we would not want to get confused by a
configured URL that happens to point to "-".

So instead, let's add a helper to builtin/bundle.c. Since both the
bundle code and some of the callers refer to the bundle by name for
error messages, let's use the string "<stdin>" to make the output a bit
nicer to read.

Signed-off-by: Jeff King <peff@peff.net>
---
It occurs to me while sending this that even though the new tests check
stdin, they don't confirm that it works with an unseekable descriptor.
It does. I don't know if the lack of fseek/lseek calls gives us enough
confidence, or if we want to protect this with a test that does:

  cat some.bundle | git bundle unbundle -

 builtin/bundle.c       | 26 ++++++++++++++++++++++----
 t/t6020-bundle-misc.sh | 15 +++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index acceef62001..02dab1cfa02 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -107,6 +107,23 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	return ret;
 }
 
+/*
+ * Similar to read_bundle_header(), but handle "-" as stdin.
+ */
+static int open_bundle(const char *path, struct bundle_header *header,
+		       const char **name)
+{
+	if (!strcmp(path, "-")) {
+		if (name)
+			*name = "<stdin>";
+		return read_bundle_header_fd(0, header, "<stdin>");
+	}
+
+	if (name)
+		*name = path;
+	return read_bundle_header(path, header);
+}
+
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
@@ -118,12 +135,13 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 		OPT_END()
 	};
 	char *bundle_file;
+	const char *name;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
@@ -134,7 +152,7 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 		goto cleanup;
 	}
 
-	fprintf(stderr, _("%s is okay\n"), bundle_file);
+	fprintf(stderr, _("%s is okay\n"), name);
 	ret = 0;
 cleanup:
 	free(bundle_file);
@@ -155,7 +173,7 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
@@ -185,7 +203,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 7d40994991e..45d93588c91 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -606,4 +606,19 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
 	)
 '
 
+test_expect_success 'read bundle over stdin' '
+	git bundle create some.bundle HEAD &&
+
+	git bundle verify - <some.bundle 2>err &&
+	grep "<stdin> is okay" err &&
+
+	git bundle list-heads some.bundle >expect &&
+	git bundle list-heads - <some.bundle >actual &&
+	test_cmp expect actual &&
+
+	git bundle unbundle some.bundle >expect &&
+	git bundle unbundle - <some.bundle >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.40.0.rc1.500.g967c04631e


  reply	other threads:[~2023-03-04 10:26 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-25 12:58 `git bundle create -` may not write to `stdout` Michael Henry
2023-02-26 23:16 ` Jeff King
2023-03-03 22:31   ` Junio C Hamano
2023-03-03 22:54     ` Jeff King
2023-03-03 23:05       ` Junio C Hamano
2023-03-04  1:28         ` Jeff King
2023-03-04  1:46           ` Jeff King
2023-03-04 10:22             ` [PATCH 0/5] handling "-" as stdin/stdout in git bundle Jeff King
2023-03-04 10:26               ` Jeff King [this message]
2023-03-04 10:26               ` [PATCH 2/5] bundle: document handling of "-" as stdin Jeff King
2023-03-04 10:27               ` [PATCH 3/5] bundle: don't blindly apply prefix_filename() to "-" Jeff King
2023-03-04 10:31               ` [PATCH 4/5] parse-options: consistently allocate memory in fix_filename() Jeff King
2023-03-04 10:31               ` [PATCH 5/5] parse-options: use prefix_filename_except_for_dash() helper Jeff King
2023-03-04 10:55               ` [RFC/PATCH] bundle: turn on --all-progress-implied by default Jeff King
2023-03-06  3:44                 ` Robin H. Johnson
2023-03-06  5:38                   ` Jeff King
2023-03-06  9:25                     ` Jeff King
2023-03-06 17:41                 ` Junio C Hamano
2023-03-06 17:34             ` `git bundle create -` may not write to `stdout` Junio C Hamano
2023-03-04  1:14       ` Junio C Hamano
2023-03-04  1:43         ` Jeff King
2023-03-03 23:59     ` Michael Henry
2023-03-04  2:22       ` Jeff King
2023-03-04 10:08         ` Michael Henry

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=ZAMcxlaQ1h2bN1fm@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@drmikehenry.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.