[PATCH 0/6] ls-tree.c: clean-up works

[PATCH 0/6] ls-tree.c: clean-up works

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

This patchset aims to do works on refactoring and cleanup
of ls-tree.c.

The patches can be viewed as two parts:

      * Part 1: from Ævar Arnfjörð Bjarmason which mentioned in
      his RFC patchset[1] and Ævar suggests to submit a separate clean-up
      series. After I read them, I accept them all.

      * Part 2: fÆvarrom me which introduced in my RFC patchset[2] and after
      I accepted Ævar's patches, I keep two patches from [2], the left
      patches in [2] may no longer be needed.

[1] https://public-inbox.org/git/RFC-cover-0.4-00000000000-20221117T134528Z-avarab@gmail.com/
[2] https://public-inbox.org/git/20221117113023.65865-1-tenglong.tl@alibaba-inc.com/

Thanks.

Teng Long (2):
  ls-tree: cleanup the redundant SPACE
  t3104: remove shift code in 'test_ls_tree_format'

Ævar Arnfjörð Bjarmason (4):
  ls-tree: don't use "show_tree_data" for "fast" callbacks
  ls-tree: use a "struct options"
  ls-tree: fold "show_tree_data" into "cb" struct
  ls-tree: make "line_termination" less generic

 builtin/ls-tree.c         | 257 ++++++++++++++++++++++----------------
 t/t3104-ls-tree-format.sh |   1 -
 2 files changed, 150 insertions(+), 108 deletions(-)

-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 1/6] ls-tree: don't use "show_tree_data" for "fast" callbacks

[Teng Long]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

As noted in [1] the code that made it in as part of
9c4d58ff2c3 (ls-tree: split up "fast path" callbacks, 2022-03-23) was
a "maybe a good idea, maybe not" RFC-quality patch. I hadn't looked
very carefully at the resulting patterns.

The implementation shared the "struct show_tree_data data", which was
introduced in e81517155e0 (ls-tree: introduce struct "show_tree_data",
2022-03-23) both for use in 455923e0a15 (ls-tree: introduce "--format"
option, 2022-03-23), and because the "fat" callback hadn't been split
up as 9c4d58ff2c3 did.

Now that that's been done we can see that most of what
show_tree_common() was doing could be done lazily by the callbacks
themselves, who in the pre-image were often using an odd mis-match of
their own arguments and those same arguments stuck into the "data"
structure. Let's also have the callers initialize the "type", rather
than grabbing it from the "data" structure afterwards.

1. https://lore.kernel.org/git/cover-0.7-00000000000-20220310T134811Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Teng Long <dyronteng@gmail.com>
---
 builtin/ls-tree.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index c3ea09281af..cbb6782f9a5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -173,19 +173,11 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 	return recurse;
 }
 
-static int show_tree_common(struct show_tree_data *data, int *recurse,
-			    const struct object_id *oid, struct strbuf *base,
-			    const char *pathname, unsigned mode)
+static int show_tree_common(int *recurse, struct strbuf *base,
+			    const char *pathname, enum object_type type)
 {
-	enum object_type type = object_type(mode);
 	int ret = -1;
-
 	*recurse = 0;
-	data->mode = mode;
-	data->type = type;
-	data->oid = oid;
-	data->pathname = pathname;
-	data->base = base;
 
 	if (type == OBJ_BLOB) {
 		if (ls_options & LS_TREE_ONLY)
@@ -217,15 +209,15 @@ static int show_tree_default(const struct object_id *oid, struct strbuf *base,
 {
 	int early;
 	int recurse;
-	struct show_tree_data data = { 0 };
+	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
+	early = show_tree_common(&recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
-	printf("%06o %s %s\t", data.mode, type_name(data.type),
-	       find_unique_abbrev(data.oid, abbrev));
-	show_tree_common_default_long(base, pathname, data.base->len);
+	printf("%06o %s %s\t", mode, type_name(object_type(mode)),
+	       find_unique_abbrev(oid, abbrev));
+	show_tree_common_default_long(base, pathname, base->len);
 	return recurse;
 }
 
@@ -235,16 +227,16 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 {
 	int early;
 	int recurse;
-	struct show_tree_data data = { 0 };
 	char size_text[24];
+	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
+	early = show_tree_common(&recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
-	if (data.type == OBJ_BLOB) {
+	if (type == OBJ_BLOB) {
 		unsigned long size;
-		if (oid_object_info(the_repository, data.oid, &size) == OBJ_BAD)
+		if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
 			xsnprintf(size_text, sizeof(size_text), "BAD");
 		else
 			xsnprintf(size_text, sizeof(size_text),
@@ -253,9 +245,9 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 		xsnprintf(size_text, sizeof(size_text), "-");
 	}
 
-	printf("%06o %s %s %7s\t", data.mode, type_name(data.type),
-	       find_unique_abbrev(data.oid, abbrev), size_text);
-	show_tree_common_default_long(base, pathname, data.base->len);
+	printf("%06o %s %s %7s\t", mode, type_name(type),
+	       find_unique_abbrev(oid, abbrev), size_text);
+	show_tree_common_default_long(base, pathname, base->len);
 	return recurse;
 }
 
@@ -266,9 +258,9 @@ static int show_tree_name_only(const struct object_id *oid, struct strbuf *base,
 	int early;
 	int recurse;
 	const size_t baselen = base->len;
-	struct show_tree_data data = { 0 };
+	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
+	early = show_tree_common(&recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
@@ -286,9 +278,9 @@ static int show_tree_object(const struct object_id *oid, struct strbuf *base,
 {
 	int early;
 	int recurse;
-	struct show_tree_data data = { 0 };
+	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
+	early = show_tree_common(&recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 2/6] ls-tree: use a "struct options"

[Teng Long]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

As a first step towards being able to turn this code into an API some
day let's change the "static" options in builtin/ls-tree.c into a
"struct ls_tree_options" that can be constructed dynamically without
the help of parse_options().

Because we're now using non-static variables for this we'll need to
clear_pathspec() at the end of cmd_ls_tree(), least various tests
start failing under SANITIZE=leak. The memory leak was already there
before, now it's just being brought to the surface.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 203 ++++++++++++++++++++++++++--------------------
 1 file changed, 116 insertions(+), 87 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index cbb6782f9a5..54f7b604cb7 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -14,37 +14,11 @@
 #include "parse-options.h"
 #include "pathspec.h"
 
-static int line_termination = '\n';
-#define LS_RECURSIVE 1
-#define LS_TREE_ONLY (1 << 1)
-#define LS_SHOW_TREES (1 << 2)
-static int abbrev;
-static int ls_options;
-static struct pathspec pathspec;
-static int chomp_prefix;
-static const char *ls_tree_prefix;
-static const char *format;
-struct show_tree_data {
-	unsigned mode;
-	enum object_type type;
-	const struct object_id *oid;
-	const char *pathname;
-	struct strbuf *base;
-};
-
 static const  char * const ls_tree_usage[] = {
 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
 	NULL
 };
 
-static enum ls_tree_cmdmode {
-	MODE_DEFAULT = 0,
-	MODE_LONG,
-	MODE_NAME_ONLY,
-	MODE_NAME_STATUS,
-	MODE_OBJECT_ONLY,
-} cmdmode;
-
 static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
 			      const enum object_type type, unsigned int padded)
 {
@@ -64,10 +38,39 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
 	}
 }
 
+struct ls_tree_options {
+	int line_termination;
+	int abbrev;
+	enum ls_tree_path_options {
+		LS_RECURSIVE = 1 << 0,
+		LS_TREE_ONLY = 1 << 1,
+		LS_SHOW_TREES = 1 << 2,
+	} ls_options;
+	struct pathspec pathspec;
+	int chomp_prefix;
+	const char *ls_tree_prefix;
+	const char *format;
+};
+
+struct show_tree_data {
+	unsigned mode;
+	enum object_type type;
+	const struct object_id *oid;
+	const char *pathname;
+	struct strbuf *base;
+};
+
+struct show_tree_data_cb {
+	struct ls_tree_options *options;
+	struct show_tree_data *data;
+};
+
 static size_t expand_show_tree(struct strbuf *sb, const char *start,
 			       void *context)
 {
-	struct show_tree_data *data = context;
+	struct show_tree_data_cb *wrapper = context;
+	struct ls_tree_options *options = wrapper->options;
+	struct show_tree_data *data = wrapper->data;
 	const char *end;
 	const char *p;
 	unsigned int errlen;
@@ -92,10 +95,10 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,
 	} else if (skip_prefix(start, "(objectsize)", &p)) {
 		expand_objectsize(sb, data->oid, data->type, 0);
 	} else if (skip_prefix(start, "(objectname)", &p)) {
-		strbuf_add_unique_abbrev(sb, data->oid, abbrev);
+		strbuf_add_unique_abbrev(sb, data->oid, options->abbrev);
 	} else if (skip_prefix(start, "(path)", &p)) {
 		const char *name = data->base->buf;
-		const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
+		const char *prefix = options->chomp_prefix ? options->ls_tree_prefix : NULL;
 		struct strbuf quoted = STRBUF_INIT;
 		struct strbuf sbuf = STRBUF_INIT;
 		strbuf_addstr(data->base, data->pathname);
@@ -111,18 +114,19 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,
 	return len;
 }
 
-static int show_recursive(const char *base, size_t baselen, const char *pathname)
+static int show_recursive(struct ls_tree_options *options, const char *base,
+			  size_t baselen, const char *pathname)
 {
 	int i;
 
-	if (ls_options & LS_RECURSIVE)
+	if (options->ls_options & LS_RECURSIVE)
 		return 1;
 
-	if (!pathspec.nr)
+	if (!options->pathspec.nr)
 		return 0;
 
-	for (i = 0; i < pathspec.nr; i++) {
-		const char *spec = pathspec.items[i].match;
+	for (i = 0; i < options->pathspec.nr; i++) {
+		const char *spec = options->pathspec.items[i].match;
 		size_t len, speclen;
 
 		if (strncmp(base, spec, baselen))
@@ -142,13 +146,13 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname
 }
 
 static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
-			 const char *pathname, unsigned mode, void *context UNUSED)
+			 const char *pathname, unsigned mode, void *context)
 {
+	struct ls_tree_options *options = context;
 	size_t baselen;
 	int recurse = 0;
 	struct strbuf sb = STRBUF_INIT;
 	enum object_type type = object_type(mode);
-
 	struct show_tree_data data = {
 		.mode = mode,
 		.type = type,
@@ -156,81 +160,89 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 		.pathname = pathname,
 		.base = base,
 	};
+	struct show_tree_data_cb cb_data = {
+		.data = &data,
+		.options = options,
+	};
 
-	if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
+	if (type == OBJ_TREE && show_recursive(options, base->buf, base->len, pathname))
 		recurse = READ_TREE_RECURSIVE;
-	if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
+	if (type == OBJ_TREE && recurse && !(options->ls_options & LS_SHOW_TREES))
 		return recurse;
-	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
+	if (type == OBJ_BLOB && (options->ls_options & LS_TREE_ONLY))
 		return 0;
 
 	baselen = base->len;
-	strbuf_expand(&sb, format, expand_show_tree, &data);
-	strbuf_addch(&sb, line_termination);
+	strbuf_expand(&sb, options->format, expand_show_tree, &cb_data);
+	strbuf_addch(&sb, options->line_termination);
 	fwrite(sb.buf, sb.len, 1, stdout);
 	strbuf_release(&sb);
 	strbuf_setlen(base, baselen);
 	return recurse;
 }
 
-static int show_tree_common(int *recurse, struct strbuf *base,
-			    const char *pathname, enum object_type type)
+static int show_tree_common(struct ls_tree_options *options, int *recurse,
+			    struct strbuf *base, const char *pathname,
+			    enum object_type type)
 {
 	int ret = -1;
 	*recurse = 0;
 
 	if (type == OBJ_BLOB) {
-		if (ls_options & LS_TREE_ONLY)
+		if (options->ls_options & LS_TREE_ONLY)
 			ret = 0;
 	} else if (type == OBJ_TREE &&
-		   show_recursive(base->buf, base->len, pathname)) {
+		   show_recursive(options, base->buf, base->len, pathname)) {
 		*recurse = READ_TREE_RECURSIVE;
-		if (!(ls_options & LS_SHOW_TREES))
+		if (!(options->ls_options & LS_SHOW_TREES))
 			ret = *recurse;
 	}
 
 	return ret;
 }
 
-static void show_tree_common_default_long(struct strbuf *base,
+static void show_tree_common_default_long(struct ls_tree_options *options,
+					  struct strbuf *base,
 					  const char *pathname,
 					  const size_t baselen)
 {
 	strbuf_addstr(base, pathname);
 	write_name_quoted_relative(base->buf,
-				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
-				   line_termination);
+				   options->chomp_prefix ? options->ls_tree_prefix : NULL, stdout,
+				   options->line_termination);
 	strbuf_setlen(base, baselen);
 }
 
 static int show_tree_default(const struct object_id *oid, struct strbuf *base,
 			     const char *pathname, unsigned mode,
-			     void *context UNUSED)
+			     void *context)
 {
+	struct ls_tree_options *options = context;
 	int early;
 	int recurse;
 	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&recurse, base, pathname, type);
+	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
 	printf("%06o %s %s\t", mode, type_name(object_type(mode)),
-	       find_unique_abbrev(oid, abbrev));
-	show_tree_common_default_long(base, pathname, base->len);
+	       find_unique_abbrev(oid, options->abbrev));
+	show_tree_common_default_long(options, base, pathname, base->len);
 	return recurse;
 }
 
 static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 			  const char *pathname, unsigned mode,
-			  void *context UNUSED)
+			  void *context)
 {
+	struct ls_tree_options *options = context;
 	int early;
 	int recurse;
 	char size_text[24];
 	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&recurse, base, pathname, type);
+	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
@@ -246,48 +258,58 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 	}
 
 	printf("%06o %s %s %7s\t", mode, type_name(type),
-	       find_unique_abbrev(oid, abbrev), size_text);
-	show_tree_common_default_long(base, pathname, base->len);
+	       find_unique_abbrev(oid, options->abbrev), size_text);
+	show_tree_common_default_long(options, base, pathname, base->len);
 	return recurse;
 }
 
 static int show_tree_name_only(const struct object_id *oid, struct strbuf *base,
 			       const char *pathname, unsigned mode,
-			       void *context UNUSED)
+			       void *context)
 {
+	struct ls_tree_options *options = context;
 	int early;
 	int recurse;
 	const size_t baselen = base->len;
 	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&recurse, base, pathname, type);
+	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
 	strbuf_addstr(base, pathname);
 	write_name_quoted_relative(base->buf,
-				   chomp_prefix ? ls_tree_prefix : NULL,
-				   stdout, line_termination);
+				   options->chomp_prefix ? options->ls_tree_prefix : NULL,
+				   stdout, options->line_termination);
 	strbuf_setlen(base, baselen);
 	return recurse;
 }
 
 static int show_tree_object(const struct object_id *oid, struct strbuf *base,
 			    const char *pathname, unsigned mode,
-			    void *context UNUSED)
+			    void *context)
 {
+	struct ls_tree_options *options = context;
 	int early;
 	int recurse;
 	enum object_type type = object_type(mode);
 
-	early = show_tree_common(&recurse, base, pathname, type);
+	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
-	printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
+	printf("%s%c", find_unique_abbrev(oid, options->abbrev), options->line_termination);
 	return recurse;
 }
 
+enum ls_tree_cmdmode {
+	MODE_DEFAULT = 0,
+	MODE_LONG,
+	MODE_NAME_ONLY,
+	MODE_NAME_STATUS,
+	MODE_OBJECT_ONLY,
+};
+
 struct ls_tree_cmdmode_to_fmt {
 	enum ls_tree_cmdmode mode;
 	const char *const fmt;
@@ -327,14 +349,18 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	struct tree *tree;
 	int i, full_tree = 0;
 	read_tree_fn_t fn = NULL;
+	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+	struct ls_tree_options options = {
+		.line_termination = '\n',
+	};
 	const struct option ls_tree_options[] = {
-		OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
+		OPT_BIT('d', NULL, &options.ls_options, N_("only show trees"),
 			LS_TREE_ONLY),
-		OPT_BIT('r', NULL, &ls_options, N_("recurse into subtrees"),
+		OPT_BIT('r', NULL, &options.ls_options, N_("recurse into subtrees"),
 			LS_RECURSIVE),
-		OPT_BIT('t', NULL, &ls_options, N_("show trees when recursing"),
+		OPT_BIT('t', NULL, &options.ls_options, N_("show trees when recursing"),
 			LS_SHOW_TREES),
-		OPT_SET_INT('z', NULL, &line_termination,
+		OPT_SET_INT('z', NULL, &options.line_termination,
 			    N_("terminate entries with NUL byte"), 0),
 		OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
 			    MODE_LONG),
@@ -344,29 +370,30 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			    MODE_NAME_STATUS),
 		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
 			    MODE_OBJECT_ONLY),
-		OPT_SET_INT(0, "full-name", &chomp_prefix,
+		OPT_SET_INT(0, "full-name", &options.chomp_prefix,
 			    N_("use full path names"), 0),
 		OPT_BOOL(0, "full-tree", &full_tree,
 			 N_("list entire tree; not just current directory "
 			    "(implies --full-name)")),
-		OPT_STRING_F(0, "format", &format, N_("format"),
+		OPT_STRING_F(0, "format", &options.format, N_("format"),
 					 N_("format to use for the output"),
 					 PARSE_OPT_NONEG),
-		OPT__ABBREV(&abbrev),
+		OPT__ABBREV(&options.abbrev),
 		OPT_END()
 	};
 	struct ls_tree_cmdmode_to_fmt *m2f = ls_tree_cmdmode_format;
+	int ret;
 
 	git_config(git_default_config, NULL);
-	ls_tree_prefix = prefix;
+	options.ls_tree_prefix = prefix;
 	if (prefix)
-		chomp_prefix = strlen(prefix);
+		options.chomp_prefix = strlen(prefix);
 
 	argc = parse_options(argc, argv, prefix, ls_tree_options,
 			     ls_tree_usage, 0);
 	if (full_tree) {
-		ls_tree_prefix = prefix = NULL;
-		chomp_prefix = 0;
+		options.ls_tree_prefix = prefix = NULL;
+		options.chomp_prefix = 0;
 	}
 	/*
 	 * We wanted to detect conflicts between --name-only and
@@ -378,10 +405,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
-	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
-		ls_options |= LS_SHOW_TREES;
+	    ((LS_TREE_ONLY|LS_RECURSIVE) & options.ls_options))
+		options.ls_options |= LS_SHOW_TREES;
 
-	if (format && cmdmode)
+	if (options.format && cmdmode)
 		usage_msg_opt(
 			_("--format can't be combined with other format-altering options"),
 			ls_tree_usage, ls_tree_options);
@@ -396,13 +423,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	 * cannot be lifted until it is converted to use
 	 * match_pathspec() or tree_entry_interesting()
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
-				  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+	parse_pathspec(&options.pathspec, PATHSPEC_ALL_MAGIC &
+		       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
 		       PATHSPEC_PREFER_CWD,
 		       prefix, argv + 1);
-	for (i = 0; i < pathspec.nr; i++)
-		pathspec.items[i].nowildcard_len = pathspec.items[i].len;
-	pathspec.has_wildcard = 0;
+	for (i = 0; i < options.pathspec.nr; i++)
+		options.pathspec.items[i].nowildcard_len = options.pathspec.items[i].len;
+	options.pathspec.has_wildcard = 0;
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die("not a tree object");
@@ -412,11 +439,11 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	 */
 	while (m2f) {
 		if (!m2f->fmt) {
-			fn = format ? show_tree_fmt : show_tree_default;
-		} else if (format && !strcmp(format, m2f->fmt)) {
+			fn = options.format ? show_tree_fmt : show_tree_default;
+		} else if (options.format && !strcmp(options.format, m2f->fmt)) {
 			cmdmode = m2f->mode;
 			fn = m2f->fn;
-		} else if (!format && cmdmode == m2f->mode) {
+		} else if (!options.format && cmdmode == m2f->mode) {
 			fn = m2f->fn;
 		} else {
 			m2f++;
@@ -425,5 +452,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	return !!read_tree(the_repository, tree, &pathspec, fn, NULL);
+	ret = !!read_tree(the_repository, tree, &options.pathspec, fn, &options);
+	clear_pathspec(&options.pathspec);
+	return ret;
 }
-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 3/6] ls-tree: fold "show_tree_data" into "cb" struct

[Teng Long]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

After the the preceding two commits the only user of the
"show_tree_data" struct needed it along with the "options" member,
let's instead fold all of that into a "show_tree_data" struct that
we'll use only for that callback.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 54f7b604cb7..da664eecfb9 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -53,6 +53,7 @@ struct ls_tree_options {
 };
 
 struct show_tree_data {
+	struct ls_tree_options *options;
 	unsigned mode;
 	enum object_type type;
 	const struct object_id *oid;
@@ -60,17 +61,11 @@ struct show_tree_data {
 	struct strbuf *base;
 };
 
-struct show_tree_data_cb {
-	struct ls_tree_options *options;
-	struct show_tree_data *data;
-};
-
 static size_t expand_show_tree(struct strbuf *sb, const char *start,
 			       void *context)
 {
-	struct show_tree_data_cb *wrapper = context;
-	struct ls_tree_options *options = wrapper->options;
-	struct show_tree_data *data = wrapper->data;
+	struct show_tree_data *data = context;
+	struct ls_tree_options *options = data->options;
 	const char *end;
 	const char *p;
 	unsigned int errlen;
@@ -153,17 +148,14 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 	int recurse = 0;
 	struct strbuf sb = STRBUF_INIT;
 	enum object_type type = object_type(mode);
-	struct show_tree_data data = {
+	struct show_tree_data cb_data = {
+		.options = options,
 		.mode = mode,
 		.type = type,
 		.oid = oid,
 		.pathname = pathname,
 		.base = base,
 	};
-	struct show_tree_data_cb cb_data = {
-		.data = &data,
-		.options = options,
-	};
 
 	if (type == OBJ_TREE && show_recursive(options, base->buf, base->len, pathname))
 		recurse = READ_TREE_RECURSIVE;
-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 4/6] ls-tree: make "line_termination" less generic

[Teng Long]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

The "ls-tree" command isn't capable of ending "lines" with anything
except '\n' or '\0', and in the latter case we can avoid calling
write_name_quoted_relative() entirely. Let's do that, less for
optimization and more for clarity, the write_name_quoted_relative()
API itself does much the same thing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 58 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index da664eecfb9..a743959f2b3 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -39,7 +39,7 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
 }
 
 struct ls_tree_options {
-	int line_termination;
+	unsigned null_termination:1;
 	int abbrev;
 	enum ls_tree_path_options {
 		LS_RECURSIVE = 1 << 0,
@@ -166,7 +166,7 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 
 	baselen = base->len;
 	strbuf_expand(&sb, options->format, expand_show_tree, &cb_data);
-	strbuf_addch(&sb, options->line_termination);
+	strbuf_addch(&sb, options->null_termination ? '\0' : '\n');
 	fwrite(sb.buf, sb.len, 1, stdout);
 	strbuf_release(&sb);
 	strbuf_setlen(base, baselen);
@@ -198,10 +198,22 @@ static void show_tree_common_default_long(struct ls_tree_options *options,
 					  const char *pathname,
 					  const size_t baselen)
 {
+	const char *prefix = options->chomp_prefix ? options->ls_tree_prefix : NULL;
+
 	strbuf_addstr(base, pathname);
-	write_name_quoted_relative(base->buf,
-				   options->chomp_prefix ? options->ls_tree_prefix : NULL, stdout,
-				   options->line_termination);
+
+	if (options->null_termination) {
+		struct strbuf sb = STRBUF_INIT;
+		const char *name = relative_path(base->buf, prefix, &sb);
+
+		fputs(name, stdout);
+		fputc('\0', stdout);
+
+		strbuf_release(&sb);
+	} else {
+		write_name_quoted_relative(base->buf, prefix, stdout, '\n');
+	}
+
 	strbuf_setlen(base, baselen);
 }
 
@@ -264,15 +276,25 @@ static int show_tree_name_only(const struct object_id *oid, struct strbuf *base,
 	int recurse;
 	const size_t baselen = base->len;
 	enum object_type type = object_type(mode);
+	const char *prefix;
 
 	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
+	prefix = options->chomp_prefix ? options->ls_tree_prefix : NULL;
 	strbuf_addstr(base, pathname);
-	write_name_quoted_relative(base->buf,
-				   options->chomp_prefix ? options->ls_tree_prefix : NULL,
-				   stdout, options->line_termination);
+	if (options->null_termination) {
+		struct strbuf sb = STRBUF_INIT;
+		const char *name = relative_path(base->buf, prefix, &sb);
+
+		fputs(name, stdout);
+		fputc('\0', stdout);
+
+		strbuf_release(&sb);
+	} else {
+		write_name_quoted_relative(base->buf, prefix, stdout, '\n');
+	}
 	strbuf_setlen(base, baselen);
 	return recurse;
 }
@@ -285,12 +307,19 @@ static int show_tree_object(const struct object_id *oid, struct strbuf *base,
 	int early;
 	int recurse;
 	enum object_type type = object_type(mode);
+	const char *str;
 
 	early = show_tree_common(options, &recurse, base, pathname, type);
 	if (early >= 0)
 		return early;
 
-	printf("%s%c", find_unique_abbrev(oid, options->abbrev), options->line_termination);
+	str = find_unique_abbrev(oid, options->abbrev);
+	if (options->null_termination) {
+		fputs(str, stdout);
+		fputc('\0', stdout);
+	} else  {
+		puts(str);
+	}
 	return recurse;
 }
 
@@ -342,9 +371,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	int i, full_tree = 0;
 	read_tree_fn_t fn = NULL;
 	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
-	struct ls_tree_options options = {
-		.line_termination = '\n',
-	};
+	int null_termination = 0;
+	struct ls_tree_options options = { 0 };
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &options.ls_options, N_("only show trees"),
 			LS_TREE_ONLY),
@@ -352,8 +380,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_RECURSIVE),
 		OPT_BIT('t', NULL, &options.ls_options, N_("show trees when recursing"),
 			LS_SHOW_TREES),
-		OPT_SET_INT('z', NULL, &options.line_termination,
-			    N_("terminate entries with NUL byte"), 0),
+		OPT_BOOL('z', NULL, &null_termination,
+			 N_("terminate entries with NUL byte")),
 		OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
 			    MODE_LONG),
 		OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
@@ -383,6 +411,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, ls_tree_options,
 			     ls_tree_usage, 0);
+	options.null_termination = null_termination;
+
 	if (full_tree) {
 		options.ls_tree_prefix = prefix = NULL;
 		options.chomp_prefix = 0;
-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 5/6] ls-tree: cleanup the redundant SPACE

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

An redundant space was found in ls-tree.c, which is no doubt
a small change, but it might be OK to make a commit on its own.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index a743959f2b3..72eb70823d5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -14,7 +14,7 @@
 #include "parse-options.h"
 #include "pathspec.h"
 
-static const  char * const ls_tree_usage[] = {
+static const char * const ls_tree_usage[] = {
 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
 	NULL
 };
-- 
2.39.0.98.g174eaa53973.dirty

[PATCH 6/6] t3104: remove shift code in 'test_ls_tree_format'

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

In t3104-ls-tree-format.sh, There is a legacy 'shift 2' code
and the relevant code block no longer depends on it anymore,
so let's remove it for a small cleanup.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 t/t3104-ls-tree-format.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t3104-ls-tree-format.sh b/t/t3104-ls-tree-format.sh
index 383896667b6..74053978f49 100755
--- a/t/t3104-ls-tree-format.sh
+++ b/t/t3104-ls-tree-format.sh
@@ -20,7 +20,6 @@ test_ls_tree_format () {
 	format=$1 &&
 	opts=$2 &&
 	fmtopts=$3 &&
-	shift 2 &&
 
 	test_expect_success "ls-tree '--format=<$format>' is like options '$opts $fmtopts'" '
 		git ls-tree $opts -r HEAD >expect &&
-- 
2.39.0.98.g174eaa53973.dirty

Re: [PATCH 5/6] ls-tree: cleanup the redundant SPACE

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
>
> An redundant space was found in ls-tree.c, which is no doubt
> a small change, but it might be OK to make a commit on its own.

No need to update this series for just this, but if you're doing an
update it would be nice to mention that this minor typo/unwanted syntax
has been there since 61fdbcf98b4 (ls-tree: migrate to parse-options,
2009-11-13).

I.e. to clarify that this is rather ancient, and not some fix-up for a
recent topic (or fixing things earlier in this topic itself).

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/ls-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index a743959f2b3..72eb70823d5 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -14,7 +14,7 @@
>  #include "parse-options.h"
>  #include "pathspec.h"
>  
> -static const  char * const ls_tree_usage[] = {
> +static const char * const ls_tree_usage[] = {
>  	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>  	NULL
>  };

Re: [PATCH 6/6] t3104: remove shift code in 'test_ls_tree_format'

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
>
> In t3104-ls-tree-format.sh, There is a legacy 'shift 2' code
> and the relevant code block no longer depends on it anymore,
> so let's remove it for a small cleanup.

Like with 6/6, we can do without an update for this, but if you're
updating I'd say something like this instead:

	The "shift 2" added here when these tests were added in
	455923e0a15 (ls-tree: introduce "--format" option, 2022-03-23)
	has never been need. Let's remove it, as we're not in the habit
	of clearing arguments from "$@" unless we need to use "$@"
	itself (or "$*") later.

FWIW I think this dates back to my
https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/;
where there was no reason to do it either. I wouldn't normally "shift"
for no reason, so it probably dates back to some earlier version where I
used "$@", which didn't make it to the list.

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  t/t3104-ls-tree-format.sh | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/t/t3104-ls-tree-format.sh b/t/t3104-ls-tree-format.sh
> index 383896667b6..74053978f49 100755
> --- a/t/t3104-ls-tree-format.sh
> +++ b/t/t3104-ls-tree-format.sh
> @@ -20,7 +20,6 @@ test_ls_tree_format () {
>  	format=$1 &&
>  	opts=$2 &&
>  	fmtopts=$3 &&
> -	shift 2 &&
>  
>  	test_expect_success "ls-tree '--format=<$format>' is like options '$opts $fmtopts'" '
>  		git ls-tree $opts -r HEAD >expect &&

Re: [PATCH 0/6] ls-tree.c: clean-up works

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 12 2023, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
>
> This patchset aims to do works on refactoring and cleanup
> of ls-tree.c.
>
> The patches can be viewed as two parts:
>
>       * Part 1: from Ævar Arnfjörð Bjarmason which mentioned in
>       his RFC patchset[1] and Ævar suggests to submit a separate clean-up
>       series. After I read them, I accept them all.
>
>       * Part 2: fÆvarrom me which introduced in my RFC patchset[2] and after
>       I accepted Ævar's patches, I keep two patches from [2], the left
>       patches in [2] may no longer be needed.
>
> [1] https://public-inbox.org/git/RFC-cover-0.4-00000000000-20221117T134528Z-avarab@gmail.com/
> [2] https://public-inbox.org/git/20221117113023.65865-1-tenglong.tl@alibaba-inc.com/
>
> Thanks.

Thanks for picking this up. It would be great to have this queued up.

It looks good to me, although that's mostly self-approving, as the
range-diff between this and my [1] (and your description, but I wanted
to sanity check it) shows it's mostly the same changes:
	
p	1:  a7f86973d57 ! 1:  5004f03c4d2 ls-tree: don't use "show_tree_data" for "fast" callbacks
	    @@ Commit message
	     
	         1. https://lore.kernel.org/git/cover-0.7-00000000000-20220310T134811Z-avarab@gmail.com/
	     
	    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	    +    Signed-off-by: Teng Long <dyronteng@gmail.com>
	    +
	      ## builtin/ls-tree.c ##
	     @@ builtin/ls-tree.c: static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
	      	return recurse;
	2:  b046e6f5692 ! 2:  de5b4d0767a ls-tree: use a "struct options"
	    @@ Commit message
	         before, now it's just being brought to the surface.
	     
	         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	    +    Signed-off-by: Teng Long <dyroneteng@gmail.com>
	     
	      ## builtin/ls-tree.c ##
	     @@
	3:  329bc581b79 ! 3:  8f4b59183f6 ls-tree: fold "show_tree_data" into "cb" struct
	    @@ Commit message
	         we'll use only for that callback.
	     
	         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	    +    Signed-off-by: Teng Long <dyroneteng@gmail.com>
	     
	      ## builtin/ls-tree.c ##
	     @@ builtin/ls-tree.c: struct ls_tree_options {
	4:  de4f89645fa ! 4:  d701aceb101 ls-tree: make "line_termination" less generic
	    @@ Commit message
	         API itself does much the same thing.
	     
	         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	    +    Signed-off-by: Teng Long <dyroneteng@gmail.com>
	     
	      ## builtin/ls-tree.c ##
	     @@ builtin/ls-tree.c: static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
	-:  ----------- > 5:  72bf99a1c74 ls-tree: cleanup the redundant SPACE
	-:  ----------- > 6:  978b89e5ea8 t3104: remove shift code in 'test_ls_tree_format'

I approve of that addition of the missing Signed-off-by (which I just
forgot about).

I left comments on the 2x new patches, but they were both nit-y
suggestions for maybe rephrasing the commit messages a bit, but as noted
I don't think it's worth another re-roll (just maybe if others comment
on the other patches and/or find other issues worth re-rolling).