[RFC,01/20] cat-file: reuse struct ref_format

Commit Message

Olga Telezhnaya Feb. 22, 2019, 4:05 p.m. UTC

Start using ref_format struct instead of simple char*.
Need that for further reusing of formatting logic from ref-filter.

Signed-off-by: Olga Telezhnaia <[e-mail]>
---
 builtin/cat-file.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)


--
https://github.com/git/git/pull/568

Comments

Jeff King Feb. 28, 2019, 9:04 p.m. UTC | #1

On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:

> Start using ref_format struct instead of simple char*.
> Need that for further reusing of formatting logic from ref-filter.

Makes sense.

>  struct batch_options {
> +	struct ref_format format;
>  	int enabled;
>  	int follow_symlinks;
>  	int print_contents;
> @@ -24,7 +26,6 @@ struct batch_options {
>  	int all_objects;
>  	int unordered;
>  	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
> -	const char *format;
>  };

Not a huge deal, but unless there's a compelling reason to move the
field around in the struct, the diff is easier to read if the deleted
and added lines stay in the same place.

> @@ -491,9 +492,6 @@ static int batch_objects(struct batch_options *opt)
>  	int save_warning;
>  	int retval = 0;
>  
> -	if (!opt->format)
> -		opt->format = "%(objectname) %(objecttype) %(objectsize)";
> -

This assignment moves down to cmd_cat_file(). I don't see any reason
that shouldn't work, but it makes reviewing easier if there aren't
unexpected changes (so if it doesn't need moved in the grand scheme of
things, leave it as it was; if it does, it should either come in its own
patch, or get a note in the commit message as to why it needed to move).

-Peff

Patch

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0f092382e175c..e5de596611800 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -15,8 +15,10 @@ 
 #include "sha1-array.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "ref-filter.h"
 
 struct batch_options {
+	struct ref_format format;
 	int enabled;
 	int follow_symlinks;
 	int print_contents;
@@ -24,7 +26,6 @@  struct batch_options {
 	int all_objects;
 	int unordered;
 	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
-	const char *format;
 };
 
 static const char *force_path;
@@ -365,7 +366,7 @@  static void batch_object_write(const char *obj_name,
 	}
 
 	strbuf_reset(scratch);
-	strbuf_expand(scratch, opt->format, expand_format, data);
+	strbuf_expand(scratch, opt->format.format, expand_format, data);
 	strbuf_addch(scratch, '\n');
 	batch_write(opt, scratch->buf, scratch->len);
 
@@ -491,9 +492,6 @@  static int batch_objects(struct batch_options *opt)
 	int save_warning;
 	int retval = 0;
 
-	if (!opt->format)
-		opt->format = "%(objectname) %(objecttype) %(objectsize)";
-
 	/*
 	 * Expand once with our special mark_query flag, which will prime the
 	 * object_info to be handed to oid_object_info_extended for each
@@ -501,7 +499,7 @@  static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	data.mark_query = 1;
-	strbuf_expand(&output, opt->format, expand_format, &data);
+	strbuf_expand(&output, opt->format.format, expand_format, &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
 	if (opt->cmdmode)
@@ -617,7 +615,7 @@  static int batch_option_callback(const struct option *opt,
 
 	bo->enabled = 1;
 	bo->print_contents = !strcmp(opt->long_name, "batch");
-	bo->format = arg;
+	bo->format.format = arg;
 
 	return 0;
 }
@@ -626,7 +624,7 @@  int cmd_cat_file(int argc, const char **argv, const char *prefix)
 {
 	int opt = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
-	struct batch_options batch = {0};
+	struct batch_options batch = { REF_FORMAT_INIT };
 	int unknown_type = 0;
 
 	const struct option options[] = {
@@ -707,6 +705,9 @@  int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
+	if (!batch.format.format)
+		batch.format.format = "%(objectname) %(objecttype) %(objectsize)";
+
 	if (batch.enabled)
 		return batch_objects(&batch);