[PATCH 0/4] bundle: show progress on "unbundle"

This straightforward series addr progress output on "git bundle
unbundle", we already had progress output if bundles were fetched from
via the transport.c (i.e. "git clone/fetch" etc.), but not from "git
bundle unbundle" directly.

Ævar Arnfjörð Bjarmason (4):
  bundle API: start writing API documentation
  bundle API: change "flags" to be "extra_index_pack_args"
  index-pack: add --progress-title option
  bundle: show progress on "unbundle"

 Documentation/git-index-pack.txt |  6 ++++++
 builtin/bundle.c                 | 16 ++++++++++++++--
 builtin/index-pack.c             |  6 ++++++
 bundle.c                         | 17 +++++++++++------
 bundle.h                         | 15 +++++++++++++--
 transport.c                      |  5 ++++-
 6 files changed, 54 insertions(+), 11 deletions(-)

-- 
2.32.0.988.g189fd9ae38

Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.

Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.

We could pass NULL explicitly in cmd_bundle_unbundle(), but let's
instead initialize an empty strvec and pass it, in anticipation of a
subsequent commit wanting to add arguments to it.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c |  5 +++--
 bundle.c         | 17 +++++++++++------
 bundle.h         |  8 ++++++--
 transport.c      |  5 ++++-
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1..10f6f45770 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_END()
 	};
-	char *bundle_file;
+	char* bundle_file;
+	struct strvec extra_args = STRVEC_INIT;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);

@@ -177,7 +178,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
 cleanup:

diff --git a/bundle.c b/bundle.c
index 15ce62285c..f2b28a2092 100644
--- a/bundle.c
+++ b/bundle.c

@@ -733,18 +733,23 @@ int create_bundle(struct repository *r, const char *path,
 }
 
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int i;
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	strvec_push(&ip.args, "index-pack");
+	strvec_push(&ip.args, "--fix-thin");
+	strvec_push(&ip.args, "--stdin");
+	if (extra_index_pack_args) {
+		struct strvec *extra = extra_index_pack_args;
+		for (i = 0; i < extra->nr; i++)
+			strvec_push(&ip.args, extra->v[i]);
+		strvec_clear(extra_index_pack_args);
+	}
 
 	if (verify_bundle(r, header, 0))
 		return -1;
-	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;

diff --git a/bundle.h b/bundle.h
index 84a6df1b65..d47a7a3c69 100644
--- a/bundle.h
+++ b/bundle.h

@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
 		  int argc, const char **argv, struct strvec *pack_options,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
 
 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).
  */
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags);
+	     int bundle_fd, struct strvec *extra_index_pack_args);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
 

diff --git a/transport.c b/transport.c
index 7f3d0d46b3..8781186cf1 100644
--- a/transport.c
+++ b/transport.c

@@ -163,12 +163,15 @@ static int fetch_refs_from_bundle(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct bundle_transport_data *data = transport->data;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 	int ret;
 
+	strvec_push(&extra_index_pack_args, "-v");
+
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle(transport, 0, NULL);
 	ret = unbundle(the_repository, &data->header, data->fd,
-			   transport->progress ? BUNDLE_VERBOSE : 0);
+		       &extra_index_pack_args);
 	transport->hash_algo = data->header.hash_algo;
 	return ret;
 }

-- 
2.32.0.988.g189fd9ae38

There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 bundle.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bundle.h b/bundle.h
index 1927d8cd6a..84a6df1b65 100644
--- a/bundle.h
+++ b/bundle.h

@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
 int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,

-- 
2.32.0.988.g189fd9ae38

Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it
data. This option will allow us to set a more relevant progress bar
title.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-index-pack.txt | 6 ++++++
 builtin/index-pack.c             | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e79..9fd5d8a2d4 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt

@@ -82,6 +82,12 @@ OPTIONS
 --strict::
 	Die, if the pack contains broken objects or links.
 
+--progress-title::
+	For internal use only.
++
+Set the title of the "Receiving objects" progress bar (it's "Indexing
+objects" under `--stdin`).
+
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3fbc5d7077..0237623943 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -122,6 +122,7 @@ static int strict;
 static int do_fsck_object;
 static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
 static int verbose;
+static const char *progress_title;
 static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;

@@ -1159,6 +1160,7 @@ static void parse_pack_objects(unsigned char *hash)
 
 	if (verbose)
 		progress = start_progress(
+				progress_title ? progress_title :
 				from_stdin ? _("Receiving objects") : _("Indexing objects"),
 				nr_objects);
 	for (i = 0; i < nr_objects; i++) {

@@ -1808,6 +1810,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
+			} else if (!strcmp(arg, "--progress-title")) {
+				if (progress_title || (i+1) >= argc)
+					usage(index_pack_usage);
+				progress_title = argv[++i];
 			} else if (!strcmp(arg, "--show-resolving-progress")) {
 				show_resolving_progress = 1;
 			} else if (!strcmp(arg, "--report-end-of-input")) {

-- 
2.32.0.988.g189fd9ae38

The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).

Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 10f6f45770..f027cce3fe 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
+	int progress = isatty(2);
+
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &progress,
+			 N_("show progress meter")),
 		OPT_END()
 	};
 	char* bundle_file;

@@ -178,6 +182,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
+
+	if (progress) {
+		strvec_push(&extra_args, "-v");
+		strvec_push(&extra_args, "--progress-title");
+		strvec_push(&extra_args, _("Unbundling objects"));
+	}
+
 	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);

-- 
2.32.0.988.g189fd9ae38

This straightforward series addr progress output on "git bundle
unbundle", we already had progress output if bundles were fetched from
via the transport.c (i.e. "git clone/fetch" etc.), but not from "git
bundle unbundle" directly.

This was submitted as
https://lore.kernel.org/git/cover-0.4-0000000000-20210727T004015Z-avarab@gmail.com/
before v2.33, hopefully now with the release out these rather trivial
patches can be queued up. The only change since v1 is an extended
commit message in 3/4 discussing the initial motivation for this change.

Ævar Arnfjörð Bjarmason (4):
  bundle API: start writing API documentation
  bundle API: change "flags" to be "extra_index_pack_args"
  index-pack: add --progress-title option
  bundle: show progress on "unbundle"

 Documentation/git-index-pack.txt |  6 ++++++
 builtin/bundle.c                 | 16 ++++++++++++++--
 builtin/index-pack.c             |  6 ++++++
 bundle.c                         | 17 +++++++++++------
 bundle.h                         | 15 +++++++++++++--
 transport.c                      |  5 ++++-
 6 files changed, 54 insertions(+), 11 deletions(-)

Range-diff against v1:
1:  70865046bea = 1:  dc8591f6d0b bundle API: start writing API documentation
2:  f19af15c9da = 2:  3d7bd9c33be bundle API: change "flags" to be "extra_index_pack_args"
3:  98262f4cb89 ! 3:  67197064a8b index-pack: add --progress-title option
    @@ Commit message
     
         Add a --progress-title option to index-pack, when data is piped into
         index-pack its progress is a proxy for whatever's feeding it
    -    data. This option will allow us to set a more relevant progress bar
    -    title.
    +    data.
    +
    +    This option will allow us to set a more relevant progress bar title in
    +    "git bundle unbundle", and is also used in my "bundle-uri" RFC
    +    patches[1] by a new caller in fetch-pack.c.
    +
    +    1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
4:  853d72848a0 = 4:  e4ca8b26962 bundle: show progress on "unbundle"
-- 
2.33.0.662.g438caf9576d

Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.

Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.

We could pass NULL explicitly in cmd_bundle_unbundle(), but let's
instead initialize an empty strvec and pass it, in anticipation of a
subsequent commit wanting to add arguments to it.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c |  5 +++--
 bundle.c         | 17 +++++++++++------
 bundle.h         |  8 ++++++--
 transport.c      |  5 ++++-
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1b..10f6f45770a 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_END()
 	};
-	char *bundle_file;
+	char* bundle_file;
+	struct strvec extra_args = STRVEC_INIT;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);

@@ -177,7 +178,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
 cleanup:

diff --git a/bundle.c b/bundle.c
index ab63f402261..16d7e7f86f8 100644
--- a/bundle.c
+++ b/bundle.c

@@ -569,18 +569,23 @@ int create_bundle(struct repository *r, const char *path,
 }
 
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int i;
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	strvec_push(&ip.args, "index-pack");
+	strvec_push(&ip.args, "--fix-thin");
+	strvec_push(&ip.args, "--stdin");
+	if (extra_index_pack_args) {
+		struct strvec *extra = extra_index_pack_args;
+		for (i = 0; i < extra->nr; i++)
+			strvec_push(&ip.args, extra->v[i]);
+		strvec_clear(extra_index_pack_args);
+	}
 
 	if (verify_bundle(r, header, 0))
 		return -1;
-	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;

diff --git a/bundle.h b/bundle.h
index 84a6df1b65d..d47a7a3c69c 100644
--- a/bundle.h
+++ b/bundle.h

@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
 		  int argc, const char **argv, struct strvec *pack_options,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
 
 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).
  */
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags);
+	     int bundle_fd, struct strvec *extra_index_pack_args);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
 

diff --git a/transport.c b/transport.c
index 17e9629710a..8bc4b5fcd3c 100644
--- a/transport.c
+++ b/transport.c

@@ -162,12 +162,15 @@ static int fetch_refs_from_bundle(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct bundle_transport_data *data = transport->data;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 	int ret;
 
+	strvec_push(&extra_index_pack_args, "-v");
+
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle(transport, 0, NULL);
 	ret = unbundle(the_repository, &data->header, data->fd,
-			   transport->progress ? BUNDLE_VERBOSE : 0);
+		       &extra_index_pack_args);
 	transport->hash_algo = data->header.hash_algo;
 	return ret;
 }

-- 
2.33.0.662.g438caf9576d

There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 bundle.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h

@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
 int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,

-- 
2.33.0.662.g438caf9576d

Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it
data.

This option will allow us to set a more relevant progress bar title in
"git bundle unbundle", and is also used in my "bundle-uri" RFC
patches[1] by a new caller in fetch-pack.c.

1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-index-pack.txt | 6 ++++++
 builtin/index-pack.c             | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e798..9fd5d8a2d45 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt

@@ -82,6 +82,12 @@ OPTIONS
 --strict::
 	Die, if the pack contains broken objects or links.
 
+--progress-title::
+	For internal use only.
++
+Set the title of the "Receiving objects" progress bar (it's "Indexing
+objects" under `--stdin`).
+
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865c..0841c039ae2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -122,6 +122,7 @@ static int strict;
 static int do_fsck_object;
 static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
 static int verbose;
+static const char *progress_title;
 static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;

@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)
 
 	if (verbose)
 		progress = start_progress(
+				progress_title ? progress_title :
 				from_stdin ? _("Receiving objects") : _("Indexing objects"),
 				nr_objects);
 	for (i = 0; i < nr_objects; i++) {

@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
+			} else if (!strcmp(arg, "--progress-title")) {
+				if (progress_title || (i+1) >= argc)
+					usage(index_pack_usage);
+				progress_title = argv[++i];
 			} else if (!strcmp(arg, "--show-resolving-progress")) {
 				show_resolving_progress = 1;
 			} else if (!strcmp(arg, "--report-end-of-input")) {

-- 
2.33.0.662.g438caf9576d

The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).

Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 10f6f45770a..f027cce3fef 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
+	int progress = isatty(2);
+
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &progress,
+			 N_("show progress meter")),
 		OPT_END()
 	};
 	char* bundle_file;

@@ -178,6 +182,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
+
+	if (progress) {
+		strvec_push(&extra_args, "-v");
+		strvec_push(&extra_args, "--progress-title");
+		strvec_push(&extra_args, _("Unbundling objects"));
+	}
+
 	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);

-- 
2.33.0.662.g438caf9576d

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

nit: what's the use of the "/**" start to these doc comments?

I see examples in the codebase of both, but we are not consistent even
within a single file. Here is how I counted instances of each:

$ git grep "^/\\*\\*\$" -- *.h | wc -l
266
$ git grep "^/\\*\$" -- *.h | wc -l
775

So we use "/*" three times as often as "/**". Should we attempt to
be more consistent in the future?

Thanks,
-Stolee

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:
...

nit: errant movement of "*" here.

+	struct strvec extra_args = STRVEC_INIT;

...

-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||

I'm assuming that you will be adding something that adds to extra_args
in a future commit. It might be better to just convert the "0" to "NULL"
here and add extra_args when you actually use it.

 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int i;
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	strvec_push(&ip.args, "index-pack");
+	strvec_push(&ip.args, "--fix-thin");
+	strvec_push(&ip.args, "--stdin");
+	if (extra_index_pack_args) {
+		struct strvec *extra = extra_index_pack_args;

Creating a shorter variable name seems unnecessary.

+		for (i = 0; i < extra->nr; i++)
+			strvec_push(&ip.args, extra->v[i]);

This seems like a good opportunity to create and use a
strvec_concat() method.

+		strvec_clear(extra_index_pack_args);

Why is it the responsibility of this method to clear these args?
I suppose it is convenient. It just seems a bit wrong to me.

Previously, this was conditioned on 'transport->progress', but above
you unconditionally add the "-v" option. Seems like a bug.

+		       &extra_index_pack_args);

Thanks,
-Stolee

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

+--progress-title::
+	For internal use only.
++
+Set the title of the "Receiving objects" progress bar (it's "Indexing
+objects" under `--stdin`).

May I suggest a minor edit:

  Set the title of the progress bar. The title is "Receiving objects"
  by default and "Indexing objects" when `--stdin` is specified.

Unfortunate that we are not using the parse-opts API. Not your fault.

+				if (progress_title || (i+1) >= argc)

style nit:

	if (progress_title || i + 1 >= argc)

Although, I notice a similar line elsewhere in the file, so this gets
a pass.

	if (index_name || (i+1) >= argc)

+					usage(index_pack_usage);
+				progress_title = argv[++i];

One downside to this organization is that `--progress-title=X` will
not work here. There are other `--<option-name>=X` options in this
builtin, and the index output name is specified with the short name
`-o X`. We should probably err to match the `--<option-name>=X`
pattern in this file for now. An eventual conversion to standard
option parsing would be helpful here, but I don't think is worth
blocking this series.

Thanks,
-Stolee

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

We should probably update Documentation/git-bundle.txt, specifically
the synopsis, which currently reads:

'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]
		    [--version=<version>] <file> <git-rev-list-args>
'git bundle' verify [-q | --quiet] <file>
'git bundle' list-heads <file> [<refname>...]
'git bundle' unbundle <file> [<refname>...]

Add [--progress] to the unbundle line. The --progress option is
documented further down in the file, although it is confusing
where it applies.

What about the --all-progress and --all-progress-implied options?
Reading the docs, it seems that they won't apply to 'unbundle',
but it doesn't hurt to ask.

+
+	if (progress) {
+		strvec_push(&extra_args, "-v");
+		strvec_push(&extra_args, "--progress-title");
+		strvec_push(&extra_args, _("Unbundling objects"));

If the previous patch changes to match the --progress-title=X
pattern of the other options in index-pack, then these two lines
will need to change, probably to a strvec_pushf().

+	}
+
 	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||

Since this is the first real use of extra_args, as I mentioned
before it would not be the end of the world to have extra_args
appear for the first time within this patch.

Thanks,
-Stolee

On Tue, Aug 24 2021, Derrick Stolee wrote:

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).

Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 10f6f45770a..f027cce3fef 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
+	int progress = isatty(2);
+
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &progress,
+			 N_("show progress meter")),

We should probably update Documentation/git-bundle.txt, specifically
the synopsis, which currently reads:

'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]
		    [--version=<version>] <file> <git-rev-list-args>
'git bundle' verify [-q | --quiet] <file>
'git bundle' list-heads <file> [<refname>...]
'git bundle' unbundle <file> [<refname>...]

Add [--progress] to the unbundle line. The --progress option is
documented further down in the file, although it is confusing
where it applies.

Will fix...

What about the --all-progress and --all-progress-implied options?
Reading the docs, it seems that they won't apply to 'unbundle',
but it doesn't hurt to ask.

...and clarify...

quoted

+
+	if (progress) {
+		strvec_push(&extra_args, "-v");
+		strvec_push(&extra_args, "--progress-title");
+		strvec_push(&extra_args, _("Unbundling objects"));

If the previous patch changes to match the --progress-title=X
pattern of the other options in index-pack, then these two lines
will need to change, probably to a strvec_pushf().

quoted

+	}
+
 	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||

Since this is the first real use of extra_args, as I mentioned
before it would not be the end of the world to have extra_args
appear for the first time within this patch.

Sure, will change it. I figured reducing the size of the subsequent
diffs would be better, but will just start by passing NULL.

On Tue, Aug 24 2021, Derrick Stolee wrote:

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

+--progress-title::
+	For internal use only.
++
+Set the title of the "Receiving objects" progress bar (it's "Indexing
+objects" under `--stdin`).

May I suggest a minor edit:

  Set the title of the progress bar. The title is "Receiving objects"
  by default and "Indexing objects" when `--stdin` is specified.

Thanks.

quoted

@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
+			} else if (!strcmp(arg, "--progress-title")) {

Unfortunate that we are not using the parse-opts API. Not your fault.

quoted

+				if (progress_title || (i+1) >= argc)

style nit:

	if (progress_title || i + 1 >= argc)

Although, I notice a similar line elsewhere in the file, so this gets
a pass.

	if (index_name || (i+1) >= argc)

Yeah, it's exactly copy/pasted from another thing doing the same sort of
parsing in this file. Will keep it the same for this new code, and just
note it.

I think any subsequent change to refactor it will be easier if it's all
consistent, v.s. some using i + 1, another putting it in parenthesis
etc.

quoted

+					usage(index_pack_usage);
+				progress_title = argv[++i];

One downside to this organization is that `--progress-title=X` will
not work here. There are other `--<option-name>=X` options in this
builtin, and the index output name is specified with the short name
`-o X`. We should probably err to match the `--<option-name>=X`
pattern in this file for now. An eventual conversion to standard
option parsing would be helpful here, but I don't think is worth
blocking this series.

*nod*.

On Tue, Aug 24 2021, Derrick Stolee wrote:

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:
...

quoted

--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -165,7 +165,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_END()
 	};
-	char *bundle_file;
+	char* bundle_file;

nit: errant movement of "*" here.

Ah, thanks.

quoted

+	struct strvec extra_args = STRVEC_INIT;

...

quoted

-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||

I'm assuming that you will be adding something that adds to extra_args
in a future commit. It might be better to just convert the "0" to "NULL"
here and add extra_args when you actually use it.

*nod*, commented on in the later commit.

quoted

 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int i;
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	strvec_push(&ip.args, "index-pack");
+	strvec_push(&ip.args, "--fix-thin");
+	strvec_push(&ip.args, "--stdin");
+	if (extra_index_pack_args) {
+		struct strvec *extra = extra_index_pack_args;

Creating a shorter variable name seems unnecessary.

Will skip it.

quoted

+		for (i = 0; i < extra->nr; i++)
+			strvec_push(&ip.args, extra->v[i]);

This seems like a good opportunity to create and use a
strvec_concat() method.

Yeah, I guess I could start with that. Will try it.

quoted

+		strvec_clear(extra_index_pack_args);

Why is it the responsibility of this method to clear these args?
I suppose it is convenient. It just seems a bit wrong to me.

Because of...

quoted

 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).

... this, i.e. it's how the run-command.[ch] API already works for the
same sort of thing elsewhere, I figured making them consistent was
better than having them differ.

I think that while in general the rule of having each function allocate
& clear its own memory is a good one, that a notable good exception in
our codebase is various "one-shot" functions such as the run-command
API, i.e. APIs where the vast majority of callers just want to set
things up for a one-off run. Having those common cases not require a
that_api_release(&ctx) afterwards seems like a good idea in general.

quoted

  */
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags);
+	     int bundle_fd, struct strvec *extra_index_pack_args);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
 

diff --git a/transport.c b/transport.c
index 17e9629710a..8bc4b5fcd3c 100644
--- a/transport.c
+++ b/transport.c

@@ -162,12 +162,15 @@ static int fetch_refs_from_bundle(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct bundle_transport_data *data = transport->data;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 	int ret;
 
+	strvec_push(&extra_index_pack_args, "-v");
+
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle(transport, 0, NULL);
 	ret = unbundle(the_repository, &data->header, data->fd,
-			   transport->progress ? BUNDLE_VERBOSE : 0);

Previously, this was conditioned on 'transport->progress', but above
you unconditionally add the "-v" option. Seems like a bug.

Yes! Oops. Will fix. Thanks.

On 8/24/2021 5:41 PM, Ævar Arnfjörð Bjarmason wrote:

On Tue, Aug 24 2021, Derrick Stolee wrote:

quoted

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

+		strvec_clear(extra_index_pack_args);

Why is it the responsibility of this method to clear these args?
I suppose it is convenient. It just seems a bit wrong to me.

Because of...

quoted

quoted

 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).

... this, i.e. it's how the run-command.[ch] API already works for the
same sort of thing elsewhere, I figured making them consistent was
better than having them differ.

I think that while in general the rule of having each function allocate
& clear its own memory is a good one, that a notable good exception in
our codebase is various "one-shot" functions such as the run-command
API, i.e. APIs where the vast majority of callers just want to set
things up for a one-off run. Having those common cases not require a
that_api_release(&ctx) afterwards seems like a good idea in general.

Makes sense to me. Thanks for explaining it.

-Stolee

On Tue, Aug 24 2021, Derrick Stolee wrote:

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 bundle.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h

@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
+
+/**

nit: what's the use of the "/**" start to these doc comments?

I see examples in the codebase of both, but we are not consistent even
within a single file. Here is how I counted instances of each:

$ git grep "^/\\*\\*\$" -- *.h | wc -l
266
$ git grep "^/\\*\$" -- *.h | wc -l
775

So we use "/*" three times as often as "/**". Should we attempt to
be more consistent in the future?

They're not the same thing. "/*\n" is a normal comment, "/**\n" is an
API documentation comment.

Looking around I don't think this was documented in CodingGuidelines,
but see bdfdaa4978d (strbuf.h: integrate api-strbuf.txt documentation,
2015-01-16) and 6afbbdda333 (strbuf.h: unify documentation comments
beginnings, 2015-01-16).

This is commonly supported by various tooling, e.g. in GNU Emacs a "/**"
comment is highlighted differently than a "/*" comment
(font-lock-doc-face v.s. font-lock-comment-face).

So e.g. something_followed_by_open_close_parens() in a comment in C code
will be highlighted as a function name with a "/**" comment, but not
with a "/*" comment. I imagine that the same is true of various other
editors/tooling.

This straightforward series addr progress output on "git bundle
unbundle", we already had progress output if bundles were fetched from
via the transport.c (i.e. "git clone/fetch" etc.), but not from "git
bundle unbundle" directly.

This v3 should address all the comments Derrick Stolee had in one way
or another, thanks a lot for the review!

Ævar Arnfjörð Bjarmason (5):
  bundle API: start writing API documentation
  strvec: add a strvec_pushvec()
  bundle API: change "flags" to be "extra_index_pack_args"
  index-pack: add --progress-title option
  bundle: show progress on "unbundle"

 Documentation/git-bundle.txt     |  2 +-
 Documentation/git-index-pack.txt |  6 ++++++
 builtin/bundle.c                 | 15 ++++++++++++++-
 builtin/index-pack.c             |  6 ++++++
 bundle.c                         | 14 ++++++++------
 bundle.h                         | 15 +++++++++++++--
 strvec.c                         |  8 ++++++++
 strvec.h                         |  7 +++++++
 submodule.c                      |  4 +---
 transport.c                      |  6 +++++-
 10 files changed, 69 insertions(+), 14 deletions(-)

Range-diff against v2:
1:  dc8591f6d0b ! 1:  9fb2f7a3a80 bundle API: start writing API documentation
    @@ Commit message
         start. We'll add a parameter to this function in a subsequent commit,
         but let's start by documenting it.
     
    +    The "/**" comment (as opposed to "/*") signifies the start of API
    +    documentation. See [1] and bdfdaa4978d (strbuf.h: integrate
    +    api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:
    +    unify documentation comments beginnings, 2015-01-16) for a discussion
    +    of that convention.
    +
    +    1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## bundle.h ##
-:  ----------- > 2:  321b8ba3f0e strvec: add a strvec_pushvec()
2:  3d7bd9c33be ! 3:  637039634e7 bundle API: change "flags" to be "extra_index_pack_args"
    @@ Commit message
         of being able to pass arbitrary arguments to "unbundle" will be used
         in a subsequent commit.
     
    -    We could pass NULL explicitly in cmd_bundle_unbundle(), but let's
    -    instead initialize an empty strvec and pass it, in anticipation of a
    -    subsequent commit wanting to add arguments to it.
    -
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/bundle.c ##
    -@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    - 	struct option options[] = {
    - 		OPT_END()
    - 	};
    --	char *bundle_file;
    -+	char* bundle_file;
    -+	struct strvec extra_args = STRVEC_INIT;
    - 
    - 	argc = parse_options_cmd_bundle(argc, argv, prefix,
    - 			builtin_bundle_unbundle_usage, options, &bundle_file);
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
      	}
      	if (!startup_info->have_repository)
      		die(_("Need a repository to unbundle."));
     -	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
    -+	ret = !!unbundle(the_repository, &header, bundle_fd, &extra_args) ||
    ++	ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
      		list_bundle_refs(&header, argc, argv);
      	bundle_header_release(&header);
      cleanup:
    @@ bundle.c: int create_bundle(struct repository *r, const char *path,
     -	const char *argv_index_pack[] = {"index-pack",
     -					 "--fix-thin", "--stdin", NULL, NULL};
      	struct child_process ip = CHILD_PROCESS_INIT;
    -+	int i;
      
     -	if (flags & BUNDLE_VERBOSE)
     -		argv_index_pack[3] = "-v";
    @@ bundle.c: int create_bundle(struct repository *r, const char *path,
     +	strvec_push(&ip.args, "--fix-thin");
     +	strvec_push(&ip.args, "--stdin");
     +	if (extra_index_pack_args) {
    -+		struct strvec *extra = extra_index_pack_args;
    -+		for (i = 0; i < extra->nr; i++)
    -+			strvec_push(&ip.args, extra->v[i]);
    ++		strvec_pushvec(&ip.args, extra_index_pack_args);
     +		strvec_clear(extra_index_pack_args);
     +	}
      
    @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
     +	struct strvec extra_index_pack_args = STRVEC_INIT;
      	int ret;
      
    -+	strvec_push(&extra_index_pack_args, "-v");
    ++	if (transport->progress)
    ++		strvec_push(&extra_index_pack_args, "-v");
     +
      	if (!data->get_refs_from_bundle_called)
      		get_refs_from_bundle(transport, 0, NULL);
      	ret = unbundle(the_repository, &data->header, data->fd,
     -			   transport->progress ? BUNDLE_VERBOSE : 0);
    -+		       &extra_index_pack_args);
    ++		       transport->progress ? &extra_index_pack_args : NULL);
      	transport->hash_algo = data->header.hash_algo;
      	return ret;
      }
3:  67197064a8b ! 4:  e44d825e5df index-pack: add --progress-title option
    @@ Commit message
         index-pack: add --progress-title option
     
         Add a --progress-title option to index-pack, when data is piped into
    -    index-pack its progress is a proxy for whatever's feeding it
    -    data.
    +    index-pack its progress is a proxy for whatever's feeding it data.
     
         This option will allow us to set a more relevant progress bar title in
         "git bundle unbundle", and is also used in my "bundle-uri" RFC
         patches[1] by a new caller in fetch-pack.c.
     
    +    The code change in cmd_index_pack() won't handle
    +    "--progress-title=xyz", only "--progress-title xyz", and the "(i+1)"
    +    style (as opposed to "i + 1") is a bit odd.
    +
    +    Not using the "--long-option=value" style is inconsistent with
    +    existing long options handled by cmd_index_pack(), but makes the code
    +    that needs to call it better (two strvec_push(), instead of needing a
    +    strvec_pushf()).
    +
    +    Since the option is internal-only the inconsistency shouldn't
    +    matter. I'm copying the pattern to handle it as-is from the handling
    +    of the existing "-o" option in the same function, see 9cf6d3357aa (Add
    +    git-index-pack utility, 2005-10-12) for its addition.
    +
    +    Eventually we'd like to migrate all of this this to parse_options(),
    +    which would make these differences in behavior go away.
    +
         1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
    @@ Documentation/git-index-pack.txt: OPTIONS
     +--progress-title::
     +	For internal use only.
     ++
    -+Set the title of the "Receiving objects" progress bar (it's "Indexing
    -+objects" under `--stdin`).
    ++Set the title of the progress bar. The title is "Receiving objects" by
    ++default and "Indexing objects" when `--stdin` is specified.
     +
      --check-self-contained-and-connected::
      	Die if the pack contains broken links. For internal use only.
4:  e4ca8b26962 < -:  ----------- bundle: show progress on "unbundle"
-:  ----------- > 5:  cd38b0f0fed bundle: show progress on "unbundle"
-- 
2.33.0.733.ga72a4f1c2e1

There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.

The "/**" comment (as opposed to "/*") signifies the start of API
documentation. See [1] and bdfdaa4978d (strbuf.h: integrate
api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:
unify documentation comments beginnings, 2015-01-16) for a discussion
of that convention.

1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 bundle.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h

@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
 int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,

-- 
2.33.0.733.ga72a4f1c2e1

Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.

Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c |  2 +-
 bundle.c         | 14 ++++++++------
 bundle.h         |  8 ++++++--
 transport.c      |  6 +++++-
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1b..f9360c32c6c 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -177,7 +177,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
 cleanup:

diff --git a/bundle.c b/bundle.c
index ab63f402261..10d20bb0c48 100644
--- a/bundle.c
+++ b/bundle.c

@@ -569,18 +569,20 @@ int create_bundle(struct repository *r, const char *path,
 }
 
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	strvec_push(&ip.args, "index-pack");
+	strvec_push(&ip.args, "--fix-thin");
+	strvec_push(&ip.args, "--stdin");
+	if (extra_index_pack_args) {
+		strvec_pushvec(&ip.args, extra_index_pack_args);
+		strvec_clear(extra_index_pack_args);
+	}
 
 	if (verify_bundle(r, header, 0))
 		return -1;
-	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;

diff --git a/bundle.h b/bundle.h
index 84a6df1b65d..d47a7a3c69c 100644
--- a/bundle.h
+++ b/bundle.h

@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
 		  int argc, const char **argv, struct strvec *pack_options,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
 
 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).
  */
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags);
+	     int bundle_fd, struct strvec *extra_index_pack_args);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
 

diff --git a/transport.c b/transport.c
index 17e9629710a..99b2498e5dd 100644
--- a/transport.c
+++ b/transport.c

@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct bundle_transport_data *data = transport->data;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 	int ret;
 
+	if (transport->progress)
+		strvec_push(&extra_index_pack_args, "-v");
+
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle(transport, 0, NULL);
 	ret = unbundle(the_repository, &data->header, data->fd,
-			   transport->progress ? BUNDLE_VERBOSE : 0);
+		       transport->progress ? &extra_index_pack_args : NULL);
 	transport->hash_algo = data->header.hash_algo;
 	return ret;
 }

-- 
2.33.0.733.ga72a4f1c2e1

Add a strvec_pushvec() function to concatenate two "struct strvec *"
together, and modify code added in 50d89ad6542 (submodule: use
argv_array instead of hand-building arrays, 2012-09-01) to use it. In
a subsequent commit we'll gain another API user.

This could also have been named strvec_concat()[1], but I opted to
make its name consistent with the strbuf_addbuf() function instead. We
only name these sorts of functions *_concat() in one instance:
parse_options_concat().

1. http://lore.kernel.org/git/30620e13-4509-1905-7644-9962b6adf9c5@gmail.com

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 strvec.c    | 8 ++++++++
 strvec.h    | 7 +++++++
 submodule.c | 4 +---
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/strvec.c b/strvec.c
index 61a76ce6cb9..54ed8427c13 100644
--- a/strvec.c
+++ b/strvec.c

@@ -56,6 +56,14 @@ void strvec_pushv(struct strvec *array, const char **items)
 		strvec_push(array, *items);
 }
 
+void strvec_pushvec(struct strvec *array, const struct strvec *items)
+{
+	int i;
+
+	for (i = 0; i < items->nr; i++)
+		strvec_push(array, items->v[i]);
+}
+
 void strvec_pop(struct strvec *array)
 {
 	if (!array->nr)

diff --git a/strvec.h b/strvec.h
index fdcad75b45b..9003bde2b7d 100644
--- a/strvec.h
+++ b/strvec.h

@@ -62,6 +62,13 @@ void strvec_pushl(struct strvec *, ...);
 /* Push a null-terminated array of strings onto the end of the array. */
 void strvec_pushv(struct strvec *, const char **);
 
+/**
+ * Push the contents of another "struct strvec *" onto the end of the
+ * array. Like strvec_pushv(), this is a convenience wrapper that
+ * calls strvec_push() in a loop.
+ */
+void strvec_pushvec(struct strvec *, const struct strvec *);
+
 /**
  * Remove the final element from the array. If there are no
  * elements in the array, do nothing.

diff --git a/submodule.c b/submodule.c
index 8e611fe1dbf..84b5f5dc0e0 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1606,7 +1606,6 @@ int fetch_populated_submodules(struct repository *r,
 			       int default_option,
 			       int quiet, int max_parallel_jobs)
 {
-	int i;
 	struct submodule_parallel_fetch spf = SPF_INIT;
 
 	spf.r = r;

@@ -1622,8 +1621,7 @@ int fetch_populated_submodules(struct repository *r,
 		die(_("index file corrupt"));
 
 	strvec_push(&spf.args, "fetch");
-	for (i = 0; i < options->nr; i++)
-		strvec_push(&spf.args, options->v[i]);
+	strvec_pushvec(&spf.args, options);
 	strvec_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 

-- 
2.33.0.733.ga72a4f1c2e1

Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it data.

This option will allow us to set a more relevant progress bar title in
"git bundle unbundle", and is also used in my "bundle-uri" RFC
patches[1] by a new caller in fetch-pack.c.

The code change in cmd_index_pack() won't handle
"--progress-title=xyz", only "--progress-title xyz", and the "(i+1)"
style (as opposed to "i + 1") is a bit odd.

Not using the "--long-option=value" style is inconsistent with
existing long options handled by cmd_index_pack(), but makes the code
that needs to call it better (two strvec_push(), instead of needing a
strvec_pushf()).

Since the option is internal-only the inconsistency shouldn't
matter. I'm copying the pattern to handle it as-is from the handling
of the existing "-o" option in the same function, see 9cf6d3357aa (Add
git-index-pack utility, 2005-10-12) for its addition.

Eventually we'd like to migrate all of this this to parse_options(),
which would make these differences in behavior go away.

1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-index-pack.txt | 6 ++++++
 builtin/index-pack.c             | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e798..1f1e3592251 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt

@@ -82,6 +82,12 @@ OPTIONS
 --strict::
 	Die, if the pack contains broken objects or links.
 
+--progress-title::
+	For internal use only.
++
+Set the title of the progress bar. The title is "Receiving objects" by
+default and "Indexing objects" when `--stdin` is specified.
+
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865c..0841c039ae2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -122,6 +122,7 @@ static int strict;
 static int do_fsck_object;
 static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
 static int verbose;
+static const char *progress_title;
 static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;

@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)
 
 	if (verbose)
 		progress = start_progress(
+				progress_title ? progress_title :
 				from_stdin ? _("Receiving objects") : _("Indexing objects"),
 				nr_objects);
 	for (i = 0; i < nr_objects; i++) {

@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
+			} else if (!strcmp(arg, "--progress-title")) {
+				if (progress_title || (i+1) >= argc)
+					usage(index_pack_usage);
+				progress_title = argv[++i];
 			} else if (!strcmp(arg, "--show-resolving-progress")) {
 				show_resolving_progress = 1;
 			} else if (!strcmp(arg, "--report-end-of-input")) {

-- 
2.33.0.733.ga72a4f1c2e1

The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).

Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).

Unlike "git bundle create" we don't handle "--quiet" here, nor
"--all-progress" and "--all-progress-implied". Those are all specific
to "create" (and "verify", in the case of "--quiet").

The structure of the existing documentation is a bit unclear, e.g. the
documentation for the "--quiet" option added in
79862b6b77c (bundle-create: progress output control, 2019-11-10) only
describes how it works for "create", and not for "verify". That and
other issues in it should be fixed, but I'd like to avoid untangling
that mess right now. Let's just support the standard "--no-progress"
implicitly here, and leave cleaning up the general behavior of "git
bundle" for a later change.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-bundle.txt |  2 +-
 builtin/bundle.c             | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index ac0d0038350..71b5ecabd1f 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt

@@ -13,7 +13,7 @@ SYNOPSIS
 		    [--version=<version>] <file> <git-rev-list-args>
 'git bundle' verify [-q | --quiet] <file>
 'git bundle' list-heads <file> [<refname>...]
-'git bundle' unbundle <file> [<refname>...]
+'git bundle' unbundle [--progress] <file> [<refname>...]
 
 DESCRIPTION
 -----------

diff --git a/builtin/bundle.c b/builtin/bundle.c
index f9360c32c6c..1fbfe280c57 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -162,10 +162,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
+	int progress = isatty(2);
+
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &progress,
+			 N_("show progress meter")),
 		OPT_END()
 	};
 	char *bundle_file;
+	struct strvec extra_args = STRVEC_INIT;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);

@@ -177,7 +182,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
+
+	if (progress) {
+		strvec_push(&extra_args, "-v");
+		strvec_push(&extra_args, "--progress-title");
+		strvec_push(&extra_args, _("Unbundling objects"));
+	}
+
+	ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
+			 &extra_args : NULL) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
 cleanup:

-- 
2.33.0.733.ga72a4f1c2e1

This straightforward series addr progress output on "git bundle
unbundle", we already had progress output if bundles were fetched from
via the transport.c (i.e. "git clone/fetch" etc.), but not from "git
bundle unbundle" directly.

In the v3 I added conditionals to pass the &extra_index_pack_args only
when we had something meanignful to pass along, based on Derrick
Stolee's feedback. Based on the feedback to v4 always passing it is
now back, which makes the progression and end-state more readable.

I removed the new strvec_pushvec() function, now I just use
strvec_pushl() or strvec_pushv(), depending. Perhaps a
strvec_pushvec() makes sense, but let's consider that separate from
this series.

There's also comment & commit message changes here in response to
feedback.

Ævar Arnfjörð Bjarmason (4):
  bundle API: start writing API documentation
  bundle API: change "flags" to be "extra_index_pack_args"
  index-pack: add --progress-title option
  bundle: show progress on "unbundle"

 Documentation/git-bundle.txt     |  2 +-
 Documentation/git-index-pack.txt |  6 ++++++
 builtin/bundle.c                 | 11 ++++++++++-
 builtin/index-pack.c             |  6 ++++++
 bundle.c                         | 12 ++++++------
 bundle.h                         | 14 ++++++++++++--
 transport.c                      |  6 +++++-
 7 files changed, 46 insertions(+), 11 deletions(-)

Range-diff against v3:
1:  9fb2f7a3a80 = 1:  05be8cb0fc3 bundle API: start writing API documentation
2:  321b8ba3f0e < -:  ----------- strvec: add a strvec_pushvec()
3:  637039634e7 ! 2:  9255c766484 bundle API: change "flags" to be "extra_index_pack_args"
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/bundle.c ##
    +@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    + 		OPT_END()
    + 	};
    + 	char *bundle_file;
    ++	struct strvec extra_index_pack_args = STRVEC_INIT;
    + 
    + 	argc = parse_options_cmd_bundle(argc, argv, prefix,
    + 			builtin_bundle_unbundle_usage, options, &bundle_file);
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
      	}
      	if (!startup_info->have_repository)
      		die(_("Need a repository to unbundle."));
     -	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
    -+	ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
    ++	ret = !!unbundle(the_repository, &header, bundle_fd,
    ++			 &extra_index_pack_args) ||
      		list_bundle_refs(&header, argc, argv);
      	bundle_header_release(&header);
      cleanup:
    @@ bundle.c: int create_bundle(struct repository *r, const char *path,
     -	const char *argv_index_pack[] = {"index-pack",
     -					 "--fix-thin", "--stdin", NULL, NULL};
      	struct child_process ip = CHILD_PROCESS_INIT;
    ++	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
      
     -	if (flags & BUNDLE_VERBOSE)
     -		argv_index_pack[3] = "-v";
    -+	strvec_push(&ip.args, "index-pack");
    -+	strvec_push(&ip.args, "--fix-thin");
    -+	strvec_push(&ip.args, "--stdin");
     +	if (extra_index_pack_args) {
    -+		strvec_pushvec(&ip.args, extra_index_pack_args);
    ++		strvec_pushv(&ip.args, extra_index_pack_args->v);
     +		strvec_clear(extra_index_pack_args);
     +	}
      
    @@ bundle.h: int create_bundle(struct repository *r, const char *path,
       * We'll invoke "git index-pack --stdin --fix-thin" for you on the
       * provided `bundle_fd` from read_bundle_header().
     + *
    -+ * Provide extra_index_pack_args to pass any extra arguments
    ++ * Provide "extra_index_pack_args" to pass any extra arguments
     + * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
    -+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
    -+ * (like the run-command.h API itself does).
    ++ * "extra_index_pack_args" (if any) will be strvec_clear()'d for you.
       */
      int unbundle(struct repository *r, struct bundle_header *header,
     -	     int bundle_fd, int flags);
    @@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
      		get_refs_from_bundle(transport, 0, NULL);
      	ret = unbundle(the_repository, &data->header, data->fd,
     -			   transport->progress ? BUNDLE_VERBOSE : 0);
    -+		       transport->progress ? &extra_index_pack_args : NULL);
    ++		       &extra_index_pack_args);
      	transport->hash_algo = data->header.hash_algo;
      	return ret;
      }
4:  e44d825e5df ! 3:  338c0e1e518 index-pack: add --progress-title option
    @@ Commit message
         Not using the "--long-option=value" style is inconsistent with
         existing long options handled by cmd_index_pack(), but makes the code
         that needs to call it better (two strvec_push(), instead of needing a
    -    strvec_pushf()).
    +    strvec_pushf()). Since the option is internal-only the inconsistency
    +    shouldn't matter.
     
    -    Since the option is internal-only the inconsistency shouldn't
    -    matter. I'm copying the pattern to handle it as-is from the handling
    -    of the existing "-o" option in the same function, see 9cf6d3357aa (Add
    -    git-index-pack utility, 2005-10-12) for its addition.
    -
    -    Eventually we'd like to migrate all of this this to parse_options(),
    -    which would make these differences in behavior go away.
    +    I'm copying the pattern to handle it as-is from the handling of the
    +    existing "-o" option in the same function, see 9cf6d3357aa (Add
    +    git-index-pack utility, 2005-10-12) for its addition. That's a short
    +    option, but the code to implement the two is the same in functionality
    +    and style. Eventually we'd like to migrate all of this this to
    +    parse_options(), which would make these differences in behavior go
    +    away.
     
         1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
     
5:  cd38b0f0fed ! 4:  8f4c7f99799 bundle: show progress on "unbundle"
    @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co
      		OPT_END()
      	};
      	char *bundle_file;
    -+	struct strvec extra_args = STRVEC_INIT;
    - 
    - 	argc = parse_options_cmd_bundle(argc, argv, prefix,
    - 			builtin_bundle_unbundle_usage, options, &bundle_file);
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
      	}
      	if (!startup_info->have_repository)
      		die(_("Need a repository to unbundle."));
    --	ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
    -+
    -+	if (progress) {
    -+		strvec_push(&extra_args, "-v");
    -+		strvec_push(&extra_args, "--progress-title");
    -+		strvec_push(&extra_args, _("Unbundling objects"));
    -+	}
    -+
    -+	ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
    -+			 &extra_args : NULL) ||
    ++	if (progress)
    ++		strvec_pushl(&extra_index_pack_args, "-v", "--progress-title",
    ++			     _("Unbundling objects"), NULL);
    + 	ret = !!unbundle(the_repository, &header, bundle_fd,
    + 			 &extra_index_pack_args) ||
      		list_bundle_refs(&header, argc, argv);
    - 	bundle_header_release(&header);
    - cleanup:
-- 
2.33.0.813.g41c39388776

There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.

The "/**" comment (as opposed to "/*") signifies the start of API
documentation. See [1] and bdfdaa4978d (strbuf.h: integrate
api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:
unify documentation comments beginnings, 2015-01-16) for a discussion
of that convention.

1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 bundle.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h

@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
 int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,

-- 
2.33.0.813.g41c39388776

Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.

Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/bundle.c |  4 +++-
 bundle.c         | 12 ++++++------
 bundle.h         |  7 +++++--
 transport.c      |  6 +++++-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1b..9b86c8529c7 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -166,6 +166,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	char *bundle_file;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);

@@ -177,7 +178,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd,
+			 &extra_index_pack_args) ||
 		list_bundle_refs(&header, argc, argv);
 	bundle_header_release(&header);
 cleanup:

diff --git a/bundle.c b/bundle.c
index ab63f402261..a0bb687b0f4 100644
--- a/bundle.c
+++ b/bundle.c

@@ -569,18 +569,18 @@ int create_bundle(struct repository *r, const char *path,
 }
 
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags)
+	     int bundle_fd, struct strvec *extra_index_pack_args)
 {
-	const char *argv_index_pack[] = {"index-pack",
-					 "--fix-thin", "--stdin", NULL, NULL};
 	struct child_process ip = CHILD_PROCESS_INIT;
+	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
-	if (flags & BUNDLE_VERBOSE)
-		argv_index_pack[3] = "-v";
+	if (extra_index_pack_args) {
+		strvec_pushv(&ip.args, extra_index_pack_args->v);
+		strvec_clear(extra_index_pack_args);
+	}
 
 	if (verify_bundle(r, header, 0))
 		return -1;
-	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;

diff --git a/bundle.h b/bundle.h
index 84a6df1b65d..06009fe6b1f 100644
--- a/bundle.h
+++ b/bundle.h

@@ -26,16 +26,19 @@ int create_bundle(struct repository *r, const char *path,
 		  int argc, const char **argv, struct strvec *pack_options,
 		  int version);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
 
 /**
  * Unbundle after reading the header with read_bundle_header().
  *
  * We'll invoke "git index-pack --stdin --fix-thin" for you on the
  * provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide "extra_index_pack_args" to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * "extra_index_pack_args" (if any) will be strvec_clear()'d for you.
  */
 int unbundle(struct repository *r, struct bundle_header *header,
-	     int bundle_fd, int flags);
+	     int bundle_fd, struct strvec *extra_index_pack_args);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
 

diff --git a/transport.c b/transport.c
index 17e9629710a..ab9b03ae9ff 100644
--- a/transport.c
+++ b/transport.c

@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
 			       int nr_heads, struct ref **to_fetch)
 {
 	struct bundle_transport_data *data = transport->data;
+	struct strvec extra_index_pack_args = STRVEC_INIT;
 	int ret;
 
+	if (transport->progress)
+		strvec_push(&extra_index_pack_args, "-v");
+
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle(transport, 0, NULL);
 	ret = unbundle(the_repository, &data->header, data->fd,
-			   transport->progress ? BUNDLE_VERBOSE : 0);
+		       &extra_index_pack_args);
 	transport->hash_algo = data->header.hash_algo;
 	return ret;
 }

-- 
2.33.0.813.g41c39388776

Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it data.

This option will allow us to set a more relevant progress bar title in
"git bundle unbundle", and is also used in my "bundle-uri" RFC
patches[1] by a new caller in fetch-pack.c.

The code change in cmd_index_pack() won't handle
"--progress-title=xyz", only "--progress-title xyz", and the "(i+1)"
style (as opposed to "i + 1") is a bit odd.

Not using the "--long-option=value" style is inconsistent with
existing long options handled by cmd_index_pack(), but makes the code
that needs to call it better (two strvec_push(), instead of needing a
strvec_pushf()). Since the option is internal-only the inconsistency
shouldn't matter.

I'm copying the pattern to handle it as-is from the handling of the
existing "-o" option in the same function, see 9cf6d3357aa (Add
git-index-pack utility, 2005-10-12) for its addition. That's a short
option, but the code to implement the two is the same in functionality
and style. Eventually we'd like to migrate all of this this to
parse_options(), which would make these differences in behavior go
away.

1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-index-pack.txt | 6 ++++++
 builtin/index-pack.c             | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e798..1f1e3592251 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt

@@ -82,6 +82,12 @@ OPTIONS
 --strict::
 	Die, if the pack contains broken objects or links.
 
+--progress-title::
+	For internal use only.
++
+Set the title of the progress bar. The title is "Receiving objects" by
+default and "Indexing objects" when `--stdin` is specified.
+
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865c..0841c039ae2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -122,6 +122,7 @@ static int strict;
 static int do_fsck_object;
 static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
 static int verbose;
+static const char *progress_title;
 static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;

@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)
 
 	if (verbose)
 		progress = start_progress(
+				progress_title ? progress_title :
 				from_stdin ? _("Receiving objects") : _("Indexing objects"),
 				nr_objects);
 	for (i = 0; i < nr_objects; i++) {

@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
+			} else if (!strcmp(arg, "--progress-title")) {
+				if (progress_title || (i+1) >= argc)
+					usage(index_pack_usage);
+				progress_title = argv[++i];
 			} else if (!strcmp(arg, "--show-resolving-progress")) {
 				show_resolving_progress = 1;
 			} else if (!strcmp(arg, "--report-end-of-input")) {

-- 
2.33.0.813.g41c39388776

The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).

Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).

Unlike "git bundle create" we don't handle "--quiet" here, nor
"--all-progress" and "--all-progress-implied". Those are all specific
to "create" (and "verify", in the case of "--quiet").

The structure of the existing documentation is a bit unclear, e.g. the
documentation for the "--quiet" option added in
79862b6b77c (bundle-create: progress output control, 2019-11-10) only
describes how it works for "create", and not for "verify". That and
other issues in it should be fixed, but I'd like to avoid untangling
that mess right now. Let's just support the standard "--no-progress"
implicitly here, and leave cleaning up the general behavior of "git
bundle" for a later change.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/git-bundle.txt | 2 +-
 builtin/bundle.c             | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index ac0d0038350..71b5ecabd1f 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt

@@ -13,7 +13,7 @@ SYNOPSIS
 		    [--version=<version>] <file> <git-rev-list-args>
 'git bundle' verify [-q | --quiet] <file>
 'git bundle' list-heads <file> [<refname>...]
-'git bundle' unbundle <file> [<refname>...]
+'git bundle' unbundle [--progress] <file> [<refname>...]
 
 DESCRIPTION
 -----------

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 9b86c8529c7..91975def2da 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
+	int progress = isatty(2);
+
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &progress,
+			 N_("show progress meter")),
 		OPT_END()
 	};
 	char *bundle_file;

@@ -178,6 +182,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
+	if (progress)
+		strvec_pushl(&extra_index_pack_args, "-v", "--progress-title",
+			     _("Unbundling objects"), NULL);
 	ret = !!unbundle(the_repository, &header, bundle_fd,
 			 &extra_index_pack_args) ||
 		list_bundle_refs(&header, argc, argv);