[v4,6/8] fetch: move display format parsing into main function

Commit Message

Patrick Steinhardt May 9, 2023, 1:02 p.m. UTC

Parsing the display format happens inside of `display_state_init()`. As
we only need to check for a simple config entry, this is a natural
location to put this code as it means that display-state logic is neatly
contained in a single location.

We're about to introduce a output format though that is intended to be
parseable by machines, for example inside of a script. In that case it
becomes a bit awkward of an interface if you have to call git-fetch(1)
with the `fetch.output` config key set. We're thus going to introduce a
new `--output-format` switch for git-fetch(1) so that the output format
can be configured more directly.

This means we'll have to hook parsing of the display format into the
command line options parser. Having the code to determine the actual
output format scattered across two different sites is hard to reason
about though.

Refactor the code such that callers are expected to pass the display
format that is to be used into `display_state_init()`. This allows us to
lift up the code into the main function, where we can then hook it into
command line options parser in a follow-up commit.

Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 builtin/fetch.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

Comments

Junio C Hamano May 9, 2023, 8:35 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> We're about to introduce a output format though that is intended to be
> parseable by machines, for example inside of a script. In that case it
> becomes a bit awkward of an interface if you have to call git-fetch(1)
> with the `fetch.output` config key set. We're thus going to introduce a
> new `--output-format` switch for git-fetch(1) so that the output format
> can be configured more directly.

Good.  I was wondering about that code in the context of the
previous patch, especially the error message had a hard-coded
assumption that it comes from a configuration variable.

And the changes to display_state_init() to lift the responsibility
of finding and validating .format out of the function, and the
changes to intermediate functions to pass the .format through the
callchain, are all expected and there was nothing questionable.

> @@ -2157,6 +2149,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
>  	const char *bundle_uri;
> +	enum display_format display_format = DISPLAY_FORMAT_UNKNOWN;
>  	struct string_list list = STRING_LIST_INIT_DUP;
>  	struct remote *remote = NULL;
>  	int result = 0;
> @@ -2183,6 +2176,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_fetch_options, builtin_fetch_usage, 0);
>  
> +	if (display_format == DISPLAY_FORMAT_UNKNOWN) {
> +		const char *format = "full";
> +
> +		git_config_get_string_tmp("fetch.output", &format);
> +		if (!strcasecmp(format, "full"))
> +			display_format = DISPLAY_FORMAT_FULL;
> +		else if (!strcasecmp(format, "compact"))
> +			display_format = DISPLAY_FORMAT_COMPACT;
> +		else
> +			die(_("invalid value for '%s': '%s'"),
> +			    "fetch.output", format);
> +	}

OK, but isn't the usual way to do this to have configuration parser
before parse_options() and then let parse_options() override
whatever display_format set by it?

That way, we do not have to worry about DISPLAY_FORMAT_UNKNOWN at
all.  Just initialize the variable to whatever default format at the
beginning of this function, read "fetch.output" to override it if
the configuration exists, and then let parse_options() to handle
"--output-format" or "--porcelain" or whatever to further override
it.

Thanks.

Glen Choo May 9, 2023, 10:30 p.m. UTC | #2

Patrick Steinhardt <[e-mail]> writes:

> with the `fetch.output` config key set. We're thus going to introduce a
> new `--output-format` switch for git-fetch(1) so that the output format
> can be configured more directly.

This is stale as of v3, since isn't named --output-format any more. Let
me see if there are other instances of this.

(I should have caught this earlier; I only saw it because Junio quoted
it in a reply of his.)

Patrick Steinhardt May 10, 2023, 12:35 p.m. UTC | #3

On Tue, May 09, 2023 at 03:30:33PM -0700, Glen Choo wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> > with the `fetch.output` config key set. We're thus going to introduce a
> > new `--output-format` switch for git-fetch(1) so that the output format
> > can be configured more directly.
> 
> This is stale as of v3, since isn't named --output-format any more. Let
> me see if there are other instances of this.
> 
> (I should have caught this earlier; I only saw it because Junio quoted
> it in a reply of his.)

Good catch, fixed now.

Patrick

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9e7e45344d..e15d43dc1e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -788,14 +788,13 @@  static int refcol_width(const struct ref *ref, int compact_format)
 }
 
 static void display_state_init(struct display_state *display_state, struct ref *ref_map,
-			       const char *raw_url)
+			       const char *raw_url, enum display_format format)
 {
-	const char *format = "full";
 	int i;
 
 	memset(display_state, 0, sizeof(*display_state));
-
 	strbuf_init(&display_state->buf, 0);
+	display_state->format = format;
 
 	if (raw_url)
 		display_state->url = transport_anonymize_url(raw_url);
@@ -812,15 +811,6 @@  static void display_state_init(struct display_state *display_state, struct ref *
 	if (verbosity < 0)
 		return;
 
-	git_config_get_string_tmp("fetch.output", &format);
-	if (!strcasecmp(format, "full"))
-		display_state->format = DISPLAY_FORMAT_FULL;
-	else if (!strcasecmp(format, "compact"))
-		display_state->format = DISPLAY_FORMAT_COMPACT;
-	else
-		die(_("invalid value for '%s': '%s'"),
-		    "fetch.output", format);
-
 	switch (display_state->format) {
 	case DISPLAY_FORMAT_FULL:
 	case DISPLAY_FORMAT_COMPACT: {
@@ -1614,7 +1604,8 @@  static int backfill_tags(struct display_state *display_state,
 }
 
 static int do_fetch(struct transport *transport,
-		    struct refspec *rs)
+		    struct refspec *rs,
+		    enum display_format display_format)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
@@ -1700,7 +1691,7 @@  static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	display_state_init(&display_state, ref_map, transport->url);
+	display_state_init(&display_state, ref_map, transport->url, display_format);
 
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
@@ -2078,7 +2069,8 @@  static inline void fetch_one_setup_partial(struct remote *remote)
 }
 
 static int fetch_one(struct remote *remote, int argc, const char **argv,
-		     int prune_tags_ok, int use_stdin_refspecs)
+		     int prune_tags_ok, int use_stdin_refspecs,
+		     enum display_format display_format)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH;
 	int i;
@@ -2145,7 +2137,7 @@  static int fetch_one(struct remote *remote, int argc, const char **argv,
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
 	sigchain_push(SIGPIPE, SIG_IGN);
-	exit_code = do_fetch(gtransport, &rs);
+	exit_code = do_fetch(gtransport, &rs, display_format);
 	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);
@@ -2157,6 +2149,7 @@  int cmd_fetch(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	const char *bundle_uri;
+	enum display_format display_format = DISPLAY_FORMAT_UNKNOWN;
 	struct string_list list = STRING_LIST_INIT_DUP;
 	struct remote *remote = NULL;
 	int result = 0;
@@ -2183,6 +2176,19 @@  int cmd_fetch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
 
+	if (display_format == DISPLAY_FORMAT_UNKNOWN) {
+		const char *format = "full";
+
+		git_config_get_string_tmp("fetch.output", &format);
+		if (!strcasecmp(format, "full"))
+			display_format = DISPLAY_FORMAT_FULL;
+		else if (!strcasecmp(format, "compact"))
+			display_format = DISPLAY_FORMAT_COMPACT;
+		else
+			die(_("invalid value for '%s': '%s'"),
+			    "fetch.output", format);
+	}
+
 	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
 		recurse_submodules = recurse_submodules_cli;
 
@@ -2311,7 +2317,8 @@  int cmd_fetch(int argc, const char **argv, const char *prefix)
 	} else if (remote) {
 		if (filter_options.choice || has_promisor_remote())
 			fetch_one_setup_partial(remote);
-		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);
+		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
+				   display_format);
 	} else {
 		int max_children = max_jobs;