{"thread":{"id":"56167","subject":"[PATCH 0/4] bundle: show progress on \"unbundle\"","startedAt":"2021-07-27T00:41:56Z","lastAt":"2021-09-05T07:34:57Z","messageCount":42,"participants":["Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"431270","messageId":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","threadId":"56167","inReplyTo":null,"subject":"[PATCH 0/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:41:38Z","receivedAt":"2021-07-27T00:41:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This straightforward series addr progress output on \"git bundle\nunbundle\", we already had progress output if bundles were fetched from\nvia the transport.c (i.e. \"git clone/fetch\" etc.), but not from \"git\nbundle unbundle\" directly.\n\nÆvar Arnfjörð Bjarmason (4):\n bundle API: start writing API documentation\n bundle API: change \"flags\" to be \"extra_index_pack_args\"\n index-pack: add --progress-title option\n bundle: show progress on \"unbundle\"\n\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/bundle.c | 16 ++++++++++++++--\n builtin/index-pack.c | 6 ++++++\n bundle.c | 17 +++++++++++------\n bundle.h | 15 +++++++++++++--\n transport.c | 5 ++++-\n 6 files changed, 54 insertions(+), 11 deletions(-)\n\n-- \n2.32.0.988.g189fd9ae38\n\n"},{"id":"431271","messageId":"patch-2.4-088e4306d8-20210727T004015Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:41:40Z","receivedAt":"2021-07-27T00:42:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"flags\" parameter was added in be042aff24c (Teach progress\neye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been\nmore than the one flag: BUNDLE_VERBOSE.\n\nLet's have the only caller who cares about that pass \"-v\" itself\ninstead through new \"extra_index_pack_args\" parameter. The flexibility\nof being able to pass arbitrary arguments to \"unbundle\" will be used\nin a subsequent commit.\n\nWe could pass NULL explicitly in cmd_bundle_unbundle(), but let's\ninstead initialize an empty strvec and pass it, in anticipation of a\nsubsequent commit wanting to add arguments to it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 5 +++--\n bundle.c | 17 +++++++++++------\n bundle.h | 8 ++++++--\n transport.c | 5 ++++-\n 4 files changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 053a51bea1..10f6f45770 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tchar *bundle_file;\n+\tchar* bundle_file;\n+\tstruct strvec extra_args = STRVEC_INIT;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n@@ -177,7 +178,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\ndiff --git a/bundle.c b/bundle.c\nindex 15ce62285c..f2b28a2092 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -733,18 +733,23 @@ int create_bundle(struct repository *r, const char *path,\n }\n \n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags)\n+\t int bundle_fd, struct strvec *extra_index_pack_args)\n {\n-\tconst char *argv_index_pack[] = {\"index-pack\",\n-\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint i;\n \n-\tif (flags & BUNDLE_VERBOSE)\n-\t\targv_index_pack[3] = \"-v\";\n+\tstrvec_push(&ip.args, \"index-pack\");\n+\tstrvec_push(&ip.args, \"--fix-thin\");\n+\tstrvec_push(&ip.args, \"--stdin\");\n+\tif (extra_index_pack_args) {\n+\t\tstruct strvec *extra = extra_index_pack_args;\n+\t\tfor (i = 0; i < extra->nr; i++)\n+\t\t\tstrvec_push(&ip.args, extra->v[i]);\n+\t\tstrvec_clear(extra_index_pack_args);\n+\t}\n \n \tif (verify_bundle(r, header, 0))\n \t\treturn -1;\n-\tip.argv = argv_index_pack;\n \tip.in = bundle_fd;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\ndiff --git a/bundle.h b/bundle.h\nindex 84a6df1b65..d47a7a3c69 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int argc, const char **argv, struct strvec *pack_options,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n-#define BUNDLE_VERBOSE 1\n \n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n * provided `bundle_fd` from read_bundle_header().\n+ *\n+ * Provide extra_index_pack_args to pass any extra arguments\n+ * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n+ * extra_index_pack_args (if any) will be strvec_clear()'d for you\n+ * (like the run-command.h API itself does).\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags);\n+\t int bundle_fd, struct strvec *extra_index_pack_args);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 7f3d0d46b3..8781186cf1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -163,12 +163,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct bundle_transport_data *data = transport->data;\n+\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n \n+\tstrvec_push(&extra_index_pack_args, \"-v\");\n+\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n+\t\t &extra_index_pack_args);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n-- \n2.32.0.988.g189fd9ae38\n\n"},{"id":"431272","messageId":"patch-1.4-8c88cd377a-20210727T004015Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH 1/4] bundle API: start writing API documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:41:39Z","receivedAt":"2021-07-27T00:42:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are no other API docs in bundle.h, but this is at least a\nstart. We'll add a parameter to this function in a subsequent commit,\nbut let's start by documenting it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.h | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/bundle.h b/bundle.h\nindex 1927d8cd6a..84a6df1b65 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n #define BUNDLE_VERBOSE 1\n+\n+/**\n+ * Unbundle after reading the header with read_bundle_header().\n+ *\n+ * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n+ * provided `bundle_fd` from read_bundle_header().\n+ */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n-- \n2.32.0.988.g189fd9ae38\n\n"},{"id":"431273","messageId":"patch-3.4-f32c722f32-20210727T004015Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH 3/4] index-pack: add --progress-title option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:41:41Z","receivedAt":"2021-07-27T00:42:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --progress-title option to index-pack, when data is piped into\nindex-pack its progress is a proxy for whatever's feeding it\ndata. This option will allow us to set a more relevant progress bar\ntitle.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/index-pack.c | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7fa74b9e79..9fd5d8a2d4 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -82,6 +82,12 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--progress-title::\n+\tFor internal use only.\n++\n+Set the title of the \"Receiving objects\" progress bar (it's \"Indexing\n+objects\" under `--stdin`).\n+\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3fbc5d7077..0237623943 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -122,6 +122,7 @@ static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;\n static int verbose;\n+static const char *progress_title;\n static int show_resolving_progress;\n static int show_stat;\n static int check_self_contained_and_connected;\n@@ -1159,6 +1160,7 @@ static void parse_pack_objects(unsigned char *hash)\n \n \tif (verbose)\n \t\tprogress = start_progress(\n+\t\t\t\tprogress_title ? progress_title :\n \t\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n@@ -1808,6 +1810,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tinput_len = sizeof(*hdr);\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n+\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n+\t\t\t\tif (progress_title || (i+1) >= argc)\n+\t\t\t\t\tusage(index_pack_usage);\n+\t\t\t\tprogress_title = argv[++i];\n \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n \t\t\t\tshow_resolving_progress = 1;\n \t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n-- \n2.32.0.988.g189fd9ae38\n\n"},{"id":"431274","messageId":"patch-4.4-01643f2ccd-20210727T004015Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH 4/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T00:41:42Z","receivedAt":"2021-07-27T00:42:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\nobjects and references by archive, 2007-02-22) did not show progress\noutput, even though the underlying API learned how to show progress in\nbe042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n2011-09-18).\n\nNow we'll show \"Unbundling objects\" using the new --progress-title\noption to \"git index-pack\", to go with its existing \"Receiving\nobjects\" and \"Indexing objects\" (which it shows when invoked with\n\"--stdin\", and with a pack file, respectively).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 10f6f45770..f027cce3fe 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n+\tint progress = isatty(2);\n+\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"show progress meter\")),\n \t\tOPT_END()\n \t};\n \tchar* bundle_file;\n@@ -178,6 +182,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n+\n+\tif (progress) {\n+\t\tstrvec_push(&extra_args, \"-v\");\n+\t\tstrvec_push(&extra_args, \"--progress-title\");\n+\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n+\t}\n+\n \tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n-- \n2.32.0.988.g189fd9ae38\n\n"},{"id":"433368","messageId":"cover-v2-0.4-00000000000-20210823T110136Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH v2 0/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T11:02:55Z","receivedAt":"2021-08-23T11:03:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This straightforward series addr progress output on \"git bundle\nunbundle\", we already had progress output if bundles were fetched from\nvia the transport.c (i.e. \"git clone/fetch\" etc.), but not from \"git\nbundle unbundle\" directly.\n\nThis was submitted as\nhttps://lore.kernel.org/git/cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com/\nbefore v2.33, hopefully now with the release out these rather trivial\npatches can be queued up. The only change since v1 is an extended\ncommit message in 3/4 discussing the initial motivation for this change.\n\nÆvar Arnfjörð Bjarmason (4):\n bundle API: start writing API documentation\n bundle API: change \"flags\" to be \"extra_index_pack_args\"\n index-pack: add --progress-title option\n bundle: show progress on \"unbundle\"\n\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/bundle.c | 16 ++++++++++++++--\n builtin/index-pack.c | 6 ++++++\n bundle.c | 17 +++++++++++------\n bundle.h | 15 +++++++++++++--\n transport.c | 5 ++++-\n 6 files changed, 54 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n1: 70865046bea = 1: dc8591f6d0b bundle API: start writing API documentation\n2: f19af15c9da = 2: 3d7bd9c33be bundle API: change \"flags\" to be \"extra_index_pack_args\"\n3: 98262f4cb89 ! 3: 67197064a8b index-pack: add --progress-title option\n @@ Commit message\n \n Add a --progress-title option to index-pack, when data is piped into\n index-pack its progress is a proxy for whatever's feeding it\n - data. This option will allow us to set a more relevant progress bar\n - title.\n + data.\n +\n + This option will allow us to set a more relevant progress bar title in\n + \"git bundle unbundle\", and is also used in my \"bundle-uri\" RFC\n + patches[1] by a new caller in fetch-pack.c.\n +\n + 1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n4: 853d72848a0 = 4: e4ca8b26962 bundle: show progress on \"unbundle\"\n-- \n2.33.0.662.g438caf9576d\n\n"},{"id":"433369","messageId":"patch-v2-2.4-3d7bd9c33be-20210823T110136Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v2-0.4-00000000000-20210823T110136Z-avarab@gmail.com","subject":"[PATCH v2 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T11:02:57Z","receivedAt":"2021-08-23T11:03:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"flags\" parameter was added in be042aff24c (Teach progress\neye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been\nmore than the one flag: BUNDLE_VERBOSE.\n\nLet's have the only caller who cares about that pass \"-v\" itself\ninstead through new \"extra_index_pack_args\" parameter. The flexibility\nof being able to pass arbitrary arguments to \"unbundle\" will be used\nin a subsequent commit.\n\nWe could pass NULL explicitly in cmd_bundle_unbundle(), but let's\ninstead initialize an empty strvec and pass it, in anticipation of a\nsubsequent commit wanting to add arguments to it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 5 +++--\n bundle.c | 17 +++++++++++------\n bundle.h | 8 ++++++--\n transport.c | 5 ++++-\n 4 files changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 053a51bea1b..10f6f45770a 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tchar *bundle_file;\n+\tchar* bundle_file;\n+\tstruct strvec extra_args = STRVEC_INIT;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n@@ -177,7 +178,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\ndiff --git a/bundle.c b/bundle.c\nindex ab63f402261..16d7e7f86f8 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -569,18 +569,23 @@ int create_bundle(struct repository *r, const char *path,\n }\n \n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags)\n+\t int bundle_fd, struct strvec *extra_index_pack_args)\n {\n-\tconst char *argv_index_pack[] = {\"index-pack\",\n-\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint i;\n \n-\tif (flags & BUNDLE_VERBOSE)\n-\t\targv_index_pack[3] = \"-v\";\n+\tstrvec_push(&ip.args, \"index-pack\");\n+\tstrvec_push(&ip.args, \"--fix-thin\");\n+\tstrvec_push(&ip.args, \"--stdin\");\n+\tif (extra_index_pack_args) {\n+\t\tstruct strvec *extra = extra_index_pack_args;\n+\t\tfor (i = 0; i < extra->nr; i++)\n+\t\t\tstrvec_push(&ip.args, extra->v[i]);\n+\t\tstrvec_clear(extra_index_pack_args);\n+\t}\n \n \tif (verify_bundle(r, header, 0))\n \t\treturn -1;\n-\tip.argv = argv_index_pack;\n \tip.in = bundle_fd;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\ndiff --git a/bundle.h b/bundle.h\nindex 84a6df1b65d..d47a7a3c69c 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int argc, const char **argv, struct strvec *pack_options,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n-#define BUNDLE_VERBOSE 1\n \n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n * provided `bundle_fd` from read_bundle_header().\n+ *\n+ * Provide extra_index_pack_args to pass any extra arguments\n+ * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n+ * extra_index_pack_args (if any) will be strvec_clear()'d for you\n+ * (like the run-command.h API itself does).\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags);\n+\t int bundle_fd, struct strvec *extra_index_pack_args);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 17e9629710a..8bc4b5fcd3c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -162,12 +162,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct bundle_transport_data *data = transport->data;\n+\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n \n+\tstrvec_push(&extra_index_pack_args, \"-v\");\n+\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n+\t\t &extra_index_pack_args);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n-- \n2.33.0.662.g438caf9576d\n\n"},{"id":"433370","messageId":"patch-v2-1.4-dc8591f6d0b-20210823T110136Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v2-0.4-00000000000-20210823T110136Z-avarab@gmail.com","subject":"[PATCH v2 1/4] bundle API: start writing API documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T11:02:56Z","receivedAt":"2021-08-23T11:03:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are no other API docs in bundle.h, but this is at least a\nstart. We'll add a parameter to this function in a subsequent commit,\nbut let's start by documenting it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.h | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/bundle.h b/bundle.h\nindex 1927d8cd6a4..84a6df1b65d 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n #define BUNDLE_VERBOSE 1\n+\n+/**\n+ * Unbundle after reading the header with read_bundle_header().\n+ *\n+ * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n+ * provided `bundle_fd` from read_bundle_header().\n+ */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n-- \n2.33.0.662.g438caf9576d\n\n"},{"id":"433371","messageId":"patch-v2-3.4-67197064a8b-20210823T110136Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v2-0.4-00000000000-20210823T110136Z-avarab@gmail.com","subject":"[PATCH v2 3/4] index-pack: add --progress-title option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T11:02:58Z","receivedAt":"2021-08-23T11:03:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --progress-title option to index-pack, when data is piped into\nindex-pack its progress is a proxy for whatever's feeding it\ndata.\n\nThis option will allow us to set a more relevant progress bar title in\n\"git bundle unbundle\", and is also used in my \"bundle-uri\" RFC\npatches[1] by a new caller in fetch-pack.c.\n\n1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/index-pack.c | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7fa74b9e798..9fd5d8a2d45 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -82,6 +82,12 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--progress-title::\n+\tFor internal use only.\n++\n+Set the title of the \"Receiving objects\" progress bar (it's \"Indexing\n+objects\" under `--stdin`).\n+\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865c..0841c039ae2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -122,6 +122,7 @@ static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;\n static int verbose;\n+static const char *progress_title;\n static int show_resolving_progress;\n static int show_stat;\n static int check_self_contained_and_connected;\n@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)\n \n \tif (verbose)\n \t\tprogress = start_progress(\n+\t\t\t\tprogress_title ? progress_title :\n \t\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tinput_len = sizeof(*hdr);\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n+\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n+\t\t\t\tif (progress_title || (i+1) >= argc)\n+\t\t\t\t\tusage(index_pack_usage);\n+\t\t\t\tprogress_title = argv[++i];\n \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n \t\t\t\tshow_resolving_progress = 1;\n \t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n-- \n2.33.0.662.g438caf9576d\n\n"},{"id":"433372","messageId":"patch-v2-4.4-e4ca8b26962-20210823T110136Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v2-0.4-00000000000-20210823T110136Z-avarab@gmail.com","subject":"[PATCH v2 4/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T11:02:59Z","receivedAt":"2021-08-23T11:03:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\nobjects and references by archive, 2007-02-22) did not show progress\noutput, even though the underlying API learned how to show progress in\nbe042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n2011-09-18).\n\nNow we'll show \"Unbundling objects\" using the new --progress-title\noption to \"git index-pack\", to go with its existing \"Receiving\nobjects\" and \"Indexing objects\" (which it shows when invoked with\n\"--stdin\", and with a pack file, respectively).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 10f6f45770a..f027cce3fef 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n+\tint progress = isatty(2);\n+\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"show progress meter\")),\n \t\tOPT_END()\n \t};\n \tchar* bundle_file;\n@@ -178,6 +182,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n+\n+\tif (progress) {\n+\t\tstrvec_push(&extra_args, \"-v\");\n+\t\tstrvec_push(&extra_args, \"--progress-title\");\n+\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n+\t}\n+\n \tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n-- \n2.33.0.662.g438caf9576d\n\n"},{"id":"433597","messageId":"1193fbe3-f4f6-d8e7-3730-770d2932655d@gmail.com","threadId":"56167","inReplyTo":"patch-v2-1.4-dc8591f6d0b-20210823T110136Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/4] bundle API: start writing API documentation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-24T17:01:38Z","receivedAt":"2021-08-24T17:37:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n> There are no other API docs in bundle.h, but this is at least a\n> start. We'll add a parameter to this function in a subsequent commit,\n> but let's start by documenting it.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> bundle.h | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/bundle.h b/bundle.h\n> index 1927d8cd6a4..84a6df1b65d 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n> \t\t int version);\n> int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n> #define BUNDLE_VERBOSE 1\n> +\n> +/**\n\nnit: what's the use of the \"/**\" start to these doc comments?\n\nI see examples in the codebase of both, but we are not consistent even\nwithin a single file. Here is how I counted instances of each:\n\n$ git grep \"^/\\\\*\\\\*\\$\" -- *.h | wc -l\n266\n$ git grep \"^/\\\\*\\$\" -- *.h | wc -l\n775\n\nSo we use \"/*\" three times as often as \"/**\". Should we attempt to\nbe more consistent in the future?\n\nThanks,\n-Stolee\n"},{"id":"433598","messageId":"30620e13-4509-1905-7644-9962b6adf9c5@gmail.com","threadId":"56167","inReplyTo":"patch-v2-2.4-3d7bd9c33be-20210823T110136Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-24T17:09:02Z","receivedAt":"2021-08-24T17:49:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n...\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n> \tstruct option options[] = {\n> \t\tOPT_END()\n> \t};\n> -\tchar *bundle_file;\n> +\tchar* bundle_file;\n\nnit: errant movement of \"*\" here.\n\n> +\tstruct strvec extra_args = STRVEC_INIT;\n...\n> -\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n> +\tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n\nI'm assuming that you will be adding something that adds to extra_args\nin a future commit. It might be better to just convert the \"0\" to \"NULL\"\nhere and add extra_args when you actually use it.\n\n> int unbundle(struct repository *r, struct bundle_header *header,\n> -\t int bundle_fd, int flags)\n> +\t int bundle_fd, struct strvec *extra_index_pack_args)\n> {\n> -\tconst char *argv_index_pack[] = {\"index-pack\",\n> -\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n> \tstruct child_process ip = CHILD_PROCESS_INIT;\n> +\tint i;\n> \n> -\tif (flags & BUNDLE_VERBOSE)\n> -\t\targv_index_pack[3] = \"-v\";\n> +\tstrvec_push(&ip.args, \"index-pack\");\n> +\tstrvec_push(&ip.args, \"--fix-thin\");\n> +\tstrvec_push(&ip.args, \"--stdin\");\n> +\tif (extra_index_pack_args) {\n> +\t\tstruct strvec *extra = extra_index_pack_args;\n\nCreating a shorter variable name seems unnecessary.\n\n> +\t\tfor (i = 0; i < extra->nr; i++)\n> +\t\t\tstrvec_push(&ip.args, extra->v[i]);\n\nThis seems like a good opportunity to create and use a\nstrvec_concat() method.\n\n> +\t\tstrvec_clear(extra_index_pack_args);\n\nWhy is it the responsibility of this method to clear these args?\nI suppose it is convenient. It just seems a bit wrong to me.\n\n> /**\n> * Unbundle after reading the header with read_bundle_header().\n> *\n> * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n> * provided `bundle_fd` from read_bundle_header().\n> + *\n> + * Provide extra_index_pack_args to pass any extra arguments\n> + * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n> + * extra_index_pack_args (if any) will be strvec_clear()'d for you\n> + * (like the run-command.h API itself does).\n> */\n> int unbundle(struct repository *r, struct bundle_header *header,\n> -\t int bundle_fd, int flags);\n> +\t int bundle_fd, struct strvec *extra_index_pack_args);\n> int list_bundle_refs(struct bundle_header *header,\n> \t\tint argc, const char **argv);\n> \n> diff --git a/transport.c b/transport.c\n> index 17e9629710a..8bc4b5fcd3c 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -162,12 +162,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n> \t\t\t int nr_heads, struct ref **to_fetch)\n> {\n> \tstruct bundle_transport_data *data = transport->data;\n> +\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n> \tint ret;\n> \n> +\tstrvec_push(&extra_index_pack_args, \"-v\");\n> +\n> \tif (!data->get_refs_from_bundle_called)\n> \t\tget_refs_from_bundle(transport, 0, NULL);\n> \tret = unbundle(the_repository, &data->header, data->fd,\n> -\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n\nPreviously, this was conditioned on 'transport->progress', but above\nyou unconditionally add the \"-v\" option. Seems like a bug.\n\n> +\t\t &extra_index_pack_args);\n\nThanks,\n-Stolee\n"},{"id":"433599","messageId":"f081ac63-cb5c-1777-d9df-828382637ed1@gmail.com","threadId":"56167","inReplyTo":"patch-v2-3.4-67197064a8b-20210823T110136Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/4] index-pack: add --progress-title option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-24T17:18:01Z","receivedAt":"2021-08-24T17:53:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n> +--progress-title::\n> +\tFor internal use only.\n> ++\n> +Set the title of the \"Receiving objects\" progress bar (it's \"Indexing\n> +objects\" under `--stdin`).\n\nMay I suggest a minor edit:\n\n Set the title of the progress bar. The title is \"Receiving objects\"\n by default and \"Indexing objects\" when `--stdin` is specified.\n\n> @@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n> \t\t\t\tinput_len = sizeof(*hdr);\n> \t\t\t} else if (!strcmp(arg, \"-v\")) {\n> \t\t\t\tverbose = 1;\n> +\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n\nUnfortunate that we are not using the parse-opts API. Not your fault.\n\n> +\t\t\t\tif (progress_title || (i+1) >= argc)\n\nstyle nit:\n\n\tif (progress_title || i + 1 >= argc)\n\nAlthough, I notice a similar line elsewhere in the file, so this gets\na pass.\n\n\tif (index_name || (i+1) >= argc)\n\n> +\t\t\t\t\tusage(index_pack_usage);\n> +\t\t\t\tprogress_title = argv[++i];\n\nOne downside to this organization is that `--progress-title=X` will\nnot work here. There are other `--=X` options in this\nbuiltin, and the index output name is specified with the short name\n`-o X`. We should probably err to match the `--=X`\npattern in this file for now. An eventual conversion to standard\noption parsing would be helpful here, but I don't think is worth\nblocking this series.\n\nThanks,\n-Stolee\n\n"},{"id":"433600","messageId":"aacc0d53-6a95-b5f2-3098-94f6ffc8d2b2@gmail.com","threadId":"56167","inReplyTo":"patch-v2-4.4-e4ca8b26962-20210823T110136Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/4] bundle: show progress on \"unbundle\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-24T17:23:42Z","receivedAt":"2021-08-24T17:54:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n> The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\n> objects and references by archive, 2007-02-22) did not show progress\n> output, even though the underlying API learned how to show progress in\n> be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n> 2011-09-18).\n> \n> Now we'll show \"Unbundling objects\" using the new --progress-title\n> option to \"git index-pack\", to go with its existing \"Receiving\n> objects\" and \"Indexing objects\" (which it shows when invoked with\n> \"--stdin\", and with a pack file, respectively).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/bundle.c | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/builtin/bundle.c b/builtin/bundle.c\n> index 10f6f45770a..f027cce3fef 100644\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n> \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n> \tint bundle_fd = -1;\n> \tint ret;\n> +\tint progress = isatty(2);\n> +\n> \tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"progress\", &progress,\n> +\t\t\t N_(\"show progress meter\")),\n\nWe should probably update Documentation/git-bundle.txt, specifically\nthe synopsis, which currently reads:\n\n'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\n\t\t [--version=] \n'git bundle' verify [-q | --quiet] \n'git bundle' list-heads [...]\n'git bundle' unbundle [...]\n\nAdd [--progress] to the unbundle line. The --progress option is\ndocumented further down in the file, although it is confusing\nwhere it applies.\n\nWhat about the --all-progress and --all-progress-implied options?\nReading the docs, it seems that they won't apply to 'unbundle',\nbut it doesn't hurt to ask.\n\n> +\n> +\tif (progress) {\n> +\t\tstrvec_push(&extra_args, \"-v\");\n> +\t\tstrvec_push(&extra_args, \"--progress-title\");\n> +\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n\nIf the previous patch changes to match the --progress-title=X\npattern of the other options in index-pack, then these two lines\nwill need to change, probably to a strvec_pushf().\n\n> +\t}\n> +\n> \tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n\nSince this is the first real use of extra_args, as I mentioned\nbefore it would not be the end of the world to have extra_args\nappear for the first time within this patch.\n\nThanks,\n-Stolee\n"},{"id":"433626","messageId":"87czq2ed3j.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"aacc0d53-6a95-b5f2-3098-94f6ffc8d2b2@gmail.com","subject":"Re: [PATCH v2 4/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T21:39:31Z","receivedAt":"2021-08-24T21:40:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Derrick Stolee wrote:\n\n> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n>> The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\n>> objects and references by archive, 2007-02-22) did not show progress\n>> output, even though the underlying API learned how to show progress in\n>> be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n>> 2011-09-18).\n>> \n>> Now we'll show \"Unbundling objects\" using the new --progress-title\n>> option to \"git index-pack\", to go with its existing \"Receiving\n>> objects\" and \"Indexing objects\" (which it shows when invoked with\n>> \"--stdin\", and with a pack file, respectively).\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/bundle.c | 11 +++++++++++\n>> 1 file changed, 11 insertions(+)\n>> \n>> diff --git a/builtin/bundle.c b/builtin/bundle.c\n>> index 10f6f45770a..f027cce3fef 100644\n>> --- a/builtin/bundle.c\n>> +++ b/builtin/bundle.c\n>> @@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n>> \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n>> \tint bundle_fd = -1;\n>> \tint ret;\n>> +\tint progress = isatty(2);\n>> +\n>> \tstruct option options[] = {\n>> +\t\tOPT_BOOL(0, \"progress\", &progress,\n>> +\t\t\t N_(\"show progress meter\")),\n>\n> We should probably update Documentation/git-bundle.txt, specifically\n> the synopsis, which currently reads:\n>\n> 'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\n> \t\t [--version=] \n> 'git bundle' verify [-q | --quiet] \n> 'git bundle' list-heads [...]\n> 'git bundle' unbundle [...]\n>\n> Add [--progress] to the unbundle line. The --progress option is\n> documented further down in the file, although it is confusing\n> where it applies.\n\nWill fix...\n\n> What about the --all-progress and --all-progress-implied options?\n> Reading the docs, it seems that they won't apply to 'unbundle',\n> but it doesn't hurt to ask.\n\n...and clarify...\n\n>> +\n>> +\tif (progress) {\n>> +\t\tstrvec_push(&extra_args, \"-v\");\n>> +\t\tstrvec_push(&extra_args, \"--progress-title\");\n>> +\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n>\n> If the previous patch changes to match the --progress-title=X\n> pattern of the other options in index-pack, then these two lines\n> will need to change, probably to a strvec_pushf().\n>\n>> +\t}\n>> +\n>> \tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n>\n> Since this is the first real use of extra_args, as I mentioned\n> before it would not be the end of the world to have extra_args\n> appear for the first time within this patch.\n\nSure, will change it. I figured reducing the size of the subsequent\ndiffs would be better, but will just start by passing NULL.\n"},{"id":"433627","messageId":"87a6l6ed1e.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"f081ac63-cb5c-1777-d9df-828382637ed1@gmail.com","subject":"Re: [PATCH v2 3/4] index-pack: add --progress-title option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T21:40:05Z","receivedAt":"2021-08-24T21:41:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Derrick Stolee wrote:\n\n> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n>> +--progress-title::\n>> +\tFor internal use only.\n>> ++\n>> +Set the title of the \"Receiving objects\" progress bar (it's \"Indexing\n>> +objects\" under `--stdin`).\n>\n> May I suggest a minor edit:\n>\n> Set the title of the progress bar. The title is \"Receiving objects\"\n> by default and \"Indexing objects\" when `--stdin` is specified.\n\nThanks.\n\n>> @@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>> \t\t\t\tinput_len = sizeof(*hdr);\n>> \t\t\t} else if (!strcmp(arg, \"-v\")) {\n>> \t\t\t\tverbose = 1;\n>> +\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n>\n> Unfortunate that we are not using the parse-opts API. Not your fault.\n>\n>> +\t\t\t\tif (progress_title || (i+1) >= argc)\n>\n> style nit:\n>\n> \tif (progress_title || i + 1 >= argc)\n>\n> Although, I notice a similar line elsewhere in the file, so this gets\n> a pass.\n>\n> \tif (index_name || (i+1) >= argc)\n\nYeah, it's exactly copy/pasted from another thing doing the same sort of\nparsing in this file. Will keep it the same for this new code, and just\nnote it.\n\nI think any subsequent change to refactor it will be easier if it's all\nconsistent, v.s. some using i + 1, another putting it in parenthesis\netc.\n\n>> +\t\t\t\t\tusage(index_pack_usage);\n>> +\t\t\t\tprogress_title = argv[++i];\n>\n> One downside to this organization is that `--progress-title=X` will\n> not work here. There are other `--=X` options in this\n> builtin, and the index output name is specified with the short name\n> `-o X`. We should probably err to match the `--=X`\n> pattern in this file for now. An eventual conversion to standard\n> option parsing would be helpful here, but I don't think is worth\n> blocking this series.\n\n*nod*.\n"},{"id":"433630","messageId":"877dgaecum.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"30620e13-4509-1905-7644-9962b6adf9c5@gmail.com","subject":"Re: [PATCH v2 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T21:41:22Z","receivedAt":"2021-08-24T21:45:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Derrick Stolee wrote:\n\n> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n> ...\n>> --- a/builtin/bundle.c\n>> +++ b/builtin/bundle.c\n>> @@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n>> \tstruct option options[] = {\n>> \t\tOPT_END()\n>> \t};\n>> -\tchar *bundle_file;\n>> +\tchar* bundle_file;\n>\n> nit: errant movement of \"*\" here.\n\nAh, thanks.\n\n>> +\tstruct strvec extra_args = STRVEC_INIT;\n> ...\n>> -\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n>> +\tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n>\n> I'm assuming that you will be adding something that adds to extra_args\n> in a future commit. It might be better to just convert the \"0\" to \"NULL\"\n> here and add extra_args when you actually use it.\n\n*nod*, commented on in the later commit.\n\n>> int unbundle(struct repository *r, struct bundle_header *header,\n>> -\t int bundle_fd, int flags)\n>> +\t int bundle_fd, struct strvec *extra_index_pack_args)\n>> {\n>> -\tconst char *argv_index_pack[] = {\"index-pack\",\n>> -\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n>> \tstruct child_process ip = CHILD_PROCESS_INIT;\n>> +\tint i;\n>> \n>> -\tif (flags & BUNDLE_VERBOSE)\n>> -\t\targv_index_pack[3] = \"-v\";\n>> +\tstrvec_push(&ip.args, \"index-pack\");\n>> +\tstrvec_push(&ip.args, \"--fix-thin\");\n>> +\tstrvec_push(&ip.args, \"--stdin\");\n>> +\tif (extra_index_pack_args) {\n>> +\t\tstruct strvec *extra = extra_index_pack_args;\n>\n> Creating a shorter variable name seems unnecessary.\n\nWill skip it.\n\n>> +\t\tfor (i = 0; i < extra->nr; i++)\n>> +\t\t\tstrvec_push(&ip.args, extra->v[i]);\n>\n> This seems like a good opportunity to create and use a\n> strvec_concat() method.\n\nYeah, I guess I could start with that. Will try it.\n\n>> +\t\tstrvec_clear(extra_index_pack_args);\n>\n> Why is it the responsibility of this method to clear these args?\n> I suppose it is convenient. It just seems a bit wrong to me.\n\nBecause of...\n\n>> /**\n>> * Unbundle after reading the header with read_bundle_header().\n>> *\n>> * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n>> * provided `bundle_fd` from read_bundle_header().\n>> + *\n>> + * Provide extra_index_pack_args to pass any extra arguments\n>> + * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n>> + * extra_index_pack_args (if any) will be strvec_clear()'d for you\n>> + * (like the run-command.h API itself does).\n\n... this, i.e. it's how the run-command.[ch] API already works for the\nsame sort of thing elsewhere, I figured making them consistent was\nbetter than having them differ.\n\nI think that while in general the rule of having each function allocate\n& clear its own memory is a good one, that a notable good exception in\nour codebase is various \"one-shot\" functions such as the run-command\nAPI, i.e. APIs where the vast majority of callers just want to set\nthings up for a one-off run. Having those common cases not require a\nthat_api_release(&ctx) afterwards seems like a good idea in general.\n\n>> */\n>> int unbundle(struct repository *r, struct bundle_header *header,\n>> -\t int bundle_fd, int flags);\n>> +\t int bundle_fd, struct strvec *extra_index_pack_args);\n>> int list_bundle_refs(struct bundle_header *header,\n>> \t\tint argc, const char **argv);\n>> \n>> diff --git a/transport.c b/transport.c\n>> index 17e9629710a..8bc4b5fcd3c 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -162,12 +162,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n>> \t\t\t int nr_heads, struct ref **to_fetch)\n>> {\n>> \tstruct bundle_transport_data *data = transport->data;\n>> +\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n>> \tint ret;\n>> \n>> +\tstrvec_push(&extra_index_pack_args, \"-v\");\n>> +\n>> \tif (!data->get_refs_from_bundle_called)\n>> \t\tget_refs_from_bundle(transport, 0, NULL);\n>> \tret = unbundle(the_repository, &data->header, data->fd,\n>> -\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n>\n> Previously, this was conditioned on 'transport->progress', but above\n> you unconditionally add the \"-v\" option. Seems like a bug.\n\nYes! Oops. Will fix. Thanks.\n"},{"id":"433632","messageId":"a70b7c7a-ecd7-b4f8-c253-65c4bf5de4a4@gmail.com","threadId":"56167","inReplyTo":"877dgaecum.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-24T21:48:26Z","receivedAt":"2021-08-24T21:48:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2021 5:41 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Aug 24 2021, Derrick Stolee wrote:\n> \n>> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> +\t\tstrvec_clear(extra_index_pack_args);\n>>\n>> Why is it the responsibility of this method to clear these args?\n>> I suppose it is convenient. It just seems a bit wrong to me.\n> \n> Because of...\n> \n>>> /**\n>>> * Unbundle after reading the header with read_bundle_header().\n>>> *\n>>> * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n>>> * provided `bundle_fd` from read_bundle_header().\n>>> + *\n>>> + * Provide extra_index_pack_args to pass any extra arguments\n>>> + * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n>>> + * extra_index_pack_args (if any) will be strvec_clear()'d for you\n>>> + * (like the run-command.h API itself does).\n> \n> ... this, i.e. it's how the run-command.[ch] API already works for the\n> same sort of thing elsewhere, I figured making them consistent was\n> better than having them differ.\n> \n> I think that while in general the rule of having each function allocate\n> & clear its own memory is a good one, that a notable good exception in\n> our codebase is various \"one-shot\" functions such as the run-command\n> API, i.e. APIs where the vast majority of callers just want to set\n> things up for a one-off run. Having those common cases not require a\n> that_api_release(&ctx) afterwards seems like a good idea in general.\n\nMakes sense to me. Thanks for explaining it.\n\n-Stolee\n"},{"id":"433652","messageId":"874kbeecfu.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"1193fbe3-f4f6-d8e7-3730-770d2932655d@gmail.com","subject":"Re: [PATCH v2 1/4] bundle API: start writing API documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T21:48:32Z","receivedAt":"2021-08-24T21:54:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Derrick Stolee wrote:\n\n> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:\n>> There are no other API docs in bundle.h, but this is at least a\n>> start. We'll add a parameter to this function in a subsequent commit,\n>> but let's start by documenting it.\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> bundle.h | 7 +++++++\n>> 1 file changed, 7 insertions(+)\n>> \n>> diff --git a/bundle.h b/bundle.h\n>> index 1927d8cd6a4..84a6df1b65d 100644\n>> --- a/bundle.h\n>> +++ b/bundle.h\n>> @@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n>> \t\t int version);\n>> int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n>> #define BUNDLE_VERBOSE 1\n>> +\n>> +/**\n>\n> nit: what's the use of the \"/**\" start to these doc comments?\n>\n> I see examples in the codebase of both, but we are not consistent even\n> within a single file. Here is how I counted instances of each:\n>\n> $ git grep \"^/\\\\*\\\\*\\$\" -- *.h | wc -l\n> 266\n> $ git grep \"^/\\\\*\\$\" -- *.h | wc -l\n> 775\n>\n> So we use \"/*\" three times as often as \"/**\". Should we attempt to\n> be more consistent in the future?\n\nThey're not the same thing. \"/*\\n\" is a normal comment, \"/**\\n\" is an\nAPI documentation comment.\n\nLooking around I don't think this was documented in CodingGuidelines,\nbut see bdfdaa4978d (strbuf.h: integrate api-strbuf.txt documentation,\n2015-01-16) and 6afbbdda333 (strbuf.h: unify documentation comments\nbeginnings, 2015-01-16).\n\nThis is commonly supported by various tooling, e.g. in GNU Emacs a \"/**\"\ncomment is highlighted differently than a \"/*\" comment\n(font-lock-doc-face v.s. font-lock-comment-face).\n\nSo e.g. something_followed_by_open_close_parens() in a comment in C code\nwill be highlighted as a function name with a \"/**\" comment, but not\nwith a \"/*\" comment. I imagine that the same is true of various other\neditors/tooling.\n"},{"id":"433832","messageId":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com","subject":"[PATCH v3 0/5] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:46Z","receivedAt":"2021-08-26T14:05:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This straightforward series addr progress output on \"git bundle\nunbundle\", we already had progress output if bundles were fetched from\nvia the transport.c (i.e. \"git clone/fetch\" etc.), but not from \"git\nbundle unbundle\" directly.\n\nThis v3 should address all the comments Derrick Stolee had in one way\nor another, thanks a lot for the review!\n\nÆvar Arnfjörð Bjarmason (5):\n bundle API: start writing API documentation\n strvec: add a strvec_pushvec()\n bundle API: change \"flags\" to be \"extra_index_pack_args\"\n index-pack: add --progress-title option\n bundle: show progress on \"unbundle\"\n\n Documentation/git-bundle.txt | 2 +-\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/bundle.c | 15 ++++++++++++++-\n builtin/index-pack.c | 6 ++++++\n bundle.c | 14 ++++++++------\n bundle.h | 15 +++++++++++++--\n strvec.c | 8 ++++++++\n strvec.h | 7 +++++++\n submodule.c | 4 +---\n transport.c | 6 +++++-\n 10 files changed, 69 insertions(+), 14 deletions(-)\n\nRange-diff against v2:\n1: dc8591f6d0b ! 1: 9fb2f7a3a80 bundle API: start writing API documentation\n @@ Commit message\n start. We'll add a parameter to this function in a subsequent commit,\n but let's start by documenting it.\n \n + The \"/**\" comment (as opposed to \"/*\") signifies the start of API\n + documentation. See [1] and bdfdaa4978d (strbuf.h: integrate\n + api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:\n + unify documentation comments beginnings, 2015-01-16) for a discussion\n + of that convention.\n +\n + 1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## bundle.h ##\n-: ----------- > 2: 321b8ba3f0e strvec: add a strvec_pushvec()\n2: 3d7bd9c33be ! 3: 637039634e7 bundle API: change \"flags\" to be \"extra_index_pack_args\"\n @@ Commit message\n of being able to pass arbitrary arguments to \"unbundle\" will be used\n in a subsequent commit.\n \n - We could pass NULL explicitly in cmd_bundle_unbundle(), but let's\n - instead initialize an empty strvec and pass it, in anticipation of a\n - subsequent commit wanting to add arguments to it.\n -\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/bundle.c ##\n -@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n - \tstruct option options[] = {\n - \t\tOPT_END()\n - \t};\n --\tchar *bundle_file;\n -+\tchar* bundle_file;\n -+\tstruct strvec extra_args = STRVEC_INIT;\n - \n - \targc = parse_options_cmd_bundle(argc, argv, prefix,\n - \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n -\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n -+\tret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||\n ++\tret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\n @@ bundle.c: int create_bundle(struct repository *r, const char *path,\n -\tconst char *argv_index_pack[] = {\"index-pack\",\n -\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n -+\tint i;\n \n -\tif (flags & BUNDLE_VERBOSE)\n -\t\targv_index_pack[3] = \"-v\";\n @@ bundle.c: int create_bundle(struct repository *r, const char *path,\n +\tstrvec_push(&ip.args, \"--fix-thin\");\n +\tstrvec_push(&ip.args, \"--stdin\");\n +\tif (extra_index_pack_args) {\n -+\t\tstruct strvec *extra = extra_index_pack_args;\n -+\t\tfor (i = 0; i < extra->nr; i++)\n -+\t\t\tstrvec_push(&ip.args, extra->v[i]);\n ++\t\tstrvec_pushvec(&ip.args, extra_index_pack_args);\n +\t\tstrvec_clear(extra_index_pack_args);\n +\t}\n \n @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,\n +\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n \n -+\tstrvec_push(&extra_index_pack_args, \"-v\");\n ++\tif (transport->progress)\n ++\t\tstrvec_push(&extra_index_pack_args, \"-v\");\n +\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n -\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n -+\t\t &extra_index_pack_args);\n ++\t\t transport->progress ? &extra_index_pack_args : NULL);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n3: 67197064a8b ! 4: e44d825e5df index-pack: add --progress-title option\n @@ Commit message\n index-pack: add --progress-title option\n \n Add a --progress-title option to index-pack, when data is piped into\n - index-pack its progress is a proxy for whatever's feeding it\n - data.\n + index-pack its progress is a proxy for whatever's feeding it data.\n \n This option will allow us to set a more relevant progress bar title in\n \"git bundle unbundle\", and is also used in my \"bundle-uri\" RFC\n patches[1] by a new caller in fetch-pack.c.\n \n + The code change in cmd_index_pack() won't handle\n + \"--progress-title=xyz\", only \"--progress-title xyz\", and the \"(i+1)\"\n + style (as opposed to \"i + 1\") is a bit odd.\n +\n + Not using the \"--long-option=value\" style is inconsistent with\n + existing long options handled by cmd_index_pack(), but makes the code\n + that needs to call it better (two strvec_push(), instead of needing a\n + strvec_pushf()).\n +\n + Since the option is internal-only the inconsistency shouldn't\n + matter. I'm copying the pattern to handle it as-is from the handling\n + of the existing \"-o\" option in the same function, see 9cf6d3357aa (Add\n + git-index-pack utility, 2005-10-12) for its addition.\n +\n + Eventually we'd like to migrate all of this this to parse_options(),\n + which would make these differences in behavior go away.\n +\n 1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ Documentation/git-index-pack.txt: OPTIONS\n +--progress-title::\n +\tFor internal use only.\n ++\n -+Set the title of the \"Receiving objects\" progress bar (it's \"Indexing\n -+objects\" under `--stdin`).\n ++Set the title of the progress bar. The title is \"Receiving objects\" by\n ++default and \"Indexing objects\" when `--stdin` is specified.\n +\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n4: e4ca8b26962 < -: ----------- bundle: show progress on \"unbundle\"\n-: ----------- > 5: cd38b0f0fed bundle: show progress on \"unbundle\"\n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433833","messageId":"patch-v3-1.5-9fb2f7a3a80-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v3 1/5] bundle API: start writing API documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:47Z","receivedAt":"2021-08-26T14:05:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are no other API docs in bundle.h, but this is at least a\nstart. We'll add a parameter to this function in a subsequent commit,\nbut let's start by documenting it.\n\nThe \"/**\" comment (as opposed to \"/*\") signifies the start of API\ndocumentation. See [1] and bdfdaa4978d (strbuf.h: integrate\napi-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:\nunify documentation comments beginnings, 2015-01-16) for a discussion\nof that convention.\n\n1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.h | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/bundle.h b/bundle.h\nindex 1927d8cd6a4..84a6df1b65d 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n #define BUNDLE_VERBOSE 1\n+\n+/**\n+ * Unbundle after reading the header with read_bundle_header().\n+ *\n+ * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n+ * provided `bundle_fd` from read_bundle_header().\n+ */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433834","messageId":"patch-v3-3.5-637039634e7-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v3 3/5] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:49Z","receivedAt":"2021-08-26T14:06:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"flags\" parameter was added in be042aff24c (Teach progress\neye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been\nmore than the one flag: BUNDLE_VERBOSE.\n\nLet's have the only caller who cares about that pass \"-v\" itself\ninstead through new \"extra_index_pack_args\" parameter. The flexibility\nof being able to pass arbitrary arguments to \"unbundle\" will be used\nin a subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 2 +-\n bundle.c | 14 ++++++++------\n bundle.h | 8 ++++++--\n transport.c | 6 +++++-\n 4 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 053a51bea1b..f9360c32c6c 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -177,7 +177,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\ndiff --git a/bundle.c b/bundle.c\nindex ab63f402261..10d20bb0c48 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -569,18 +569,20 @@ int create_bundle(struct repository *r, const char *path,\n }\n \n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags)\n+\t int bundle_fd, struct strvec *extra_index_pack_args)\n {\n-\tconst char *argv_index_pack[] = {\"index-pack\",\n-\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n \n-\tif (flags & BUNDLE_VERBOSE)\n-\t\targv_index_pack[3] = \"-v\";\n+\tstrvec_push(&ip.args, \"index-pack\");\n+\tstrvec_push(&ip.args, \"--fix-thin\");\n+\tstrvec_push(&ip.args, \"--stdin\");\n+\tif (extra_index_pack_args) {\n+\t\tstrvec_pushvec(&ip.args, extra_index_pack_args);\n+\t\tstrvec_clear(extra_index_pack_args);\n+\t}\n \n \tif (verify_bundle(r, header, 0))\n \t\treturn -1;\n-\tip.argv = argv_index_pack;\n \tip.in = bundle_fd;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\ndiff --git a/bundle.h b/bundle.h\nindex 84a6df1b65d..d47a7a3c69c 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int argc, const char **argv, struct strvec *pack_options,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n-#define BUNDLE_VERBOSE 1\n \n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n * provided `bundle_fd` from read_bundle_header().\n+ *\n+ * Provide extra_index_pack_args to pass any extra arguments\n+ * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n+ * extra_index_pack_args (if any) will be strvec_clear()'d for you\n+ * (like the run-command.h API itself does).\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags);\n+\t int bundle_fd, struct strvec *extra_index_pack_args);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 17e9629710a..99b2498e5dd 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct bundle_transport_data *data = transport->data;\n+\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n \n+\tif (transport->progress)\n+\t\tstrvec_push(&extra_index_pack_args, \"-v\");\n+\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n+\t\t transport->progress ? &extra_index_pack_args : NULL);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433835","messageId":"patch-v3-2.5-321b8ba3f0e-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:48Z","receivedAt":"2021-08-26T14:06:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a strvec_pushvec() function to concatenate two \"struct strvec *\"\ntogether, and modify code added in 50d89ad6542 (submodule: use\nargv_array instead of hand-building arrays, 2012-09-01) to use it. In\na subsequent commit we'll gain another API user.\n\nThis could also have been named strvec_concat()[1], but I opted to\nmake its name consistent with the strbuf_addbuf() function instead. We\nonly name these sorts of functions *_concat() in one instance:\nparse_options_concat().\n\n1. http://lore.kernel.org/git/30620e13-4509-1905-7644-9962b6adf9c5@gmail.com\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n strvec.c | 8 ++++++++\n strvec.h | 7 +++++++\n submodule.c | 4 +---\n 3 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex 61a76ce6cb9..54ed8427c13 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -56,6 +56,14 @@ void strvec_pushv(struct strvec *array, const char **items)\n \t\tstrvec_push(array, *items);\n }\n \n+void strvec_pushvec(struct strvec *array, const struct strvec *items)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < items->nr; i++)\n+\t\tstrvec_push(array, items->v[i]);\n+}\n+\n void strvec_pop(struct strvec *array)\n {\n \tif (!array->nr)\ndiff --git a/strvec.h b/strvec.h\nindex fdcad75b45b..9003bde2b7d 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -62,6 +62,13 @@ void strvec_pushl(struct strvec *, ...);\n /* Push a null-terminated array of strings onto the end of the array. */\n void strvec_pushv(struct strvec *, const char **);\n \n+/**\n+ * Push the contents of another \"struct strvec *\" onto the end of the\n+ * array. Like strvec_pushv(), this is a convenience wrapper that\n+ * calls strvec_push() in a loop.\n+ */\n+void strvec_pushvec(struct strvec *, const struct strvec *);\n+\n /**\n * Remove the final element from the array. If there are no\n * elements in the array, do nothing.\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1dbf..84b5f5dc0e0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1606,7 +1606,6 @@ int fetch_populated_submodules(struct repository *r,\n \t\t\t int default_option,\n \t\t\t int quiet, int max_parallel_jobs)\n {\n-\tint i;\n \tstruct submodule_parallel_fetch spf = SPF_INIT;\n \n \tspf.r = r;\n@@ -1622,8 +1621,7 @@ int fetch_populated_submodules(struct repository *r,\n \t\tdie(_(\"index file corrupt\"));\n \n \tstrvec_push(&spf.args, \"fetch\");\n-\tfor (i = 0; i < options->nr; i++)\n-\t\tstrvec_push(&spf.args, options->v[i]);\n+\tstrvec_pushvec(&spf.args, options);\n \tstrvec_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433836","messageId":"patch-v3-4.5-e44d825e5df-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v3 4/5] index-pack: add --progress-title option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:50Z","receivedAt":"2021-08-26T14:06:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --progress-title option to index-pack, when data is piped into\nindex-pack its progress is a proxy for whatever's feeding it data.\n\nThis option will allow us to set a more relevant progress bar title in\n\"git bundle unbundle\", and is also used in my \"bundle-uri\" RFC\npatches[1] by a new caller in fetch-pack.c.\n\nThe code change in cmd_index_pack() won't handle\n\"--progress-title=xyz\", only \"--progress-title xyz\", and the \"(i+1)\"\nstyle (as opposed to \"i + 1\") is a bit odd.\n\nNot using the \"--long-option=value\" style is inconsistent with\nexisting long options handled by cmd_index_pack(), but makes the code\nthat needs to call it better (two strvec_push(), instead of needing a\nstrvec_pushf()).\n\nSince the option is internal-only the inconsistency shouldn't\nmatter. I'm copying the pattern to handle it as-is from the handling\nof the existing \"-o\" option in the same function, see 9cf6d3357aa (Add\ngit-index-pack utility, 2005-10-12) for its addition.\n\nEventually we'd like to migrate all of this this to parse_options(),\nwhich would make these differences in behavior go away.\n\n1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/index-pack.c | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7fa74b9e798..1f1e3592251 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -82,6 +82,12 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--progress-title::\n+\tFor internal use only.\n++\n+Set the title of the progress bar. The title is \"Receiving objects\" by\n+default and \"Indexing objects\" when `--stdin` is specified.\n+\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865c..0841c039ae2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -122,6 +122,7 @@ static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;\n static int verbose;\n+static const char *progress_title;\n static int show_resolving_progress;\n static int show_stat;\n static int check_self_contained_and_connected;\n@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)\n \n \tif (verbose)\n \t\tprogress = start_progress(\n+\t\t\t\tprogress_title ? progress_title :\n \t\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tinput_len = sizeof(*hdr);\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n+\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n+\t\t\t\tif (progress_title || (i+1) >= argc)\n+\t\t\t\t\tusage(index_pack_usage);\n+\t\t\t\tprogress_title = argv[++i];\n \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n \t\t\t\tshow_resolving_progress = 1;\n \t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433837","messageId":"patch-v3-5.5-cd38b0f0fed-20210826T140414Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v3 5/5] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T14:05:51Z","receivedAt":"2021-08-26T14:06:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\nobjects and references by archive, 2007-02-22) did not show progress\noutput, even though the underlying API learned how to show progress in\nbe042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n2011-09-18).\n\nNow we'll show \"Unbundling objects\" using the new --progress-title\noption to \"git index-pack\", to go with its existing \"Receiving\nobjects\" and \"Indexing objects\" (which it shows when invoked with\n\"--stdin\", and with a pack file, respectively).\n\nUnlike \"git bundle create\" we don't handle \"--quiet\" here, nor\n\"--all-progress\" and \"--all-progress-implied\". Those are all specific\nto \"create\" (and \"verify\", in the case of \"--quiet\").\n\nThe structure of the existing documentation is a bit unclear, e.g. the\ndocumentation for the \"--quiet\" option added in\n79862b6b77c (bundle-create: progress output control, 2019-11-10) only\ndescribes how it works for \"create\", and not for \"verify\". That and\nother issues in it should be fixed, but I'd like to avoid untangling\nthat mess right now. Let's just support the standard \"--no-progress\"\nimplicitly here, and leave cleaning up the general behavior of \"git\nbundle\" for a later change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-bundle.txt | 2 +-\n builtin/bundle.c | 15 ++++++++++++++-\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex ac0d0038350..71b5ecabd1f 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t\t [--version=] \n 'git bundle' verify [-q | --quiet] \n 'git bundle' list-heads [...]\n-'git bundle' unbundle [...]\n+'git bundle' unbundle [--progress] [...]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex f9360c32c6c..1fbfe280c57 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -162,10 +162,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n+\tint progress = isatty(2);\n+\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"show progress meter\")),\n \t\tOPT_END()\n \t};\n \tchar *bundle_file;\n+\tstruct strvec extra_args = STRVEC_INIT;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n@@ -177,7 +182,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\tret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||\n+\n+\tif (progress) {\n+\t\tstrvec_push(&extra_args, \"-v\");\n+\t\tstrvec_push(&extra_args, \"--progress-title\");\n+\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n+\t}\n+\n+\tret = !!unbundle(the_repository, &header, bundle_fd, progress ?\n+\t\t\t &extra_args : NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\n-- \n2.33.0.733.ga72a4f1c2e1\n\n"},{"id":"433995","messageId":"xmqq8s0m9xbl.fsf@gitster.g","threadId":"56167","inReplyTo":"patch-v3-2.5-321b8ba3f0e-20210826T140414Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-28T01:23:26Z","receivedAt":"2021-08-28T01:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Add a strvec_pushvec() function to concatenate two \"struct strvec *\"\n> together, and modify code added in 50d89ad6542 (submodule: use\n> argv_array instead of hand-building arrays, 2012-09-01) to use it. In\n> a subsequent commit we'll gain another API user.\n>\n> This could also have been named strvec_concat()[1], but I opted to\n> make its name consistent with the strbuf_addbuf() function instead. We\n> only name these sorts of functions *_concat() in one instance:\n> parse_options_concat().\n\nFWIW pushvec() is a much better name than concat() for this.\n\nconcat(A, B) could be an operation that is non-destructive for both\nA and B that returns a new vector C. I would imagine that it would\nbe how, if not majority of, but at least a nontrivial percentage of,\nreaders expect a function called concat() to behave. A better name\nfor destructive version would probably be append(), if there is no\nother constraints on naming.\n\nThe name pushvec(A, B) makes it clear, thanks to similarity with\nother push*(A, ...) variants, that A is mutated using the other\narguments. The name is much more clear with less chance of\nmisunderstanding.\n\n> +void strvec_pushvec(struct strvec *array, const struct strvec *items)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < items->nr; i++)\n> +\t\tstrvec_push(array, items->v[i]);\n> +}\n\nThis implementation is not wrong per-se, but is somewhat\ndisappointing. When items->nr is large, especially relative to the\noriginal array->alloc, it would incur unnecessary reallocations that\nwe can easily avoid by pre-sizing the array before pushing the\nelements of items from it.\n\nIn the original code that became the first user of this helper, it\nmay not have made much difference, but now it is becoming a more\ngenerally reusable API function, we should care.\n\nOther than that, looks quite straight-forward.\n\nThanks.\n"},{"id":"433996","messageId":"xmqq4kba9x1h.fsf@gitster.g","threadId":"56167","inReplyTo":"xmqq8s0m9xbl.fsf@gitster.g","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-28T01:29:30Z","receivedAt":"2021-08-28T01:29:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +void strvec_pushvec(struct strvec *array, const struct strvec *items)\n>> +{\n>> +\tint i;\n>> +\n>> +\tfor (i = 0; i < items->nr; i++)\n>> +\t\tstrvec_push(array, items->v[i]);\n>> +}\n>\n> This implementation is not wrong per-se, but is somewhat\n> disappointing. When items->nr is large, especially relative to the\n> original array->alloc, it would incur unnecessary reallocations that\n> we can easily avoid by pre-sizing the array before pushing the\n> elements of items from it.\n>\n> In the original code that became the first user of this helper, it\n> may not have made much difference, but now it is becoming a more\n> generally reusable API function, we should care.\n\nAnd if we do not care, you can rewrite the code that became the\nfirst user of this helper to instead call strvec_pushv() on the\nitems->v array that is guaranteed to be NULL terminated, without\ninventing this new helper.\n\nI think I am fine with either way.\n"},{"id":"433997","messageId":"xmqqy28m8hsu.fsf@gitster.g","threadId":"56167","inReplyTo":"patch-v3-3.5-637039634e7-20210826T140414Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/5] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-28T01:44:01Z","receivedAt":"2021-08-28T01:44:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since the \"flags\" parameter was added in be042aff24c (Teach progress\n> eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been\n> more than the one flag: BUNDLE_VERBOSE.\n>\n> Let's have the only caller who cares about that pass \"-v\" itself\n> instead through new \"extra_index_pack_args\" parameter. The flexibility\n> of being able to pass arbitrary arguments to \"unbundle\" will be used\n> in a subsequent commit.\n\nAs bundle API has been stable over a long period of time, I do not\nthink the loss of flexibility is too bad (i.e. we no longer can cary\nany extra information that cannot be expressed as a textual argument\ngiven to the index-pack process), compared to gained flexibility of\nthe index-pack command line we can use.\n\n> int unbundle(struct repository *r, struct bundle_header *header,\n> -\t int bundle_fd, int flags)\n> +\t int bundle_fd, struct strvec *extra_index_pack_args)\n> {\n> -\tconst char *argv_index_pack[] = {\"index-pack\",\n> -\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n> \tstruct child_process ip = CHILD_PROCESS_INIT;\n> \n> -\tif (flags & BUNDLE_VERBOSE)\n> -\t\targv_index_pack[3] = \"-v\";\n> +\tstrvec_push(&ip.args, \"index-pack\");\n> +\tstrvec_push(&ip.args, \"--fix-thin\");\n> +\tstrvec_push(&ip.args, \"--stdin\");\n\nI would have expected you'd use pushl() here.\n\n> +\tif (extra_index_pack_args) {\n> +\t\tstrvec_pushvec(&ip.args, extra_index_pack_args);\n> +\t\tstrvec_clear(extra_index_pack_args);\n> +\t}\n> \n> \tif (verify_bundle(r, header, 0))\n> \t\treturn -1;\n> -\tip.argv = argv_index_pack;\n> \tip.in = bundle_fd;\n> \tip.no_stdout = 1;\n> \tip.git_cmd = 1;\n> diff --git a/bundle.h b/bundle.h\n> index 84a6df1b65d..d47a7a3c69c 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,\n> \t\t int argc, const char **argv, struct strvec *pack_options,\n> \t\t int version);\n> int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n> -#define BUNDLE_VERBOSE 1\n> \n> /**\n> * Unbundle after reading the header with read_bundle_header().\n> *\n> * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n> * provided `bundle_fd` from read_bundle_header().\n> + *\n> + * Provide extra_index_pack_args to pass any extra arguments\n> + * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n> + * extra_index_pack_args (if any) will be strvec_clear()'d for you\n\nGood to comment on this. I found it a bit surprising that the in\nargument \"extra_index_pack_args\" is eaten by the consumer, instead\nof leaving it up to the caller when to clear (or reuse before clear)\nit.\n\n> + * (like the run-command.h API itself does).\n\nThis is not even run-command.h, so the mention of \"run-command.h API\nitself\" sounded strange to me. Without \"itself\", perhaps I would\nhave slightly less puzzled. IIRC, run_command() only clears upon\nsuccess and does not clear if start_command() fails, so it is not a\nvery good comparison. I think the comment reads better without this\nlast parenthesized note.\n\n> */\n> int unbundle(struct repository *r, struct bundle_header *header,\n> -\t int bundle_fd, int flags);\n> +\t int bundle_fd, struct strvec *extra_index_pack_args);\n> int list_bundle_refs(struct bundle_header *header,\n> \t\tint argc, const char **argv);\n> \n> diff --git a/transport.c b/transport.c\n> index 17e9629710a..99b2498e5dd 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,\n> \t\t\t int nr_heads, struct ref **to_fetch)\n> {\n> \tstruct bundle_transport_data *data = transport->data;\n> +\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n> \tint ret;\n> \n> +\tif (transport->progress)\n> +\t\tstrvec_push(&extra_index_pack_args, \"-v\");\n> +\n> \tif (!data->get_refs_from_bundle_called)\n> \t\tget_refs_from_bundle(transport, 0, NULL);\n> \tret = unbundle(the_repository, &data->header, data->fd,\n> -\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n> +\t\t transport->progress ? &extra_index_pack_args : NULL);\n\nWhy conditional?\n\nIf transport->progress is false, extra_index_pack_args is a strvec\nthat has 0 elements in it, and the strvec_pushvec() code knows how\nto handle it correctly and efficiently anyway. \n\nI do not see a reason why we want to make the caller harder to read\nwith the same conditional twice (i.e. if we pass NULL in non-verbose\ncase, we can unconditionally put \"-v\" in the array, and clear the\narray on the side of this caller, and that would allow us lose the\nclear from the unbundle() function).\n\nOther than that, looks very simple. Nicely done.\n\nThanks.\n"},{"id":"433998","messageId":"xmqqsfyu8hhb.fsf@gitster.g","threadId":"56167","inReplyTo":"patch-v3-4.5-e44d825e5df-20210826T140414Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/5] index-pack: add --progress-title option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-28T01:50:56Z","receivedAt":"2021-08-28T01:51:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since the option is internal-only the inconsistency shouldn't\n> matter.\n\nOK.\n\n> I'm copying the pattern to handle it as-is from the handling\n> of the existing \"-o\" option in the same function, see 9cf6d3357aa (Add\n> git-index-pack utility, 2005-10-12) for its addition.\n\nThat is apples-to-oranges comparison, as the original is not even a\nlong option so it won't be -o=filename anyway. So the \"i+1\" (as\nopposed to \"i + 1\") loses justification. Since the option is\nimplementation detail, having to spell its value separately is OK,\nthough.\n\n> Eventually we'd like to migrate all of this this to parse_options(),\n> which would make these differences in behavior go away.\n\nPerhaps, but if it does not matter now, it shouldn't be worth\ncode-churn later, either.\n\nThanks.\n\n"},{"id":"433999","messageId":"xmqqmtp28hbv.fsf@gitster.g","threadId":"56167","inReplyTo":"patch-v3-5.5-cd38b0f0fed-20210826T140414Z-avarab@gmail.com","subject":"Re: [PATCH v3 5/5] bundle: show progress on \"unbundle\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-28T01:54:12Z","receivedAt":"2021-08-28T01:54:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +\tif (progress) {\n> +\t\tstrvec_push(&extra_args, \"-v\");\n> +\t\tstrvec_push(&extra_args, \"--progress-title\");\n> +\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n\nNice. I would have expected to see pushl() though.\n\n> +\t}\n> +\n> +\tret = !!unbundle(the_repository, &header, bundle_fd, progress ?\n> +\t\t\t &extra_args : NULL) ||\n\nAgain, I wouldn't make the &extra_args conditional to progress\nhere. Future code change may decide to pass more args to underlying\nindex-pack and the criteria for doing so may be different from\nprogress.\n\nIf this code cares about readability, it should uncondtionally pass\n&extra_args.\n\nIf this code cares about readability *and* micro-optimization, then\nthe condition should be on !!extra_args.nr, not on whatever set of\nconditions happen to be used in today's code to throw items into\nextra_args array.\n\nOther than that, this was a pleasant read.\n\nThanks.\n"},{"id":"434005","messageId":"YSm3ofxlRB1ViBf5@coredump.intra.peff.net","threadId":"56167","inReplyTo":"xmqq4kba9x1h.fsf@gitster.g","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-28T04:12:17Z","receivedAt":"2021-08-28T04:12:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 27, 2021 at 06:29:30PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> >> +void strvec_pushvec(struct strvec *array, const struct strvec *items)\n> >> +{\n> >> +\tint i;\n> >> +\n> >> +\tfor (i = 0; i < items->nr; i++)\n> >> +\t\tstrvec_push(array, items->v[i]);\n> >> +}\n> >\n> > This implementation is not wrong per-se, but is somewhat\n> > disappointing. When items->nr is large, especially relative to the\n> > original array->alloc, it would incur unnecessary reallocations that\n> > we can easily avoid by pre-sizing the array before pushing the\n> > elements of items from it.\n> >\n> > In the original code that became the first user of this helper, it\n> > may not have made much difference, but now it is becoming a more\n> > generally reusable API function, we should care.\n> \n> And if we do not care, you can rewrite the code that became the\n> first user of this helper to instead call strvec_pushv() on the\n> items->v array that is guaranteed to be NULL terminated, without\n> inventing this new helper.\n\nI came here to say that. ;)\n\nI do not mind using pushv() directly, or a pushvec() that is a\nconvenience wrapper for pushv(). Even better if that wrapper is smart\nenough to pre-allocate based on items->nr, as you mentioned, but that\ncan also come later.\n\nOne thing that did surprise me: the use of \"int\" here for iterating,\nrather than size_t. But it seems that strvec is already storing ints,\nwhich is an accident!\n\nI think we'd want the patch below. It can be applied independently\n(though if we do take the index-iterating version of Ævar's patch, I\nthink it should switch to size_t).\n\n-- >8 --\nSubject: [PATCH] strvec: use size_t to store nr and alloc\n\nWe converted argv_array (which later became strvec) to use size_t in\n819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\norder to avoid the possibility of integer overflow. But later, commit\nd70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\nconverted these back to ints!\n\nThose two commits were part of the same patch series. I'm pretty sure\nwhat happened is that they were originally written in the opposite order\nand then cleaned up and re-ordered during an interactive rebase. And\nwhen resolving the inevitable conflict, I mistakenly took the \"rename\"\npatch completely, accidentally dropping the type change.\n\nWe can correct it now; better late than never.\n\nSigned-off-by: Jeff King \n---\n strvec.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/strvec.h b/strvec.h\nindex fdcad75b45..6b3cbd6758 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -29,8 +29,8 @@ extern const char *empty_strvec[];\n */\n struct strvec {\n \tconst char **v;\n-\tint nr;\n-\tint alloc;\n+\tsize_t nr;\n+\tsize_t alloc;\n };\n \n #define STRVEC_INIT { empty_strvec, 0, 0 }\n-- \n2.33.0.399.g06f2549587\n\n"},{"id":"434033","messageId":"xmqqzgsz7qnq.fsf@gitster.g","threadId":"56167","inReplyTo":"YSm3ofxlRB1ViBf5@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-29T23:54:49Z","receivedAt":"2021-08-29T23:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think we'd want the patch below. It can be applied independently\n> (though if we do take the index-iterating version of Ævar's patch, I\n> think it should switch to size_t).\n\nYeah, I do not see a strong need for _pushvec(), especially the\nvariant that does not preallocate, when we have _pushv(). But the\ntype fix below does make sense.\n\n> -- >8 --\n> Subject: [PATCH] strvec: use size_t to store nr and alloc\n>\n> We converted argv_array (which later became strvec) to use size_t in\n> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n> order to avoid the possibility of integer overflow. But later, commit\n> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n> converted these back to ints!\n>\n> Those two commits were part of the same patch series. I'm pretty sure\n> what happened is that they were originally written in the opposite order\n> and then cleaned up and re-ordered during an interactive rebase. And\n> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n> patch completely, accidentally dropping the type change.\n>\n> We can correct it now; better late than never.\n>\n> Signed-off-by: Jeff King \n> ---\n> strvec.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/strvec.h b/strvec.h\n> index fdcad75b45..6b3cbd6758 100644\n> --- a/strvec.h\n> +++ b/strvec.h\n> @@ -29,8 +29,8 @@ extern const char *empty_strvec[];\n> */\n> struct strvec {\n> \tconst char **v;\n> -\tint nr;\n> -\tint alloc;\n> +\tsize_t nr;\n> +\tsize_t alloc;\n> };\n> \n> #define STRVEC_INIT { empty_strvec, 0, 0 }\n"},{"id":"434128","messageId":"85e539bc-7af5-595e-bcf4-bf577eb3bdfd@gmail.com","threadId":"56167","inReplyTo":"xmqqzgsz7qnq.fsf@gitster.g","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-30T17:30:48Z","receivedAt":"2021-08-30T17:30:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/29/2021 7:54 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> I think we'd want the patch below. It can be applied independently\n>> (though if we do take the index-iterating version of Ævar's patch, I\n>> think it should switch to size_t).\n> \n> Yeah, I do not see a strong need for _pushvec(), especially the\n> variant that does not preallocate, when we have _pushv(). But the\n> type fix below does make sense.\n\nThanks for chiming in. I was not aware of _pushv() when I asked\nabout the _pushvec() variant. Sorry, Ævar, for sending you down\nan unnecessary direction.\n \n>> -- >8 --\n>> Subject: [PATCH] strvec: use size_t to store nr and alloc\n>>\n>> We converted argv_array (which later became strvec) to use size_t in\n>> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n>> order to avoid the possibility of integer overflow. But later, commit\n>> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n>> converted these back to ints!\n>>\n>> Those two commits were part of the same patch series. I'm pretty sure\n>> what happened is that they were originally written in the opposite order\n>> and then cleaned up and re-ordered during an interactive rebase. And\n>> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n>> patch completely, accidentally dropping the type change.\n>>\n>> We can correct it now; better late than never.\n...\n>> struct strvec {\n>> \tconst char **v;\n>> -\tint nr;\n>> -\tint alloc;\n>> +\tsize_t nr;\n>> +\tsize_t alloc;\n>> };\n\nThis is also a good change to take.\n\nThanks,\n-Stolee\n"},{"id":"434609","messageId":"87zgsu8ui3.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"xmqqmtp28hbv.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-02T22:47:05Z","receivedAt":"2021-09-02T22:47:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 27 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> +\tif (progress) {\n>> +\t\tstrvec_push(&extra_args, \"-v\");\n>> +\t\tstrvec_push(&extra_args, \"--progress-title\");\n>> +\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n>\n> Nice. I would have expected to see pushl() though.\n\nWill fix. Thanks!\n\n>> +\t}\n>> +\n>> +\tret = !!unbundle(the_repository, &header, bundle_fd, progress ?\n>> +\t\t\t &extra_args : NULL) ||\n>\n> Again, I wouldn't make the &extra_args conditional to progress\n> here. Future code change may decide to pass more args to underlying\n> index-pack and the criteria for doing so may be different from\n> progress.\n>\n> If this code cares about readability, it should uncondtionally pass\n> &extra_args.\n\nI agree, but I landed myself in a game of reviewer ping-pong. I had it\nthat way originally, then Derrick Stolee suggested changing it in this\nway in <30620e13-4509-1905-7644-9962b6adf9c5@gmail.com>, I'll just\nchange it back :)\n"},{"id":"434612","messageId":"87v93i8svd.fsf@evledraar.gmail.com","threadId":"56167","inReplyTo":"YSm3ofxlRB1ViBf5@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-02T23:19:33Z","receivedAt":"2021-09-02T23:23:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Aug 28 2021, Jeff King wrote:\n\n> On Fri, Aug 27, 2021 at 06:29:30PM -0700, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>> \n>> >> +void strvec_pushvec(struct strvec *array, const struct strvec *items)\n>> >> +{\n>> >> +\tint i;\n>> >> +\n>> >> +\tfor (i = 0; i < items->nr; i++)\n>> >> +\t\tstrvec_push(array, items->v[i]);\n>> >> +}\n>> >\n>> > This implementation is not wrong per-se, but is somewhat\n>> > disappointing. When items->nr is large, especially relative to the\n>> > original array->alloc, it would incur unnecessary reallocations that\n>> > we can easily avoid by pre-sizing the array before pushing the\n>> > elements of items from it.\n>> >\n>> > In the original code that became the first user of this helper, it\n>> > may not have made much difference, but now it is becoming a more\n>> > generally reusable API function, we should care.\n>> \n>> And if we do not care, you can rewrite the code that became the\n>> first user of this helper to instead call strvec_pushv() on the\n>> items->v array that is guaranteed to be NULL terminated, without\n>> inventing this new helper.\n>\n> I came here to say that. ;)\n>\n> I do not mind using pushv() directly, or a pushvec() that is a\n> convenience wrapper for pushv(). Even better if that wrapper is smart\n> enough to pre-allocate based on items->nr, as you mentioned, but that\n> can also come later.\n>\n> One thing that did surprise me: the use of \"int\" here for iterating,\n> rather than size_t. But it seems that strvec is already storing ints,\n> which is an accident!\n\nIs it really? If you temporarily try to say convert that to \"size_t *nr\"\nto have the compiler catch all the cases where we use \"nr\", and then\ns/size_t/int/g those all, you'll find that e.g. setup_revisions() and\nthe like expect to take either \"int argc\" or the strvec equivalent.\n\nWe can sensibly convert some of those to size_t, but not all, and the\nint v.s. size_t inconsistency as a result feels weird.\n\nSince the main point of this API is to be a wrapper for what a C main()\nwould take, shouldn't its prototype mirror its prototype? I.e. we should\nstick to \"int\" here?\n\n> I think we'd want the patch below. It can be applied independently\n> (though if we do take the index-iterating version of Ævar's patch, I\n> think it should switch to size_t).\n>\n> -- >8 --\n> Subject: [PATCH] strvec: use size_t to store nr and alloc\n>\n> We converted argv_array (which later became strvec) to use size_t in\n> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n> order to avoid the possibility of integer overflow. But later, commit\n> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n> converted these back to ints!\n>\n> Those two commits were part of the same patch series. I'm pretty sure\n> what happened is that they were originally written in the opposite order\n> and then cleaned up and re-ordered during an interactive rebase. And\n> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n> patch completely, accidentally dropping the type change.\n>\n> We can correct it now; better late than never.\n>\n> Signed-off-by: Jeff King \n> ---\n> strvec.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/strvec.h b/strvec.h\n> index fdcad75b45..6b3cbd6758 100644\n> --- a/strvec.h\n> +++ b/strvec.h\n> @@ -29,8 +29,8 @@ extern const char *empty_strvec[];\n> */\n> struct strvec {\n> \tconst char **v;\n> -\tint nr;\n> -\tint alloc;\n> +\tsize_t nr;\n> +\tsize_t alloc;\n> };\n> \n> #define STRVEC_INIT { empty_strvec, 0, 0 }\n\n"},{"id":"434624","messageId":"xmqq7dfygsg3.fsf@gitster.g","threadId":"56167","inReplyTo":"87v93i8svd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-03T05:05:00Z","receivedAt":"2021-09-03T05:05:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since the main point of this API is to be a wrapper for what a C main()\n> would take, shouldn't its prototype mirror its prototype? I.e. we should\n> stick to \"int\" here?\n\nThat's a fair-enough argument.\n\nIn other words, we do not mind the upper limit of 2 billion strings,\neven though we do care that we can have more than 4 billion bytes in\nany of these strings ;-)\n"},{"id":"434628","messageId":"YTIBnT8Ue1HZXs82@coredump.intra.peff.net","threadId":"56167","inReplyTo":"87v93i8svd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-03T11:06:05Z","receivedAt":"2021-09-03T11:06:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 03, 2021 at 01:19:33AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > One thing that did surprise me: the use of \"int\" here for iterating,\n> > rather than size_t. But it seems that strvec is already storing ints,\n> > which is an accident!\n> \n> Is it really? If you temporarily try to say convert that to \"size_t *nr\"\n> to have the compiler catch all the cases where we use \"nr\", and then\n> s/size_t/int/g those all, you'll find that e.g. setup_revisions() and\n> the like expect to take either \"int argc\" or the strvec equivalent.\n\nIt most definitely is an accident, in the sense that the commit which\nchanged it did not mean to. :)\n\n> We can sensibly convert some of those to size_t, but not all, and the\n> int v.s. size_t inconsistency as a result feels weird.\n> \n> Since the main point of this API is to be a wrapper for what a C main()\n> would take, shouldn't its prototype mirror its prototype? I.e. we should\n> stick to \"int\" here?\n\nThat isn't the main point of this API. The reason the name changed away\nfrom argv_array was so that it would be more obviously a generally\nuseful array of strings.\n\nBut even if that were the use, the point isn't that we expect to see 2B\nor even 4B strings in any reasonable use case. The point is that we\ndon't want to accidentally overflow the integer counter, leading to\nreads and writes of random bits of memory. And all it takes to do that\nis some code like:\n\n while (strbuf_readline(stdin, &buf))\n\tstrvec_push(&foo, buf.buf);\n\nOn a system with 32-bit size_t, you are not likely to actually allocate\n2B strings before you'd fail. But on most 64-bit systems, you have\nplenty of address space (and these days, often RAM) to do so, but you\nstill only need 2B strings, because \"int\" is 32 bits.\n\nSo code like setup_revisions() which uses an int is fine. It is reading\nfrom a source with that much smaller \"int\" limit in the first place.\nWhether strvec can handle bigger arrays or not does not matter either\nway. And when it later uses ints to operate on such a strvec, it's OK in\npractice; even if the size_t gets truncated on the fly to an int, we\nknow we did not put more than an int's worth of items into the array in\nthe first place.\n\nBut code which has potentially larger inputs (either because it reads\niteratively into the strvec as above, or because it is itself using a\nlarger data type) is now also OK, because it's using size_t.\n\nWhat's not OK is code which operates on a potentially-unbounded strvec\nusing an int. E.g., following the code I showed above with something\nlike:\n\n int nr = foo.nr;\n foo.v[nr] = xstrdup(\"replacement string\");\n\nwhich may write to memory outside of foo.v.\n\nObviously that's nonsense nobody would ever write directly. It is\npossible for some code to do this inadvertently (say, passing a ptr/int\npair through a function interface), but I think it's fairly unlikely in\npractice. So while I do think more code ought to be using size_t in\ngeneral, I don't think it's necessary to audit this carefully. The\nimportant thing to me is protecting strvec itself.\n\n-Peff\n"},{"id":"434711","messageId":"cover-v4-0.4-00000000000-20210905T072750Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v3-0.5-00000000000-20210826T140414Z-avarab@gmail.com","subject":"[PATCH v4 0/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-05T07:34:41Z","receivedAt":"2021-09-05T07:34:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This straightforward series addr progress output on \"git bundle\nunbundle\", we already had progress output if bundles were fetched from\nvia the transport.c (i.e. \"git clone/fetch\" etc.), but not from \"git\nbundle unbundle\" directly.\n\nIn the v3 I added conditionals to pass the &extra_index_pack_args only\nwhen we had something meanignful to pass along, based on Derrick\nStolee's feedback. Based on the feedback to v4 always passing it is\nnow back, which makes the progression and end-state more readable.\n\nI removed the new strvec_pushvec() function, now I just use\nstrvec_pushl() or strvec_pushv(), depending. Perhaps a\nstrvec_pushvec() makes sense, but let's consider that separate from\nthis series.\n\nThere's also comment & commit message changes here in response to\nfeedback.\n\nÆvar Arnfjörð Bjarmason (4):\n bundle API: start writing API documentation\n bundle API: change \"flags\" to be \"extra_index_pack_args\"\n index-pack: add --progress-title option\n bundle: show progress on \"unbundle\"\n\n Documentation/git-bundle.txt | 2 +-\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/bundle.c | 11 ++++++++++-\n builtin/index-pack.c | 6 ++++++\n bundle.c | 12 ++++++------\n bundle.h | 14 ++++++++++++--\n transport.c | 6 +++++-\n 7 files changed, 46 insertions(+), 11 deletions(-)\n\nRange-diff against v3:\n1: 9fb2f7a3a80 = 1: 05be8cb0fc3 bundle API: start writing API documentation\n2: 321b8ba3f0e < -: ----------- strvec: add a strvec_pushvec()\n3: 637039634e7 ! 2: 9255c766484 bundle API: change \"flags\" to be \"extra_index_pack_args\"\n @@ Commit message\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/bundle.c ##\n +@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n + \t\tOPT_END()\n + \t};\n + \tchar *bundle_file;\n ++\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n + \n + \targc = parse_options_cmd_bundle(argc, argv, prefix,\n + \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n -\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n -+\tret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||\n ++\tret = !!unbundle(the_repository, &header, bundle_fd,\n ++\t\t\t &extra_index_pack_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\n @@ bundle.c: int create_bundle(struct repository *r, const char *path,\n -\tconst char *argv_index_pack[] = {\"index-pack\",\n -\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n ++\tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n \n -\tif (flags & BUNDLE_VERBOSE)\n -\t\targv_index_pack[3] = \"-v\";\n -+\tstrvec_push(&ip.args, \"index-pack\");\n -+\tstrvec_push(&ip.args, \"--fix-thin\");\n -+\tstrvec_push(&ip.args, \"--stdin\");\n +\tif (extra_index_pack_args) {\n -+\t\tstrvec_pushvec(&ip.args, extra_index_pack_args);\n ++\t\tstrvec_pushv(&ip.args, extra_index_pack_args->v);\n +\t\tstrvec_clear(extra_index_pack_args);\n +\t}\n \n @@ bundle.h: int create_bundle(struct repository *r, const char *path,\n * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n * provided `bundle_fd` from read_bundle_header().\n + *\n -+ * Provide extra_index_pack_args to pass any extra arguments\n ++ * Provide \"extra_index_pack_args\" to pass any extra arguments\n + * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n -+ * extra_index_pack_args (if any) will be strvec_clear()'d for you\n -+ * (like the run-command.h API itself does).\n ++ * \"extra_index_pack_args\" (if any) will be strvec_clear()'d for you.\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n -\t int bundle_fd, int flags);\n @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n -\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n -+\t\t transport->progress ? &extra_index_pack_args : NULL);\n ++\t\t &extra_index_pack_args);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n4: e44d825e5df ! 3: 338c0e1e518 index-pack: add --progress-title option\n @@ Commit message\n Not using the \"--long-option=value\" style is inconsistent with\n existing long options handled by cmd_index_pack(), but makes the code\n that needs to call it better (two strvec_push(), instead of needing a\n - strvec_pushf()).\n + strvec_pushf()). Since the option is internal-only the inconsistency\n + shouldn't matter.\n \n - Since the option is internal-only the inconsistency shouldn't\n - matter. I'm copying the pattern to handle it as-is from the handling\n - of the existing \"-o\" option in the same function, see 9cf6d3357aa (Add\n - git-index-pack utility, 2005-10-12) for its addition.\n -\n - Eventually we'd like to migrate all of this this to parse_options(),\n - which would make these differences in behavior go away.\n + I'm copying the pattern to handle it as-is from the handling of the\n + existing \"-o\" option in the same function, see 9cf6d3357aa (Add\n + git-index-pack utility, 2005-10-12) for its addition. That's a short\n + option, but the code to implement the two is the same in functionality\n + and style. Eventually we'd like to migrate all of this this to\n + parse_options(), which would make these differences in behavior go\n + away.\n \n 1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n \n5: cd38b0f0fed ! 4: 8f4c7f99799 bundle: show progress on \"unbundle\"\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co\n \t\tOPT_END()\n \t};\n \tchar *bundle_file;\n -+\tstruct strvec extra_args = STRVEC_INIT;\n - \n - \targc = parse_options_cmd_bundle(argc, argv, prefix,\n - \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n --\tret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||\n -+\n -+\tif (progress) {\n -+\t\tstrvec_push(&extra_args, \"-v\");\n -+\t\tstrvec_push(&extra_args, \"--progress-title\");\n -+\t\tstrvec_push(&extra_args, _(\"Unbundling objects\"));\n -+\t}\n -+\n -+\tret = !!unbundle(the_repository, &header, bundle_fd, progress ?\n -+\t\t\t &extra_args : NULL) ||\n ++\tif (progress)\n ++\t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n ++\t\t\t _(\"Unbundling objects\"), NULL);\n + \tret = !!unbundle(the_repository, &header, bundle_fd,\n + \t\t\t &extra_index_pack_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n - \tbundle_header_release(&header);\n - cleanup:\n-- \n2.33.0.813.g41c39388776\n\n"},{"id":"434712","messageId":"patch-v4-1.4-05be8cb0fc3-20210905T072750Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v4-0.4-00000000000-20210905T072750Z-avarab@gmail.com","subject":"[PATCH v4 1/4] bundle API: start writing API documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-05T07:34:42Z","receivedAt":"2021-09-05T07:34:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are no other API docs in bundle.h, but this is at least a\nstart. We'll add a parameter to this function in a subsequent commit,\nbut let's start by documenting it.\n\nThe \"/**\" comment (as opposed to \"/*\") signifies the start of API\ndocumentation. See [1] and bdfdaa4978d (strbuf.h: integrate\napi-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:\nunify documentation comments beginnings, 2015-01-16) for a discussion\nof that convention.\n\n1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.h | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/bundle.h b/bundle.h\nindex 1927d8cd6a4..84a6df1b65d 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n #define BUNDLE_VERBOSE 1\n+\n+/**\n+ * Unbundle after reading the header with read_bundle_header().\n+ *\n+ * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n+ * provided `bundle_fd` from read_bundle_header().\n+ */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n-- \n2.33.0.813.g41c39388776\n\n"},{"id":"434713","messageId":"patch-v4-2.4-9255c766484-20210905T072750Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v4-0.4-00000000000-20210905T072750Z-avarab@gmail.com","subject":"[PATCH v4 2/4] bundle API: change \"flags\" to be \"extra_index_pack_args\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-05T07:34:43Z","receivedAt":"2021-09-05T07:34:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"flags\" parameter was added in be042aff24c (Teach progress\neye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been\nmore than the one flag: BUNDLE_VERBOSE.\n\nLet's have the only caller who cares about that pass \"-v\" itself\ninstead through new \"extra_index_pack_args\" parameter. The flexibility\nof being able to pass arbitrary arguments to \"unbundle\" will be used\nin a subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 4 +++-\n bundle.c | 12 ++++++------\n bundle.h | 7 +++++--\n transport.c | 6 +++++-\n 4 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 053a51bea1b..9b86c8529c7 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -166,6 +166,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tchar *bundle_file;\n+\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n@@ -177,7 +178,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd,\n+\t\t\t &extra_index_pack_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n cleanup:\ndiff --git a/bundle.c b/bundle.c\nindex ab63f402261..a0bb687b0f4 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -569,18 +569,18 @@ int create_bundle(struct repository *r, const char *path,\n }\n \n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags)\n+\t int bundle_fd, struct strvec *extra_index_pack_args)\n {\n-\tconst char *argv_index_pack[] = {\"index-pack\",\n-\t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n \n-\tif (flags & BUNDLE_VERBOSE)\n-\t\targv_index_pack[3] = \"-v\";\n+\tif (extra_index_pack_args) {\n+\t\tstrvec_pushv(&ip.args, extra_index_pack_args->v);\n+\t\tstrvec_clear(extra_index_pack_args);\n+\t}\n \n \tif (verify_bundle(r, header, 0))\n \t\treturn -1;\n-\tip.argv = argv_index_pack;\n \tip.in = bundle_fd;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\ndiff --git a/bundle.h b/bundle.h\nindex 84a6df1b65d..06009fe6b1f 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -26,16 +26,19 @@ int create_bundle(struct repository *r, const char *path,\n \t\t int argc, const char **argv, struct strvec *pack_options,\n \t\t int version);\n int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);\n-#define BUNDLE_VERBOSE 1\n \n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n * We'll invoke \"git index-pack --stdin --fix-thin\" for you on the\n * provided `bundle_fd` from read_bundle_header().\n+ *\n+ * Provide \"extra_index_pack_args\" to pass any extra arguments\n+ * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n+ * \"extra_index_pack_args\" (if any) will be strvec_clear()'d for you.\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, int flags);\n+\t int bundle_fd, struct strvec *extra_index_pack_args);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 17e9629710a..ab9b03ae9ff 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct bundle_transport_data *data = transport->data;\n+\tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n \n+\tif (transport->progress)\n+\t\tstrvec_push(&extra_index_pack_args, \"-v\");\n+\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle(transport, 0, NULL);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t\t transport->progress ? BUNDLE_VERBOSE : 0);\n+\t\t &extra_index_pack_args);\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n-- \n2.33.0.813.g41c39388776\n\n"},{"id":"434714","messageId":"patch-v4-3.4-338c0e1e518-20210905T072750Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v4-0.4-00000000000-20210905T072750Z-avarab@gmail.com","subject":"[PATCH v4 3/4] index-pack: add --progress-title option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-05T07:34:44Z","receivedAt":"2021-09-05T07:34:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --progress-title option to index-pack, when data is piped into\nindex-pack its progress is a proxy for whatever's feeding it data.\n\nThis option will allow us to set a more relevant progress bar title in\n\"git bundle unbundle\", and is also used in my \"bundle-uri\" RFC\npatches[1] by a new caller in fetch-pack.c.\n\nThe code change in cmd_index_pack() won't handle\n\"--progress-title=xyz\", only \"--progress-title xyz\", and the \"(i+1)\"\nstyle (as opposed to \"i + 1\") is a bit odd.\n\nNot using the \"--long-option=value\" style is inconsistent with\nexisting long options handled by cmd_index_pack(), but makes the code\nthat needs to call it better (two strvec_push(), instead of needing a\nstrvec_pushf()). Since the option is internal-only the inconsistency\nshouldn't matter.\n\nI'm copying the pattern to handle it as-is from the handling of the\nexisting \"-o\" option in the same function, see 9cf6d3357aa (Add\ngit-index-pack utility, 2005-10-12) for its addition. That's a short\noption, but the code to implement the two is the same in functionality\nand style. Eventually we'd like to migrate all of this this to\nparse_options(), which would make these differences in behavior go\naway.\n\n1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-index-pack.txt | 6 ++++++\n builtin/index-pack.c | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7fa74b9e798..1f1e3592251 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -82,6 +82,12 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--progress-title::\n+\tFor internal use only.\n++\n+Set the title of the progress bar. The title is \"Receiving objects\" by\n+default and \"Indexing objects\" when `--stdin` is specified.\n+\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865c..0841c039ae2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -122,6 +122,7 @@ static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;\n static int verbose;\n+static const char *progress_title;\n static int show_resolving_progress;\n static int show_stat;\n static int check_self_contained_and_connected;\n@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)\n \n \tif (verbose)\n \t\tprogress = start_progress(\n+\t\t\t\tprogress_title ? progress_title :\n \t\t\t\tfrom_stdin ? _(\"Receiving objects\") : _(\"Indexing objects\"),\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tinput_len = sizeof(*hdr);\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n+\t\t\t} else if (!strcmp(arg, \"--progress-title\")) {\n+\t\t\t\tif (progress_title || (i+1) >= argc)\n+\t\t\t\t\tusage(index_pack_usage);\n+\t\t\t\tprogress_title = argv[++i];\n \t\t\t} else if (!strcmp(arg, \"--show-resolving-progress\")) {\n \t\t\t\tshow_resolving_progress = 1;\n \t\t\t} else if (!strcmp(arg, \"--report-end-of-input\")) {\n-- \n2.33.0.813.g41c39388776\n\n"},{"id":"434715","messageId":"patch-v4-4.4-8f4c7f99799-20210905T072750Z-avarab@gmail.com","threadId":"56167","inReplyTo":"cover-v4-0.4-00000000000-20210905T072750Z-avarab@gmail.com","subject":"[PATCH v4 4/4] bundle: show progress on \"unbundle\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-05T07:34:45Z","receivedAt":"2021-09-05T07:34:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"unbundle\" command added in 2e0afafebd8 (Add git-bundle: move\nobjects and references by archive, 2007-02-22) did not show progress\noutput, even though the underlying API learned how to show progress in\nbe042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),\n2011-09-18).\n\nNow we'll show \"Unbundling objects\" using the new --progress-title\noption to \"git index-pack\", to go with its existing \"Receiving\nobjects\" and \"Indexing objects\" (which it shows when invoked with\n\"--stdin\", and with a pack file, respectively).\n\nUnlike \"git bundle create\" we don't handle \"--quiet\" here, nor\n\"--all-progress\" and \"--all-progress-implied\". Those are all specific\nto \"create\" (and \"verify\", in the case of \"--quiet\").\n\nThe structure of the existing documentation is a bit unclear, e.g. the\ndocumentation for the \"--quiet\" option added in\n79862b6b77c (bundle-create: progress output control, 2019-11-10) only\ndescribes how it works for \"create\", and not for \"verify\". That and\nother issues in it should be fixed, but I'd like to avoid untangling\nthat mess right now. Let's just support the standard \"--no-progress\"\nimplicitly here, and leave cleaning up the general behavior of \"git\nbundle\" for a later change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-bundle.txt | 2 +-\n builtin/bundle.c | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex ac0d0038350..71b5ecabd1f 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t\t [--version=] \n 'git bundle' verify [-q | --quiet] \n 'git bundle' list-heads [...]\n-'git bundle' unbundle [...]\n+'git bundle' unbundle [--progress] [...]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 9b86c8529c7..91975def2da 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n+\tint progress = isatty(2);\n+\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"show progress meter\")),\n \t\tOPT_END()\n \t};\n \tchar *bundle_file;\n@@ -178,6 +182,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n+\tif (progress)\n+\t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n+\t\t\t _(\"Unbundling objects\"), NULL);\n \tret = !!unbundle(the_repository, &header, bundle_fd,\n \t\t\t &extra_index_pack_args) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n-- \n2.33.0.813.g41c39388776\n\n"}]}