{"thread":{"id":"59309","subject":"`git bundle create -` may not write to `stdout`","startedAt":"2023-02-25T13:05:19Z","lastAt":"2023-03-06T17:43:09Z","messageCount":24,"participants":["Michael Henry","Jeff King","Junio C Hamano","Robin H. Johnson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"472715","messageId":"80beb487-cd93-06ed-88cf-87a96a829ff6@drmikehenry.com","threadId":"59309","inReplyTo":null,"subject":"`git bundle create -` may not write to `stdout`","fromName":"Michael Henry","fromEmail":"git@drmikehenry.com","sentAt":"2023-02-25T12:58:33Z","receivedAt":"2023-02-25T13:05:19Z","isPatch":false,"sender":{"key":"git@drmikehenry.com","avatar":null},"body":"All,\n\nI've encountered some unexpected behavior with `git bundle create -`.\n\nSummary\n=======\n\nUsing `-` to create a bundle file on `stdout` works only when the current\nworking directory is at the root of the repository; when in a\nsubdirectory, `-`\nis treated as the name of a file instead.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n========================================================================\n\nThe below steps are using Git's `next` branch (cloned today) to demonstrate.\n\n- Bundle creation to `stdout` works in the repository root:\n\n      $ ./git bundle create -q - HEAD ^HEAD~ > head.bundle\n      $ ./git bundle list-heads head.bundle\n      a93b1961839a603a8ac2df08fd80c48bd140fe02 HEAD\n\n- From the `Documentation` directory, `-` is treated like a filename:\n\n      $ cd Documentation/\n      $ ../git bundle create -q - HEAD ^HEAD~ > head.bundle\n      $ ../git bundle list-heads head.bundle\n      error: 'Documentation/head.bundle' does not look like a v2 or v3\nbundle file\n      $ ../git bundle list-heads ./-\n      a93b1961839a603a8ac2df08fd80c48bd140fe02 HEAD\n\n- Consider this patch to display the bundle file path:\n\n      diff --git a/bundle.c b/bundle.c\n      index 6ab6cd7378..05be3ed520 100644\n      --- a/bundle.c\n      +++ b/bundle.c\n      @@ -524,6 +524,7 @@ int create_bundle(struct repository *r, const\nchar *path,\n                      goto err;\n              }\n       \n      +    fprintf(stderr, \"create_bundle(): path=\\\"%s\\\"\\n\", path);\n              bundle_to_stdout = !strcmp(path, \"-\");\n              if (bundle_to_stdout)\n                      bundle_fd = 1;\n\n  When in a subdirectory, it seems that the current working directory is\nbeing\n  changed to the root, and the bundle file's path is being adjusted\naccordingly:\n\n      $ ../git bundle create -q - HEAD ^HEAD~ > head.bundle\n      create_bundle(): path=\"Documentation/-\"\n\nWhat did you expect to happen? (Expected behavior)\n==================================================\n\nI expected `-` to be treated as `stdout` in subdirectories as well as in the\nrepository root directory.\n\nWhat happened instead? (Actual behavior)\n========================================\n\nWhen in a subdirectory `some/subdir`, `-` is converted to\n`some/subdir/-` and\ntreated like a regular file instead of `stdout`.\n\nWhat's different between what you expected and what actually happened?\n======================================================================\n\nI expected the bundle file to appear on `stdout`; instead, it was\nwritten to a\nfile named `-` in the current directory.\n\nAnything else you want to add:\n==============================\n\nIt's unclear to me whether creating a bundle file to `stdout` is documented\nbehavior.  I can't find direct mention of it in\n`Documentation/git-bundle.txt`,\nthough that document does have this text:\n\n    --all-progress::\n            When --stdout is specified then progress report is\n            displayed during the object count and compression phases\n            but inhibited during the write-out phase. The reason is\n            that in some cases the output stream is directly linked\n            to another command which may wish to display progress\n            status of its own as it processes incoming pack data.\n            This flag is like --progress except that it forces progress\n            report for the write-out phase as well even if --stdout is\n            used.\n\nThe switch `--stdout` doesn't seem to exist, though; perhaps it was a past\nfeature that got removed but the documentation hung around?\n\nI find the ability to create a bundle to `stdout` a useful feature, though a\nniche use case: I'm post-processing the bundle file's contents before\nwriting to\na file, and bundling to `stdout` saves the creation of a potentially large\ntemporary file.  I'm currently using the temporary file approach, however,\nbecause I'm not sure that bundling to `stdout` is intended as a supported\nfeature; I'll leave that for you to determine.\n\nThanks,\nMichael Henry\n\n==============================================================================\n\n[System Info]\ngit version:\ngit version 2.40.0.rc0.245.ga93b196183\ncpu: x86_64\nbuilt from commit: a93b1961839a603a8ac2df08fd80c48bd140fe02\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.0-52-generic #58-Ubuntu SMP Thu Oct 13 08:03:55 UTC\n2022 x86_64\ncompiler info: gnuc: 11.3\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n"},{"id":"472764","messageId":"Y/voNv1OQ1Cf/N5a@coredump.intra.peff.net","threadId":"59309","inReplyTo":"80beb487-cd93-06ed-88cf-87a96a829ff6@drmikehenry.com","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-26T23:16:06Z","receivedAt":"2023-02-26T23:16:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2023 at 07:58:33AM -0500, Michael Henry wrote:\n\n> Summary\n> =======\n> \n> Using `-` to create a bundle file on `stdout` works only when the\n> current working directory is at the root of the repository; when in a\n> subdirectory, `-` is treated as the name of a file instead.\n\nHmm, yeah. The problem is that we call prefix_filename() to accommodate\nthe fact that we'll have changed directory to the root of the working\ntree, and it has no knowledge that \"-\" is special for bundle creation.\n\nThe most directed fix is this:\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex acceef6200..145b814f48 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -59,7 +59,9 @@ static int parse_options_cmd_bundle(int argc,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \tif (!argc)\n \t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n-\t*bundle_file = prefix_filename(prefix, argv[0]);\n+\t*bundle_file = strcmp(argv[0], \"-\") ?\n+\t\t       prefix_filename(prefix, argv[0]) :\n+\t\t       xstrdup(argv[0]);\n \treturn argc;\n }\n \n\nthough I wonder if there are similar cases for other commands,\nespecially ones where the call to prefix_filename() is lurking behind an\nOPT_FILENAME() option.\n\nIt's tempting to treat this name as special at that level, like:\n\ndiff --git a/abspath.c b/abspath.c\nindex 39e06b5848..e89697c85f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -269,7 +269,7 @@ char *prefix_filename(const char *pfx, const char *arg)\n \n \tif (!pfx_len)\n \t\t; /* nothing to prefix */\n-\telse if (is_absolute_path(arg))\n+\telse if (is_absolute_path(arg) || !strcmp(arg, \"-\"))\n \t\tpfx_len = 0;\n \telse\n \t\tstrbuf_add(&path, pfx, pfx_len);\n\nbut I suspect that may bite us in the long run. Something like:\n\n  git mv -- foo -\n\nshould treat \"-\" literally, and not as some special token (stdout does\nnot even make sense in this context).\n\n> Anything else you want to add:\n> ==============================\n> \n> It's unclear to me whether creating a bundle file to `stdout` is documented\n> behavior.  I can't find direct mention of it in\n> `Documentation/git-bundle.txt`,\n> though that document does have this text:\n> \n>     --all-progress::\n>             When --stdout is specified then progress report is\n>             displayed during the object count and compression phases\n>             but inhibited during the write-out phase. The reason is\n>             that in some cases the output stream is directly linked\n>             to another command which may wish to display progress\n>             status of its own as it processes incoming pack data.\n>             This flag is like --progress except that it forces progress\n>             report for the write-out phase as well even if --stdout is\n>             used.\n> \n> The switch `--stdout` doesn't seem to exist, though; perhaps it was a past\n> feature that got removed but the documentation hung around?\n\nIt comes from 79862b6b77 (bundle-create: progress output control,\n2019-11-10), which I think was trying to copy the explanation from the\nsimilar options in pack-objects (which underlies git-bundle, and we just\nforward those options to it).\n\nEvery reference to \"--stdout\" there should probably be replaced with\n\"when writing a bundle to stdout\" (or alternatively, these should\nperhaps just be pointers to the pack-objects equivalents).\n\nAnd yes, we should document \"-\" (probably when discussing <file> in the\n\"create\" section of OPTIONS), as I don't see any mention of it. Its\nbehavior is intentional (it goes all the way back to 2e0afafebd (Add\ngit-bundle: move objects and references by archive, 2007-02-22)).\n\nI think having \"--stdout\" would have been a nicer choice to avoid this\nambiguity, but at this point it is probably not worth going through\ndeprecation dance to get rid of it.\n\n> I find the ability to create a bundle to `stdout` a useful feature,\n> though a niche use case: I'm post-processing the bundle file's\n> contents before writing to a file, and bundling to `stdout` saves the\n> creation of a potentially large temporary file.  I'm currently using\n> the temporary file approach, however, because I'm not sure that\n> bundling to `stdout` is intended as a supported feature; I'll leave\n> that for you to determine.\n\nI think bundling to stdout is perfectly reasonable. Even without\npost-processing, if you need to use a command to get it to its ultimate\nstorage spot (e.g., by piping into a network command like \"nc\" or\nsomething), it's nice to avoid the temporary file because it could\npotentially be huge.\n\nSo it seems like we'd want a three-patch series:\n\n  1. The first hunk I showed above, along with a test to demonstrate the\n     fix.\n\n  2. Remove bogus references to --stdout in the docs.\n\n  3. Document \"-\".\n\nDo you want to try your hand at that? (It's OK if not, but I like to\ntrick^W give opportunities to new folks to contribute).\n\n-Peff\n"},{"id":"472976","messageId":"xmqqv8jhcvrq.fsf@gitster.g","threadId":"59309","inReplyTo":"Y/voNv1OQ1Cf/N5a@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T22:31:05Z","receivedAt":"2023-03-03T22:35:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The most directed fix is this:\n>\n> diff --git a/builtin/bundle.c b/builtin/bundle.c\n> index acceef6200..145b814f48 100644\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -59,7 +59,9 @@ static int parse_options_cmd_bundle(int argc,\n>  \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n>  \tif (!argc)\n>  \t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n> -\t*bundle_file = prefix_filename(prefix, argv[0]);\n> +\t*bundle_file = strcmp(argv[0], \"-\") ?\n> +\t\t       prefix_filename(prefix, argv[0]) :\n> +\t\t       xstrdup(argv[0]);\n>  \treturn argc;\n>  }\n\nThis fell thru cracks, it seems.\n\nOPT_FILENAME() needs to do exactly this, and has its own helper\nfunction in parse-options.c::fix_filename(), but its memory\nownership rules is somewhat screwed (it was perfectly fine in the\noriginal context of \"we parse the bounded number of options the user\nwould give us from the command line, and giving more than one\ninstance of these last-one-wins option may leak but we do not\ncare---if you do not like leaking, don't give duplicates\", but with\nautomated leak checking that does not care about such nuances, it\nwill trigger unnecessary errors), and cannot be reused here.\n\nHere is your \"most directed fix\" packaged into a call to a helper\nfunction.  Given that we may want to slim the cache.h header, it may\nnot want to be declared there, but for now, its declaration sits\nnext to prefix_filename().\n\n\ndiff --git c/abspath.c w/abspath.c\nindex 39e06b5848..b2fd9ff321 100644\n--- c/abspath.c\n+++ w/abspath.c\n@@ -280,3 +280,10 @@ char *prefix_filename(const char *pfx, const char *arg)\n #endif\n \treturn strbuf_detach(&path, NULL);\n }\n+\n+char *prefix_filename_except_for_dash(const char *pfx, const char *arg)\n+{\n+\tif (!strcmp(arg, \"-\"))\n+\t\treturn xstrdup(arg);\n+\treturn prefix_filename(pfx, arg);\n+}\ndiff --git c/builtin/bundle.c w/builtin/bundle.c\nindex acceef6200..3056dbeee3 100644\n--- c/builtin/bundle.c\n+++ w/builtin/bundle.c\n@@ -59,7 +59,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \tif (!argc)\n \t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n-\t*bundle_file = prefix_filename(prefix, argv[0]);\n+\t*bundle_file = prefix_filename_except_for_dash(prefix, argv[0]);\n \treturn argc;\n }\n \ndiff --git c/cache.h w/cache.h\nindex 12789903e8..38867de41a 100644\n--- c/cache.h\n+++ w/cache.h\n@@ -638,6 +638,9 @@ char *prefix_path_gently(const char *prefix, int len, int *remaining, const char\n  */\n char *prefix_filename(const char *prefix, const char *path);\n \n+/* Likewise, but path==\"-\" always yields \"-\" */\n+char *prefix_filename_except_for_dash(const char *prefix, const char *path);\n+\n int check_filename(const char *prefix, const char *name);\n void verify_filename(const char *prefix,\n \t\t     const char *name,\ndiff --git c/parse-options.c w/parse-options.c\nindex fd4743293f..bd1ed906a8 100644\n--- c/parse-options.c\n+++ w/parse-options.c\n@@ -59,6 +59,10 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \treturn 0;\n }\n \n+/*\n+ * NEEDSWORK: fix memory ownership rules around here and then use\n+ * prefix_filename_except_for_dash() helper.\n+ */\n static void fix_filename(const char *prefix, const char **file)\n {\n \tif (!file || !*file || !prefix || is_absolute_path(*file)\ndiff --git c/t/t6020-bundle-misc.sh w/t/t6020-bundle-misc.sh\nindex 7d40994991..d14f7cea91 100755\n--- c/t/t6020-bundle-misc.sh\n+++ w/t/t6020-bundle-misc.sh\n@@ -606,4 +606,15 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n \t)\n '\n \n+test_expect_success 'send a bundle to standard output' '\n+\tgit bundle create - --all HEAD >bundle-one &&\n+\tmkdir -p down &&\n+\tgit -C down bundle create - --all HEAD >bundle-two &&\n+\tgit bundle verify bundle-one &&\n+\tgit bundle verify bundle-two &&\n+\tgit ls-remote bundle-one >expect &&\n+\tgit ls-remote bundle-two >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"472977","messageId":"ZAJ6oI3clNH2O3R7@coredump.intra.peff.net","threadId":"59309","inReplyTo":"xmqqv8jhcvrq.fsf@gitster.g","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-03T22:54:24Z","receivedAt":"2023-03-03T22:56:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 02:31:05PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The most directed fix is this:\n> >\n> > diff --git a/builtin/bundle.c b/builtin/bundle.c\n> > index acceef6200..145b814f48 100644\n> > --- a/builtin/bundle.c\n> > +++ b/builtin/bundle.c\n> > @@ -59,7 +59,9 @@ static int parse_options_cmd_bundle(int argc,\n> >  \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n> >  \tif (!argc)\n> >  \t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n> > -\t*bundle_file = prefix_filename(prefix, argv[0]);\n> > +\t*bundle_file = strcmp(argv[0], \"-\") ?\n> > +\t\t       prefix_filename(prefix, argv[0]) :\n> > +\t\t       xstrdup(argv[0]);\n> >  \treturn argc;\n> >  }\n> \n> This fell thru cracks, it seems.\n\nI was waiting to give Michael a chance to respond to my offer. :)\n\n> OPT_FILENAME() needs to do exactly this, and has its own helper\n> function in parse-options.c::fix_filename(), but its memory\n> ownership rules is somewhat screwed (it was perfectly fine in the\n> original context of \"we parse the bounded number of options the user\n> would give us from the command line, and giving more than one\n> instance of these last-one-wins option may leak but we do not\n> care---if you do not like leaking, don't give duplicates\", but with\n> automated leak checking that does not care about such nuances, it\n> will trigger unnecessary errors), and cannot be reused here.\n\nHeh, I was worried about it kicking in for spots that \"-\" was not\nmeaningful, but I checked only prefix_filename() itself, and didn't\nthink to check OPT_FILENAME()'s full code path.\n\n(I do still think we don't want to push it down into prefix_filename(),\nbecause it gets used for paths and pathspecs given raw on the command\nline. It does make me wonder if there are places where OPT_FILENAME() is\ndoing the wrong thing).\n\n> Here is your \"most directed fix\" packaged into a call to a helper\n> function.  Given that we may want to slim the cache.h header, it may\n> not want to be declared there, but for now, its declaration sits\n> next to prefix_filename().\n\nYeah, a helper may be nice, though if this is the only spot, I'd be\ntempted not to even bother until fix_filename() is fixed. The obvious\nfix is for it to always allocate, but callers will need to be adjusted.\nI suspect it will trigger a bunch of complaints from the leak-checking\ntests (because the caller does not expect it to be allocated, so it's a\nsometimes-leak now, but it will become an always-leak).\n\n> diff --git c/t/t6020-bundle-misc.sh w/t/t6020-bundle-misc.sh\n> index 7d40994991..d14f7cea91 100755\n> --- c/t/t6020-bundle-misc.sh\n> +++ w/t/t6020-bundle-misc.sh\n> @@ -606,4 +606,15 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'send a bundle to standard output' '\n> +\tgit bundle create - --all HEAD >bundle-one &&\n> +\tmkdir -p down &&\n> +\tgit -C down bundle create - --all HEAD >bundle-two &&\n> +\tgit bundle verify bundle-one &&\n> +\tgit bundle verify bundle-two &&\n> +\tgit ls-remote bundle-one >expect &&\n> +\tgit ls-remote bundle-two >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis test looks good to me. Let's also not forget about the doc fixes. I\ndon't think there's much urgency to get this into v2.40, but I can put\nit together in the next day or three.\n\n-Peff\n"},{"id":"472979","messageId":"xmqqpm9pcu6t.fsf@gitster.g","threadId":"59309","inReplyTo":"ZAJ6oI3clNH2O3R7@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T23:05:14Z","receivedAt":"2023-03-03T23:05:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This test looks good to me. Let's also not forget about the doc fixes. I\n> don't think there's much urgency to get this into v2.40,\n\nDoc?  Meaning \n\n\t<file> can be \"-\" to mean the standard output (for writing)\n\tor the standard input (for reading)\n\nor something?\n\nGiven that the other three subcommands also take <file>\n\n    'git bundle' create [-q | --quiet | --progress | --all-progress] ...\n                        [--version=<version>] <file> <git-rev-list-args>\n    'git bundle' verify [-q | --quiet] <file>\n    'git bundle' list-heads <file> [<refname>...]\n    'git bundle' unbundle [--progress] <file> [<refname>...]\n\nbut read_bundle_header() function all three calls begins like so:\n\n    int read_bundle_header(const char *path, struct bundle_header *header)\n    {\n            int fd = open(path, O_RDONLY);\n\n            if (fd < 0)\n                    return error(_(\"could not open '%s'\"), path);\n            return read_bundle_header_fd(fd, header, path);\n    }\n\nthis function needs to be fixed first ;-)\n\nOf course none of these is urgent for the upcoming 2.40 ;-).\n\n> but I can put\n> it together in the next day or three.\n\nThanks.  Just for reference, here is what I have (just a log\nmessage, the patch is the same and does not support input yet).\n\n----- >8 -----\nSubject: [PATCH] bundle: don't blindly apply prefix_filename() to \"-\"\n\nA user can specify a filename to a command from the command line,\neither as the value given to a command line option, or a command\nline argument.  When it is given as a relative filename, in the\nuser's mind, it is relative to the directory \"git\" was started from,\nbut by the time the filename is used, \"git\" would almost always have\nchdir()'ed up to the root level of the working tree.\n\nThe given filename, if it is relative, needs to be prefixed with the\npath to the current directory, and it typically is done by calling\nprefix_filename() helper function.  For commands that can also take\n\"-\" to use the standard input or the standard output, however, this\nneeds to be done with care.\n\n\"git bundle create\" uses the next word on the command line as the\noutput filename, and can take \"-\" to mean \"write to the standard\noutput\".  It blindly called prefix_filename(), so running it in a\nsubdirectory did not quite work as expected.\n\nIntroduce a new helper, prefix_filename_except_for_dash(), and use\nit to help \"git bundle create\" codepath.\n\nReported-by: Michael Henry\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n abspath.c              |  7 +++++++\n builtin/bundle.c       |  2 +-\n cache.h                |  3 +++\n parse-options.c        |  4 ++++\n t/t6020-bundle-misc.sh | 11 +++++++++++\n 5 files changed, 26 insertions(+), 1 deletion(-)\n"},{"id":"472981","messageId":"72e2af95-478d-4a15-d8cc-ce910cb7d520@drmikehenry.com","threadId":"59309","inReplyTo":"xmqqv8jhcvrq.fsf@gitster.g","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Michael Henry","fromEmail":"git@drmikehenry.com","sentAt":"2023-03-03T23:59:14Z","receivedAt":"2023-03-03T23:59:19Z","isPatch":false,"sender":{"key":"git@drmikehenry.com","avatar":null},"body":"On 3/3/23 17:31, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\nJeff,\n\nI'm not subscribed to the git mailing list (I thought it might\nbe too high volume for me), but I thought I'd be getting any\nreplies regarding my bug report.  For some reason, though,  I'm\nnot getting copied on any of the emails you've sent (from\npeff@peff.net).  I am getting Junio's emails, but today was the\nfirst time I'd seen a response.  I am able to see the\nconversation via the archive at\nhttps://marc.info/?l=git&m=167733032407411&w=2, so I've read\nyour responses there.\n\nIt looks like Junio is also making fast progress on this, and\nI'm happy stand back and watch the magic happen this time.\n\nThanks,\nMichael Henry\n\n"},{"id":"472982","messageId":"xmqqa60tco76.fsf@gitster.g","threadId":"59309","inReplyTo":"ZAJ6oI3clNH2O3R7@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-04T01:14:37Z","receivedAt":"2023-03-04T01:16:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> (I do still think we don't want to push it down into prefix_filename(),\n> because it gets used for paths and pathspecs given raw on the command\n> line. It does make me wonder if there are places where OPT_FILENAME() is\n> doing the wrong thing).\n\nTo be quite honest, I had the opposite reaction ;-)  At least for\nOPT_FILENAME() thing, I think it is well known that you should work\naround with \"git cmd --opt ./-\" if you do mean a file whose name\nhappens to be a single dash.  Teaching prefix_filename() the same\ntrick does not look _too_ bad.\n\nThere is a problem that commands that use prefix_filename() may not\nbe prepared to read from the standard input or write to the standard\noutput.  For some such callers it may be just the matter of\nreplacing an unconditional open() with\n\n\t-\tfd = open(filename, O_RDONLY);\n\t+\tif (!strcmp(filename, \"-\"))\n\t+\t\tfd = 0;\n\t+\telse\n\t+\t\tfd = open(filename, O_RDONLY);\n\nor something, but if some callers have fundamental reasons why they\ndo not want to work with the standard input, it may make sense to\ntreat \"-\" as a normal filename, and for them, blindly prefixing the\nleading directory name would be much better than special casing \"-\".\n\nSo, I dunno.\n\n\n"},{"id":"472984","messageId":"ZAKexHiit5vOmv7M@coredump.intra.peff.net","threadId":"59309","inReplyTo":"xmqqpm9pcu6t.fsf@gitster.g","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T01:28:36Z","receivedAt":"2023-03-04T01:28:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 03:05:14PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This test looks good to me. Let's also not forget about the doc fixes. I\n> > don't think there's much urgency to get this into v2.40,\n> \n> Doc?  Meaning \n> \n> \t<file> can be \"-\" to mean the standard output (for writing)\n> \tor the standard input (for reading)\n> \n> or something?\n\nYeah, I was referring to my earlier mail in the thread, which said:\n\n>> So it seems like we'd want a three-patch series:\n>> \n>>   1. The first hunk I showed above, along with a test to demonstrate the\n>>      fix.\n>> \n>>   2. Remove bogus references to --stdout in the docs.\n>> \n>>   3. Document \"-\".\n\nYour patch is (1), but we'd want (2) and (3) still.\n\n> Given that the other three subcommands also take <file>\n> \n>     'git bundle' create [-q | --quiet | --progress | --all-progress] ...\n>                         [--version=<version>] <file> <git-rev-list-args>\n>     'git bundle' verify [-q | --quiet] <file>\n>     'git bundle' list-heads <file> [<refname>...]\n>     'git bundle' unbundle [--progress] <file> [<refname>...]\n> \n> but read_bundle_header() function all three calls begins like so:\n> \n>     int read_bundle_header(const char *path, struct bundle_header *header)\n>     {\n>             int fd = open(path, O_RDONLY);\n> \n>             if (fd < 0)\n>                     return error(_(\"could not open '%s'\"), path);\n>             return read_bundle_header_fd(fd, header, path);\n>     }\n> \n> this function needs to be fixed first ;-)\n\nI wasn't thinking of changing the behavior for input, but just focusing\nthe docs in the right spot (the \"create\" option), like:\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex 18a022b4b4..ea6b5c24d1 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -65,9 +65,9 @@ OPTIONS\n create [options] <file> <git-rev-list-args>::\n \tUsed to create a bundle named 'file'.  This requires the\n \t'<git-rev-list-args>' arguments to define the bundle contents.\n \t'options' contains the options specific to the 'git bundle create'\n-\tsubcommand.\n+\tsubcommand. If 'file' is `-`, the bundle is written to stdout.\n \n verify <file>::\n \tUsed to check that a bundle file is valid and will apply\n \tcleanly to the current repository.  This includes checks on the\n\n> > but I can put\n> > it together in the next day or three.\n> \n> Thanks.  Just for reference, here is what I have (just a log\n> message, the patch is the same and does not support input yet).\n\nI don't mind supporting \"-\" for input, but I don't think it's strictly\nnecessary and nobody is really asking for it. I'm also not sure it won't\nrun afoul of problems in the lower-level code. I seem to recall that the\nbundle code may want to seek() on read, but a quick grep doesn't seem to\nturn anything up (so I'm not sure if I'm mis-remembering or just didn't\nlook hard enough).\n\n> ----- >8 -----\n> Subject: [PATCH] bundle: don't blindly apply prefix_filename() to \"-\"\n\nThanks.\n\n-Peff\n"},{"id":"472985","messageId":"ZAKiV9nAet4wlEEu@coredump.intra.peff.net","threadId":"59309","inReplyTo":"xmqqa60tco76.fsf@gitster.g","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T01:43:51Z","receivedAt":"2023-03-04T01:43:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 05:14:37PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > (I do still think we don't want to push it down into prefix_filename(),\n> > because it gets used for paths and pathspecs given raw on the command\n> > line. It does make me wonder if there are places where OPT_FILENAME() is\n> > doing the wrong thing).\n> \n> To be quite honest, I had the opposite reaction ;-)  At least for\n> OPT_FILENAME() thing, I think it is well known that you should work\n> around with \"git cmd --opt ./-\" if you do mean a file whose name\n> happens to be a single dash.  Teaching prefix_filename() the same\n> trick does not look _too_ bad.\n\nFor OPT_FILENAME() I agree you are generally thinking about \"-\" as\nspecial anyway. But if it's the argument to an option, you do not\ngenerally need to prefix it.\n\nAfter a little grepping, here's a real world example where we wouldn't\nwant prefix_filename() to treat \"-\" specially:\n\n  git archive --add-virtual-file=-:1234abcd\n\nUsing \"stdin\" there doesn't make sense. It's OK because it's not using\nOPT_FILENAME(), but it is using prefix_filename().\n\nBut I'd worry much more about non-option arguments. There you _do_ need\nto say \"./-\" if you are intermingled with options. But a \"--\" separator\nsplits that. So something like:\n\n  git mv -- - to\n\nshould move the literal file \"-\", and it _should_ be prefixed with the\nsubdirectory if there is one. Having it mean stdin makes no sense there,\nas we are not reading or writing the file, but rather operating on the\npath.\n\nNow this particular case isn't a problem, because it uses\ninternal_prefix_pathspec(), and not prefix_filename(). And maybe most\nthings are in that boat. Grepping around, most spots I see using\nprefix_filename() could reasonably handle \"-\" as stdin/stdout, though\nthere are definitely some outliers.  E.g., \"git format-patch -o\" wants a\ndirectory, and \"git merge-file\" probably doesn't want to handle stdin.\n\nSo I think we probably do want to stick with \"-\" as being not-special by\ndefault for prefix_filename(), and individual sites would opt into it.\n\n> There is a problem that commands that use prefix_filename() may not\n> be prepared to read from the standard input or write to the standard\n> output.  For some such callers it may be just the matter of\n> replacing an unconditional open() with\n> \n> \t-\tfd = open(filename, O_RDONLY);\n> \t+\tif (!strcmp(filename, \"-\"))\n> \t+\t\tfd = 0;\n> \t+\telse\n> \t+\t\tfd = open(filename, O_RDONLY);\n> \n> or something, but if some callers have fundamental reasons why they\n> do not want to work with the standard input, it may make sense to\n> treat \"-\" as a normal filename, and for them, blindly prefixing the\n> leading directory name would be much better than special casing \"-\".\n\nRight. I think we should stick with the status quo and change over\nindividual callers if people want that effect (but I do not really hear\nanybody clamoring for it, so it seems very non-urgent).\n\n-Peff\n"},{"id":"472986","messageId":"ZAKi8MzGWk5PZUJk@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAKexHiit5vOmv7M@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T01:46:24Z","receivedAt":"2023-03-04T01:46:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 08:28:36PM -0500, Jeff King wrote:\n\n> > Given that the other three subcommands also take <file>\n> > \n> >     'git bundle' create [-q | --quiet | --progress | --all-progress] ...\n> >                         [--version=<version>] <file> <git-rev-list-args>\n> >     'git bundle' verify [-q | --quiet] <file>\n> >     'git bundle' list-heads <file> [<refname>...]\n> >     'git bundle' unbundle [--progress] <file> [<refname>...]\n> > \n> > but read_bundle_header() function all three calls begins like so:\n> > \n> >     int read_bundle_header(const char *path, struct bundle_header *header)\n> >     {\n> >             int fd = open(path, O_RDONLY);\n> > \n> >             if (fd < 0)\n> >                     return error(_(\"could not open '%s'\"), path);\n> >             return read_bundle_header_fd(fd, header, path);\n> >     }\n> > \n> > this function needs to be fixed first ;-)\n> \n> I wasn't thinking of changing the behavior for input, but just focusing\n> the docs in the right spot (the \"create\" option), like:\n\nOh, hmph. I didn't realize that both my patch and yours are touching a\nshared options-parser that affects both reading and writing. So the\npatch by itself is fixing \"git bundle create -\" but breaking \"git bundle\nverify -\". We either need to teach the reading side to handle \"-\", or we\nhave to teach parse_options_cmd_bundle() to handle the two cases\ndifferently.\n\n-Peff\n"},{"id":"472988","messageId":"ZAKrd3vWyRxX3ZJ/@coredump.intra.peff.net","threadId":"59309","inReplyTo":"72e2af95-478d-4a15-d8cc-ce910cb7d520@drmikehenry.com","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T02:22:47Z","receivedAt":"2023-03-04T02:22:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 06:59:14PM -0500, Michael Henry wrote:\n\n> I'm not subscribed to the git mailing list (I thought it might\n> be too high volume for me), but I thought I'd be getting any\n> replies regarding my bug report.  For some reason, though,  I'm\n> not getting copied on any of the emails you've sent (from\n> peff@peff.net).  I am getting Junio's emails, but today was the\n> first time I'd seen a response.  I am able to see the\n> conversation via the archive at\n> https://marc.info/?l=git&m=167733032407411&w=2, so I've read\n> your responses there.\n\nAh, thanks for letting me know. They seem to be stuck in my mail\nserver's queue. The problem is that your server supports TLS, but only\nTLS v1.0. Recent versions of openssl (at least packaged for Debian) bump\nthe default to TLS v1.2 (because older versions are considered less\nsecure).\n\nI relaxed the settings on my server, so you should have gotten a flurry\nof deliveries (including this one). But you might run into delivery\nproblems from other people at some point.\n\n-Peff\n"},{"id":"472991","messageId":"27ae765a-f545-0c0c-706f-c4fb52a97958@drmikehenry.com","threadId":"59309","inReplyTo":"ZAKrd3vWyRxX3ZJ/@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Michael Henry","fromEmail":"git@drmikehenry.com","sentAt":"2023-03-04T10:08:03Z","receivedAt":"2023-03-04T10:08:08Z","isPatch":false,"sender":{"key":"git@drmikehenry.com","avatar":null},"body":"On 3/3/23 21:22, Jeff King wrote:\n> The problem is that your server supports TLS, but only TLS\n> v1.0.\n\nThat's good information; I hadn't realized that about my email\nprovider.\n\n> I relaxed the settings on my server, so you should have gotten\n> a flurry of deliveries (including this one). But you might run\n> into delivery problems from other people at some point.\n\nThanks very much for the pointer; I'll definitely look into\ngetting this fixed.  This is an unexpected side benefit of\nposting to the Git mailing list :-)\n\nMichael Henry\n\n"},{"id":"472992","messageId":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAKi8MzGWk5PZUJk@coredump.intra.peff.net","subject":"[PATCH 0/5] handling \"-\" as stdin/stdout in git bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:22:40Z","receivedAt":"2023-03-04T10:22:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2023 at 08:46:24PM -0500, Jeff King wrote:\n\n> > I wasn't thinking of changing the behavior for input, but just focusing\n> > the docs in the right spot (the \"create\" option), like:\n> \n> Oh, hmph. I didn't realize that both my patch and yours are touching a\n> shared options-parser that affects both reading and writing. So the\n> patch by itself is fixing \"git bundle create -\" but breaking \"git bundle\n> verify -\". We either need to teach the reading side to handle \"-\", or we\n> have to teach parse_options_cmd_bundle() to handle the two cases\n> differently.\n\nSo here's a potential series. It does teach \"-\" to the reading side.\nWhich is technically a behavior change, but one that I hope people will\nfind pretty reasonable.\n\nThe patches are:\n\n  [1/5]: bundle: let \"-\" mean stdin for reading operations\n\n    Does what it says. I did it as a preparatory patch before yours, so\n    that there's never a state where we are treating \"-\" as special for\n    prefixing but it doesn't actually work yet. :-/\n\n  [2/5]: bundle: document handling of \"-\" as stdin\n\n    One half of the doc-fixes discussed earlier, but of course covering\n    all operations now.\n\n  [3/5]: bundle: don't blindly apply prefix_filename() to \"-\"\n\n    Your patch, but rebased and with the comment above fix_filename()\n    dropped (since it's handled below). I saw you prepared yours on\n    \"maint\". I did this one on \"master\", because some of the\n    leak-handling in the other patches needs it. Since this has been\n    broken for ages, and since \"master\" is about to become \"maint\" when\n    2.40 release in a few days, it seemed simpler to just wait. But if\n    we really want to, I think we could reorder and split it into two\n    topics.\n\n  [4/5]: parse-options: consistently allocate memory in fix_filename()\n\n    Leak-fixes that also get us ready for using your new helper. :)\n\n  [5/5]: parse-options: use prefix_filename_except_for_dash() helper\n\n    And using the new helper. This could arguably be squashed into 4,\n    but I wasn't sure at the outset what order to do it in (another\n    option is putting 4 earlier, and then just converting it over in\n    your patch 3).\n\n Documentation/git-bundle.txt        |  8 +++++---\n abspath.c                           |  7 +++++++\n builtin/archive.c                   |  3 ++-\n builtin/bundle.c                    | 28 +++++++++++++++++++++++-----\n builtin/checkout.c                  |  3 ++-\n builtin/reset.c                     |  4 +++-\n builtin/tag.c                       |  4 +++-\n cache.h                             |  3 +++\n parse-options.c                     | 12 ++++++------\n t/helper/test-parse-pathspec-file.c |  3 ++-\n t/t6020-bundle-misc.sh              | 26 ++++++++++++++++++++++++++\n 11 files changed, 82 insertions(+), 19 deletions(-)\n\n-Peff\n"},{"id":"472993","messageId":"ZAMcxlaQ1h2bN1fm@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[PATCH 1/5] bundle: let \"-\" mean stdin for reading operations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:26:14Z","receivedAt":"2023-03-04T10:26:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For writing, \"bundle create -\" indicates that the bundle should be\nwritten to stdout. But there's no matching handling of \"-\" for reading\noperations. This is inconsistent, and a little inflexible (though one\ncan always use \"/dev/stdin\" on systems that support it).\n\nHowever, it's easy to change. Once upon a time, the bundle-reading code\nrequired a seekable descriptor, but that was fixed long ago in\ne9ee84cf28b (bundle: allowing to read from an unseekable fd,\n2011-10-13). So we just need to handle \"-\" explicitly when opening the\nfile.\n\nWe _could_ do this by handling \"-\" in read_bundle_header(), which the\nreading functions all call already. But that is probably a bad idea.\nIt's also used by low-level code like the transport functions, and we\nmay want to be more careful there. We do not know that stdin is even\navailable to us, and certainly we would not want to get confused by a\nconfigured URL that happens to point to \"-\".\n\nSo instead, let's add a helper to builtin/bundle.c. Since both the\nbundle code and some of the callers refer to the bundle by name for\nerror messages, let's use the string \"<stdin>\" to make the output a bit\nnicer to read.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt occurs to me while sending this that even though the new tests check\nstdin, they don't confirm that it works with an unseekable descriptor.\nIt does. I don't know if the lack of fseek/lseek calls gives us enough\nconfidence, or if we want to protect this with a test that does:\n\n  cat some.bundle | git bundle unbundle -\n\n builtin/bundle.c       | 26 ++++++++++++++++++++++----\n t/t6020-bundle-misc.sh | 15 +++++++++++++++\n 2 files changed, 37 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex acceef62001..02dab1cfa02 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -107,6 +107,23 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \treturn ret;\n }\n \n+/*\n+ * Similar to read_bundle_header(), but handle \"-\" as stdin.\n+ */\n+static int open_bundle(const char *path, struct bundle_header *header,\n+\t\t       const char **name)\n+{\n+\tif (!strcmp(path, \"-\")) {\n+\t\tif (name)\n+\t\t\t*name = \"<stdin>\";\n+\t\treturn read_bundle_header_fd(0, header, \"<stdin>\");\n+\t}\n+\n+\tif (name)\n+\t\t*name = path;\n+\treturn read_bundle_header(path, header);\n+}\n+\n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n@@ -118,12 +135,13 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\tOPT_END()\n \t};\n \tchar *bundle_file;\n+\tconst char *name;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\tif ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n \t}\n@@ -134,7 +152,7 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\tgoto cleanup;\n \t}\n \n-\tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n+\tfprintf(stderr, _(\"%s is okay\\n\"), name);\n \tret = 0;\n cleanup:\n \tfree(bundle_file);\n@@ -155,7 +173,7 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\tif ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n \t}\n@@ -185,7 +203,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\tif ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n \t}\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 7d40994991e..45d93588c91 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -606,4 +606,19 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n \t)\n '\n \n+test_expect_success 'read bundle over stdin' '\n+\tgit bundle create some.bundle HEAD &&\n+\n+\tgit bundle verify - <some.bundle 2>err &&\n+\tgrep \"<stdin> is okay\" err &&\n+\n+\tgit bundle list-heads some.bundle >expect &&\n+\tgit bundle list-heads - <some.bundle >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit bundle unbundle some.bundle >expect &&\n+\tgit bundle unbundle - <some.bundle >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.rc1.500.g967c04631e\n\n"},{"id":"472994","messageId":"ZAMc4DPOSjqxY/UT@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[PATCH 2/5] bundle: document handling of \"-\" as stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:26:40Z","receivedAt":"2023-03-04T10:26:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have always allowed \"bundle create -\" to write to stdout, but it was\nnever documented. And a recent patch let reading operations like \"bundle\nlist-heads -\" read from stdin.\n\nLet's document all of these cases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nArguably could be squashed into the previous patch, but the bit for\n\"create\" is weird then.\n\n Documentation/git-bundle.txt | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex 18a022b4b40..d19f4cf2b3c 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -66,7 +66,7 @@ create [options] <file> <git-rev-list-args>::\n \tUsed to create a bundle named 'file'.  This requires the\n \t'<git-rev-list-args>' arguments to define the bundle contents.\n \t'options' contains the options specific to the 'git bundle create'\n-\tsubcommand.\n+\tsubcommand. If 'file' is `-`, the bundle is written to stdout.\n \n verify <file>::\n \tUsed to check that a bundle file is valid and will apply\n@@ -77,19 +77,21 @@ verify <file>::\n \tFinally, information about additional capabilities, such as \"object\n \tfilter\", is printed. See \"Capabilities\" in linkgit:gitformat-bundle[5]\n \tfor more information. The exit code is zero for success, but will\n-\tbe nonzero if the bundle file is invalid.\n+\tbe nonzero if the bundle file is invalid. If 'file' is `-`, the\n+\tbundle is read from stdin.\n \n list-heads <file>::\n \tLists the references defined in the bundle.  If followed by a\n \tlist of references, only references matching those given are\n-\tprinted out.\n+\tprinted out. If 'file' is `-`, the bundle is read from stdin.\n \n unbundle <file>::\n \tPasses the objects in the bundle to 'git index-pack'\n \tfor storage in the repository, then prints the names of all\n \tdefined references. If a list of references is given, only\n \treferences matching those in the list are printed. This command is\n \treally plumbing, intended to be called only by 'git fetch'.\n+\tIf 'file' is `-`, the bundle is read from stdin.\n \n <git-rev-list-args>::\n \tA list of arguments, acceptable to 'git rev-parse' and\n-- \n2.40.0.rc1.500.g967c04631e\n\n"},{"id":"472995","messageId":"ZAMdLKmQaWR3neVl@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[PATCH 3/5] bundle: don't blindly apply prefix_filename() to \"-\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:27:56Z","receivedAt":"2023-03-04T10:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nA user can specify a filename to a command from the command line,\neither as the value given to a command line option, or a command\nline argument.  When it is given as a relative filename, in the\nuser's mind, it is relative to the directory \"git\" was started from,\nbut by the time the filename is used, \"git\" would almost always have\nchdir()'ed up to the root level of the working tree.\n\nThe given filename, if it is relative, needs to be prefixed with the\npath to the current directory, and it typically is done by calling\nprefix_filename() helper function.  For commands that can also take\n\"-\" to use the standard input or the standard output, however, this\nneeds to be done with care.\n\n\"git bundle create\" uses the next word on the command line as the\noutput filename, and can take \"-\" to mean \"write to the standard\noutput\".  It blindly called prefix_filename(), so running it in a\nsubdirectory did not quite work as expected.\n\nIntroduce a new helper, prefix_filename_except_for_dash(), and use\nit to help \"git bundle create\" codepath.\n\nReported-by: Michael Henry\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't modify the commit message. I guess we could mention that it\nworks for files that read, too (but that we didn't bother to test every\ncommand).\n\n abspath.c              |  7 +++++++\n builtin/bundle.c       |  2 +-\n cache.h                |  3 +++\n t/t6020-bundle-misc.sh | 11 +++++++++++\n 4 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 39e06b58486..9a81c5525be 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -280,3 +280,10 @@ char *prefix_filename(const char *pfx, const char *arg)\n #endif\n \treturn strbuf_detach(&path, NULL);\n }\n+\n+char *prefix_filename_except_for_dash(const char *pfx, const char *arg)\n+{\n+\tif (!strcmp(arg, \"-\"))\n+\t\treturn xstrdup(arg);\n+\treturn prefix_filename(pfx, arg);\n+}\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 02dab1cfa02..eca39b64bf9 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -59,7 +59,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \tif (!argc)\n \t\tusage_msg_opt(_(\"need a <file> argument\"), usagestr, options);\n-\t*bundle_file = prefix_filename(prefix, argv[0]);\n+\t*bundle_file = prefix_filename_except_for_dash(prefix, argv[0]);\n \treturn argc;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 12789903e88..38867de41af 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -638,6 +638,9 @@ char *prefix_path_gently(const char *prefix, int len, int *remaining, const char\n  */\n char *prefix_filename(const char *prefix, const char *path);\n \n+/* Likewise, but path==\"-\" always yields \"-\" */\n+char *prefix_filename_except_for_dash(const char *prefix, const char *path);\n+\n int check_filename(const char *prefix, const char *name);\n void verify_filename(const char *prefix,\n \t\t     const char *name,\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 45d93588c91..6a5877a09ef 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -621,4 +621,15 @@ test_expect_success 'read bundle over stdin' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'send a bundle to standard output' '\n+\tgit bundle create - --all HEAD >bundle-one &&\n+\tmkdir -p down &&\n+\tgit -C down bundle create - --all HEAD >bundle-two &&\n+\tgit bundle verify bundle-one &&\n+\tgit bundle verify bundle-two &&\n+\tgit ls-remote bundle-one >expect &&\n+\tgit ls-remote bundle-two >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.rc1.500.g967c04631e\n\n"},{"id":"472996","messageId":"ZAMd+qJ60McWx1Hp@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[PATCH 4/5] parse-options: consistently allocate memory in fix_filename()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:31:22Z","receivedAt":"2023-03-04T10:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When handling OPT_FILENAME(), we have to stick the \"prefix\" (if any) in\nfront of the filename to make up for the fact that Git has chdir()'d to\nthe top of the repository. We can do this with prefix_filename(), but\nthere are a few special cases we handle ourselves.\n\nUnfortunately the memory allocation is inconsistent here; if we do make\nit to prefix_filename(), we'll allocate a string which the caller must\nfree to avoid a leak. But if we hit our special cases, we'll return the\nstring as-is, and a caller which tries to free it will crash. So there's\nno way to win.\n\nLet's consistently allocate, so that callers can do the right thing.\n\nThere are now three cases to care about in the function (and hence a\nthree-armed if/else):\n\n  1. we got a NULL input (and should leave it as NULL, though arguably\n     this is the sign of a bug; let's keep the status quo for now and we\n     can pick at that scab later)\n\n  2. we hit a special case that means we leave the name intact; we\n     should duplicate the string. This includes our special \"-\"\n     matching. Prior to this patch, it also included empty prefixes and\n     absolute filenames. But we can observe that prefix_filename()\n     already handles these, so we don't need to detect them.\n\n  3. everything else goes to prefix_filename()\n\nI've dropped the \"const\" from the \"char **file\" parameter to indicate\nthat we're allocating, though in practice it's not really important.\nThis is all being shuffled through a void pointer via opt->value before\nit hits code which ever looks at the string. And it's even a bit weird,\nbecause we are really taking _in_ a const string and using the same\nout-parameter for a non-const string. A better function signature would\nbe:\n\n  static char *fix_filename(const char *prefix, const char *file);\n\nbut that would mean the caller dereferences the double-pointer (and the\nNULL check is currently handled inside this function). So I took the\npath of least-change here.\n\nNote that we have to fix several callers in this commit, too, or we'll\nbreak the leak-checking tests. These are \"new\" leaks in the sense that\nthey are now triggered by the test suite, but these spots have always\nbeen leaky when Git is run in a subdirectory of the repository. I fixed\nall of the cases that trigger with GIT_TEST_PASSING_SANITIZE_LEAK. There\nmay be others in scripts that have other leaks, but we can fix them\nlater along with those other leaks (and again, you _couldn't_ fix them\nbefore this patch, so this is the necessary first step).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not sure if UNLEAK() has fallen out of favor, but I did use it in\none spot here. It's the exact case it was designed for: we don't care\nabout free-ing because we're returning from a builtin's main function,\nand doing a free() requires an awkward \"save the return value, then\nfree, then return\" dance.\n\nBut I'm happy to change it if people are too offended by it. :)\n\n builtin/archive.c                   |  3 ++-\n builtin/checkout.c                  |  3 ++-\n builtin/reset.c                     |  4 +++-\n builtin/tag.c                       |  4 +++-\n parse-options.c                     | 14 ++++++++------\n t/helper/test-parse-pathspec-file.c |  3 ++-\n 6 files changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex f094390ee01..d0a583ea958 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -81,7 +81,7 @@ static int run_remote_archiver(int argc, const char **argv,\n int cmd_archive(int argc, const char **argv, const char *prefix)\n {\n \tconst char *exec = \"git-upload-archive\";\n-\tconst char *output = NULL;\n+\tchar *output = NULL;\n \tconst char *remote = NULL;\n \tstruct option local_opts[] = {\n \t\tOPT_FILENAME('o', \"output\", &output,\n@@ -106,5 +106,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n+\tUNLEAK(output);\n \treturn write_archive(argc, argv, prefix, the_repository, output, 0);\n }\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a5155cf55c1..85f591c85f3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -75,7 +75,7 @@ struct checkout_opts {\n \tconst char *ignore_unmerged_opt;\n \tint ignore_unmerged;\n \tint pathspec_file_nul;\n-\tconst char *pathspec_from_file;\n+\tchar *pathspec_from_file;\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -1876,6 +1876,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t\t    options, checkout_usage, &new_branch_info);\n \tbranch_info_release(&new_branch_info);\n \tclear_pathspec(&opts.pathspec);\n+\tfree(opts.pathspec_from_file);\n \tFREE_AND_NULL(options);\n \treturn ret;\n }\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 0697fa89de2..45a3143567c 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -317,7 +317,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tint reset_type = NONE, update_ref_status = 0, quiet = 0;\n \tint no_refresh = 0;\n \tint patch_mode = 0, pathspec_file_nul = 0, unborn;\n-\tconst char *rev, *pathspec_from_file = NULL;\n+\tconst char *rev;\n+\tchar *pathspec_from_file = NULL;\n \tstruct object_id oid;\n \tstruct pathspec pathspec;\n \tint intent_to_add = 0;\n@@ -495,5 +496,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n cleanup:\n \tclear_pathspec(&pathspec);\n+\tfree(pathspec_from_file);\n \treturn update_ref_status;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d428c45dc8d..ccefcbd716f 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -433,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tint create_reflog = 0;\n \tint annotate = 0, force = 0;\n \tint cmdmode = 0, create_tag_object = 0;\n-\tconst char *msgfile = NULL, *keyid = NULL;\n+\tchar *msgfile = NULL;\n+\tconst char *keyid = NULL;\n \tstruct msg_arg msg = { .buf = STRBUF_INIT };\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -643,5 +644,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&msg.buf);\n \tstrbuf_release(&err);\n+\tfree(msgfile);\n \treturn ret;\n }\ndiff --git a/parse-options.c b/parse-options.c\nindex fd4743293fc..25bae8b585b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -59,12 +59,14 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \treturn 0;\n }\n \n-static void fix_filename(const char *prefix, const char **file)\n+static void fix_filename(const char *prefix, char **file)\n {\n-\tif (!file || !*file || !prefix || is_absolute_path(*file)\n-\t    || !strcmp(\"-\", *file))\n-\t\treturn;\n-\t*file = prefix_filename(prefix, *file);\n+\tif (!file || !*file)\n+\t\t; /* leave as NULL */\n+\telse if (!strcmp(\"-\", *file))\n+\t\t*file = xstrdup(*file);\n+\telse\n+\t\t*file = prefix_filename(prefix, *file);\n }\n \n static enum parse_opt_result opt_command_mode_error(\n@@ -177,7 +179,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t\terr = get_arg(p, opt, flags, (const char **)opt->value);\n \n \t\tif (!err)\n-\t\t\tfix_filename(p->prefix, (const char **)opt->value);\n+\t\t\tfix_filename(p->prefix, (char **)opt->value);\n \t\treturn err;\n \n \tcase OPTION_CALLBACK:\ndiff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c\nindex b3e08cef4b3..71d2131fbad 100644\n--- a/t/helper/test-parse-pathspec-file.c\n+++ b/t/helper/test-parse-pathspec-file.c\n@@ -6,7 +6,7 @@\n int cmd__parse_pathspec_file(int argc, const char **argv)\n {\n \tstruct pathspec pathspec;\n-\tconst char *pathspec_from_file = NULL;\n+\tchar *pathspec_from_file = NULL;\n \tint pathspec_file_nul = 0, i;\n \n \tstatic const char *const usage[] = {\n@@ -29,5 +29,6 @@ int cmd__parse_pathspec_file(int argc, const char **argv)\n \t\tprintf(\"%s\\n\", pathspec.items[i].original);\n \n \tclear_pathspec(&pathspec);\n+\tfree(pathspec_from_file);\n \treturn 0;\n }\n-- \n2.40.0.rc1.500.g967c04631e\n\n"},{"id":"472997","messageId":"ZAMeExFKTULOFZF1@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[PATCH 5/5] parse-options: use prefix_filename_except_for_dash() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:31:47Z","receivedAt":"2023-03-04T10:31:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since our fix_filename()'s only remaining special case is handling \"-\",\nwe can use the newly-minted helper function that handles this already.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we do eventually find that \"!file || !*file\" is a sign of a bug, we\ncould perhaps just simplify this whole function away. :)\n\n parse-options.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 25bae8b585b..6dd4c090e03 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -63,10 +63,8 @@ static void fix_filename(const char *prefix, char **file)\n {\n \tif (!file || !*file)\n \t\t; /* leave as NULL */\n-\telse if (!strcmp(\"-\", *file))\n-\t\t*file = xstrdup(*file);\n \telse\n-\t\t*file = prefix_filename(prefix, *file);\n+\t\t*file = prefix_filename_except_for_dash(prefix, *file);\n }\n \n static enum parse_opt_result opt_command_mode_error(\n-- \n2.40.0.rc1.500.g967c04631e\n"},{"id":"472999","messageId":"ZAMjkffYmp+DNmr+@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAMb8LSpm2gOrpeY@coredump.intra.peff.net","subject":"[RFC/PATCH] bundle: turn on --all-progress-implied by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-04T10:55:13Z","receivedAt":"2023-03-04T10:55:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2023 at 05:22:40AM -0500, Jeff King wrote:\n\n>   [2/5]: bundle: document handling of \"-\" as stdin\n> \n>     One half of the doc-fixes discussed earlier, but of course covering\n>     all operations now.\n\nAnd here's the other one. I split it out into its own series because\nit's quite a bit more complicated, and there's no textual dependency.\n\nI'm marking it as \"RFC\" because it's undoing some of what Robin (cc'd)\ndid in the commit referenced below. But I remain unconvinced that it's a\nuseful direction.\n\n-- >8 --\nSubject: bundle: turn on --all-progress-implied by default\n\nIn 79862b6b77c (bundle-create: progress output control, 2019-11-10),\n\"bundle create\" learned about the --all-progress and\n--all-progress-implied options, which were copied from pack-objects.\nI think these were a mistake.\n\nIn pack-objects, \"all-progress-implied\" is about switching the behavior\nbetween a regular on-disk \"git repack\" and the use of pack-objects for\npush/fetch (where a fetch does not want progress from the server during\nthe write stage; the client will print progress as it receives the\ndata). But there's no such distinction for bundles. Prior to\n79862b6b77c, we always printed the write stage. Afterwards, a vanilla:\n\n  git bundle create foo.bundle\n\nomits the write progress, appearing to hang (especially if your\nrepository is large or your disk is slow). That seems like a regression.\n\nIt's possible that the flexibility to disable the write-phase progress\n_could_ be useful for bundle. E.g., if you did something like:\n\n  ssh some-host git bundle create foo.bundle |\n  git bundle unbundle\n\nBut if you are running both in real-time, why are you using bundles in\nthe first place? You're better off doing a real fetch.\n\nBut even if we did want to support that, it should be the exception, and\nvanilla \"bundle create\" should display the full progress. So we'd want\nto name the option \"--no-write-progress\" or something.\n\nThe \"--all-progress\" option itself is even worse. It exists in\npack-objects only for historical reasons. It's a mistake because it\nimplies \"--progress\", and we added \"--all-progress-implied\" to fix that.\nThere is no reason to propagate that mistake to new commands.\n\nLikewise, the documentation for these options was pulled from\npack-objects. But it doesn't make any sense in this context. It talks\nabout \"--stdout\", but that is not even an option that git-bundle\nsupports.\n\nThis patch flips the default for \"--all-progress-implied\" back to\n\"true\", fixing the regression in 79862b6b77c. This turns that option\ninto a noop, and means that \"--all-progress\" is really the same as\n\"--progress\". We _could_ drop them completely, but since they've been\nshipped with Git since v2.25.0, it's polite to continue accepting them.\n\nI didn't implement any sort of \"--no-write-progress\" here. I'm not at\nall convinced it's necessary, and the discussion from the original\nthread:\n\n  https://lore.kernel.org/git/20191110204126.30553-2-robbat2@gentoo.org/\n\nshows that that the main focus was on getting --progress and --quiet\nsupport, and not any kind of clever \"real-time bundle over the network\"\nfeature. But technically this patch is making it impossible to do\nsomething that you _could_ do post-79862b6b77c.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm actually kind of confused about what happened with 79862b6b77c. If\nyou go to the subthread linked above, you can see that I made similar\narguments there, and Junio seemed to agree. Then the next thing I could\nfind is the series appearing in What's cooking:\n\n  https://lore.kernel.org/git/xmqqftikxs4z.fsf@gitster-ct.c.googlers.com/\n\nmarked as \"will merge to master\". Is there more discussion I couldn't\nfind? Was it accidentally graduated?\n\n Documentation/git-bundle.txt | 18 +-----------------\n builtin/bundle.c             | 15 ++++++++-------\n t/t6020-bundle-misc.sh       |  6 ++++++\n 3 files changed, 15 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex 18a022b4b40..a4227b6561d 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -9,7 +9,7 @@ git-bundle - Move objects and refs by archive\n SYNOPSIS\n --------\n [verse]\n-'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\n+'git bundle' create [-q | --quiet | --progress]\n \t\t    [--version=<version>] <file> <git-rev-list-args>\n 'git bundle' verify [-q | --quiet] <file>\n 'git bundle' list-heads <file> [<refname>...]\n@@ -115,22 +115,6 @@ unbundle <file>::\n \tis specified. This flag forces progress status even if\n \tthe standard error stream is not directed to a terminal.\n \n---all-progress::\n-\tWhen --stdout is specified then progress report is\n-\tdisplayed during the object count and compression phases\n-\tbut inhibited during the write-out phase. The reason is\n-\tthat in some cases the output stream is directly linked\n-\tto another command which may wish to display progress\n-\tstatus of its own as it processes incoming pack data.\n-\tThis flag is like --progress except that it forces progress\n-\treport for the write-out phase as well even if --stdout is\n-\tused.\n-\n---all-progress-implied::\n-\tThis is used to imply --all-progress whenever progress display\n-\tis activated.  Unlike --all-progress this flag doesn't actually\n-\tforce any progress display by itself.\n-\n --version=<version>::\n \tSpecify the bundle version.  Version 2 is the older format and can only be\n \tused with SHA-1 repositories; the newer version 3 contains capabilities that\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex acceef62001..c2b916e027a 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -12,7 +12,7 @@\n  */\n \n #define BUILTIN_BUNDLE_CREATE_USAGE \\\n-\tN_(\"git bundle create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\\n\" \\\n+\tN_(\"git bundle create [-q | --quiet | --progress]\\n\" \\\n \t   \"                  [--version=<version>] <file> <git-rev-list-args>\")\n #define BUILTIN_BUNDLE_VERIFY_USAGE \\\n \tN_(\"git bundle verify [-q | --quiet] <file>\")\n@@ -64,7 +64,7 @@ static int parse_options_cmd_bundle(int argc,\n }\n \n static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n-\tint all_progress_implied = 0;\n+\tint all_progress_implied = 1;\n \tint progress = isatty(STDERR_FILENO);\n \tstruct strvec pack_opts;\n \tint version = -1;\n@@ -74,11 +74,12 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \t\t\t    N_(\"do not show progress meter\"), 0),\n \t\tOPT_SET_INT(0, \"progress\", &progress,\n \t\t\t    N_(\"show progress meter\"), 1),\n-\t\tOPT_SET_INT(0, \"all-progress\", &progress,\n-\t\t\t    N_(\"show progress meter during object writing phase\"), 2),\n-\t\tOPT_BOOL(0, \"all-progress-implied\",\n-\t\t\t &all_progress_implied,\n-\t\t\t N_(\"similar to --all-progress when progress meter is shown\")),\n+\t\tOPT_SET_INT_F(0, \"all-progress\", &progress,\n+\t\t\t      N_(\"historical; same as --progress\"), 2,\n+\t\t\t      PARSE_OPT_HIDDEN),\n+\t\tOPT_HIDDEN_BOOL(0, \"all-progress-implied\",\n+\t\t\t\t&all_progress_implied,\n+\t\t\t\tN_(\"historical; does nothing\")),\n \t\tOPT_INTEGER(0, \"version\", &version,\n \t\t\t    N_(\"specify bundle format version\")),\n \t\tOPT_END()\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 7d40994991e..978c5b17ba5 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n \t)\n '\n \n+test_expect_success 'bundle progress includes write phase' '\n+\tGIT_PROGRESS_DELAY=0 \\\n+\t\tgit bundle create --progress out.bundle --all 2>err &&\n+\tgrep 'Writing' err\n+'\n+\n test_done\n-- \n2.40.0.rc1.500.g967c04631e\n\n"},{"id":"473039","messageId":"robbat2-20230306T033734-620860556Z@orbis-terrarum.net","threadId":"59309","inReplyTo":"ZAMjkffYmp+DNmr+@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default","fromName":"Robin H. Johnson","fromEmail":"robbat2@gentoo.org","sentAt":"2023-03-06T03:44:11Z","receivedAt":"2023-03-06T03:44:18Z","isPatch":true,"sender":{"key":"robbat2@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/373898?v=4"},"body":"On Sat, Mar 04, 2023 at 05:55:13AM -0500, Jeff King wrote:\n...\n> I'm marking it as \"RFC\" because it's undoing some of what Robin (cc'd)\n> did in the commit referenced below. But I remain unconvinced that it's a\n> useful direction.\n> -- >8 --\n> Subject: bundle: turn on --all-progress-implied by default\n(snip)\n\nSigned-off-by: Robin H. Johnson <robbat2@gentoo.org>\n\n+1 to at least roll it into a RC.\n\nIIRC this was a mechanical mapping to expose the pack options into the\nbundle command; and a cleanup is worthwhile.\n\n(snip)\n> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\n> index 7d40994991e..978c5b17ba5 100755\n> --- a/t/t6020-bundle-misc.sh\n> +++ b/t/t6020-bundle-misc.sh\n> @@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'bundle progress includes write phase' '\n> +\tGIT_PROGRESS_DELAY=0 \\\n> +\t\tgit bundle create --progress out.bundle --all 2>err &&\n> +\tgrep 'Writing' err\n> +'\n> +\n>  test_done\n\nSuggestion: How about adding a test for --quiet that ensures no other\noutput?\n\n-- \nRobin Hugh Johnson\nGentoo Linux: Dev, Infra Lead, Foundation Treasurer\nE-Mail   : robbat2@gentoo.org\nGnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85\nGnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136\n"},{"id":"473040","messageId":"ZAV8XW34CjUy5RyF@coredump.intra.peff.net","threadId":"59309","inReplyTo":"robbat2-20230306T033734-620860556Z@orbis-terrarum.net","subject":"Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-06T05:38:37Z","receivedAt":"2023-03-06T05:38:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 06, 2023 at 03:44:11AM +0000, Robin H. Johnson wrote:\n\n> > diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\n> > index 7d40994991e..978c5b17ba5 100755\n> > --- a/t/t6020-bundle-misc.sh\n> > +++ b/t/t6020-bundle-misc.sh\n> > @@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '\n> >  \t)\n> >  '\n> >  \n> > +test_expect_success 'bundle progress includes write phase' '\n> > +\tGIT_PROGRESS_DELAY=0 \\\n> > +\t\tgit bundle create --progress out.bundle --all 2>err &&\n> > +\tgrep 'Writing' err\n> > +'\n> > +\n> >  test_done\n> \n> Suggestion: How about adding a test for --quiet that ensures no other\n> output?\n\nI had sort of assumed that we had those already, from the earlier work,\nbut it looks like we don't. Squashing this in would do it, I think (we\nneed test_terminal since otherwise we'd be quiet anyway):\n\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 978c5b17ba5..7bbb351b7be 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -10,6 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bundle.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n for cmd in create verify list-heads unbundle\n do\n@@ -612,4 +613,10 @@ test_expect_success 'bundle progress includes write phase' '\n \tgrep 'Writing' err\n '\n \n+test_expect_success TTY '--quiet disables all bundle progress' '\n+\ttest_terminal env GIT_PROGRESS_DELAY=0 \\\n+\t\tgit bundle --quiet create out.bundle --all 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_done\n"},{"id":"473044","messageId":"ZAWxoAd7vOcDqUCL@coredump.intra.peff.net","threadId":"59309","inReplyTo":"ZAV8XW34CjUy5RyF@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-06T09:25:52Z","receivedAt":"2023-03-06T09:25:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 06, 2023 at 12:38:38AM -0500, Jeff King wrote:\n\n> @@ -612,4 +613,10 @@ test_expect_success 'bundle progress includes write phase' '\n>  \tgrep 'Writing' err\n>  '\n>  \n> +test_expect_success TTY '--quiet disables all bundle progress' '\n> +\ttest_terminal env GIT_PROGRESS_DELAY=0 \\\n> +\t\tgit bundle --quiet create out.bundle --all 2>err &&\n> +\ttest_must_be_empty err\n> +'\n\nErr, this should be \"create --quiet\", of course, not the other way\naround. I did run it before sending, but I removed the \"--quiet\" to\ndouble-check that it generates progress with test_terminal. But then\nadded it back in to the wrong spot. :-/\n\n-Peff\n"},{"id":"473065","messageId":"xmqq4jqxbx78.fsf@gitster.g","threadId":"59309","inReplyTo":"ZAKi8MzGWk5PZUJk@coredump.intra.peff.net","subject":"Re: `git bundle create -` may not write to `stdout`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T17:34:35Z","receivedAt":"2023-03-06T17:36:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Oh, hmph. I didn't realize that both my patch and yours are touching a\n> shared options-parser that affects both reading and writing. So the\n> patch by itself is fixing \"git bundle create -\" but breaking \"git bundle\n> verify -\". We either need to teach the reading side to handle \"-\", or we\n> have to teach parse_options_cmd_bundle() to handle the two cases\n> differently.\n\nYes.  Teaching that \"-\" is to read from the standard input to\nconsumers would be a natural thing to do, with the well-understood\nescape hatch to use \"./-\" if the user really means a file.\n\nThanks.\n"},{"id":"473068","messageId":"xmqqzg8paib2.fsf@gitster.g","threadId":"59309","inReplyTo":"ZAMjkffYmp+DNmr+@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T17:41:37Z","receivedAt":"2023-03-06T17:43:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm actually kind of confused about what happened with 79862b6b77c. If\n> you go to the subthread linked above, you can see that I made similar\n> arguments there, and Junio seemed to agree. Then the next thing I could\n> find is the series appearing in What's cooking:\n>\n>   https://lore.kernel.org/git/xmqqftikxs4z.fsf@gitster-ct.c.googlers.com/\n>\n> marked as \"will merge to master\". Is there more discussion I couldn't\n> find? Was it accidentally graduated?\n\nI am stumped as well X-<.  It does look like it was merged down by\naccident.  Sorry for the confusion.\n"}]}